[GitHub] [commons-collections] aherbert commented on a diff in pull request #336: Fix flaky test failure in SynchronizedBagTest#testCollectionToArray2

[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