On Fri, 3 Apr 2026 13:13:51 GMT, Daniel Fuchs <[email protected]> wrote:
>> Some more TestNG -> JUnit conversion for java/net tests.
>> This change converts tests under:
>>
>> test/jdk/java/net/DatagramSocketImp/
>> test/jdk/java/net/MulticastSocket/
>> test/jdk/java/net/Socket/
>> test/jdk/java/net/SocketImpl/
>
> Daniel Fuchs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review feedback
Hopefully these comments are useful, if not please let me know. Again these are
only suggestions; I am not an OpenJDK member, so feel free to ignore my
comments.
test/jdk/java/net/MulticastSocket/SendPortZero.java line 88:
> 86: { new MulticastSocket(), wildcardZeroPkt },
> 87: // Not currently tested. See JDK-8236807
> 88: //{ new MulticastSocket(), wildcardValidPkt }
Is this still correct? Previously it used the same `multicastSocket` here. Now
`wildcardValidPkt` uses the port of the `multicastSocket` but a `new
MulticastSocket()` is passed as socket.
(Though it is commented out, so if this is an issue it would probably resurface
for JDK-8236807.)
test/jdk/java/net/MulticastSocket/SetLoopbackOption.java line 303:
> 301: public static void main (String args[]) throws Exception {
> 302: new SetLoopbackOption().run();
> 303: }
Is it still worth keeping this `main` method?
I guess now it might still work, but if any more advanced JUnit functionality
is used, such as `@AfterAll`, then this `main` method would behave incorrectly.
test/jdk/java/net/Socket/ConnectionReset.java line 55:
> 53: int bytesAvailable = in.available();
> 54: System.out.format("available => %d%n", bytesAvailable);
> 55: assertTrue(bytesAvailable == 0);
`assertEquals`?
test/jdk/java/net/Socket/ConnectionReset.java line 66:
> 64: } catch (IOException ioe) {
> 65: System.out.format("read => %s (expected)%n", ioe);
> 66: }
`assertThrows`?
test/jdk/java/net/Socket/ConnectionReset.java line 85:
> 83: System.out.format("available => %d%n", bytesAvailable);
> 84: assertTrue(bytesAvailable <= remaining);
> 85: try {
Try to refactor this if possible? It is not clear if this is actually an
`assertThrows` or whether it should just have a `assertNotEquals(-1,
bytesRead);`.
test/jdk/java/net/Socket/ConnectionReset.java line 122:
> 120: } catch (IOException ioe) {
> 121: System.out.format("read => %s (expected)%n", ioe);
> 122: }
`assertThrows`?
test/jdk/java/net/Socket/ConnectionReset.java line 125:
> 123: int bytesAvailable = in.available();
> 124: System.out.format("available => %d%n", bytesAvailable);
> 125: assertTrue(bytesAvailable == 0);
`assertEquals`?
test/jdk/java/net/Socket/ConnectionReset.java line 141:
> 139: int remaining = data.length;
> 140: for (int i=0; i<REPEAT_COUNT; i++) {
> 141: try {
Similar to above, would it be possible to refactor this to make it clearer
whether this is actually expected to throw an exception or just use
`assertNotEquals(-1, bytesRead)`?
test/jdk/java/net/Socket/UdpSocket.java line 46:
> 44: new Socket("doesnotmatter", 12345, false);
> 45: fail("Socket constructor was expected to throw
> IllegalArgumentException" +
> 46: " for stream=false, but didn't");
`assertThrows`?
test/jdk/java/net/Socket/UdpSocket.java line 55:
> 53: new Socket(InetAddress.getLoopbackAddress(), 12345, false);
> 54: fail("Socket constructor was expected to throw
> IllegalArgumentException" +
> 55: " for stream=false, but didn't");
`assertThrows`?
test/jdk/java/net/SocketImpl/SocketImplCombinations.java line 136:
> 134: Socket s = new Socket((SocketImpl) null) { };
> 135: try (s) {
> 136: assertTrue(getSocketImpl(s) == null);
`assertNull`?
(applies multiple times in this file; see other ` == ` checks)
test/jdk/java/net/SocketImpl/SocketImplCombinations.java line 153:
> 151: try (s) {
> 152: SocketImpl si = getSocketImpl(s);
> 153: assertTrue(si instanceof CustomSocketImpl);
`assertInstanceOf`?
(applies multiple times in this file; see other ` instanceof ` checks)
test/jdk/java/net/SocketImpl/SocketImplCombinations.java line 317:
> 315: serverSocketAccept(socket, (ss, s) -> {
> 316: assertTrue(isPlatformSocketImpl(getSocketImpl(ss)));
> 317: assertTrue(s == socket);
`assertSame`?
(applies multiple times in this file; see other ` == ` checks)
test/jdk/java/net/SocketImpl/SocketImplCombinations.java line 785:
> 783: int port = get(si, "port");
> 784: int localport = get(si, "localport");
> 785: assertTrue(fd.valid() && address != null && port != 0 &&
> localport != 0);
Split this into separate assertions to make it easier to troubleshoot test
failures?
test/jdk/java/net/SocketImpl/TestDefaultBehavior.java line 57:
> 55: assertEquals(0, csi.supportedOptions().size());
> 56:
> 57: Assertions.assertThrows(NPE, () -> csi.setOption(null, null));
Add static import for this?
-------------
PR Review: https://git.openjdk.org/jdk/pull/30564#pullrequestreview-4059474018
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036859627
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036870643
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036871652
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036872034
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036874995
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036876675
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036876302
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036878610
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036880772
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036880994
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036893723
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036905074
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036909962
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036915011
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3036916384
