Code Review Request for Bug #4802647

[Brandon Passanisi]

Hello core-libs.  I was wondering if somebody could please review the 
following updated webrev for this bug.  There was an e-mail thread 
regarding the first webrev [1] that prompted the changes in this webrev.

    Webrev:  http://cr.openjdk.java.net/~mduigou/4802647/1/webrev/
    Bug URL: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4802647

Here is a summary of the changes:

1. I followed the suggestion to look into Collection/MOAT.java and 
decided that updating this test was better than using the new test 
program I had written in the previous webrev named 
RemoveAllRetainAllNPE.java

2. I noticed that MOAT.java didn't directly test AbstractCollection.java 
and AbstractSet.java.  So, I added the testing of these classes to 
MOAT.java on lines 62 and 63 of the updated webrev.

3. While MOAT.java does test removeAll(null) and retainAll(null), it doesn't test the scenario where the collection is empty when calling c.removeAll(null) and c.retainAll(null).  The current testing in MOAT.java uses a collection with one element when testing removeAll(null) and retainAll(null).  So, I added code for the testing of removeAll(null) and retainAll(null) with an empty collection, which is the scenario for the behavior in the bug report.  These are the changes on lines 759 - 766, 775 - 782 of MOAT.java.

4. The added classes NewAbstractColection and NewAbstractSet were intentionally written to be very basic and were inspired from the source in the bug report.  Is there a better way to write these classes or are they ok as-is?

5. The changes to MOAT.java in this updated webrev result in more classes that exhibit the same bug behavior described in this bug report for AbstractCollection (no NPE thrown).  These classes are: java.util.ArrayList and java.util.concurrent.CopyOnWriteArrayList.  Should a new bug be filed for these cases?

Thanks.


[1]: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-November/008283.html


On 12/2/2011 2:34 AM, Alan Bateman wrote:
> On 01/12/2011 22:42, Brandon Passanisi wrote:
>> Hi Jason.  Thanks for your response.  I was thinking about how I can 
>> improve the test using your suggestion. I could possibly do the 
>> following:
>>
>>    1. Find all of the subclasses of AbstractCollection which override
>>    removeAll(Collection<?>) and which also contain the spec language
>>    which specifies that NullPointerException is thrown if the specified
>>    collection is null.
>>
>>    2. Find all of the subclasses of AbstractCollection which override
>>    retainAll(Collection<?>) and which also contain the spec language
>>    which specifies that NullPointerException is thrown if the specified
>>    collection is null.
>>
>>    3. Add the classes found in #1 and #2 to the test.
>>
>>    4. If any of the new classes added because of  #3 result in a test
>>    failure, it might be a good idea to file a new bug as Bug #4802647
>>    specifically mentions subclasses of AbstractCollection which do not
>>    override remainAll, retainAll.
>>
>>    5. The public subclasses of AbstractCollection which do not override
>>    removeAll, retainAll (probably) shouldn't be included in the test as
>>    the currently existing NewAbstractCollection represents this 
>> scenario.
>>
>> What do you think?
>>
> Brandon - it's probably worth getting familiar with the existing 
> tests, in in particular Martin's "Mother Of All Test" 
> (Collection/MOAT.java).
>
> -Alan.

-- 
Oracle <http://www.oracle.com>
Brandon Passanisi | Principle Member of Technical Staff

Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
developing practices and products that help protect the environment