[GitHub] [commons-collections] kinow commented on a diff in pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…

[GitBox <gi...@apache.org>]

kinow commented on code in PR #300:
URL: https://github.com/apache/commons-collections/pull/300#discussion_r857185327


##########
src/test/java/org/apache/commons/collections4/map/ReferenceMapTest.java:
##########
@@ -315,6 +316,24 @@ public void testDataSizeAfterSerialization() throws IOException, ClassNotFoundEx
 
     }
 
+    /**
+     * Test whether remove is not removing last entry after calling hasNext.
+     * <p>
+     * See <a href="https://issues.apache.org/jira/browse/COLLECTIONS-802">COLLECTIONS-802: ReferenceMap iterator remove violates contract</a>
+     */
+    @Test
+    public void testIteratorLastEntryCanBeRemovedAfterHasNext() {
+        ReferenceMap<Integer, Integer> map = new ReferenceMap<>();
+        map.put(1, 2);
+        Iterator<Map.Entry<Integer, Integer>> iter = map.entrySet().iterator();
+        assertTrue(iter.hasNext());
+        iter.next();
+        // below line should not affect remove
+        assertFalse(iter.hasNext());
+        iter.remove();
+        assertTrue("Expect empty but have entry: " + map, map.isEmpty());
+    }

Review Comment:
   Fix looks OK, and the test is failing on `master`, passing on this branch. We could also modify the test to be more similar to the one reported in the issue.
   
   ```diff
   diff --git a/src/test/java/org/apache/commons/collections4/map/ReferenceMapTest.java b/src/test/java/org/apache/commons/collections4/map/ReferenceMapTest.java
   index 509ac514..c6625909 100644
   --- a/src/test/java/org/apache/commons/collections4/map/ReferenceMapTest.java
   +++ b/src/test/java/org/apache/commons/collections4/map/ReferenceMapTest.java
   @@ -327,7 +327,8 @@ public class ReferenceMapTest<K, V> extends AbstractIterableMapTest<K, V> {
            map.put(1, 2);
            Iterator<Map.Entry<Integer, Integer>> iter = map.entrySet().iterator();
            assertTrue(iter.hasNext());
   -        iter.next();
   +        assertTrue(iter.hasNext());
   +        assertEquals(Integer.valueOf(1), iter.next().getKey());
            // below line should not affect remove
            assertFalse(iter.hasNext());
            iter.remove();
   ```
   But not really important, the last `assertTrue` is where the test fails on `master`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org