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
Webrev:http://cr.openjdk.java.net/~tyan/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/>>
Thanks
Tristan
