On Mon, 27 Feb 2023 14:04:42 GMT, Matthew Donovan <d...@openjdk.org> wrote:
>> Removed SSLSocketParametersTest.sh script (which just called a Java file)
>> and configured the java code to run directly with jtreg
>
> Matthew Donovan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> cleaned up exception handling
Marked as reviewed by smarks (Reviewer).
OK, I think this is pretty good as it stands. It achieves the original goal of
converting the shell script test to a regular Java jtreg test; the shell script
is removed entirely, and the Java code is cleaned up and is 20% shorter.
Hooray! So I think this can go in as it stands.
Optionally, if you're game to continue working on this, there are some
additional cleanups that can be done. This is now digging into the Java code
that was already in the test before you got involved. Specifically:
1) The `HelloImpl` and `HelloClient` classes aren't adding much value and can
pretty much be deleted if the object is exported with something like:
Hello stub = (Hello) UnicastRemoteObject.exportObject(new HelloImpl(),
port, csf, ssf);
or similar.
2) The `ClientFactory` and `ServerFactory` classes aren't adding much value.
They override the create methods and print a message, but otherwise don't do
anything beyond what's done in their respective superclasses.
3) The exception handling is now fairly simple, but one still needs to read
through the code to figure out what is actually being tested. The
`expectedException` boolean also makes things a little harder to read since it
can invert the logic. I observe that the various test frameworks (such as
Test-NG or JUnit) have APIs for this such as `assertThrows` or `expectThrows`
which return the caught exception, allowing additional assertions to be made
over it, and failing the test if the expected exception type isn't thrown. I
think this would improve the test cases where an exception is expected, but it
might not be worth the effort of converting to one of the frameworks.
It's up to you (and your project lead) as to how to proceed. You could continue
here, or in another PR, or just consider this done and move onto the next thing.
-------------
PR: https://git.openjdk.org/jdk/pull/11910
