Tests: test that collect and remove have expected behaviour.

[Berserker66]

What is this fixing or adding?

test that collect and remove have expected behaviour

How was this tested?

test

If this makes graphical changes, please attach screenshots.

[el-u]

Please consider reviewing #2093 as well for some progress on a related topic.

Out of scope feature request removed.

[el-u]

Seems like the variable base_state could be removed and empty_prog_items be used instead (once the bug on the line below has been fixed.)

Out of scope feature request removed.

[el-u]

I am not sure what to do about the remaining Python 3.8 failures of Link's Awakening and The Messenger. The collect->remove test fails, not because the code of #2093 is incorrect, but because the testing mechanic (equality tests of Counter objects) is simply not adequate before Python 3.10. Maybe the collect->remove portion of the test could be gated for Python >= 3.10 only.

Out of scope feature request removed.

[NewSoupVi]

Yacht Dice and Tunic are doing stuff like this

    def collect(self, state: CollectionState, item: Item) -> bool:
        change = super().collect(state, item)
        if change and ...:
            state.prog_items[self.player]["stale"] = 1
        return change

    def remove(self, state: CollectionState, item: Item) -> bool:
        change = super().remove(state, item)
        if change and ...:
            state.prog_items[self.player]["stale"] = 1
        return change

to track the staleness of some custom state.

This unit test would break that.
I believe this to be a perfectly valid use case.

It actually works out for Yacht Dice (because in Yacht Dice's case, the fake prog item is called state_is_fresh and is set to 0 instead of being state_is_stale and being set to 1)
The new Tunic changes would fail this unit test.

What Tunic does is it lazily calculates and caches the accessibility of its areas by a specific metric and then marks them as "succeeded" (2), "failed" (1) or "unchecked" (0), as key-value pairs on state.prog_items (key = area name, value = unchecked/failed/succeeded).

A collect causes all "failed" areas to go back to "unchecked", but leaves "succeeded" areas as "succeeded" (cause gaining an item can never fail a previously successful check).
A remove causes all "succeeded" areas to go back to "unchecked", but leaves "failed" as "failed" (cause losing an item can't make something succeed that previously failed).

This means that collect -> remove could leave a lot of areas as "failed" (1) that were previously "unchecked" (0). Which is correct. Information was literally gained about the accessibility of areas, that is fine to persist through the remove.

[Berserker66]

This unit test would break that. I believe this to be a perfectly valid use case.

I believe it's not, and is why CollectionState extending exists.

[qwint]

for a simple test that can catch mistakes this is perfect, code makes sense and has already caught multiple mistakes when I have used it,

That being said it is not the end all be all of testing collect/remove because any complex collect/remove that changes depending on what the current state is, will not be fully tested because state will always be empty (or fail the test) for each new item.
I don't think this is a reason not to merge as-is, but we should be aware that some issues (like HK's split cloak issues, see #1844) may not be caught with just this test

[qwint]

per discussion in ap-core-dev, this line is setting by reference and causing errors to go unnoticed

a couple of things seem to work, that I don't have much of an opinion on which is better

• deepcopy when copying prog_items
• copying CollectionState instead and using that state's prog_items when comparing
• copy a new CollectionState for each item_name and using that state's collect() instead of copying the state back to multiworld each loop

[NewSoupVi]

Ftr, I was just made aware that this PR includes a test that non-progression doesn't change state

Wanted to make you aware of an alternative PR that just straight up asserts this
Core: CollectionState - Never pick up non-advancements #3786

[qwint]

Opened #4468 that handles the issue I still have with the PR, the other approaches I outlined in my last comment are still valid, but I implemented the approach that made the most sense to me

[qwint]

FYI test failures are appropriate and handled by #4433