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.
test/jdk/java/net/DatagramSocket/ConnectPortZero.java line 48:
> 46: * @summary Check that DatagramSocket, MulticastSocket and
> DatagramSocketAdaptor
> 47: * throw expected Exception when connecting to port 0
> 48: * @run junit/othervm ConnectPortZero
Maybe this is a good occasion to amend `@run` lines with `${test.main.class}`.
test/jdk/java/net/DatagramSocket/ConnectPortZero.java line 62:
> 60: }
> 61:
> 62: public static List<Arguments> testCases() throws IOException {
You don't need to switch from `Object[][]` to `List<Arguments>`, both would
work, FYI.
[Note that this comment applies to more places.]
test/jdk/java/net/DatagramSocket/ConnectPortZero.java line 78:
> 76: @MethodSource("testCases")
> 77: public void testConnect(DatagramSocket ds, InetAddress addr) {
> 78: assertFalse(ds.isConnected());
I see more than a TestNG-to-JUnit migration. I'd be reluctant on piggybacking
such changes along with already loaded TestNG-to-JUnit migration changes. That
said, I trust your judgement on the matter. I'm assuming you've considered this
and decided to keep these improvements.
[Note that this comment applies to more places.]
test/jdk/java/net/DatagramSocket/OldDatagramSocketImplTest.java line 53:
> 51: public void testOldImplConnect() {
> 52: try (var ds = new DatagramSocket(new OldDatagramSocketImpl()) {})
> {
> 53: ds.connect(new InetSocketAddress(LOOPBACK, 6667));
Shouldn't we rather wrap this in `assertThrows`?
test/jdk/java/net/DatagramSocket/SendPortZero.java line 105:
> 103: }
> 104:
> 105: @ParameterizedTest(autoCloseArguments = false) // closed in tearDown
Curious: Can we instead remove `tearDown()`?
test/jdk/java/net/DatagramSocket/SetGetSendBufferSize.java line 73:
> 71: try (var socket = supplier.open()) {
> 72: for (int i : invalidArgs) {
> 73: Exception ex = Assertions.assertThrows(IAE, () ->
> socket.setSendBufferSize(i));
You might want to `import static ...Assertions.assertThrows` too, as we did for
other `Assertions` methods.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3011734258
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3011707677
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3011720126
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3011746278
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3011757066
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3011763723
