On Sun, 5 Apr 2026 13:22:34 GMT, Marcono1234 <[email protected]> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review feedback
>
> test/jdk/java/net/MulticastSocket/SendPortZero.java line 88:
>
>> 86: { new MulticastSocket(), wildcardZeroPkt },
>> 87: // Not currently tested. See JDK-8236807
>> 88: //{ new MulticastSocket(), wildcardValidPkt }
>
> Is this still correct? Previously it used the same `multicastSocket` here.
> Now `wildcardValidPkt` uses the port of the `multicastSocket` but a `new
> MulticastSocket()` is passed as socket.
>
> (Though it is commented out, so if this is an issue it would probably
> resurface for JDK-8236807.)
I believe the test just needs a valid port where a socket is actually bound.
> test/jdk/java/net/MulticastSocket/SetLoopbackOption.java line 303:
>
>> 301: public static void main (String args[]) throws Exception {
>> 302: new SetLoopbackOption().run();
>> 303: }
>
> Is it still worth keeping this `main` method?
>
> I guess now it might still work, but if any more advanced JUnit functionality
> is used, such as `@AfterAll`, then this `main` method would behave
> incorrectly.
It was here before - so I decided to keep it.
> test/jdk/java/net/Socket/ConnectionReset.java line 55:
>
>> 53: int bytesAvailable = in.available();
>> 54: System.out.format("available => %d%n", bytesAvailable);
>> 55: assertTrue(bytesAvailable == 0);
>
> `assertEquals`?
Done
> test/jdk/java/net/Socket/ConnectionReset.java line 66:
>
>> 64: } catch (IOException ioe) {
>> 65: System.out.format("read => %s (expected)%n", ioe);
>> 66: }
>
> `assertThrows`?
Done
> test/jdk/java/net/Socket/ConnectionReset.java line 85:
>
>> 83: System.out.format("available => %d%n", bytesAvailable);
>> 84: assertTrue(bytesAvailable <= remaining);
>> 85: try {
>
> Try to refactor this if possible? It is not clear if this is actually an
> `assertThrows` or whether it should just have a `assertNotEquals(-1,
> bytesRead);`.
I have refactored - and added some more comments
> test/jdk/java/net/Socket/ConnectionReset.java line 122:
>
>> 120: } catch (IOException ioe) {
>> 121: System.out.format("read => %s (expected)%n", ioe);
>> 122: }
>
> `assertThrows`?
Done
> test/jdk/java/net/Socket/ConnectionReset.java line 125:
>
>> 123: int bytesAvailable = in.available();
>> 124: System.out.format("available => %d%n", bytesAvailable);
>> 125: assertTrue(bytesAvailable == 0);
>
> `assertEquals`?
Done
> test/jdk/java/net/Socket/ConnectionReset.java line 141:
>
>> 139: int remaining = data.length;
>> 140: for (int i=0; i<REPEAT_COUNT; i++) {
>> 141: try {
>
> Similar to above, would it be possible to refactor this to make it clearer
> whether this is actually expected to throw an exception or just use
> `assertNotEquals(-1, bytesRead)`?
Done
> test/jdk/java/net/Socket/UdpSocket.java line 46:
>
>> 44: new Socket("doesnotmatter", 12345, false);
>> 45: fail("Socket constructor was expected to throw
>> IllegalArgumentException" +
>> 46: " for stream=false, but didn't");
>
> `assertThrows`?
I think it's fine as it is. No need to complicate things just for the purpose
of using assertThrows.
> test/jdk/java/net/Socket/UdpSocket.java line 55:
>
>> 53: new Socket(InetAddress.getLoopbackAddress(), 12345, false);
>> 54: fail("Socket constructor was expected to throw
>> IllegalArgumentException" +
>> 55: " for stream=false, but didn't");
>
> `assertThrows`?
Same here
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045392065
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045399065
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045400825
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045401914
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045404618
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045407155
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045406211
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045408158
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045414816
PR Review Comment: https://git.openjdk.org/jdk/pull/30564#discussion_r3045417228
