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

[Chris Hegarty]

On 07/11/2012 13:08, Daniel Fuchs wrote:
> 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?

I was thinking of the ServerSocket that you later explicitly close. It 
is not really not worth spending time on.

>> * 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 :-)

It is not necessary to make any changes. My comments were just trivial 
things I noticed.

-Chris.

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