On Tue, 26 Nov 2024 09:52:17 GMT, Alan Bateman <al...@openjdk.org> wrote:
> > @AlanBateman, existing tests are stressing both ctor **and** `connect()`. > > Okay, just a bit surprising to lump them into one test. One thing to consider > is one test for the Socket.connect exception scenarios that is focused on > checking the connect method throws as expected and the expected Socket state > after connect fails. Lots of scenarios to test. A separate test could add to > the tests for the Socket constructors, it would be a separate PR if you want. @AlanBateman, 1e3a52c97f3301b1459b03574c25787a282217f6 splits the introduced tests into individual files: 1. `CloseOnNioConnectFailureTest` – tests 6 scenarios you cited using sockets obtained from `SocketChannel.open(address, port).socket()` 2. `CloseOnOioConnectFailureTest` – tests 6 scenarios you cited using sockets obtained from `Socket(address)` constructors 3. `CloseOnConnectFailureMockTest` – using a `SocketImpl` mock, verifies that the socket is closed on `connect()` failures 4. `CloseOnCtorFailureMockTest` – using a `SocketImpl` mock, verifies that the socket is closed on constructor failures As you noted, the last one (i.e., `CloseOnCtorFailureMockTest`) is not _directly_ related with this ticket. Though I would appreciate it if you can reconsider keeping it, because: * a `SocketImpl` mock along with test scenarios implemented for `CloseOnConnectFailureMockTest` are reused to do the 80% of the necessary plumbing * AFAIK, there are no other tests checking the socket is closed on ctor failures All in all, I am fine with whatever direction you want me to follow – be it keeping `CloseOnCtorFailureMockTest`, or creating a new ticket+PR for it. How shall I proceed? ------------- PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2501942719
