RFR: 8343791: Socket.connect API should document whether the socket will be closed when hostname resolution fails or another error occurs [v17]

[Alan Bateman]

On Wed, 27 Nov 2024 10:45:48 GMT, Alan Bateman <alanb at openjdk.org> wrote:

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

> @AlanBateman, thanks so much for the thorough review and valuable tips. In [9454364](https://github.com/openjdk/jdk/commit/945436425fa953889448ebc4f8b7a80e1d4bd9e1), replaced the existing tests with a single `ConnectFailTest` and tried to be conservative on used JUnit features. Let me know if I captured the design you outlined.

ConnectTestFail could be changed to use parameterized tests with a method source than produces a Socket and SocketChannel.open().socket(). If you had that then it could eliminate the duplicate tests and would allow most or all of the functional interfaces to be removed.

I think it good to find shorter names for many of the methods and variables as they are way too long in places, e.g. connectedSocketShouldNotBeClosedWhenConnectWithUnresolvedAddressFails.

Did you mean to leave ThrowingSocketImpl in the PR, I thought that was for the other issue. Also are you expecting ServerSocketTestUtil to be used by other tests?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2504283711