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

[Jason Uh]

Hi Raghu,

Formatting looks good, but one last thing about the copyright headers. I 
noticed that in lines 7-9, you have the lines

   * ...  Oracle designates this
   * particular file as subject to the "Classpath" exception as provided
   * by Oracle in the LICENSE file that accompanied this code.

Can you remove that sentence? In general, I think the license that 
includes those lines is meant for source files, not for test files. If 
you look at other tests, you'll see that this final sentence isn't there.

Thanks,
Jason

On 9/4/14 6:54 AM, raghu k.nair wrote:
> 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
>>>>>>
>>>>
>>>
>>
>