Review Request - JDK-6720349: (ch) Channels tests depending on hosts inside Sun
chris.hegarty at oracle.com
Wed Nov 7 04:50:38 PST 2012
This looks really good Daniel.
* Trivially, RefusingServer. startNewServer L84 could use
* AbstractTcpServer.newServerSocket seems unnecessary
On 06/11/2012 16:10, Daniel Fuchs wrote:
> 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.
> best regards,
> -- daniel
> On 11/5/12 1:23 PM, Alan Bateman wrote:
>> On 30/10/2012 18:32, Daniel Fuchs wrote:
>>> I'm looking for reviewers for:
>>> JDK-6720349: (ch) Channels tests depending on hosts inside Sun
>>> 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
>> 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==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.
More information about the nio-dev