Code Review Request for Bug #4802647

[David Holmes]

Brandon,

I don't see the purpose of NewAbstractSet. It is identical to 
NewAbstractCollection.

Otherwise my only concern is as you raised previously, updating the test 
to check existing subclasses causes new failures for ArrayList and 
CopyOnWriteArrayList. I'd suggest fixing ArrayList as part of this CR 
and file a new CR for COWAL so that Doug Lea can fix it upstream in the 
jsr166 cvs repository.

David

On 20/12/2011 4:53 AM, Brandon Passanisi wrote:
> 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.
>