RFR: 8028127: Regtest java/security/Security/SynchronizedAccess.java is incorrect [v2]

[Matthew Donovan]

On Mon, 3 Jun 2024 09:54:16 GMT, Fernando Guallini <fguallini at openjdk.org> wrote:

>> As highlighted in the bug description, The test **security/Security/SynchronizedAccess.java** have some issues:
>> 
>> 1. it needs to implement the sigalg, otherwise it throws java.security.NoSuchAlgorithmException . Even though it does not fail as it catches the exception, it never reaches the removeProvider loop. Also, adding an extra assertion to verify the local providers were actually removed.
>> 2. It is reassigning the **provs** variable with Security.getProviders(). This will start adding/removing some of the system providers, in addition to the local providers, which it is not the test intent.
>> 3. Adding othervm flag to run in isolation.
>> 
>> Test runs in tier2
>
> Fernando Guallini has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8028127
>  - Merge branch 'openjdk:master' into JDK-8028127
>  - Regtest java/security/Security/SynchronizedAccess.java is  incorrect.

test/jdk/java/security/Security/SynchronizedAccess.java line 70:

> 68:             Provider[] provs = new Provider[10];
> 69:             for (int i = 0; i < provs.length; i++)
> 70:                 provs[i] = new MyProvider("name" + i, "1", "test");

use { } around for-loop

test/jdk/java/security/Security/SynchronizedAccess.java line 79:

> 77:                     }
> 78:                     Signature.getInstance("sigalg");
> 79:                     // skipping first provider so there is always 1 available for getInstance

Why are you leaving one provider when the next thing to do is to add them all back again?

test/jdk/java/security/Security/SynchronizedAccess.java line 84:

> 82:                     }
> 83:                 } catch (NoSuchAlgorithmException nsae) {
> 84:                     throw new RuntimeException("Should not reach here " + nsae);

The message could be a little clearer. Something like, "Expected provider for sigalg not found."

I would include `nsae` as a second, 'cause' parameter to the RuntimeException. That way the full stack is shown in the logs. It isn't strictly necessary here since it's pretty obvious where it's coming from, but it's a good habit to be in.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1629329314
PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1629337067
PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1629343776