RFR: 8303525: Refactor/cleanup open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java [v3]

[Stuart Marks]

On Mon, 16 Oct 2023 17:01:20 GMT, Matthew Donovan <mdonovan at openjdk.org> wrote:

>> This PR refactors the SSLSocketParametersTest by removing redundant/unnecessary classes and cleans up the logic around expected exceptions.
>
> Matthew Donovan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   retained a reference to the RMI server and improved naming

Nice cleanups here. I think investing a bit in the test library will be very helpful. It certainly improves this test, and it opens the possibility of cleaning up other tests as well.

test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java line 81:

> 79:                 0, new SslRMIClientSocketFactory(), serverSocketFactory);
> 80:         HelloClient client = new HelloClient();
> 81:         client.runClient(stub);

You could even inline HelloClient here, as I don't think that having a separate class is actually doing anything..

test/lib/jdk/test/lib/Asserts.java line 575:

> 573:      * @return The thrown exception.
> 574:      */
> 575:     public static Exception assertThrownException(Class<? extends Exception> expected, TestMethod testMethod) {

Cool, this is a good addition to the test library, to enable tests to use this style of assertion over testing without having to include JUnit or something. Here I would copy JUnit's `expectThrows` method signature:

https://github.com/junit-team/junit5/blob/main/junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertThrows.java#L41

That is, make this a generic method with a type variable that extends Throwable, and have the return type be this type variable. You'll probably need to adjust the `TestMethod` interface to throw Throwable. And I'd rename the method "assertThrows" as well.

The test library doesn't provide all the same overloads for messages as JUnit does. The prevailing style here is to provide an overload that takes a String message and one that doesn't, so following that seems wise.

test/lib/jdk/test/lib/Asserts.java line 580:

> 578:             fail("No exception was thrown. Expected: " + expected.getName());
> 579:         } catch (Exception exc) {
> 580:             if (expected.isAssignableFrom(exc.getClass())) {

Can use Class.isInstance() here.

test/lib/jdk/test/lib/Asserts.java line 584:

> 582:             } else {
> 583:                 fail("An unexpected exception was thrown. Expected: "
> 584:                         + expected.getName() + " Got: " + exc.getClass().getName());

Use the fail() overload that takes a Throwable.

test/lib/jdk/test/lib/Asserts.java line 588:

> 586:         }
> 587:         // fail() throws a RuntimeException so this is unreachable.
> 588:         return null;

Hm, this is unfortunate. Even though this method throws along all code paths, the compiler doesn't do this analysis. Even though this "never happens" there's still a question of "but what if?" (I guess it could happen if somebody in the future were to modify the code above.) I'd suggest unconditionally throwing an exception here (probably InternalError) to be more explicit. Returning null will result in an inexplicable NPE if somebody dereferences the return value.

-------------

PR Review: https://git.openjdk.org/jdk/pull/14932#pullrequestreview-1681110752
PR Review Comment: https://git.openjdk.org/jdk/pull/14932#discussion_r1361372680
PR Review Comment: https://git.openjdk.org/jdk/pull/14932#discussion_r1361365902
PR Review Comment: https://git.openjdk.org/jdk/pull/14932#discussion_r1361366051
PR Review Comment: https://git.openjdk.org/jdk/pull/14932#discussion_r1361366534
PR Review Comment: https://git.openjdk.org/jdk/pull/14932#discussion_r1361369417