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/13 14:51:33 UTC
[GitHub] [commons-collections] Partha-SUST16 opened a new pull request, #336: Flaky test failure fix in SynchronizedBagTest#testCollectionToArray2
Partha-SUST16 opened a new pull request, #336:
URL: https://github.com/apache/commons-collections/pull/336
### What is the purpose of this PR
- This PR fixes the flaky test `org.apache.commons.collections4.bag.SynchronizedBagTest.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 `SynchronizedBagTest` 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.SynchronizedBagTest#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] testCollectionToArray2 Time elapsed: 0.002 s <<< FAILURE!
junit.framework.AssertionFailedError: toArrays should be equal expected:<[10, 11, Thirteen, null, Nine, 16, 14, Eight, , Three, 15, 12, 6.0, Seven, 4, One, 2, 5.0]> but was:<[4, 12, , 10, 16, One, Thirteen, Nine, null, 2, 5.0, 6.0, 15, Seven, 14, Eight, Three, 11]>
`
### Fix
- Create a `HashSet` from both arrays named `a` and `array` and check if they both are equal in `testCollectionToArray2`
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1256113733
@Partha-SUST16 I updated the test for unordered arrays in commit c35d8c6fd1ae28680c9912acf031173db389b80c.
Matching the length and then matching items in a Set will not detect a count mismatch of duplicates. This test now explicitly matches each item once and only once. Please raise another PR if you find any more issues. Thanks for the contribution.
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #336:
URL: https://github.com/apache/commons-collections/pull/336#discussion_r977763819
##########
src/test/java/org/apache/commons/collections4/collection/AbstractCollectionTest.java:
##########
@@ -136,6 +143,18 @@
// These fields are used by reset() and verify(), and any test
// method that tests a modification.
+ /**
+ * 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;
+
+ /**
+ * Flag to indicate the collection makes ordering guarantees for the iterator. This is used by the default
+ * implementation of {@link #getIterationBehaviour()}
+ */
+ protected static final int ORDERED = 0x0;
Review Comment:
I do not think we need this. If something is not UNORDERED then it can be assumed it is ordered.
##########
src/test/java/org/apache/commons/collections4/collection/AbstractCollectionTest.java:
##########
@@ -1095,9 +1124,15 @@ public void testCollectionToArray2() {
array = getCollection().toArray(new Object[0]);
a = getCollection().toArray();
- assertEquals("toArrays should be equal",
- Arrays.asList(array), Arrays.asList(a));
+ if((getIterationBehaviour() & UNORDERED) != 0) {
Review Comment:
Note there is a second assertion on line 1154 using assertEquals with two lists. This should also be changed to use a hashset if unordered.
##########
src/test/java/org/apache/commons/collections4/bag/SynchronizedBagTest.java:
##########
@@ -46,6 +46,10 @@ public String getCompatibilityVersion() {
return "4";
}
+ @Override
+ protected int getIterationBehaviour(){
+ return UNORDERED;
+ }
Review Comment:
Add an empty line after this method.
##########
src/test/java/org/apache/commons/collections4/collection/AbstractCollectionTest.java:
##########
@@ -487,6 +506,16 @@ public Object[] getOtherNonNullStringElements() {
};
}
+ /**
+ * Return a flag specifying the iteration behaviour of the collection.
+ * This is used to change the assertions used by specific tests.
+ * Default implementation returns {@link #ORDERED} as iteration behaviour
+ * @return the iteration behaviour
+ */
+ protected int getIterationBehaviour(){
+ return ORDERED;
Review Comment:
Just return 0 for the default behaviour
##########
src/test/java/org/apache/commons/collections4/collection/AbstractCollectionTest.java:
##########
@@ -70,6 +70,13 @@
* <li>{@link #isFailFastSupported()}
* </ul>
* <p>
+ * <b>Indicate Collection Iteration behaviour</b>
+ * </p>
+ * Override these if your collection makes no ordering guarantees for the iterator
Review Comment:
Requires a `<p>` tag before `Override`. Looking at the rest of the javadoc the paragraphs are not closed. So the `</p>` on line 74 should change to a `<p>`.
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1254917721
Thanks @aherbert for pointing in the right direction.
To fix the flaky test I have now introduced `getIterationBehaviour()` method and in our test file that contains the flaky test I have overridden this method and returns `UNORDERED` flag.
I did not make `getIterationBehaviour()` abstract as this will result in implementing this method in all subclass of `AbstractCollectionTest`. So I made this a normal method with returning `ORDERED` as default implementation.
This fix passes all the normal tests along with the flaky test that was identified.
Please let me know your thinking on this.
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1256069956
Sorry for not checking this earlier. Updated the code with spacing fix and now all checks have passed @aherbert
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1245649217
This fix may result in hiding implementation errors.
This class is used to test many collections including lists. The toArray contract states:
```
"If this collection makes any guarantees as to what order its elements are
returned by its iterator, this method must return the elements in the
same order."
```
So by ignoring the order returned by toArray the test will not detect errors in implementations that should respect the iterator order.
Some of the collections may support duplicates, e.g. lists. By using a HashSet to collate the arrays before comparison you ignore the order and may also ignore duplicates (depending on hashCode/equals implementation of stored objects).
Note that if all items in the list are the same object class then line 1119 in the same test method performs another assertEquals using two lists built from the output of toArray. It is interesting to note that your fix and verification using NonDex does not detect this as flaky; the conclusion being that many collections tested by this method store objects of more than one class in the reference collection (and thus do not hit this line).
Ideally the AbstractCollectionTest requires a behaviour flag for this test to indicate if the collection under test has a toArray method that respects the iterator order. Then assertEquals can be used with a list, otherwise a loop over the output should compare each item in 1 array is also present somewhere in the other array, see AbstractCollectionTest.verify.
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1253261371
Thanks @aherbert for your response. I agree that my fix before indeed may hide implementation errors. I have just tried another fix. Namely, changing the use of HashMap to a LinkedHashMap inside HashBag, which addresses the non-determinism in the iteration order and should not introduce implementation errors. If I am mistaken or if you prefer a different fix, please do let me know
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
Partha-SUST16 commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1255220754
Hello @aherbert, I have updated the pull request with the file change 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] codecov-commenter commented on pull request #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1245886906
# [Codecov](https://codecov.io/gh/apache/commons-collections/pull/336?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 [#336](https://codecov.io/gh/apache/commons-collections/pull/336?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bc5edea) into [master](https://codecov.io/gh/apache/commons-collections/commit/d64c9f93e5be90430134a1a06be8b2ec72e36bf3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d64c9f9) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #336 +/- ##
=========================================
Coverage 85.98% 85.98%
Complexity 4671 4671
=========================================
Files 289 289
Lines 13445 13445
Branches 1977 1977
=========================================
Hits 11561 11561
Misses 1323 1323
Partials 561 561
```
: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] aherbert commented on pull request #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1253402931
Changing the implementation to pass a test is the wrong solution. This compromises the implementation. You should change the test. Either:
- The toArray result is expected to match the iterator order. Add both to a list and use assertEquals.
- The toArray result is expected to output the same items as the iterator. Add both to a set and use assertEquals.
The test should have a behaviour flag to be overridden by implementing test cases:
```Java
/**
* 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;
/**
* Return a flag specifying the behaviour of the collection.
* This is used to change the assertions used by specific tests.
*
* @return the behaviour
*/
protected abstract int getBehaviour();
```
Update the test to handle both cases:
```Java
if ((getBehaviour() & UNORDERED) != 0) {
// Not expected to be ordered
} else {
// Order must match
}
```
--
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 #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #336:
URL: https://github.com/apache/commons-collections/pull/336#issuecomment-1256031485
This looks fine. However you have a checkstyle issue to correct. Please see the CI build failure for JDK 11. You can run the build locally to test using the default maven goal: `mvn`
--
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] asfgit closed pull request #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2
URL: https://github.com/apache/commons-collections/pull/336
--
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