code review request: 8012971 PKCS11Test hiding exception failures

[Valerie (Yu-Ching) Peng]

Just noticed that the newly-added import statement on line 32 seems 
redundant?
Webrev seems the same?
Thanks,
Valerie

On 07/18/13 15:17, Anthony Scarpino wrote:
> On 07/18/2013 02:49 PM, Valerie (Yu-Ching) Peng wrote:
>>
>> Please find comments inline.
>>
>> On 07/17/13 13:51, Anthony Scarpino wrote:
>>> I have broken these into two webrev.  The first:
>>>
>>> JDK-8012971 PKCS11Test hiding exception failures
>>> http://cr.openjdk.java.net/~ascarpino/8012971/webrev.01/
>>>
>>> handles the minimum changes needed for PKCS11Test to function and the
>>> Problemlist updated to reflect the failures that would show up.
>> Looks fine in general, I only have some nit comments, and questions
>> since I have not touched NSS much.
>> PKCS11Test.java
>> 1) line 88, change "this finally block" to "any finally block"
> yep
>
>> 2) line 192, add space before and after the "+" for consistency with the
>> rest of the source, e.g. line 213.
> yep
>
>> 3) The comments from line 58-60 say the default value is "nss3" but the
>> default value set on line 61 is "softokn3". Seems conflicting?
>
> yep.  it nss3 use to be the default, must have forgot the change the 
> comment
>
>> 4) Is "nss3" only used for Secmod? It seems that everywhere else you use
>> the "softokn3" value. If yes, perhaps using "setSecmod()" would be
>> clearer than "useNSS()".
>
> Secmod is the only one currently, but I would rather leave it as NSS 
> because I named it for the library it's looking for.  If another test 
> comes around that is not a Secmod test, then setSecmod() doesn't seem 
> logical.
>
> webrev is updated in place.
>
>>
>> Will look at 8020424 later today.
>> Thanks,
>> Valerie
>>>
>>>
>>> The second, are the test changes that fix the problems uncovered by
>>> the above change:
>>>
>>> JDK-8020424 The NSS version should be detected before running crypto
>>> tests
>>> http://cr.openjdk.java.net/~ascarpino/8020424/webrev.00/
>>>
>>> Tony
>>>
>>
>