On Tue, 31 Mar 2026 14:03:02 GMT, Daniel Fuchs <[email protected]> wrote:
>> 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.
I will push a changeset later that does 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.
I will push a change later where the test uses try-with-resource. We don't care
if JUnit calls close again.
>> 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.
I have revised my opinion. I will remove these traces they don't log anything
interesting.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021609980
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021633093
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3021644591
