On Fri, 22 Nov 2024 21:08:11 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improve naming in tests
>
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 59:
>
>> 57:
>> 58: private static final VarHandle SOCKET_IMPL_FACTORY_HANDLE =
>> createSocketImplFactoryHandle();
>> 59:
>
> private static final int DEADSERVERPORT = 0xDEAD;
Fixed in 35d680123f83b97b2a62fe99969aa0c18a75e6b1.
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 84:
>
>> 82:
>> 83: private final AtomicInteger closeInvocationCounter = new
>> AtomicInteger(0);
>> 84:
>
> replacing Error with Exception in the variable names allows for easier and
> quicker scanning of a line
>
> private final Exception bindException;
>
> private final Exception connectException;
>
> private MockSocketImpl(Exception bindException, Exception
> connectException) {
> this.bindException = bindException;
> this.connectException = connectException;
> }
>
> and this replacement suggestion propagates down through the rest of the code
>
>
> ```
> Throwable
> |
> | IS_A
> |
> Error Exception
>
>
> avoid Error in the variable name avoid the subconscious association with an
> Error class
Replaced all occurrences of `error` in variable names with `exception` in
35d680123f83b97b2a62fe99969aa0c18a75e6b1.
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 140:
>
>> 138: InetAddress serverAddress =
>> InetAddress.getLoopbackAddress();
>> 139: int deadServerPort = 0xDEAD;
>> 140: new Socket(serverAddress, deadServerPort, null, 0);
>
> new Socket(serverAddress, DEADSERVERPORT, null, 0); // DEADSERVERPORT
> declared as a constant above
Fixed in 35d680123f83b97b2a62fe99969aa0c18a75e6b1.
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 162:
>
>> 160: MockSocketImpl socketImpl = new MockSocketImpl(null, null);
>> 161: try (Socket socket = new Socket(socketImpl) {}) {
>> 162: InetSocketAddress address =
>> InetSocketAddress.createUnresolved("no.such.host", 0xBEEF);
>
> similar to DEADSERVERPORT
>
> specifying a hex value, albeit a joke value, it is still a valid ephemeral
> port, and distracts from the main invocation InetAddress.createUnresolved, so
> consider a constant declaration for the port
>
> private static final int NOSUCHHOSTPORT = 0xBEEF;
>
> then the invocation is
> InetAddress.createUnresolved("no.such.host", NOSUCHHOSTPORT);
>
> presenting an unambiguous statement that the InetAddress not meant to be
> reachable !
Reused the `DEAD_SERVER_PORT` constant here in
35d680123f83b97b2a62fe99969aa0c18a75e6b1.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856214817
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856215001
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856214705
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1856215137
