On Tue, 21 Feb 2023 19:06:02 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> Matthew Donovan has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - added System.exit when exceptions are thrown and refactored for clarity >> - Merge branch 'master' into rmi-sslparams >> - added default switch case and additional refactoring >> - added exceptions for cases 4 and 5 >> - clarified cases 4 and 5 >> - 8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh >> to jtreg java test > > test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java line 221: > >> 219: } >> 220: } >> 221: } > > Overall this test method makes sense, as it asserts that an > IllegalArgumentException must be thrown with the given exception message. (A > comment to this effect would be helpful.) As such, the catch of > NoSuchAlgorithmException is confusing. I think this is here because > SSLContext.getDefault() lists this as a checked exception. If so, then I'd > suggest simply propagating it by adding a `throws` clause for this method. > This also means adding a `throws` clause to the `runTest()` method, possibly > just `throws Exception`. In general this is OK for jtreg tests to propagate > any checked exception out of main, since jtreg will handle and report any > unexpectedly propagated exceptions. It's thus unnecessary for test code to > have catch-clauses for anything that the test isn't actually handling. The exception handling has been cleaned up here and the other place you pointed out. ------------- PR: https://git.openjdk.org/jdk/pull/11910
