The Messenger, LADX: use collect and remove as intended #2093
Conversation
|
I'll review this later. |
worlds/ladx/test/testShop.py
Outdated
| def test_planned(self): | ||
| """Tests plandoing swords in the shop.""" | ||
| self.multiworld.plando_items[1] = self.options["plando_items"] | ||
| distribute_planned(self.multiworld) |
There was a problem hiding this comment.
This seems jank.
Set up in this way, distribute_planned is never called when running the default tests in this class, making them effectively useless.
Also, this ends up running distribute_planned after pre_fill, while it is usually the other way round in Main.py.
Ideally, you would find a way to do this with a custom setup that lays everything out properly.
But if that can't be done, try disabling the default tests here at least.
There was a problem hiding this comment.
overrode the world_setup so that the steps are actually called correctly and added that the locations shouldn't be reachable with starting state. I left the default tests to run because that'll also test the locations are reachable with all_state though I guess I could explicit do that and remove the others.
There was a problem hiding this comment.
Running the default tests is good if they test something that is useful (e.g., that the locations become reachable) and in a scenario that is related to what is being tested in the class (i.e., actually using plando here), which seems to have been achieved now.
[This was written as an answer into the existing review of that test; not sure why GitHub turned it into a separate conversation as well.]
|
It might be worth mentioning explicitly that this PR is supposed to supersede #2064. |
worlds/ladx/test/testShop.py
Outdated
| def test_planned(self): | ||
| """Tests plandoing swords in the shop.""" | ||
| self.multiworld.plando_items[1] = self.options["plando_items"] | ||
| distribute_planned(self.multiworld) |
There was a problem hiding this comment.
Running the default tests is good if they test something that is useful (e.g., that the locations become reachable) and in a scenario that is related to what is being tested in the class (i.e., actually using plando here), which seems to have been achieved now.
[This was written as an answer into the existing review of that test; not sure why GitHub turned it into a separate conversation as well.]
|
Needs review from @zig-for for touched LADX code. |
ThePhar
added
the
is: refactor/cleanup
|
Can't find the button to approve, but I approve |
|
@alwaysintreble can you resolve these conflicts (maybe merge in main too so newest tests can run) and ping me once it's good? Otherwise, looks fine to me.
|
# Conflicts: # worlds/ladx/__init__.py # worlds/messenger/__init__.py
…W#2093) Co-authored-by: el-u <[email protected]>
What is this fixing or adding?
Was pointed out to me that I shouldn't be modifying state in
collect_itemso this fixes that behavior. LADX does nearly the same thing but was failing #2062 so I just copied over the same behavior while I was at it to fix that.How was this tested?
Unit tests and debug break points to confirm the changes to state were correct.
If this makes graphical changes, please attach screenshots.