8367994: test/jdk/sun/security/pkcs11/Signature/ tests pass when they should skip by myankelev · Pull Request #27367 · openjdk/jdk

myankelev · 2025-09-18T14:56:13Z

test/jdk/sun/security/pkcs11/Signature/InitAgainPSS.java
test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java
test/jdk/sun/security/pkcs11/Signature/SigInteropPSS.java
test/jdk/sun/security/pkcs11/Signature/SigInteropPSS2.java
test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java
test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS2.java
test/jdk/sun/security/pkcs11/Signature/TestDSA.java

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8367994: test/jdk/sun/security/pkcs11/Signature/ tests pass when they should skip (Bug - P4)

Reviewers

Rajan Halade (@rhalade - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27367/head:pull/27367
$ git checkout pull/27367

Update a local copy of the PR:
$ git checkout pull/27367
$ git pull https://git.openjdk.org/jdk.git pull/27367/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27367

View PR using the GUI difftool:
$ git pr show -t 27367

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27367.diff

Using Webrev

Link to Webrev Comment

…they should skip

bridgekeeper · 2025-09-18T14:58:20Z

👋 Welcome back myankelevich! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-09-18T14:59:43Z

@myankelev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8367994: test/jdk/sun/security/pkcs11/Signature/ tests pass when they should skip

Reviewed-by: rhalade

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 107 new commits pushed to the master branch:

c55287d: 8370890: Start of release updates for JDK 27
45dcc0e: 8371914: PNG defines in CFLAGS can cause compilation errors with external libpng
2735140: 8370939: C2: SIGSEGV in SafePointNode::verify_input when processing MH call from Compile::process_late_inline_calls_no_inline()
... and 104 more: https://git.openjdk.org/jdk/compare/f5eecc454eb78fc1a3714dfe3cb94113238dd3ac...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-09-18T15:00:46Z

@myankelev The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-09-18T15:04:41Z

Webrevs

sendaoYan · 2025-09-19T05:46:35Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

+/**
+ * @test id=new_alg
+ * @bug 8080462 8226651 8242332
+ * @summary Generate a RSASSA-PSS signature and verify it using PKCS11 provider


Should we need comments why this test was splited to two tests.

There is no support for sha3 in nss for now it seems, so this is why it's split. I'm not sure if this makes any difference if highlighted in the comment, but I can add in the next commit.

wangweij · 2025-09-22T15:56:11Z

Honestly, I don't quite understand why there are 2 tests initializing skipTest as true and then assign it false. Usually it's the other way round. If you know why it has to be coded this way, please add some comments there. Thanks.

mpdonova · 2025-09-23T14:59:27Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java


 /**
- * @test
+ * @test id=old_alg


For id's, "old_alg" and "new_alg" aren't very descriptive/useful. Maybe change them to "sha" and "sha3"?

Done in the next commit

mpdonova · 2025-09-23T15:00:13Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS2.java


 /**
- * @test
+ * @test id=old_alg


use more descriptive id names

Done in the next commit

mpdonova · 2025-09-23T15:00:49Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

@@ -58,6 +79,9 @@ public class SignatureTestPSS extends PKCS11Test {
    private static boolean skipTest = true;


It looks like this field is still being and we might test some algorithms but the still throw a SkippedException. Can you get rid of this uncertainty somehow?

I'm not sure what you mean here. I think the best way would be to split the @test and test each algorithm separately. However this approach will take forever to execute, which is a dealbreaker.

I can add all the skips into one list and only then print out the skipped parameters, if this helps. However, given that the test is split into sha and sha3, all sha3 will be skipped for now. I'm not sure it would bring much more clarity in this case. What do you think?

In my opinion, tests shouldn't have logic that runs some test cases and skips others. It creates uncertainty when trying to analyze test coverage. In this case, if a SkippedException is thrown, we don't know what -- if any -- algorithms were successfully tested unless we review the logs and that's an unnecessary burden. If NSS doesn't support the SHA3 algorithms, then we shouldn't try to test them. When we update the NSS library, we need to look at what's newly supported (or removed) and update tests accordingly.

mpdonova · 2025-09-23T15:11:53Z

test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java

+                    keySize,
+                    hashAlg,
+                    mgfHashAlg);
+            skipTest = true;


should this test also be split into separate @test blocks? There are about 18 test cases and 17 of them could pass and the last one skipped and the test gets marked as skipped.

There should be no skips in this test with nss, so I don't think this should be an issue here. Splitting this into different @test would be though, as it will increase the execution time significantly.

This test should also be updated to maintain a list of skipped tests. Right now the skipTest logic is flawed and it won't be marked as skipped if last runTest resets it to false.

@rhalade yes, I agree. Didn't notice this. Thanks for noticing

myankelev · 2025-09-24T10:27:11Z

Honestly, I don't quite understand why there are 2 tests initializing skipTest as true and then assign it false. Usually it's the other way round. If you know why it has to be coded this way, please add some comments there. Thanks.

I was just keeping the previous logic that was present before and could be seen in the other files.
I personally don't think flipping the logic will bring any benefits. Especially as it may bring accidental confusion if several tests with flipped and not flipped logic are involved in some fix/enhancement in the future.
But overall I'm fine with doing this, so if you think I should - I will.

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

mpdonova · 2025-09-24T12:59:54Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

@@ -58,6 +79,9 @@ public class SignatureTestPSS extends PKCS11Test {
    private static boolean skipTest = true;


In my opinion, tests shouldn't have logic that runs some test cases and skips others. It creates uncertainty when trying to analyze test coverage. In this case, if a SkippedException is thrown, we don't know what -- if any -- algorithms were successfully tested unless we review the logs and that's an unnecessary burden. If NSS doesn't support the SHA3 algorithms, then we shouldn't try to test them. When we update the NSS library, we need to look at what's newly supported (or removed) and update tests accordingly.

bridgekeeper · 2025-10-23T03:31:40Z

@myankelev This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

myankelev · 2025-10-23T08:50:07Z

/touch

openjdk · 2025-10-23T08:51:57Z

@myankelev The pull request is being re-evaluated and the inactivity timeout has been reset.

rhalade · 2025-10-23T16:49:06Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

+                        hash,
+                        mgfHash,
+                        s);
+                skipTest = true;


I think skipTest logic in this test needs further updates. It starts with true as default value, should be false and only set skipTest to true when we encounter a test scenario that is not supported. I think there is also a case where skipTest value is overwritten at line 153 when called in a loop at line 95.

Done in the next commit. I have reworked it to be a list for easier debug in the future.

rhalade · 2025-11-21T18:40:46Z

test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java

+                    keySize,
+                    hashAlg,
+                    mgfHashAlg);
+            skipTest = true;


This test should also be updated to maintain a list of skipped tests. Right now the skipTest logic is flawed and it won't be marked as skipped if last runTest resets it to false.

myankelev · 2025-12-04T17:20:56Z

/integrate

openjdk · 2025-12-04T18:41:37Z

Going to push as commit ef7532e.
Since your change was applied there have been 109 commits pushed to the master branch:

b19163b: 8356544: Implement additional tests for ciphersuites disabled with wildcards
c7aa103: 8372844: Improve usage of test/jdk/java/text/testlib/TestUtils.java locale methods
c55287d: 8370890: Start of release updates for JDK 27
... and 106 more: https://git.openjdk.org/jdk/compare/f5eecc454eb78fc1a3714dfe3cb94113238dd3ac...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-04T18:42:42Z

@myankelev Pushed as commit ef7532e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8367994: test/jdk/sun/security/pkcs11/Signature/ tests pass when …

2370b67

…they should skip

openjdk bot changed the title ~~JDK-8367994: test/jdk/sun/security/pkcs11/Signature/ tests pass when they should skip~~ 8367994: test/jdk/sun/security/pkcs11/Signature/ tests pass when they should skip Sep 18, 2025

openjdk bot added security security-dev@openjdk.org rfr Pull request is ready for review labels Sep 18, 2025

sendaoYan reviewed Sep 19, 2025

View reviewed changes

mpdonova reviewed Sep 23, 2025

View reviewed changes

renamed id

e390ba7

mpdonova reviewed Sep 24, 2025

View reviewed changes

name changes for arrays

e4f20a3

rhalade suggested changes Oct 23, 2025

View reviewed changes

myankelev added 2 commits October 28, 2025 12:58

Merge branch 'master' into JDK-8367994

4635f23

skip reworked

234aa5d

rhalade suggested changes Nov 21, 2025

View reviewed changes

myankelev added 2 commits December 1, 2025 14:01

Merge branch 'master' into JDK-8367994

ed52c83

reworked skipped logic to be list based

415fc1f

myankelev requested a review from rhalade December 1, 2025 17:58

rhalade approved these changes Dec 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2025

openjdk bot added the integrated Pull request has been integrated label Dec 4, 2025

openjdk bot closed this Dec 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 4, 2025

		@@ -58,6 +79,9 @@ public class SignatureTestPSS extends PKCS11Test {
		private static boolean skipTest = true;

Conversation

myankelev commented Sep 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 18, 2025

Uh oh!

openjdk bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myankelev commented Sep 24, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Oct 23, 2025

Uh oh!

myankelev commented Oct 23, 2025

Uh oh!

openjdk bot commented Oct 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myankelev commented Dec 4, 2025

Uh oh!

openjdk bot commented Dec 4, 2025

Uh oh!

openjdk bot commented Dec 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

myankelev commented Sep 18, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 18, 2025 •

edited

Loading

openjdk bot commented Sep 18, 2025 •

edited

Loading

mlbridge bot commented Sep 18, 2025 •

edited

Loading