On Thu, 13 Nov 2025 15:05:28 GMT, jengebr <[email protected]> wrote: >> test/jdk/java/util/Collection/MOAT.java line 905: >> >>> 903: arraySource.add(99); >>> 904: check(c.addAll(arraySource)); >>> 905: equal(new ArrayList<Integer>(c), arraySource); >> >> Here and at line 913 it seems a bit odd to copy `c` into a new ArrayList to >> compare equality. I think it's trying to assert that `c` contains only the >> expected elements. Unfortunately `c` can be any collection, not just a List, >> and to use List equality it needs to be copied into an actual List. Doubly >> unfortunately, the new List will capture the encounter order of whatever >> collection `c` is, which might not be well-defined -- for example if `c` is >> a HashSet. So I don't think this is the right assertion. Probably a size >> check and a containsAll() check, as is done in the bottom case, is >> sufficient. > > I'm curious, why not .equals() when we know the input is a List? That is a > stricter assertion because it ensures ordering remains unchanged. > > Happy to make the change, though.
The testCollection1() method gets called with a bunch of different collections, including a HashSet, which arrives at this code in the `c` parameter. When `c` is copied into a new ArrayList, it gets whatever order is presented by `c`, which is unspecified when `c` is a HashSet. The resulting order might differ from the order in the source list, and equals() over Lists compares order, so it might result in a spurious failure. I note that creating a HashSet of capacity 8 and adding 42 and then 99 results in [42, 99] but doing the same with a HashSet of capacity 16 results in [99, 42]. The test might actually pass, but if it does, it seems like it's pretty brittle. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2525367497
