On Tue, 31 Mar 2026 06:29:57 GMT, Alan Bateman <[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 77:
>
>> 75: @ParameterizedTest
>> 76: @MethodSource("testCases")
>> 77: public void testConnect(DatagramSocket ds, InetAddress addr) {
>
> What would you think about using `try (ds) { .. }` so that it's clear that
> the DatagramSocket will be closed.
I can do it but it's not necessary since JUnit will close it anyway. But yes -
since close() is supposed to be a NOOP the second time around I can do this.
> test/jdk/java/net/DatagramSocket/DatagramTimeout.java line 57:
>
>> 55: public static List<DatagramSocket> sockets() throws IOException {
>> 56: // Closeable arguments passed to a ParameterizedTest are
>> automatically
>> 57: // closed by JUnit
>
> I personally don't like to rely on that feature, only because it's a bit too
> magic.
>
> Another argument against this is that DatagramSocket is a mutable object so a
> test is free to set socket options, bind, or close the socket, leaving it in
> a state that is not suitable for another test. So I think better to return a
> newly minted DatagramSocket for each test.
@AlanBateman that is what is happening here.
I can add the additional try-with-resource to the test method, but then it
gives the impression that the socket will be left open if not closed, and it's
not.
> test/jdk/java/net/DatagramSocket/OldDatagramSocketImplTest.java line 57:
>
>> 55: } catch (SocketException ex) {
>> 56: assertEquals("connect not implemented", ex.getMessage());
>> 57: System.out.println("PASSED: default implementation of
>> connect has thrown as expected");
>
> Is this trace message needed? I'm reminded that our the JUnit output goes to
> System.err whereas TestNG output goes to System.out, so any trace messages to
> System.out will no longer be inlined.
I like to keep both - System.err can become quite cluttered at times.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016141838
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016190942
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016209355
