jmx-dev Codereview request: 8023529 OpenMBeanInfoSupport.equals/hashCode throw NPE

[shanliang]

David Holmes wrote:
> Ok functional change seems fine.
>
> Looking at the tests a few minor nits:
>
> test/javax/management/openmbean/OpenMBeanInfoEqualsNPETest.java
>
>  163             obj1.equals(null);
>  ...
>  184             obj1.equals(null);
>
> I think line 163 can be deleted.
Yes.
>
> In both tests ...
>
> Use spaces around the + operator in strings eg:
>
> "OK-1: "+obj1.get...
>
>
> The message "with a null object" isn't quite reflective of what is 
> being tested. "null attribute" or even "null field" perhaps?
>
> When you catch NPE and report an error it would be good to also print 
> the NPE stacktrace so we can see where we hit the unexpected NPE.
OK.
>
> The tests are a little verbose ie printing each successful test 
> result, but that might be normal style in this area.
Here we need to test all fields so I print out what we test, so easy to 
check no missed.
I get used to add more info, because sometime we miss info when a test 
fails.

Here is the new version:
    http://cr.openjdk.java.net/~sjiang/jdk-8023529/02/

Thanks,
Shanliang

>
> David
> -----
>
>> Thanks,
>> Shanliang
>>>
>>> David
>>> -----
>>>
>>> On 11/09/2013 12:15 AM, shanliang wrote:
>>>> Jaroslav Bachorik wrote:
>>>>> On 09/05/2013 10:37 AM, shanliang wrote:
>>>>>
>>>>>> I have added 2 tasts to make sure that call
>>>>>> OpenMBean*InfoSupport.equals/hashCode do not throw NPE
>>>>>> The unit tests and JCK tests are passed.
>>>>>>
>>>>>> Webrev:http://cr.openjdk.java.net/~sjiang/JDK-8023529/00/
>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8023529
>>>>>>
>>>>>
>>>>> javax/management/openmbean/OpenMBeanInfoEqualsNPETest.java
>>>>>
>>>>> * rename the "obj1", "obj2" parameters to something more explicit 
>>>>> - eg.
>>>>> "referenceInfo" and "nullValueinfo" or something similar
>>>>>
>>>> No reference object here, the test has to call both obj1.equals and
>>>> obj2.equals.
>>>>> * when testing "obj.equals(null);" the messages should not contain
>>>>> reference to the parameter since you are not, in fact, testing the 
>>>>> null
>>>>> value of the given parameter but rather a null info object
>>>>> * the message "got NPE if null paramer" should read "got NPE for null
>>>>> paramter"
>>>>>
>>>> Right, should be "a null object" instead of "a null parameter."
>>>>> * "Thread.sleep(100);" on line 153 is necessary?
>>>>>
>>>> It is not necessary for the test, removed now.
>>>>> -----
>>>>>
>>>>> javax/management/openmbean/OpenMBeanInfoHashCodeNPETest.java
>>>>>
>>>>> * the message "got NPE if null paramer" should read "got NPE for null
>>>>> paramter"
>>>>> * "Thread.sleep(100);" on line 143 is necessary?
>>>>>
>>>> Thanks for the review, here is the new version:
>>>>
>>>> http://cr.openjdk.java.net/~sjiang/jdk-8023529/01/
>>>>
>>>> Shanliang
>>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Shanliang
>>>>>>
>>>>>
>>>>>
>>>>
>>