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

[Chris Hegarty]

This looks really good Daniel.

  * Trivially, RefusingServer. startNewServer L84 could use
    try-with-resources.

  * AbstractTcpServer.newServerSocket seems unnecessary

-Chris.

On 06/11/2012 16:10, Daniel Fuchs wrote:
> 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.
>>
>>
>>
>>
>>
>