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

[Daniel Fuchs]

Hi Alan,

Thanks for reviewing.
I will make the changes and send another webrev when they are ready.
Do I need a second reviewer? Or is one reviewer enough?

See some comments inline:

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.

[...]

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

Will do. If I add a new file next to TestUtil, I won't have to
modify any makefile right?

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

I think I let NetBeans do the formatting in most cases - except when it
looked confusing. I'd be happy to reformat - or to configure NetBeans
with the appropriate options - I think what looks strange is the
indentation in the try () { } with resource statements, when the
try () line is split in two. Is that what surprised you?


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

It looks as if this is platform specific. On Mac OS - (10.7.5) - the
behavior was as the text expected. On Solaris - the connection was
established immediately. On windows I don't remember what I observed.
I can replace "On Solaris" with "On some platforms".

> I also suggest dropping the message to System.err as this is inside the
> while loop and so will be printed LIMIT times.

OK.

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

OK.

> - SocketChannel/Connect.java - the comment at L40-41 will likely
> confusing future maintainers, I don't think it is needed.

OK.

> - SocketChannel/ConnectState.java - I assume there isn't any need to
> check for exceptions==null or exceptions[0]==null in expectedExceptions.

Yes. I changed my mind between the time I wrote the method and the
time I called it. Will fix :-)

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