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/09/25 08:34:04 UTC
[GitHub] [commons-collections] Partha-SUST16 opened a new pull request, #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Partha-SUST16 opened a new pull request, #341:
URL: https://github.com/apache/commons-collections/pull/341
### What is the purpose of this PR
- This PR fixes the flaky test `org.apache.commons.collections4.bag.CollectionBagTest.testCollectionToArray2`
- The mentioned test may fail or pass without changes made to the source code when it is run in different JVMs due to `HashMap`'s non-deterministic iteration order.
### Why the test fails
- As the `CollectionBagTest` uses `HashBag` as its `collection` which uses `HashMap` internally and as per `Java 8` documentation, the ordering of `HashMap` is not constant, so assigning the same hashmap as an array to different objects can change the ordering. So when comparing these two arrays with `assertEquals` the test may fail as their ordering can be different.
### Reproduce the test failure
- Run the test with 'NonDex' maven plugin. The command to recreate the flaky test failure is
` mvn -pl . edu.illinois:nondex-maven-plugin:1.1.2:nondex -debug -Dtest=org.apache.commons.collections4.bag.CollectionBagTest#testCollectionToArray2`
- Fixing the flaky test now may prevent flaky test failures in future Java versions.
### Expected result
- The tests should run successfully when run with 'NonDex' maven with the same command for recreating the flaky test.
### Actual Result
- We get the failure:
`[ERROR] org.apache.commons.collections4.bag.CollectionBagTest.testCollectionToArray2 Time elapsed: 0.025 s <<< FAILURE! junit.framework.AssertionFailedError: toArrays should be equal expected:<[10, 6.0, 11, 4, Seven, Eight, 5.0, 15, 2, 12, 16, Thirteen, null, , Three, Nine, One, One, 14]> but was:<[, null, Seven, 2, 5.0, 12, 10, 11, 14, 16, One, One, Nine, Eight, Thirteen, 15, 4, 6.0, Three]>
`
### Fix
- Override the `getIterationBehaviour()' method from `AbstractCollectionTest` and return `UNORDERED` in `CollectionBagTest`.
--
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 #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1257147706
# [Codecov](https://codecov.io/gh/apache/commons-collections/pull/341?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 [#341](https://codecov.io/gh/apache/commons-collections/pull/341?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (58c5f0b) into [master](https://codecov.io/gh/apache/commons-collections/commit/474713f727f49707a4ac77ed737d0205d22152ee?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (474713f) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #341 +/- ##
=========================================
Coverage 85.93% 85.93%
Complexity 4669 4669
=========================================
Files 289 289
Lines 13445 13445
Branches 1977 1977
=========================================
Hits 11554 11554
Misses 1327 1327
Partials 564 564
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] Partha-SUST16 commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1257649704
Hello @aherbert, I have updated the pr with those changes
--
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] aherbert commented on a diff in pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #341:
URL: https://github.com/apache/commons-collections/pull/341#discussion_r982530182
##########
src/test/java/org/apache/commons/collections4/map/AbstractMapTest.java:
##########
@@ -1801,6 +1832,12 @@ public void verify() {
super.verify();
AbstractMapTest.this.verify();
}
+
+ @Override
+ protected int getIterationBehaviour(){
+ return AbstractMapTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -1009,6 +1034,11 @@ public boolean isTestSerialization() {
return false;
}
+ @Override
+ protected int getIterationBehaviour() {
+ return AbstractMultiValuedMapTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/multiset/AbstractMultiSetTest.java:
##########
@@ -687,6 +687,12 @@ public void resetFull() {
public void verify() {
super.verify();
}
+
+ @Override
+ protected int getIterationBehaviour() {
+ return AbstractMultiSetTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -1160,6 +1195,11 @@ public void resetEmpty() {
TestMultiValuedMapKeys.this.setConfirmed(AbstractMultiValuedMapTest.this.getConfirmed().keys());
}
+ @Override
+ protected int getIterationBehaviour() {
+ return AbstractMultiValuedMapTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/map/AbstractMapTest.java:
##########
@@ -524,6 +538,18 @@ public void testSampleMappings() {
}
}
+ /**
+ * Return a flag specifying the iteration behaviour of the collection.
+ * This is used to change the assertions used by specific tests.
+ * The default implementation returns 0 which indicates ordered iteration behaviour.
+ *
+ * @return the iteration behaviour
+ * @see #UNORDERED
Review Comment:
Change to `@see AbstractCollectionTest#UNORDERED`
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -211,6 +218,18 @@ public void resetFull() {
}
}
+ /**
+ * Return a flag specifying the iteration behaviour of the map.
+ * This is used to change the assertions used by specific tests.
+ * The default implementation returns 0 which indicates ordered iteration behaviour.
+ *
+ * @return the iteration behaviour
+ * @see #UNORDERED
+ */
+ protected int getIterationBehaviour(){
Review Comment:
Add a space: `() {`
This method only applies to the iteration of the values of the multi map when it is tested as a collection. I think the method name can be the same as that used in AbstractCollectionTest. But the property should be reused. So UNORDERED must be made public in AbstractCollectionTest.
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -1257,6 +1297,12 @@ public boolean areEqualElementsDistinguishable() {
public boolean isTestSerialization() {
return false;
}
+
+ @Override
+ protected int getIterationBehaviour() {
+ return AbstractMultiValuedMapTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -1090,6 +1120,11 @@ public Collection<V> makeConfirmedFullCollection() {
return null;
}
+ @Override
+ protected int getIterationBehaviour() {
+ return AbstractMultiValuedMapTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -60,6 +60,13 @@
/** MultiValuedHashMap created by reset(). */
protected MultiValuedMap<K, V> confirmed;
+ /**
+ * Flag to indicate the map makes no ordering guarantees for the iterator. If this is not used
+ * then the behaviour is assumed to be ordered and the output order of the iterator is matched by
+ * the toArray method.
+ */
+ protected static final int UNORDERED = 0x1;
Review Comment:
Remove this. The property to use when configuring the behaviour of the AbstractCollectionTest should be the flag defined in AbstractCollectionTest.
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -954,6 +973,12 @@ public Collection<Entry<K, V>> makeConfirmedFullCollection() {
return null;
}
+
+ @Override
+ protected int getIterationBehaviour() {
+ return AbstractMultiValuedMapTest.this.getIterationBehaviour();
+ }
+
Review Comment:
Remove empty line
##########
src/test/java/org/apache/commons/collections4/multimap/AbstractMultiValuedMapTest.java:
##########
@@ -211,6 +218,18 @@ public void resetFull() {
}
}
+ /**
+ * Return a flag specifying the iteration behaviour of the map.
+ * This is used to change the assertions used by specific tests.
+ * The default implementation returns 0 which indicates ordered iteration behaviour.
+ *
+ * @return the iteration behaviour
+ * @see #UNORDERED
Review Comment:
This javadoc will change to `@see AbstractCollectionTest#UNORDERED`
##########
src/test/java/org/apache/commons/collections4/bag/HashBagTest.java:
##########
@@ -45,6 +45,11 @@ public String getCompatibilityVersion() {
return "4";
}
+ @Override
+ protected int getIterationBehaviour() {
+ return UNORDERED;
+ }
+
// public void testCreate() throws Exception {
Review Comment:
Add an empty line here
##########
src/test/java/org/apache/commons/collections4/map/AbstractMapTest.java:
##########
@@ -134,6 +141,13 @@
JDK12 = str.startsWith("1.2");
}
+ /**
+ * Flag to indicate the collection makes no ordering guarantees for the iterator. If this is not used
+ * then the behaviour is assumed to be ordered and the output order of the iterator is matched by
+ * the toArray method.
+ */
+ protected static final int UNORDERED = 0x1;
Review Comment:
Remove this. The property from AbstractCollectionTest should be used as it is configuring the test within that class.
--
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] Partha-SUST16 commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1259056693
Hello @aherbert , simmilar fix was only applicable to the `set` & `multiset` test files as they all extends the `AbstractCollectionTest` class. But for `multimap` & `bidimap` they extends the class `AbstractMapTest` which extends `AbstractObjectTest`. So similar fix like just overriding the `getIterationOrder` method directly won't work.
I have pushed for the set and multiset files only
--
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] Partha-SUST16 commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1257814925
I will update them & will update the pr.
--
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] Partha-SUST16 commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1260455179
@aherbert, I believe this now takes care of the flaky tests related to 'testCollectionToArray2' that you mentioned
--
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] Partha-SUST16 commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1257148269
I have found some more flaky tests for `HashBagTest.testCollectionToArray2`, `PredicatedBagTest.testCollectionToArray2`,
`SynchronizedBagTest.testCollectionToArray2`, `TransformedBagTest.testCollectionToArray2`, `UnmodifiableBagTest.testCollectionToArray2`. All of this is caused for the same reason (using `HashBag`) internally.
& their fix is also same.
Should I make individual pr for all of them or include here ?
--
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] aherbert merged pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
aherbert merged PR #341:
URL: https://github.com/apache/commons-collections/pull/341
--
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] aherbert commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1259686944
The Map tests create an inner class that extends AbstractCollectionTest (see `AbstractMapTest.TestMapValues`). I think this can be updated to specify the iteration behaviour as well. It would be simple to just return unordered for this. However some maps are ordered/sorted. So perhaps add a method in AbstractMapTest for getIterationBehaviour documented to be used by views of the map that are tested as a collection, namely the map values. This can be used by the inner class which extends AbstractCollectionTest. The child classes for maps that are unordered can set the correct behaviour.
--
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] aherbert commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1257208824
Please add the fixes to this PR since they are all related.
--
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] aherbert commented on pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #341:
URL: https://github.com/apache/commons-collections/pull/341#issuecomment-1257810785
I agree with the current additional fixes. However it does not correct all issues identified by nondex with this test. When I run:
```
mvn -pl . edu.illinois:nondex-maven-plugin:1.1.2:nondex -Drat.skip
```
I get a few other flaky tests identified. Some of these are also in **testCollectionToArray2**:
```
[INFO] Across all seeds:
[INFO] org.apache.commons.collections4.multimap.HashSetValuedHashMapTest#testToString
[INFO] HashSetValuedHashMapTest#testCollectionToArray2
[INFO] bulkTestMultiValuedMapKeys#testCollectionToArray2
[INFO] bulkTestAsMap#testCollectionToArray2
[INFO] PassiveExpiringMapTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.bidimap.UnmodifiableBidiMapTest#testBidiKeySetValuesOrder
[INFO] UnmodifiableBidiMapTest#testCollectionToArray2
[INFO] UnmodifiableBidiMapTest#testBidiKeySetValuesOrder
[INFO] bulkTestInverseMap#testCollectionToArray2
[INFO] org.apache.commons.collections4.set.TransformedSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.multiset.UnmodifiableMultiSetTest#testCollectionToArray2
[INFO] UnmodifiableMultiSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.multimap.ArrayListValuedHashMapTest#testMultiValuedMapIterator
[INFO] ArrayListValuedHashMapTest#testCollectionToArray2
[INFO] UnmodifiableMultiValuedMapTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.bidimap.DualHashBidiMapTest#testBidiKeySetValuesOrder
[INFO] DualHashBidiMapTest#testCollectionToArray2
[INFO] DualHashBidiMapTest#testBidiKeySetValuesOrder
[INFO] TransformedMultiValuedMapTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.set.PredicatedSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.multiset.HashMultiSetTest#testCollectionToArray2
[INFO] HashMultiSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.multiset.PredicatedMultiSetTest#testCollectionToArray2
[INFO] PredicatedMultiSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.set.CompositeSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.CollectionUtilsTest#get
[INFO] org.apache.commons.collections4.set.UnmodifiableSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.multiset.SynchronizedMultiSetTest#testCollectionToArray2
[INFO] SynchronizedMultiSetTest#testCollectionToArray2
[INFO] org.apache.commons.collections4.multimap.TransformedMultiValuedMapTest#testMultiValuedMapIterator
[INFO] org.apache.commons.collections4.multimap.ArrayListValuedHashMapTest#testToString
```
I make this 22 / 31 flaky tests are in testCollectionToArray2. Could the same fix be applied to those tests?
--
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