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

[David Holmes]

You didn't print the NPE stacktrace in OpenMBeanInfoHashCodeNPETest.java

Otherwise ok.

No need to rereview if you add the stacktrace printing.

Thanks,
David

On 11/09/2013 10:32 PM, shanliang wrote:
> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>