Review Request - JDK-6720349: (ch) Channels tests depending on hosts inside Sun

[Daniel Fuchs]

Hi,

Here is a new webrev incorporating Alan's feedback.

I fixed the indenting issues by looking at the original code
in the modified file (or in other files in the same directory),
and trying to guess how the author might have indented the lines
in question. That's just guesses on my part but hopefully it's
better now than before.

http://cr.openjdk.java.net/~dfuchs/JDK-6720349/webrev.01/

best regards,

-- daniel


On 11/5/12 1:23 PM, Alan Bateman wrote:
> On 30/10/2012 18:32, Daniel Fuchs wrote:
>> Hi,
>>
>> I'm looking for reviewers for:
>>
>>    JDK-6720349: (ch) Channels tests depending on hosts inside Sun
>> http://cr.openjdk.java.net/~dfuchs/JDK-6720349/webrev.00/
>>
>> This is to fix a test issue.
>>
>> Several tests in the jdk_nio test suites depends on remote services
>> (echo, daytime, discard, to cite a few) being available on remote
>> hosts inside Oracle network.
>>
>> The issue is that these tests cannot succeed outside of oracle network
>> without modification, and were often known to fail even inside oracle
>> networks depending on the availability of these hosts.
>>
>> This patch removes the dependencies on remote hosts by making the tests
>> instantiate and start small TCP or UDP servers within the test VM, thus
>> emulating the services that used to be provided by the internal
>> remote hosts.
>>
>> There are however - a few side effects on the tests themselves (read: on
>> what the tests are actually testing).
>>
>> Given that the servers accessed by the tests are now co-located on the
>> same machine - some assumptions made by the test code had to
>> be revisited. For instance, some of the tests assumed that by
>> setting the non blocking flag of the channel to true, and then
>> calling connect(), they would always obtain a channel in a
>> connection pending state. With servers on the same hosts, this is
>> no longer always the case (for instance on Solaris you end up
>> with a channel already connected).
>>
>> As a consequence some of the tests, on some architectures, no
>> longer exactly test what they used to be testing. I have identified
>> these parts of the test code that may not always be activated
>> and added comments to outline this.
>>
>> Tests performed:
>>
>> I ran the jdk_nio tests using JPRT (hotspotwest).
>>
>> best regards,
>>
>> -- daniel
> Thanks for doing this, this is long overdue. Also nice that there aren't
> too many changes to the tests themselves.
>
> My preference would be to put the test servers into a new class
> TestClass, to sit along aside TestThread and TestUtil. I suggest this
> because TestUtil was originally intended for utility methods for these
> tests.
>
> The only other general observation is that some of the indenting when
> statements or source lines slip into a second line is a bit
> inconsistent, I can't tell what is doing the formatting but it's just a
> bit inconsistent with the style that is used in this area (we don't have
> an JDK-wide coding convention/style guidelines).
>
> A couple of specific comments:
>
> - In Alias then the comment about Solaris should probably be replaced
> with a comment to say that the connection is established immediately. I
> also suggest dropping the message to System.err as this is inside the
> while loop and so will be printed LIMIT times.
>
> - BasicConnect.java (and many other tests) then the message that the
> connection is established immediately is printed to System.err, which
> suggests its an error or warning but it is normal so I'd suggest
> dropping it or printing it to System.out.
>
> - SocketChannel/Connect.java - the comment at L40-41 will likely
> confusing future maintainers, I don't think it is needed.
>
> - SocketChannel/ConnectState.java - I assume there isn't any need to
> check for exceptions==null or exceptions[0]==null in expectedExceptions.
> Otherwise the additionally handling of ST_PENDING_OR_CONNECT looks fine
> to me.
>
> So overall I think this will be great to get in.
>
> -Alan.
>
>
>
>
>