Hi Max,
Please find the updated webrev:
http://cr.openjdk.java.net/~ssahoo/8141039/webrev.02/
The changes includes,
- ApiTest.java : Removed printing unnecessary message inside checkException()
method.
- GetInstanceTest.java
- Moved the test back into " java/security/SecureRandom/ ".
- Removed all reference to MoreDrbgParameters and EntropySource.
Thanks,
Siba
-----Original Message-----
From: Wang Weijun
Sent: Thursday, May 12, 2016 9:14 AM
To: Sibabrata Sahoo
Cc: security-dev@openjdk.java.net
Subject: Re: [9] RFR: 8141039: Test Task: Develop new tests for JEP 273:
DRBG-Based SecureRandom Implementations
> On May 12, 2016, at 1:46 AM, Sibabrata Sahoo <sibabrata.sa...@oracle.com>
> wrote:
>
> Hi Max,
>
> Please find the updated webrev:
> http://cr.openjdk.java.net/~ssahoo/8141039/webrev.01/
>
> Changes includes:
> - Removed otherVM option and default "securerandom.drbg.config" will get
> reset after each DRBG test run. This change affected to all test files.
> - Addressed all the following comments for GetAlgorithm.java,
> EnoughSeedTest.java, MultiThreadTest.java.
> - Moved ApiTest.java, into "sun/security/provider/SecureRandom" because it
> uses MoreDrbgParameters.
> - remove SHA1 and TDEA, 3KEYTDEA" ("3 KEY TDEA", "DESEDE")
> - corrected the logic for checkException() reported bellow.
I don't see a difference. You print out messages in those "if (strength > n)"
blocks but there is no other action. What are the messages for? Information or
warning? In my opinion, a test should print out as little as possible unless an
error occurs. (Sometimes "Testing XYZ..." is OK).
> - Moved GetInstanceTest.java into "sun/security/provider/SecureRandom"
> because it uses MoreDrbgParameters.
> - I am using default as well as custom entropy source just to make sure the
> extension works for getInstance() method.
> - Again, I use MoreDrbgParameter, just to make sure the instance of it can
> be acceptable by corresponding getInstance() method.
Well, I have a different opinion.
I think most of your tests should be categorized as functional (or conformance)
tests, so they should be focused on public APIs. In this case, the test should
set "securerandom.drbg.config" to Hash_DRBG/SHA-512 instead of using
MoreDrbgParameters to set algorithm to SHA-512.
In fact, there is no requirement that MoreDrbgParameters must be accepted by
getInstance().
MoreDrbgParameters and EntropySource are implementation details. Sometimes they
are necessary (For example, CAVP tests), but you'd better avoid touching them.
I did wrote some tests on them (DRBGAlg, AbstractDrbgSpec) in an implementor's
perspective but you can ignore them.
> - About SerializedSeedTest.java, I think the SHA1PRNG bug fix will get pushed
> to repo very soon. So the test is expected to work as the fix pushed into
> repo. But if you wish, I can change it accordingly.
Both the SHA1PRNG setSeed bug and the DRBG synchronization bug are already
fixed and pushed.
Thanks
Max
