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

[Neha Joshi]

On Thu, 16 Oct 2025 16:37:00 GMT, Mikhail Yankelevich <myankelevich 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
>
> 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.`

Do we need to remove 2024 from the copyright ?

> 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.

I kept the return statement as it is and made skippedAlgoList a global variable. 
For each skip , we will add the element in the list .

> 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

The method itself is not that long and after the very first line, having spaces was reducing readability to me , that was the reason I removed the extra lines.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2436723302
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2447504600
PR Review Comment: https://git.openjdk.org/jdk/pull/27847#discussion_r2439272723