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

[raghu k.nair]

Hi Jason,
  Please review the updated webrev . I have addressed your comments.
http://cr.openjdk.java.net/~tyan/raghu/8049039/webrev03/ 
<http://cr.openjdk.java.net/%7Etyan/raghu/8049039/webrev03/>

Thanks,
Raghu
On 9/3/2014 12:47 AM, Jason Uh wrote:
> On 8/27/14 8:34 AM, raghu k.nair wrote:
>> 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.
>
> I was actually showing the issue with the entire copyright header by 
> quoting just the beginning. Sorry if I wasn't clear, but it looks like 
> you only corrected the formatting on the first two lines. Could you 
> please redo the entire copyright header for all of these files? (You 
> can look at any existing test for reference and you'll see the 
> difference.)
>
>>>> 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.
>>>
>
> Minor detail, but there is an extra space in line 53 and 55 of 
> X509Test. Could you please remove them?
>
> Otherwise, looks good to me, but you'll still need an official Reviewer.
>
> Thanks,
> Jason
>
>>>> 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/20140904/33226f2f/attachment.htm>