[COLLECTIONS-802] Fix remove failed by removing set null to currentKe…

[samabcde]

…y and currentValue.

• The problem occur when the iterator called hasNext and return false, which set null to currentKey. Hence remove method calling parent.remove(currentKey); will not remove the current entry.
• Propose to fix by removing lines setting null, other than releasing the reference earlier, can't think of other reason to set them to null.
• Rename variable to be more understandable.

[codecov-commenter]

Codecov Report

Merging #300 (43e23dd) into master (9df6f64) will increase coverage by 0.05%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master     #300      +/-   ##
============================================
+ Coverage     85.82%   85.88%   +0.05%     
- Complexity     4674     4676       +2     
============================================
  Files           292      292              
  Lines         13471    13469       -2     
  Branches       1955     1955              
============================================
+ Hits          11562    11568       +6     
+ Misses         1330     1324       -6     
+ Partials        579      577       -2     

Impacted Files	Coverage Δ
...commons/collections4/map/AbstractReferenceMap.java	90.37% <88.88%> (+2.13%)	⬆️
.../apache/commons/collections4/map/ReferenceMap.java	75.00% <0.00%> (+12.50%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9df6f64...43e23dd. Read the comment docs.

[kinow]

Hi,

I did as Ben Manes suggested in the JIRA, downloading the ApacheMapTest.java (JIRA attachment), then with the diff below applied to pom.xml and also some changes to the code (older JVM version, no var statement), found that this PR breaks other tests in ApacheMapTest. We will have to verify if those are not indicators of regressions after this change (probably we will have to update the code in other places…)

+    <dependency>
+      <groupId>com.google.guava</groupId>
+      <artifactId>guava-testlib</artifactId>
+      <version>31.1-jre</version>
+      <scope>test</scope>
+    </dependency>

Bruno

[kinow]

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

[kinow]

Ah, I think I may have spoken too fast. I executed the ApacheMapTest against master, and got this:

[ERROR] Tests run: 25662, Failures: 8, Errors: 139, Skipped: 4

And then against this branch:

[ERROR] Tests run: 25664, Failures: 2, Errors: 139, Skipped: 4

So looks like this PR is actually reducing the failures 🎉

[kinow]

Approving as the CI here is green, and the other tests in ApacheMapTest using Guava testlib appear to have improved. Sorry the noise, I thought it was supposed to have no more errors in that test 🙇

Thanks @samabcde ! Will leave it open so others can review it too.

[ben-manes]

oops. I must have forgotten, not realized, or master has new failures. I very much like Guava’s collection tests as reusable and catch these minor mistakes. I’d recommend using it elsewhere as another sanity check over your own great testing.

[kinow]

oops. I must have forgotten, not realized, or master has new failures. I very much like Guava’s collection tests as reusable and catch these minor mistakes. I’d recommend using it elsewhere as another sanity check over your own great testing.

I hadn't heard about that before, but looks super useful. But from the amount of errors, that would probably be an epic task, maybe even worth of GSoC, I think.

Thanks @ben-manes !

[ben-manes]

Checking and I accidentally attached the wrong version of ApacheMapTest. 😦

The flag allowNulls should have been false for ReferenceMap, and was there just because every collection author does this differently so I had to experiment to find Apache's setting. The corrected test case is below.

import java.util.Map;
import java.util.function.Supplier;

import org.apache.commons.collections4.map.HashedMap;
import org.apache.commons.collections4.map.LRUMap;
import org.apache.commons.collections4.map.LinkedMap;
import org.apache.commons.collections4.map.ReferenceMap;

import com.google.common.collect.testing.MapTestSuiteBuilder;
import com.google.common.collect.testing.TestStringMapGenerator;
import com.google.common.collect.testing.features.CollectionFeature;
import com.google.common.collect.testing.features.CollectionSize;
import com.google.common.collect.testing.features.MapFeature;

import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public final class ApacheMapTest extends TestCase {

  public static Test suite() {
    var test = new TestSuite();
    test.addTest(suite("HashedMap", HashedMap::new));
    test.addTest(suite("LinkedMap", LinkedMap::new));
    test.addTest(suite("LRUMap", LRUMap::new));
    test.addTest(suite("ReferenceMap", ReferenceMap::new));
    return test;
  }

  public static Test suite(String name, Supplier<Map<String, String>> factory) {
    return MapTestSuiteBuilder
        .using(new TestStringMapGenerator() {
          @Override protected Map<String, String> create(Map.Entry<String, String>[] entries) {
            var map = factory.get();
            for (var entry : entries) {
              map.put(entry.getKey(), entry.getValue());
            }
            return map;
          }
        })
        .named(name)
        .withFeatures(
            CollectionSize.ANY,
            MapFeature.GENERAL_PURPOSE,
            MapFeature.ALLOWS_ANY_NULL_QUERIES,
            CollectionFeature.SUPPORTS_ITERATOR_REMOVE)
        .createTestSuite();
  }
}

[kinow]

Thanks @ben-manes, much easier now to run the tests and compare the results with your updated test.

For master:

[ERROR] Failures: 
[ERROR]   CollectionIteratorTester.testIterator_unknownOrderRemoveSupported:111->runIteratorTest:133 failed with stimuli [hasNext, hasNext, next, hasNext, remove]
[ERROR]   CollectionIteratorTester.testIterator_unknownOrderRemoveSupported:111->runIteratorTest:133 failed with stimuli [hasNext, hasNext, next, hasNext, remove]
[ERROR]   CollectionIteratorTester.testIterator_unknownOrderRemoveSupported:111->runIteratorTest:133 failed with stimuli [hasNext, hasNext, next, hasNext, remove]
[ERROR]   CollectionIteratorTester.testIterator_unknownOrderRemoveSupported:111->runIteratorTest:133 failed with stimuli [next, next, next, hasNext, remove]
[ERROR]   CollectionIteratorTester.testIterator_unknownOrderRemoveSupported:111->runIteratorTest:133 failed with stimuli [next, next, next, hasNext, remove]
[ERROR]   CollectionIteratorTester.testIterator_unknownOrderRemoveSupported:111->runIteratorTest:133 failed with stimuli [next, next, next, hasNext, remove]
[INFO] 
[ERROR] Tests run: 25518, Failures: 6, Errors: 0, Skipped: 4
[INFO] 

Then for this branch:

[INFO] Results:
[INFO] 
[WARNING] Tests run: 25520, Failures: 0, Errors: 0, Skipped: 4

So no regressions in our CI, nor in the ApacheMapTest after this change 👏

[kinow]

Also created a follow-up issue, https://issues.apache.org/jira/browse/COLLECTIONS-811, to consider adding Guava's testlib or similar solution to our tests/CI.

[kinow]

LGTM

[kinow]

Fix is here ☝️ , the rest is renaming variables.