On Mon, 30 Mar 2026 18:08:24 GMT, Daniel Fuchs <[email protected]> wrote:
> May I get a review for this change that converts `java/net/DatagramSocket`
> TestNG tests to use JUnit.
>
> Some of these tests now take advantage from the fact that Closeable arguments
> passed to a ParameterizedTest are automatically closed by JUnit.
These review comments are only suggestions; hopefully they are useful but feel
free to ignore them if they are not.
test/jdk/java/net/DatagramSocket/Constructor.java line 57:
> 55:
> 56: @Test
> 57: public void testInvalidPortRange() {
Would it be beneficial to use `@ParameterizedTest @ValueSource(ints = { ... })`
here instead of the `for` loop over the invalid values?
Or do you think that is not worth it?
test/jdk/java/net/DatagramSocket/SendCheck.java line 60:
> 58:
> 59: static final byte[] buf = {0, 1, 2};
> 60: static DatagramSocket socket;
What is `socket` used for; is it only referenced in the commented out code
section? (or did I overlook something?)
Could be a problem that this mutable object is reused for all test runs?
test/jdk/java/net/DatagramSocket/SendCheck.java line 69:
> 67: throw new ExceptionInInitializerError(e);
> 68: }
> 69: }
Simplify this?
Suggestion:
@BeforeAll
public static void setUp() throws Exception {
socket = new DatagramSocket();
}
test/jdk/java/net/DatagramSocket/SendCheck.java line 165:
> 163: + sender + " / " + packet, e);
> 164: }
> 165: }
Could use `assertDoesNotThrow`?
test/jdk/java/net/DatagramSocket/SendReceiveMaxSize.java line 176:
> 174:
> 175: if (exception != null) {
> 176: Exception ex = Assertions.assertThrows(IOE, () ->
> sender.send(sendPkt));
Should this actually test for the specified exception class? (though currently
it seems that is always `IOE`)
Suggestion:
Exception ex = Assertions.assertThrows(exception, () ->
sender.send(sendPkt));
test/jdk/java/net/DatagramSocket/SendReceiveMaxSize.java line 176:
> 174:
> 175: if (exception != null) {
> 176: Exception ex = Assertions.assertThrows(IOE, () ->
> sender.send(sendPkt));
Out of curiosity, why no static import for `assertThrows`?
test/jdk/java/net/DatagramSocket/SendReceiveMaxSize.java line 185:
> 183: // check packet data has been fragmented and
> re-assembled correctly at receiver
> 184: assertEquals(capacity, receivePkt.getLength());
> 185: Assertions.assertArrayEquals(testData,
> receivePkt.getData());
Why no static import for `assertArrayEquals`?
test/jdk/java/net/DatagramSocket/SupportedOptionsCheck.java line 56:
> 54: if (!Platform.isWindows())
> 55: assertTrue(options.containsAll(multicastOptions));
> 56: }
This seems to be the only assertion, so would it be better to use
`assumeFalse(Platform.isWindows())` at the beginning of the method instead, or
JUnit's `@DisabledOnOs(OS.WINDOWS)`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/30502#pullrequestreview-4039200833
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017529063
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017587235
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017560369
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017572787
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017733189
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017734663
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017735721
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3017806599
