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

[Volkan Yazıcı]

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

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

@AlanBateman, thanks so much for the thorough review and valuable tips. In 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.

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

Removed tests using mocks in dfb1bb919d81cd07de5da702a91eb3895a7af4e6. I will submit a JBS+PR for these.

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

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