On Tue, 9 Sep 2025 09:02:03 GMT, Mikhail Yankelevich <myankelev...@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
