Review request for CR 8049039 Need new tests for sun.securiy.x509 classes

[raghu k.nair]

Hi Vincent / Jason,
   Please review the updated webrev  based on Jason's comments.
   webrev: http://cr.openjdk.java.net/~tyan/raghu/8049039/webrev02/ 
<http://cr.openjdk.java.net/%7Etyan/raghu/8049039/webrev02/>

Thanks,
Raghu Nair
On 8/27/2014 9:43 AM, raghu k.nair wrote:
> Hi Jason,
>  Thanks for your review comments.
> On 8/26/2014 11:37 PM, Jason Uh wrote:
>> Hi Raghu,
>>
>> I'm not an official Reviewer, but I have some comments, mostly about 
>> X509Test.java.
>>
>> 1. I'm not sure if this matters, but the formatting of your copyright 
>> header is a little strange. The text appears to be correct, but the 
>> line breaks occur at non-standard places. If you look at any other 
>> file, the first couple lines would look like this:
>>
>> /*
>>  * Copyright (c) 2014, Oracle and/or its affiliates. All rights 
>> reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>
>> Again, not sure how important this is, but it applies to every file 
>> in the webrev. I'd suggest changing them.
>>
> This happened due to default formatting by Netbeans . I will correct it.
>> 2. checkMethod():
>> Doesn't appear to be used anywhere. Do you want to keep this? If you 
>> do, could you add a comment to explain the parameters?
>>
>> Also, in general for the rest of the methods, there are a lot of 
>> parameters noted with the @param tag. If you want to leave those 
>> javadoc style comments in, could you please describe the params?
>>
> X509Test is used by many other tests ( appart from these tests as part 
> of the webrev). I will add comment and description for the params.
>
>> 3. equals() and assertByteArray() both seem to be 
>> "assertEquals()"-style methods, but they are named a bit differently. 
>> could you make the naming consistent? For example, maybe assertEquals 
>> / assertByteArrayEquals.
>>
> Yes it make sense. I will make the necessary changes .
>
> 4. getElementesTest() should probably read getElementsTest(). You use 
> this method in IssuerAlternativeNameExtensionTest.java, by the way.
>
> typo needs to be fixed.
>
> Thanks,
> Raghu
>> Otherwise, I think the tests look good to me, but again, I'm not a 
>> Reviewer, so you still need an official review.
>>
>> Thanks,
>> Jason
>>
>>
>>
>> On 08/25/2014 03:59 AM, raghu k.nair wrote:
>>> Hi Vincent / Jason,
>>>   Could you please help in reviewing the following test.
>>>
>>> Thanks,
>>> Raghu Nair
>>>
>>> On 8/12/2014 10:28 AM, raghu k.nair wrote:
>>>> Hello,
>>>>  Please review the tests for sun.security.x509 classes.
>>>>  These cover tests for GeneralName, GeneralNames, GeneralSubtree,
>>>> GeneralSubtrees, IPAddressName and IssuerAlternativeNameExtension.
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~rhalade/8049039/webrev.00/
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8049039
>>>>
>>>> Thanks,
>>>> Raghu Nair
>>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20140827/b03ac8dc/attachment.htm>