On Fri, 10 Apr 2026 17:41:34 GMT, Daniel Fuchs <[email protected]> wrote:
> Hi, may I get a review for this change that converts the remaining TestNG > tests under test/jdk/java/net to JUnit. > > An exception is `java/net/NetworkInterface/NetworkInterfaceStreamTest.java` > which depends on library classes depending on TestNG. Converting that test is > not done here but is tracked by > [JDK-8381848](https://bugs.openjdk.org/browse/JDK-8381848) > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). test/jdk/java/net/DatagramPacket/Setters.java line 54: > 52: // No Exception expected for null addresses > 53: pkt.setAddress(null); > 54: assertTrue(pkt.getAddress() == null); `assertNull`? test/jdk/java/net/InetAddress/HostsFileOrderingTest.java line 95: > 93: String [] expectedAddresses = getExpectedAddressesArray(); > 94: > 95: if (Arrays.deepEquals(resolvedAddresses, expectedAddresses)) { Could use `assertArrayEquals`? (unless you want to keep the custom error messages) test/jdk/java/net/InetSocketAddress/ToString.java line 88: > 86: > 87: @Test > 88: // InetSocketAddress.toString() throws NPE with unresolved address This comment is wrong / misleading; it was the behavior before https://bugs.openjdk.org/browse/JDK-4464064 was fixed Maybe would also be good to rename the method then from `NPETest` to `unresolvedTest` or similar? test/jdk/java/net/InetSocketAddress/ToString.java line 90: > 88: // InetSocketAddress.toString() throws NPE with unresolved address > 89: public void NPETest() { > 90: System.out.println(new InetSocketAddress("unresolved", 12345)); Replace with `assertDoesNotThrow` / `assertNotNull`? test/jdk/java/net/InetSocketAddress/ToString.java line 93: > 91: } > 92: > 93: public static Object[][] createArgs1() { Would it make sense to apply the provider name as method name (i.e. `hostPortArgs`) to make it more expressive? (same also for the other argument methods here) test/jdk/java/net/InetSocketAddress/ToString.java line 130: > 128: } catch (UnknownHostException uhe) { > 129: throw new RuntimeException("Data provider creation failed: " > + uhe, uhe); > 130: } Could probably omit this and just add a `throws Exception` to the method signature. test/jdk/java/net/SocketOption/ImmutableOptions.java line 70: > 68: assertThrows(UnsupportedOperationException.class, > 69: () -> socket.supportedOptions().clear()); > 70: } Here and below, what about moving the `supportedOptions()` call outside the `assertThrows` to ensure that it doesn't unexpectedly throw the `UnsupportedOperationException`? test/jdk/java/net/Socks/SocksIPv6Test.java line 63: > 61: > 62: static private HttpServer server; > 63: static private SocksServer socks; Should be `private static` instead of `static private` to be conventional? test/jdk/java/net/Socks/SocksIPv6Test.java line 64: > 62: static private HttpServer server; > 63: static private SocksServer socks; > 64: private static String response = "Hello."; Can be `final`? test/jdk/java/net/Socks/SocksSocketImplTest.java line 83: > 81: // expected > 82: // now verify the IOE was thrown for the correct expected > reason > 83: if (!(ioe.getCause() instanceof IllegalArgumentException)) { Use `assertThrows`? E.g.: var e = assertThrows(IOException.class, ...); assertInstanceOf(IllegalArgumentException.class, e.getCause()); That would also make it clearer which of the calls is expected to throw the `IOException`. test/jdk/java/net/UnixDomainSocketAddress/LengthTest.java line 50: > 48: public static List<String> strings() { > 49: if (namelen == -1) > 50: return List.of(""); Remove this `namelen == -1` check because it is always false? test/jdk/java/net/UnixDomainSocketAddress/LengthTest.java line 56: > 54: new String(new char[100]).replaceAll("\0", "x"), > 55: new String(new char[namelen]).replaceAll("\0", "x"), > 56: new String(new char[namelen-1]).replaceAll("\0", "x") Can simplify these? Suggestion: "x".repeat(100), "x".repeat(namelen), "x".repeat(namelen - 1) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066602731 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066617343 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066637453 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066638634 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066647831 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066650856 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066709378 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066749708 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066752479 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066778911 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066795115 PR Review Comment: https://git.openjdk.org/jdk/pull/30681#discussion_r3066799146
