On Mon, 25 Nov 2024 10:01:40 GMT, Alan Bateman <al...@openjdk.org> wrote:
> You shouldn't need a mock SocketImpl. I assume you have this because the test > is testing the Socket constructors mostly. @AlanBateman, existing tests are stressing both ctor **and** `connect()`. AFAICT, a `SocketImpl` mock is required to verify if the `Socket` ctor closes the `SocketImpl` correctly. I don't know any other way to perform this check. I also used the `SocketImpl` mock to verify the behavior of `connect()` on 1. `UHE`-causing input 2. Various `SocketImpl`-thrown exceptions The mock enabled the tests to only focus on what needs to be verified and exclude every other variable. > I see Mark is reviewing the test so I didn't put much time into looking at > the test. The main thing is for this test is that it exercises Socket.connect > and uses Socket::isClosed to test if the Socket is closed or not after the > connect fails. I think we need to test at least the following: > > * Socket unbound and unconnected, Socket connect fails, Socket should be > closed > * Socket bound and unconnected, Socket connect fails, Socket should be closed > * Socket connected, Socket connect fails, Socket should not be closed > * Socket unbound and unconnected, Socket connect to unresolved address, > Socket should be closed > * Socket bound and unconnected, Socket connect to unresolved address, Socket > should be closed > * Socket connected, Socket connect connect to unresolved address Socket > should not be closed > > You shouldn't need a mock SocketImpl. I assume you have this because the test > is testing the Socket constructors mostly. > > For the socket adapter then you could have the tests be parametrised so they > are called with an newly created Socket (unbound/unconnected), that way you > can test new Socket() and SocketChannel.open().socket(). I've pushed changes implementing explicit tests for each cited 6 scenarios and using two `Socket` factories: 1. The `Socket` constructor 2. `Socket.open().socket()` ------------- PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2500108234
