RFR: 8367096: jdk/open/test/jdk/sun/security/pkcs11/ rsa, ec, config, secmod and sslecc tests are skipping but showing as pass

[Matthew Donovan]

On Tue, 9 Sep 2025 09:02:03 GMT, Mikhail Yankelevich <myankelevich at openjdk.org> wrote:

> Tests changed:  
> * test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java
> * test/jdk/sun/security/pkcs11/PKCS11Test.java
> * test/jdk/sun/security/pkcs11/Secmod/AddPrivateKey.java
> * test/jdk/sun/security/pkcs11/Secmod/AddTrustedCert.java
> * test/jdk/sun/security/pkcs11/Secmod/Crypto.java
> * test/jdk/sun/security/pkcs11/Secmod/GetPrivateKey.java
> * test/jdk/sun/security/pkcs11/Secmod/JksSetPrivateKey.java
> * test/jdk/sun/security/pkcs11/Secmod/LoadKeystore.java
> * test/jdk/sun/security/pkcs11/Secmod/TestNssDbSqlite.java
> * test/jdk/sun/security/pkcs11/Secmod/TrustAnchors.java
> * test/jdk/sun/security/pkcs11/SecmodTest.java
> * test/jdk/sun/security/pkcs11/ec/ReadCertificates.java
> * test/jdk/sun/security/pkcs11/ec/ReadPKCS12.java
> * test/jdk/sun/security/pkcs11/ec/TestKeyFactory.java
> * test/jdk/sun/security/pkcs11/rsa/KeyWrap.java
> * test/jdk/sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java

Looks good to me, just some minor suggestions on the exception messages. Don't feel like you have to change them if you like them as-is.

test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java line 51:

> 49:             Provider p = Security.getProvider("SunPKCS11");
> 50:             if (p == null) {
> 51:                 throw new SkippedException("Skipping test - no PKCS11 provider available");

minor wording nit: you don't need to say "skipping test" in the message of a SkippedException.

test/jdk/sun/security/pkcs11/PKCS11Test.java line 784:

> 782:     private void premain(Provider p) throws Exception {
> 783:         if (skipTest(p)) {
> 784:             throw new SkippedException("Test is skipped due to skipTest() result, please see the log");

maybe the message could just say "See logs for details"? Ideally, I think we'd just refactor `skipTest()` into something like `checkSkippedTest()` and the method itself threw the SkippedException but that would require a lot of refactoring.

test/jdk/sun/security/pkcs11/ec/ReadCertificates.java line 82:

> 80:     public void main(Provider p) throws Exception {
> 81:         if (p.getService("Signature", "SHA1withECDSA") == null) {
> 82:             throw new SkippedException("Provider does not support ECDSA, skipping...");

here (and the other ec/, rsa/, sslecc/ tests), you could remove the "skipping..." from the message.

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

PR Review: https://git.openjdk.org/jdk/pull/27166#pullrequestreview-3200992297
PR Review Comment: https://git.openjdk.org/jdk/pull/27166#discussion_r2333143784
PR Review Comment: https://git.openjdk.org/jdk/pull/27166#discussion_r2333150305
PR Review Comment: https://git.openjdk.org/jdk/pull/27166#discussion_r2333155959