Hi John

1. Please add @bug to all tests.

2. Are getLibPath() and findLib() in AutoTest.java really necessary? It looks 
like PKCS11Test::getNSSLibDir is doing something similar.

3. Looks like Standard.java is not necessary now. You can just make 
KeyToolTest.java a @test and add a @run line there, something like

    @run main/othervm/timeout=600 -Dfile KeyToolTest

4. Maybe we can rename AutoTest.java to NssTest.java. The old name says nothing.

Thanks
Max

> On Sep 27, 2018, at 9:25 AM, sha.ji...@oracle.com wrote:
> 
> Hi Max,
> Please review the updated webrev: 
> http://cr.openjdk.java.net/~jjiang/8209546/webrev.01/
> All your comments are addressed, though this test is moved to problem list 
> for windows due to JDK-8204203.
> 
> Best regards,
> John Jiang
> On 2018/9/25 22:30, Weijun Wang wrote:
>> Some questions:
>> 
>> 1. Do we still need the OS check on lines 47-49? As long as getLibPath() can 
>> return something, does it mean the test should just run? Especially, does 
>> the test run on Windows?
>> 
>> 2. Is launching a separate process necessary? Can we just call 
>> KeyToolTest::main after setting system properties and copying the files.
>> 
>> 3. Is it possible to include standard.sh?
>> 
>> Thanks
>> Max
>> 
>> 
>>> On Sep 25, 2018, at 6:30 PM, sha.ji...@oracle.com
>>>  wrote:
>>> 
>>> Hi,
>>> JDK-8164639 removed NSS libs from repo, so 
>>> sun/security/tools/keytool/autotest.sh has to download NSS libs from 
>>> artifactory on macosx.
>>> This patch also refactors this shell test to a Java test.
>>> 
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jjiang/8209546/webrev.00/
>>> 
>>> Issue: 
>>> https://bugs.openjdk.java.net/browse/JDK-8209546
>>> 
>>> 
>>> Best regards,
>>> John Jiang
>>> 
>>> 
> 

Reply via email to