On Thu, 21 Nov 2024 21:50:24 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
>> Volkan Yazıcı has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Handle `SocketTimeoutException` in `NioSocketImpl::connect()`
>
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 128:
>
>> 126: @MethodSource("ctor_should_close_on_failures")
>> 127: @SuppressWarnings("resource")
>> 128: void ctor_should_close_on_failures(TestCase testCase) throws
>> Throwable {
>
> Is the style convention for Java code to use camel case for all classes,
> methods and variables ?
Switched back to camel-case in e37ee840b9681aa017f8653df68c69232a6b6c83.
(Snake-case convention for test method names is something I adopted several
years ago from other projects, since it was helping with making the failure
output more human-readable. But I'd be more than happy to stick to the
established convention in the code base.)
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 149:
>
>> 147:
>> 148: }
>> 149:
>
> static List<TestCase> ctorCloseOnFailuresTestCases() {
> return List.of(
> TestCase.BindFailureFactory.ioExceptionTestCase(),
> TestCase.ConnectFailureFactory.ioExceptionTestCase(),
>
> TestCase.ConnectFailureFactory.illegalArgumentExceptionTestCase(1));
> }
>
> c.f. name change suggestion below
Applied this suggestion in e37ee840b9681aa017f8653df68c69232a6b6c83.
> test/jdk/java/net/Socket/CloseOnFailureTest.java line 210:
>
>> 208: private static final String ERROR_MESSAGE = "intentional test
>> failure";
>> 209:
>> 210: private static final class ForBindFailure {
>
> ForConnectionFailure ForBindfFailure are factories abstraction
> Their method are factory methods (the world is addicted to of methods)
> fabricating a TestCase
>
> ofIOException
> ofIllegalArgumentException
> ofIOException()
>
> A simple change to ConnectionFailureFactory and BindFailureFactory
> With methods like ioExceptionTestCase illegalArgumentException
>
> Lead to readable code like
>
> TestCase.ConnectionFailureFactory.ioExceptionTestCase()
>
> TestCase.BindFaiureFactory.illegalExceptionTestCase()
Applied your suggestions in e37ee840b9681aa017f8653df68c69232a6b6c83.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1853602370
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1853604475
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1853603172
