On Tue, 31 Mar 2026 18:13:35 GMT, Marcono1234 <[email protected]> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Apply suggestions from code review >> >> Co-authored-by: Marcono1234 <[email protected]> > > 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? The test is simple enough. If we had one platform where it behave differently (say one platform where 65536 would be considered valid), then having a parameterized test where you could continue testing for the next value in the list after the first failure would make sense, but at this point I would not bother. Thanks for the suggestion though! > 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? IIUC we just want to perform some of the test with the port of a real opened UDP socket to avoid any kind of exception related to trying to send to a port that the system could consider unreachable. The same can be reused for all tests. The only constraint is that the socket remains opens for the duration of the test. > test/jdk/java/net/DatagramSocket/SendCheck.java line 165: > >> 163: + sender + " / " + packet, e); >> 164: } >> 165: } > > Could use `assertDoesNotThrow`? Good point. > 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`? It was not statically imported before, but the changeset I will push later has the change. > 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`? Same. Will be in next commit. > 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)`? Thanks for the suggestion. I believe this is historical and due to the old native implementation of DatagramSocket on windows. IIRC TwoStackPlainDatagramSocketImpl / DualPlainDatagramSocketImpl have been removed since JDK 18 so we can probably make the assertion on all platforms now. But I'd prefer to verify this and handle that as a separate issue. I have logged [JDK-8381474](https://bugs.openjdk.org/browse/JDK-8381474) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021627639 PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021663162 PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021676218 PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021686807 PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021688557 PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021746796
