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

[David Holmes]

On 11/09/2013 5:33 PM, shanliang wrote:
> David Holmes wrote:
>> I'm a bit confused. The two webrevs are vastly different:
>>
>> http://cr.openjdk.java.net/~sjiang/jdk-8023529/01/
>>
>> http://cr.openjdk.java.net/~sjiang/jdk-8023529/00/
>>
>> Should they be combined somehow?
> The version 01 is correct.
> The version 00 was a mistake, yesterday I had some problems to generate
> the version 01, so the version 00 was re-generated by mistake with
> another fix, sorry for this confusing.

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.

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.

The tests are a little verbose ie printing each successful test result, 
but that might be normal style in this area.

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
>>>>>
>>>>
>>>>
>>>
>