8308711: Develop additional Tests for KEM implementation #14113

sisahoo · 2023-05-24T07:02:55Z

Additional Tests for KEM API.

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-8308711: Develop additional Tests for KEM implementation

Reviewers

Weijun Wang (@wangweij - Reviewer) ⚠️ Review applies to f70d1498

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14113

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-24T07:04:25Z

👋 Welcome back ssahoo! 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 · 2023-05-24T07:06:35Z

@sisahoo 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 · 2023-05-24T07:10:23Z

Webrevs

test/jdk/javax/crypto/KEM/GenLargeNumberOfKeys.java

test/jdk/javax/crypto/KEM/KemInterop.java

test/jdk/javax/crypto/KEM/GenLargeNumberOfKeys.java

test/jdk/javax/crypto/KEM/KemTest.java

wangweij · 2023-06-01T14:16:23Z

test/jdk/javax/crypto/KEM/GenLargeNumberOfKeys.java

+        Asserts.assertTrue(Arrays.equals(d.decapsulate(enc.encapsulation()).getEncoded(),
+                sk.getEncoded()));
+        Asserts.assertTrue(Arrays.equals(d.decapsulate(enc.encapsulation()).getEncoded(),
+                enc.key().getEncoded()));


Do you really mean to test this 3 times? If so, add a comment.

Yes.. Thats a valid case to ensure repeated calls don't change anything. I can add a comment there.

wangweij · 2023-06-01T14:18:18Z

test/jdk/javax/crypto/KEM/KemTest.java

+                Asserts.assertEQ(decT.secretSize(),
+                        decT.decapsulate(enc.encapsulation()).getEncoded().length);
+                Asserts.assertEQ(decT.decapsulate(enc.encapsulation()).getEncoded().length,
+                        enc.key().getEncoded().length);


Try adding a test on the encapsulationSize() method on the decapsulator.

It is there L-130,131, Do you want any specific case?
Asserts.assertEQ(encT.encapsulationSize(), enc.encapsulation().length);
Asserts.assertEQ(encT.encapsulationSize(), decT.encapsulationSize());

Oh sorry, I just looked at the left side of assertEQ.

wangweij

Looks good to me. Thanks!

openjdk · 2023-06-01T14:50:02Z

@sisahoo 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:

8308711: Develop additional Tests for KEM implementation

Reviewed-by: weijun

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 41 new commits pushed to the master branch:

325940b: 8307105: JFileChooser InvalidPathException when selecting some system folders on Windows
101bf22: 8308891: TestCDSVMCrash.java needs @requires vm.cds
2bb1972: 8308954: [JVMCI] code installation increments decompile_count for call_site_target_value failures
0ab0963: 8308469: [PPC64] Implement alternative fast-locking scheme
ec55539: 8309138: Fix container tests for jdks with symlinked conf dir
e827164: 8309146: extend JDI StackFrame.setValue() and JDWP StackFrame.setValues minimal support for virtual threads
be36096: 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread safe
c6f20db: 8308232: nsk/jdb tests don't pass -verbose flag to the debuggee
d987176: 8307794: Test for HSS/LMS Signature Verification
050425b: 8298127: HSS/LMS Signature Verification
... and 31 more: https://git.openjdk.org/jdk/compare/927a9ed68371597eba1609f97ac03dd1de812e26...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.

sisahoo · 2023-06-02T08:11:39Z

/integrate

openjdk · 2023-06-02T08:13:23Z

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

dcd9590: 8309224: Fix xlc17 clang 15 warnings in java.desktop
8f1ce78: 8308752: Generational ZGC: Avoid final marking through stack chunks
7b0a336: 8308387: CLD created and unloading list sharing _next node pointer leads to concurrent YC missing CLD roots
60f3b87: 8309295: C2: MaxNode::signed_min() returns nullptr for int operands
8007599: 8309093: Underscore with brackets
5bd2af2: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents
325940b: 8307105: JFileChooser InvalidPathException when selecting some system folders on Windows
101bf22: 8308891: TestCDSVMCrash.java needs @requires vm.cds
2bb1972: 8308954: [JVMCI] code installation increments decompile_count for call_site_target_value failures
0ab0963: 8308469: [PPC64] Implement alternative fast-locking scheme
... and 37 more: https://git.openjdk.org/jdk/compare/927a9ed68371597eba1609f97ac03dd1de812e26...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-02T08:13:37Z

@sisahoo Pushed as commit aeb53e6.

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

8308711: Develop additional Tests for KEM implementation

dd210d4

openjdk bot added the rfr Pull request is ready for review label May 24, 2023

openjdk bot added the security security-dev@openjdk.org label May 24, 2023

wangweij reviewed May 24, 2023

View reviewed changes

test/jdk/javax/crypto/KEM/GenLargeNumberOfKeys.java Show resolved Hide resolved

test/jdk/javax/crypto/KEM/KemInterop.java Outdated Show resolved Hide resolved

test/jdk/javax/crypto/KEM/KemInterop.java Outdated Show resolved Hide resolved

sisahoo added 2 commits May 31, 2023 11:15

Merge kEM

d66d727

8308711: Comment addressed

610156c

wangweij reviewed May 31, 2023

View reviewed changes

Comments addressed

f70d149

wangweij reviewed Jun 1, 2023

View reviewed changes

wangweij approved these changes Jun 1, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 1, 2023

Comment added

c452e8a

openjdk bot added the integrated Pull request has been integrated label Jun 2, 2023

openjdk bot closed this Jun 2, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 2, 2023

8308711: Develop additional Tests for KEM implementation #14113

8308711: Develop additional Tests for KEM implementation #14113

Uh oh!

Conversation

sisahoo commented May 24, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 24, 2023

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

mlbridge bot commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wangweij Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

sisahoo Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

sisahoo Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

wangweij Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

wangweij left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sisahoo commented Jun 2, 2023

Uh oh!

openjdk bot commented Jun 2, 2023

Uh oh!

openjdk bot commented Jun 2, 2023

Uh oh!

Uh oh!

sisahoo commented May 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 24, 2023 •

edited

Loading

sisahoo Jun 1, 2023 •

edited

Loading

openjdk bot commented Jun 1, 2023 •

edited

Loading