On Thu, 13 Nov 2025 00:27:10 GMT, Stuart Marks <[email protected]> wrote:
>> jengebr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Whitespace fixes
>
> test/jdk/java/util/Collection/MOAT.java line 890:
>
>> 888: }
>> 889:
>> 890: private static void testAddAll(Collection<Integer> c) {
>
> Thanks for moving this test into MOAT. Overall it seems like a large
> reduction in test bulk, which simplifies a lot of things. Great!
Yes, thank you for pointing it out! I overlooked its existence until you
pointed me to it, I agree it's a a much better option.
> 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.
> test/jdk/java/util/Collection/MOAT.java line 922:
>
>> 920: check(c.addAll(arraySource));
>> 921: equal(c.size(), sizeBefore + arraySource.size());
>> 922: check(c.containsAll(arraySource));
>
> As I said in another comment, this (size check and containsAll check) looks
> like a better set of assertions than using List equality as in the earlier
> test cases.
>
> I'm confused about the scope of cases being covered here. It seems like there
> are potentially three different axes of cases that potentially could be
> tested:
>
> 1) source is ArrayList / other kind of List
> 2) source is empty / not empty
> 3) destination is empty / not empty
>
> That would indicate having eight test cases. That's kind of at the outer
> limit for hand-coded cases. At this point or beyond, having some automated
> case generation would be preferable. And this is MOAT and not JUnit, so test
> generation would have to be done _ad hoc_. I could imagine doing it in about
> the same amount of code (say 30 or fewer lines). But if you're not up for
> doing this, it's probably sufficient for this change to test just the
> ArrayList and non-ArrayList sources, since that should be sufficient to test
> the code path you're changing.
> At this point or beyond, having some automated case generation would be
> preferable. And this is MOAT and not JUnit, so test generation would have to
> be done ad hoc
Thank you! I will reduce the testing to the dimension #1. Your suggestion of
a JUnit-based equivalent is excellent, I'll work it in sometime in the future.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2523821114
PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2523814719
PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2523810534
