On Mon, 16 Oct 2023 17:01:20 GMT, Matthew Donovan <mdono...@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
