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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
