On Tue, 19 Mar 2024 16:09:18 GMT, Aleksei Efimov <aefi...@openjdk.org> wrote:
>> Christoph Langer 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 12 additional >> commits since the last revision: >> >> - Update module-info text >> - Merge branch 'master' into JDK-8325579 >> - Indentation >> - Merge branch 'master' into JDK-8325579 >> - Review feedback >> - Rename back to LdapSSLHandshakeFailureTest to ease reviewing >> - Merge branch 'master' into JDK-8325579 >> - Typo >> - Merge branch 'master' into JDK-8325579 >> - Rename test and refine comment >> - ... and 2 more: https://git.openjdk.org/jdk/compare/ffdffc5b...10271159 > > test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 164: > >> 162: if >> (CustomSocketFactory.customSocket.closeMethodCalledCount() <= 0) { >> 163: System.err.println("Custom Socket was not >> closed."); >> 164: System.exit(-1); > > Can we update test not to use `System.exit` and replace it with throwing `new > RuntimeException`, something like: > Suggestion: > > throw new RuntimeException("Custom Socket was not > closed"); Done. > test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 177: > >> 175: ctx.close(); >> 176: if (!checkSocketClosed(sock)) { >> 177: System.exit(-1); > > Can be replaced with: > Suggestion: > > throw new RuntimeException("Socket isn't closed"); I simplified this, removing checkSocketClosed() method > test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 184: > >> 182: e.printStackTrace(); >> 183: System.exit(-1); >> 184: } > > For simplification and `System.exit` removal purposes this catch block can be > removed with addition of `throws Exception` clause to the `main` method. > Suggestion: > > } I chose to throw a new RuntimeException(e) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532883964 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532885041 PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532884603
