You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/04/24 16:32:57 UTC
[GitHub] [commons-collections] samabcde opened a new pull request, #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
samabcde opened a new pull request, #300:
URL: https://github.com/apache/commons-collections/pull/300
…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.
--
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
[GitHub] [commons-collections] kinow commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1108008975
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.
--
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
[GitHub] [commons-collections] kinow commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1107931652
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 :tada:
--
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
[GitHub] [commons-collections] kinow commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1108007330
Thanks @ben-manes, much easier now to run the tests and compare the results with your updated test.
For `master`:
```bash
[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:
```bash
[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 :clap:
--
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
[GitHub] [commons-collections] kinow commented on a diff in pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #300:
URL: https://github.com/apache/commons-collections/pull/300#discussion_r860735919
##########
src/main/java/org/apache/commons/collections4/map/AbstractReferenceMap.java:
##########
@@ -794,23 +794,21 @@ protected void nullValue() {
public boolean hasNext() {
checkMod();
while (nextNull()) {
- ReferenceEntry<K, V> e = entry;
+ ReferenceEntry<K, V> e = next;
int i = index;
while (e == null && i > 0) {
i--;
e = (ReferenceEntry<K, V>) parent.data[i];
}
- entry = e;
+ next = e;
index = i;
if (e == null) {
- currentKey = null;
- currentValue = null;
Review Comment:
Fix is here :point_up: , the rest is renaming variables.
--
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
[GitHub] [commons-collections] kinow commented on a diff in pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by 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
[GitHub] [commons-collections] ben-manes commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
ben-manes commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1107932533
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.
--
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
[GitHub] [commons-collections] kinow commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1107932772
> 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 !
--
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
[GitHub] [commons-collections] codecov-commenter commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1107921839
# [Codecov](https://codecov.io/gh/apache/commons-collections/pull/300?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#300](https://codecov.io/gh/apache/commons-collections/pull/300?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (43e23dd) into [master](https://codecov.io/gh/apache/commons-collections/commit/9df6f64b7ea729fff5d11fce6407cba249baafaa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9df6f64) will **increase** coverage by `0.05%`.
> The diff coverage is `88.88%`.
```diff
@@ 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](https://codecov.io/gh/apache/commons-collections/pull/300?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...commons/collections4/map/AbstractReferenceMap.java](https://codecov.io/gh/apache/commons-collections/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L21hcC9BYnN0cmFjdFJlZmVyZW5jZU1hcC5qYXZh) | `90.37% <88.88%> (+2.13%)` | :arrow_up: |
| [.../apache/commons/collections4/map/ReferenceMap.java](https://codecov.io/gh/apache/commons-collections/pull/300/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L21hcC9SZWZlcmVuY2VNYXAuamF2YQ==) | `75.00% <0.00%> (+12.50%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/commons-collections/pull/300?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/commons-collections/pull/300?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9df6f64...43e23dd](https://codecov.io/gh/apache/commons-collections/pull/300?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [commons-collections] kinow closed pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
kinow closed pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
URL: https://github.com/apache/commons-collections/pull/300
--
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
[GitHub] [commons-collections] ben-manes commented on pull request #300: [COLLECTIONS-802] Fix remove failed by removing set null to currentKe…
Posted by GitBox <gi...@apache.org>.
ben-manes commented on PR #300:
URL: https://github.com/apache/commons-collections/pull/300#issuecomment-1107935459
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.
```java
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();
}
}
```
<img width="1792" alt="Screen Shot 2022-04-24 at 4 04 37 PM" src="https://user-images.githubusercontent.com/378614/165000396-a85c13ea-9b0e-4206-88f7-8c115cf66985.png">
--
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