On Tue, 26 Nov 2024 20:57:18 GMT, Volkan Yazıcı <d...@openjdk.org> wrote:
>> This PR, addressing 8343791, changes `Socket::connect()` methods to close
>> the `Socket` in the event that the connection cannot be established, the
>> timeout expires before the connection is established, or the socket address
>> is unresolved.
>>
>> `tier3` tests pass against the 9f00f61d3b7fa42a5e23a04f80bb4bb1a2076ef2.
>
> Volkan Yazıcı has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Split tests into multiple files
I scanned the test update. My personal view is that there way too much here.
JDK tests typically only use a small subset of the features that JUnit
provides. So very surprised to see the use of extensions in these tests. For
tests in this area then the priority should be on having tests that are easy to
maintain and diagnose when there are test failures, e.g. think about a test
failure in 2 years time where someone has to figure out test base with
different subclasses for different APIs. I can imagine someone being puzzled by
"Oio" in the name. My guess is that you mean "old I/O" but it looks really odd
here.
Daniel may be able to give you other advice. My view is keep it simple, don't
over engineer the tests, and think about diagnosability when tests fail. In
this case I think we just need one test ConnectFailTest, or better name, that
exercises Socket.connect with the various scenarios. Think
CloseOnConnectFailureTestBase without the ceremony. The test methods can be
parametized so they execute with sockets created with new Socket() and
SocketChannel.open().socket().
I'm not sure what to say about the mock SocketImpl and testing the Socket
constructor, maybe you can create a test only issue in JBS and contribute it
that way. I'd prefer this PR to stay focused on Socket.connect.
src/java.base/share/classes/sun/nio/ch/SocketAdaptor.java line 93:
> 91: throw new SocketException("Socket is closed");
> 92: if (sc.isConnected())
> 93: throw new SocketException("Already connected");
Would you mind changing this to use 4-space instead of 8-space indent?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2503541844
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1860436868
