RFR: 8249825: Tests sun/security/ssl/SSLSocketImpl/SetClientMode.java and NonAutoClose.java marked with @ignore

[Matthew Donovan]

On Tue, 4 Mar 2025 13:07:37 GMT, Fernando Guallini <fguallini at openjdk.org> wrote:

> The following tests are marked with @ignore (not running):
> 
> - sun/security/ssl/SSLSocketImpl/SetClientMode.java: it checks that setting the clientMode after the handshake has begun is not permitted, but this was failing intermittently due to a race condition, it was possible that SetClientMode was called before the client socket was updated with handshake isNegotiated = true. The fix is to introduce a latch to sync between client and main threads. Included additional refactoring to ensure test stability.
> 
> - sun/security/ssl/SSLSocketImpl/NonAutoClose.java: This test should only run in TLS <= 1.2, as TLSv1.3 changes the behaviour of close_notify. Included additional refactoring to ensure test stability.
> 
> Executed both tests 10K times, no test flakiness found

test/jdk/sun/security/ssl/SSLSocketImpl/NonAutoClose.java line 58:

> 56:      */
> 57:     private final static String pathToStores = "../../../../javax/net/ssl/etc";
> 58:     private final static String keyStoreFile = "keystore";

This isn't in the changeset, but we're trying to move away from the binary {key|trust}store files. We have two template classes, SSLContextTemplate and SSLSocketTemplate, that set up key and trust stores and does the context setup. Can you take a look and see if one of them can be used here?

test/jdk/sun/security/ssl/SSLSocketImpl/NonAutoClose.java line 70:

> 68:      * Turn on SSL debugging?
> 69:      */
> 70:     private final static boolean DEBUG = false;

I know this isn't technically in the change set but when I see this I change it to `DEBUG = Boolean.getBoolean("test.debug")` so the flag can be toggled without rebuilding the code.

test/jdk/sun/security/ssl/SSLSocketImpl/NonAutoClose.java line 77:

> 75:     private final static int TLS_CLIENT_VAL = 4;
> 76: 
> 77:     private void expectValue(int got, int expected, String msg) throws IOException {

Can you just use the Asserts class instead?

test/jdk/sun/security/ssl/SSLSocketImpl/SetClientMode.java line 66:

> 64:      * Where do we find the keystores?
> 65:      */
> 66:     private final static String pathToStores = "../../../../javax/net/ssl/etc";

Could you use either SSLContextTemplate or SSLSocketTemplate for this test?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r1981466359
PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r1981448111
PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r1981443329
PR Review Comment: https://git.openjdk.org/jdk/pull/23898#discussion_r1981542780