On Mon, 30 Mar 2026 19:18:13 GMT, Volkan Yazici <[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 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.]
I had surprising results when running converted tests the first time around
because of JUnit autoclosing arguments of type Closeable. So I want to make
sure that whatever argument is passed here is not something that has already
been closed or connected by another test. Let's call it sanity. It doesn't
change what the test is testing, just makes sure that assumptions about
invariants are accurate.
> 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`?
Good point. Not sure why I missed that one.
> test/jdk/java/net/DatagramSocket/SendPortZero.java line 105:
>
>> 103: }
>> 104:
>> 105: @ParameterizedTest(autoCloseArguments = false) // closed in tearDown
>
> Curious: Can we instead remove `tearDown()`?
I considered it and rejected it. In this particular case it's much simpler this
way.
> 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.
Good point.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016170392
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016208589
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016223565
PR Review Comment: https://git.openjdk.org/jdk/pull/30502#discussion_r3016227832
