8368524: Tests are skipped and shown as passed in test/jdk/sun/security/pkcs11/Cipher/KeyWrap#27847
8368524: Tests are skipped and shown as passed in test/jdk/sun/security/pkcs11/Cipher/KeyWrap#27847nehajoshinj wants to merge 11 commits intoopenjdk:masterfrom
Conversation
|
Hi @nehajoshinj, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user nehajoshinj" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
|
@nehajoshinj 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: 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 13 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@myankelev, @rhalade) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
bot
changed the title
|
@nehajoshinj The following label will be automatically applied to this pull request:
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. |
|
/covered |
|
Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
myankelev
left a comment
myankelev
left a comment
There was a problem hiding this comment.
Looks good, just a few questions
| if (keyLen > allowed) { | ||
| System.out.println("=> skip, exceeds max allowed size " + allowed); | ||
| return; | ||
| throw new SkippedException("Skip, exceeds max allowed size " + allowed); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
| if (skippedAlgoList.isEmpty()) { | ||
| System.out.println("All Tests Passed"); | ||
| } else { | ||
| System.err.println("Some tests were skipped : " + skippedAlgoList); |
There was a problem hiding this comment.
This will only log a skip this way. Please throw an exception in the end.
| System.err.println("Some tests were skipped : " + skippedAlgoList); | |
| throw new SkippedException("Some tests were skipped : " + skippedAlgoList); |
| // adapted from com/sun/crypto/provider/Cipher/KeyWrap/NISTWrapKAT.java | ||
| public class NISTWrapKAT extends PKCS11Test { | ||
|
|
||
| private static List<String> skippedAlgoList = new ArrayList<>(); |
There was a problem hiding this comment.
nit:
| private static List<String> skippedAlgoList = new ArrayList<>(); | |
| private static final List<String> skippedAlgoList = new ArrayList<>(); |
|
Looks good to me |
bridgekeeper
bot
removed
oca
Webrevs
|
|
@nehajoshinj 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 |
|
/keepalive |
|
@nehajoshinj The pull request is being re-evaluated and the inactivity timeout has been reset. |
rhalade
left a comment
rhalade
left a comment
There was a problem hiding this comment.
Please fix the copyright header in XMLEncKAT.java (missing , after 2025)
|
@nehajoshinj this pull request can not be integrated into git checkout JDK-8368524/SkipExceptionIssue
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
removed
the
merge-conflict
|
Thanks for fixing issues caused by JDK-8371262! |
|
There is a slight change in the PR , earlier this PR contained changes to handle skipped exception for below test cases.
But there is similar ticket https://bugs.openjdk.org/browse/JDK-8371262 which got merged before this change. |
|
/integrate |
|
@nehajoshinj |
|
/sponsor |
|
Going to push as commit b99be50.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@rhalade @nehajoshinj Pushed as commit b99be50. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
This PR contain changes to handle skipped exception for below test cases.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27847/head:pull/27847$ git checkout pull/27847Update a local copy of the PR:
$ git checkout pull/27847$ git pull https://git.openjdk.org/jdk.git pull/27847/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27847View PR using the GUI difftool:
$ git pr show -t 27847Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27847.diff
Using Webrev
Link to Webrev Comment