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

[Alan Bateman]

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.





-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20121105/7fc17095/attachment.html