RFR: 8366159: SkippedException is treated as a pass for pkcs11/KeyStore, pkcs11/SecretKeyFactory and pkcs11/SecureRandom

[Mikhail Yankelevich]

On Tue, 26 Aug 2025 12:45:46 GMT, Francesco Andreuzzi <duke at openjdk.org> wrote:

>> Skipped tests are treated as a pass:
>> 
>> * test/jdk/sun/security/pkcs11/KeyStore/ClientAuth.java
>> * test/jdk/sun/security/pkcs11/KeyStore/CertChainRemoval.java
>> * test/jdk/sun/security/pkcs11/SecretKeyFactory/TestGeneral.java
>> * test/jdk/sun/security/pkcs11/SecureRandom/Basic.java
>> * test/jdk/sun/security/pkcs11/SecureRandom/TestDeserialization.java
>
> test/jdk/sun/security/pkcs11/KeyStore/CertChainRemoval.java line 130:
> 
>> 128:             printKeyStore("Initial PKCS11 KeyStore: ", p11ks);
>> 129:         } catch (Exception e) {
>> 130:             throw new SkippedException("Skip test, due to " + e);
> 
> Maybe `e` can be specified as a real `cause` now?

I might be misunderstanding your point, but I think in this case a full exception stack trace would be more helpful. It covers everything this way.

> test/jdk/sun/security/pkcs11/SecretKeyFactory/TestGeneral.java line 125:
> 
>> 123:             test("Blowfish", bf_128Key, p, TestResult.PASS);
>> 124:         } catch (SkippedException skippedException){
>> 125:             throw new SkippedException("One or more tests skipped");
> 
> I'd attach `skippedException` as the `cause` here

Skips are logged above for each of the tests. I wanted to avoid changing the logic if possible.

> test/jdk/sun/security/pkcs11/SecureRandom/Basic.java line 50:
> 
>> 48:         } catch (NoSuchAlgorithmException e) {
>> 49:             e.printStackTrace();
>> 50:             throw new SkippedException("Provider " + p + " does not support SecureRandom, skipping");
> 
> If you add `e` as the cause here, you can probably get rid of the explicit `e.printStackTrace()`

It is set there on purpose. This way it will log to the `System.out` and not `System.err`. Just easier to spot this way. I can add `e` into skipped exception, but it seems to be a bit of an overkill. What do you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26942#discussion_r2301160109
PR Review Comment: https://git.openjdk.org/jdk/pull/26942#discussion_r2301149313
PR Review Comment: https://git.openjdk.org/jdk/pull/26942#discussion_r2301154694