Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/eoc: use standard NPC in the EOC_run_eocs test #77950

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Nov 17, 2024

Summary

None

Purpose of change

The EOC_run_eocs test uses a dummy npc with no id that breaks assumptions in other parts of the code, as seen in this job run: that mission is assigned with no ID, but when this dummy NPC dies it fails the mission because its ID matches, then the (pointless) debug message shows up because we've already cleared missions on the avatar.

Describe the solution

Use a standard test NPC

Describe alternatives you've considered

Robustify test infra to ensure there's no stale state for any test unit?

Testing

../build_lto/cata_test --rng-seed 1731840727 "~[slow] ~[.],starting_items" doesn't trigger the error

Wait for LTO job to finish.

Additional context

Perfect example of why we need LTO and ASan jobs to run earlier. It would have taken me <5 minutes to track this down (instead of 2 hours) if CI had run the LTO job for #77359

@github-actions github-actions bot added Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` EOC: Effects On Condition Anything concerning Effects On Condition new contributor astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Nov 17, 2024
@RenechCDDA
Copy link
Member

Uh related, #72167

Since this has come up at least twice, maybe there are even better guard rails? I have no concrete suggestions though

@andrei8l
Copy link
Contributor Author

Uh related, #72167

Same issue, yeah. But I had forgotten how to spawn test NPCs so I hacked it and it was bad.

Since this has come up at least twice, maybe there are even better guard rails? I have no concrete suggestions though

Nope, let's not hide blatant code errors with fallbacks.

@RenechCDDA
Copy link
Member

I meant something like forcing the constructor call to always assign a relevant ID. i.e. it shouldn't be possible to get a NPC reference with an invalid ID without it already yelling.

I don't know if that's feasible though. Please ignore me if this sounds unreasonable.

I would also consider checking the NPC assignment ID when completing the mission, and specifically mentioning it in the debugmsg(to help with debugging). Though I can see that being undesirable.

@Night-Pryanik Night-Pryanik merged commit a6beb86 into CleverRaven:master Nov 21, 2024
21 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. EOC: Effects On Condition Anything concerning Effects On Condition json-styled JSON lint passed, label assigned by github actions new contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants