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

[Daniel Fuchs]

On 11/7/12 1:50 PM, Chris Hegarty wrote:
> This looks really good Daniel.

Thanks.

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

Refusing server is not connected - and thus not closeable.
It's just a bean that wrap an address on which no one is listening.
I could add a dummy noop close() for consistency but why bother?

>   * AbstractTcpServer.newServerSocket seems unnecessary

I naively thought I might be able to use a subclass of
ServerSocket to trigger connection timeout with delays in
the accept() method but it did not work. The method remained.
Do you wish me to remove it? I was almost ready to push :-)

-- daniel

> -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.
>>>
>>>
>>>
>>>
>>>
>>