On Thu, 16 Oct 2025 16:37:00 GMT, Mikhail Yankelevich
<[email protected]> 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
