Assorted test fixes 2 and mutation code cleanup #39535
Conversation
ZhilkinSerg
added
Code: Tests
|
This pull request introduces 1 alert when merging 843f831 into 5a8fc16 - view on LGTM.com new alerts:
|
|
This might fix #38611 |
Not doing so affects the starvation tests.
There were two implementations of vehicle clearing in map_helpers.cpp. Refactor them into a single implementation.
Failing to do so could break the crafting tests because extra items might be available for crafting nearby, thus causing the crafting code to need to ask the player which one to use (which triggers UI code which aborts in test mode, since there's no UI).
This function was overly complicated and had corner-cases which did the wrong thing. Make it much simpler.
To aid debugging.
It could use a dangling reference when its argument was a reference into a container it erased from, leading to strange behaviour. Fix that by making a copy. Also, verify that the traits and mutations containers remain in sync as they are toggled.
Rearrange the code for two reasons:
- Avoid the weird else { return; } block.
- Avoid a potential dangling reference if the argument is a reference
into the my_mutations container.
This wasn't calling all the code that is needed when toggling traits. Remove traits via the toggle_trait function instead to ensure it behaves like normal trait removal.
In common C++ libraries, empty is an adjective, not a verb.
Each called of set_mutation or unset_mutation had to remember to call mutation_effects or mutation_loss_effects. Many of them weren't. It seems more logical to move that requirement inside (un)set_mutation. This probably fixes some bugs, although it doesn't seem to affect the tests.
Calling it leads to a lgtm warning, because it indirectly calls a virtual function. And it shouldn't do anything anyway; there's no way mutations could exist at this point in the constructor.
Let's hope. The most obviously wrong cases were mycos/marloss related mutations, which doesn't seem to correlate well with that issue, but it's hard to say. |
|
Many thanks @jbytheway for these and your other recent test fixes - they will definitely alleviate some of the frustration I've felt lately trying to write new tests. Just perusing your commits here gave me more than a few "well that explains it..." moments. Good cleanup job on the logic of |
Summary
SUMMARY: None
Purpose of change
Changes made while trying to get the tests to pass with
--order rand --rng-seed 2.Continuing from #39451.
Hopefully fixes #38611.
The game logic changes here are mostly cleanup around the mutation code where things were not always being tracked correctly. Some of this related to test failures, the rest just seemed like good cleanup while I was thinking about it.
Describe the solution
Misc changes:
clear_characternow normalizes activity level and stomach contents.clear_mapnow also clears items from the map.Character::heightis somewhat simplified. It does now behave differently in some cases, but I'm fairly sure it's more correct.Rewrite and/or refactor a bunch of the functions related to mutations & traits being added and removed for extra robustness:
empty_traitstoclear_mutations.Describe alternatives you've considered
Splitting the mutation refactoring into its own PR.
Testing
Unit tests only.