Hi Valerie Thank you for reviewing this, I fixed 2 points in latest webrev. Do you mind sponsor this for me if you’re OK with this version. http://cr.openjdk.java.net/~tyan/8044199/webrev.03/ <http://cr.openjdk.java.net/~tyan/8044199/webrev.03/> Cheers. Tristan
> On Aug 28, 2015, at 2:03 PM, Valerie Peng <valerie.p...@oracle.com > <mailto:valerie.p...@oracle.com>> wrote: > > Hi Tristan, > > Mostly fine, just 2 more nits: > 1) KeySizeTest: the @test and @bug are still on the same line. > 2) SpecTest.java: the @run command from line 40 - 45 still have the old name, > e.g. KeyModulusTest? I think u need to match them. > > Thanks, > Valerie > On 8/20/2015 9:31 AM, Tristan Yan wrote: >> >> Thanks Valerie >> >> Please review the fixed version. >> >> http://cr.openjdk.java.net/~tyan/8044199/webrev.02/ >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.02/> >> >> Tristan >> >> >> > Tristan, >> > >> > - Given that the tests are against SunRsaSign provider, I think >> > sun/security/rsa subdirectory is a better choice than java/security/rsa. >> > - Put @test and @bug on separate lines >> > - Elaborate more on what is tested and what is the expected result in >> > the bug record. The current content isn't very clear. >> > - Can u have more unique names for these tests? Currently, they are too >> > similarly named and it's somewhat confusing. The directory already >> > contains RSA, I don't feel that u need to have RSA again in the names of >> > the test especially when SunRsaSign provider doesn't support non-RSA >> > algorithms. >> > >> > For KeyTest.java >> > 1) typo on line 69, rsaPrivateKey2 should be rsaPrivateKey >> > 2) This test is based on the> >> provider specific behavior that CRT key is >> > generated by default for RSA. It'd be much clearer if you can add an >> > instanceof check on the generated RSA private key objects before the >> > KeyFactory + RSAPrivateKeySpec code. In addition, add additional code to >> > check for equality with the matching KeySpec (e.g. RSAPrivateCrtKeySpec >> > for RSAPrivateCrtKey) is used. >> > 3) line 58, add a space between '//" and text. >> > >> > For RSAKeySizeTest.java >> > 1) specify provider "SunRSASign" for all getInstance() calls. >> > 2) line 55 can be moved up to before the for-loop. >> > 3) instead of using toString(int), probably bitLength() would be a >> > better choice. >> > 4) sizeTest() doesn't really check size but rather it compares the >> > modulus values being equal. This check can be moved to RSAKeySizeTest.java. >> > >> > For RSATest.java >> > 1) specify provider "SunRSASign" for all getInstance() calls. >> > >> > Thanks, >> > Valerie >> > >> > On 8/3/2015 10:27 AM, Tristan Yan wrote: >> >> Hi >> >> >> >> Please review new tests for RSA keys and key specifications >> >> >> >> Bug:https://bugs.openjdk.java.net/browse/JDK-8044199 >> >> <https://bugs.openjdk.java.net/browse/JDK-8044199> >> >> Webrev:http://cr.openjdk.java.net/~tyan/8044199/webrev.01/ >> >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/> >> >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/ >> >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/>> >> >> <http://cr.openjdk.java.net/~tyan/8044199/webrev.01/ >> >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/> >> >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/ >> >> <http://cr.openjdk.java.net/%7Etyan/8044199/webrev.01/>>> >> >> >> >> Thanks >> >> Tristan >> >>
