On Sat, 20 Jul 2024 16:45:35 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>>> I've now introduced a ClosedSocketTest and a ClosedServerSocketTest in >>> their relevant directories. Each of them invoke various operations on a >>> closed Socket/ServerSocket instance and verify that they throw the expected >>> exception or complete normally if expected to do so. >> >> Thanks. My only comment is that using a MethodSource and >> SocketOp/ServerSocketOp in these tests looks a bit overkill. You may find it >> a bit simpler to just using assertThrow, something like >> `assertThrows(SocketException.class, s::getReceiveBufferSize)`. I don't have >> a strong opinion, I just prefer simpler tests I think. > > I've updated the PR to remove the usages of the parameterized test and > instead inline the operations within the test itself. The tests continue to > pass. > I think it would be better to not repeat the "if" in each of the thrown, > e.g. "if the bind operation fails, the socket is already bound, or the socket > is closed". Done - I've removed the repeated "if" from the throws clause. > some of the socket options have "an error in the underlying protocol, such as > a TCP error" but none of these methods are doing I/O, they are just setting > or reading socket options. They should say "if there is an error SO_RCVBUF" > or whatever. It's not important here, it's just jumping out when reading > these old descriptions. I decided to leave them as-is in this PR, given how many of those there are. I think we can decide to clean them up in a separate PR if needed. > You asked about specifying the more specific SocketException. These APIs are > pluggable via SocketImpl so specifying a more specific exception could > potentially invalidate existing implementations. I don't know if there are > any other implementations in 2024 but I think changing anything here would > require further thought on the compatibility impact. Given that context, leaving them in their current form sounds reasonable to me. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685498213
