RFR: 8370942: test/jdk/java/security/Provider/NewInstance.java and /test/jdk/java/security/cert/CertStore/NoLDAP.java may skip without notifying [v2]

[Mikhail Yankelevich]

On Mon, 3 Nov 2025 14:33:23 GMT, Neha Joshi <duke at openjdk.org> wrote:

>> This PR contain changes to handle skipped exception for below test cases.
>> 
>> -> test/jdk/java/security/cert/CertStore/NoLDAP.java
>> -> test/jdk/java/security/Provider/NewInstance.java
>> 
>> 
>> https://bugs.openjdk.org/browse/JDK-8370942
>
> Neha Joshi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8370942 : Updated error message

Changes requested by myankelevich (Committer).

test/jdk/java/security/Provider/NewInstance.java line 30:

> 28:             JDK JGSS Mechanisms
> 29:  * @library /test/lib ../..
> 30:  * @run main/othervm NewInstance

same as R28 and R29 in NoLDAP.java

test/jdk/java/security/Provider/NewInstance.java line 54:

> 52:             if (p.getName().equals("SunPCSC")) {
> 53:                 skippedProvider = "SunPCSC";
> 54:                 System.err.println("Skip test :: A smartcard might not be installed.");

This case I'd either take SunPCSC from the `Security.getProviders()` (save them to the list, take this out and traverse over it. This way you don't need to put this always skipped message. 

For SunPCSC, if you think it is needed at all, create a separate `@test` which will take the same providers list and run only if there is `SunPCSC` present and only with it. This way you can have it skipped/failed/passed if it needs to be. 

What do you think?

test/jdk/java/security/Provider/NewInstance.java line 78:

> 76:         }
> 77:         //Check if tests were skipped.
> 78:         if (skippedProvider.isEmpty()) {

I think you're checking if the string is empty when you could make an exact comparison. But I think this might be gone if you implement the suggestion at line 53 anyway :)

test/jdk/java/security/cert/CertStore/NoLDAP.java line 28:

> 26:  * @summary Sanity check that NoSuchAlgorithmException is thrown when requesting
> 27:  *   a CertStore of type "LDAP" and LDAP is not available.
> 28:  * @library /test/lib ../..

```sugest 
 * @library /test/lib
``` 

There is no need to import 2 directories up, this just adds to the build time

test/jdk/java/security/cert/CertStore/NoLDAP.java line 29:

> 27:  *   a CertStore of type "LDAP" and LDAP is not available.
> 28:  * @library /test/lib ../..
> 29:  * @run main/othervm NoLDAP

As you're not changing any env properties here, there is no need for othervm. 
`@run main NoLDAP` should be sufficient. But you don't need it at all, as `@run main NoLDAP` is there by default, it makes no difference at all.

test/jdk/java/security/cert/CertStore/NoLDAP.java line 44:

> 42:             throw new SkippedException("Test skipped :: LDAP is present");
> 43:         } catch (ClassNotFoundException ignore) {
> 44:             System.err.println("Class not found exception" + ignore.getMessage());

I'd state that this is expected, otherwise it's a bit confusing. What do you think?

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

PR Review: https://git.openjdk.org/jdk/pull/28112#pullrequestreview-3411626081
PR Review Comment: https://git.openjdk.org/jdk/pull/28112#discussion_r2486936366
PR Review Comment: https://git.openjdk.org/jdk/pull/28112#discussion_r2486974595
PR Review Comment: https://git.openjdk.org/jdk/pull/28112#discussion_r2486974940
PR Review Comment: https://git.openjdk.org/jdk/pull/28112#discussion_r2486941358
PR Review Comment: https://git.openjdk.org/jdk/pull/28112#discussion_r2486934199
PR Review Comment: https://git.openjdk.org/jdk/pull/28112#discussion_r2486948001