Hi Sean/Adam,

Please review the updated patch,
Webrev: http://cr.openjdk.java.net/~ssahoo/8184359/webrev.02/

Now there is only 1 Test file in the deleted list which has duplicate code. Due 
to that I have pointed the older JBS bug ID JDK-4936763 in 
KeyAgreementTest.java and linked the same to JDK-8184359 too.

Reverted the duplicate code merge in KeySizeTest.java due to performance issue 
observed for higher key sizes. That ensured SupportedDHKeys.java & 
UnsupportedDHKeys.java will not be removed as it was provided in previous patch.

Thanks,
Siba

-----Original Message-----
From: Sean Mullan 
Sent: Wednesday, April 11, 2018 1:42 AM
To: Sibabrata Sahoo <sibabrata.sa...@oracle.com>; Adam Petcher 
<adam.petc...@oracle.com>; security-dev@openjdk.java.net
Subject: Re: RFR: 8200219: Develop new tests for using new elliptic curves: 
curve25519 and curve448

On 4/9/18 10:13 AM, Sibabrata Sahoo wrote:
> Here is the new patch addressing all comments.
> Webrev:http://cr.openjdk.java.net/~ssahoo/8184359/webrev.01/
> 
> Changes includes:
> 1)
> KeyAgreementTest.java
> KeySizeTest.java
>            I have ensured no duplicate Test cases exist for the functionality 
> provided in these 2 files. Due to that I have remove 3 files, 
> DHGenSecretKey.java, SupportedDHKeys.java, UnsupportedDHKeys.java from 
> "open/test/jdk/com/sun/crypto/provider/KeyAgreement" folder. 

For DHGenSecretKey, can you add the bugid of 4936763 to the test file in this 
webrev where the changes were merged and also link the bugs together in JBS? 
That way we know that these tests are now covering the fixes for that bug. For 
the others, see below.

> Statements which affected due to merging the missing code from these deleted 
> files are KeyAgreementTest.java[29, 141], KeySizeTest.java[91-100, 112-162]. 
> Also I have observed that KeySizeTest.java Test takes around 2 minute to 
> complete in SOME PLATFORM to complete all 14 @run and approximately 20 
> seconds for higher keys > 4096 for "DiffieHellman" only after merge. Please 
> suggest if removing the higher Keys from the Test cases will help or 20-30 
> seconds for these higher keys is accepted here?

It's a bit concerning. One way to make the tests run faster is to split them up 
into separate tests so that they can be run concurrently by jtreg. So we might 
be making things worse by merging them :(

So I would suggest just merging DHGenSecretKey and leaving the other tests 
as-is.

> 2) I have updated the comment in KeyAgreementTest.java[129] to indicate 
> provider search order.

Ok.

--Sean

> As per Adam's comments the following changes,
> 3) KeySizeTest.java & NegativeTest.java, which need utility methods from 
> Convert.java are using the library instead of defining it's own.
> 4) MultiThreadTest.java generates two key pairs and do two key agreement 
> operations.

Reply via email to