On Thu, 28 Nov 2024 14:22:23 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Volkan Yazıcı has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Don't exceed 120 characters per line >> - Rework functional interfaces in tests > > test/jdk/java/net/Socket/ConnectFailTest.java line 57: > >> 55: * @test >> 56: * @bug 8343791 >> 57: * @summary verifies the socket state after `connect()` failures > > I think you can beef up the @summary a bit because the test checks that > Socket.connect throws the expected exception and completes with the Socket in > the expected state. Fixed. > test/jdk/java/net/Socket/ConnectFailTest.java line 59: > >> 57: * @summary verifies the socket state after `connect()` failures >> 58: * @library /test/lib >> 59: * @run junit/othervm --add-opens java.base/java.net=ALL-UNNAMED >> ConnectFailTest > > I assume the --add-opens is left over from a few iteration. For future > reference, the @modules tag will do this for you, do need to specify it to > the @run tag. You beat me to it. I was just simplifying this to `junit ConnectFailTest` and saw your review. Fixed. > test/jdk/java/net/Socket/ConnectFailTest.java line 139: > >> 137: assertFalse(socket.isBound()); >> 138: assertFalse(socket.isConnected()); >> 139: assertThrows(IOException.class, () -> >> socket.connect(UNRESOLVED_ADDRESS)); > > The specified exception for this case is UHE so the assertThrows can be > sharper. Fixed. > test/jdk/java/net/Socket/ConnectFailTest.java line 234: > >> 232: // Accept connections in the background to avoid blocking >> the caller >> 233: executorService.submit(() -> >> acceptConnections(serverSocket)); >> 234: serverSocketConsumer.accept(serverSocket); > > I think it just adds complexity to use a virtual thread here. Replaced with `newSingleThreadExecutor()`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863183980 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863183618 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863189514 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863190367
