RFR: 8368524: Tests are skipped and shown as passed in test/jdk/sun/security/pkcs11/Cipher/KeyWrap

[Mikhail Yankelevich]

On Thu, 16 Oct 2025 13:52:58 GMT, Neha Joshi <duke at openjdk.org> wrote:

> This PR contain changes to handle skipped exception for below test cases. 
> 
> * test/jdk/sun/security/pkcs11/Cipher/KeyWrap/XMLEncKAT.java
> * test/jdk/sun/security/pkcs11/Cipher/KeyWrap/TestGeneral.java
> * test/jdk/sun/security/pkcs11/Cipher/KeyWrap/NISTWrapKAT.java

Looks good, just a few questions

Looks good, just 2 questions

Marked as reviewed by myankelevich (Committer).

Looks good to me

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/NISTWrapKAT.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved.

Nit: Please update to copyright to 
` * Copyright (c) 2021, 2025, Oracle and/or its affiliates. All rights reserved.`

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/NISTWrapKAT.java line 37:

> 35: import java.security.AlgorithmParameters;
> 36: import java.security.Provider;
> 37: import javax.crypto.*;

nit: wildcard imports

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/NISTWrapKAT.java line 49:

> 47: public class NISTWrapKAT extends PKCS11Test {
> 48: 
> 49:     private static List<String> skippedAlgoList = new ArrayList<>();

nit: 
```suggestion 
    private static final List<String> skippedAlgoList = new ArrayList<>();

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/NISTWrapKAT.java line 325:

> 323:         int allowed = Cipher.getMaxAllowedKeyLength("AES");
> 324:         if (keyLen > allowed) {
> 325:             throw new SkippedException("Skip, exceeds max allowed size " + allowed);

I think it's better to keep this as is, as in case this throws skipped exception it will stop the rest of the tests and they will not run. This could cause the error to be thrown. 

One of the options that come to mind is to have the line 399 `testEnc(algo, (String)td[1], (int)td[2], (String)td[3],` catch skipped exception and add to the `skippedAlgoList`. 
Another option could be to pass boolean/status out the method and use it to check if test actually run. 
Another option could be to keep the skipped list as a global var and append it from here directly.

These are not the only options, so if you think of something better - it's your call.

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/NISTWrapKAT.java line 415:

> 413:             System.out.println("All Tests Passed");
> 414:         } else {
> 415:             System.err.println("Some tests were skipped : " + skippedAlgoList);

This will only log a skip this way. Please throw an exception in the end.

```suggestion 
            throw new SkippedException("Some tests were skipped : " + skippedAlgoList);

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/TestGeneral.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved.

Nit: copyright

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/XMLEncKAT.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.

nit: copyright

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/XMLEncKAT.java line 37:

> 35: import java.security.AlgorithmParameters;
> 36: import java.security.Provider;
> 37: import javax.crypto.*;

minor: could you please change wildcard imports to explicit ones in all the tests that were touched

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/XMLEncKAT.java line 37:

> 35: import java.security.AlgorithmParameters;
> 36: import java.security.Provider;
> 37: //import javax.crypto.*;

`//` :)

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/XMLEncKAT.java line 135:

> 133:         String wrapAlg = "AESWrap";
> 134:         if (p.getService("Cipher", wrapAlg) == null) {
> 135:             throw new SkippedException("Skip, due to no support:  " + wrapAlg);

Minor: Could you please change the wording to something like `"No support:  " + wrapAlg);`

Also, is there any particular reason that lines 132 and 137 were removed? Don't you think it was a bit easier to read? If you like it as it is, I don't mind if you keep it as it is

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

PR Review: https://git.openjdk.org/jdk/pull/27847#pullrequestreview-3345856568
PR Review: https://git.openjdk.org/jdk/pull/27847#pullrequestreview-3359722784
PR Review: https://git.openjdk.org/jdk/pull/27847#pullrequestreview-3359950319
PR Comment: https://git.openjdk.org/jdk/pull/27847#issuecomment-3425928748
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436679850
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436715573
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2447520187
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436706643
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2447517742
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436680702
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436681563
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436683968
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2447507012
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436714251