On Sat, 20 Jul 2024 15:52:08 GMT, Alan Bateman <al...@openjdk.org> wrote:
>>> When writing this test, I noticed that a few other methods on ServerSocket >>> and Socket also needed an update to their specification to match their >>> current implementation. >> >> Thanks, and doesn't surprise me that it's not explicitly specified in other >> methods. I think the spec change looks okay although 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". Up >> to you if you want to stick with what you have. >> >> Reading the updates, 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. >> >> 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. > >> 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20268#discussion_r1685496380
