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

[raghu k.nair]

Hi Vincent / Jason,
   Please find the updated webrev with minor changes like re-using 
println method , possible NPE fixes etc.

http://cr.openjdk.java.net/~rhalade/8049039/webrev.05/

Thanks,
Raghu Nair
On 9/11/2014 10:28 PM, raghu k.nair wrote:
> Hi Jason,
>   I have removed those lines from copyright headers. Here is the 
> updated webrev.
>  http://cr.openjdk.java.net/~rhalade/8049039/webrev.04/
>
> Thanks,
> Raghu Nair
> On 9/6/2014 1:58 AM, Jason Uh wrote:
>> 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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>