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/28 15:34:22 UTC
[GitHub] [commons-collections] aherbert commented on a diff in pull request #341: Fix flaky test occurred in 'CollectionBagTest.testCollectionToArray2'
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