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

(Fix for tests) Assign valid IDs to NPCs used during testing #72167

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

RenechCDDA
Copy link
Member

@RenechCDDA RenechCDDA commented Mar 4, 2024

Summary

None

Purpose of change

https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/8139343413/job/22242297663?pr=72106#step:17:78 looks like this was broken by the morale test I added in #72018

Describe the solution

Change error reporting so we can figure out what mission this is even supposed to be. The integer id is not useful

(After we figure it out) fix the actual cause --> Assign all NPCs, even test NPCs, valid IDs. Then when the test NPC dies and its ID is checked for various things (like failing missions), no unintended interactions happen.

Describe alternatives you've considered

Testing

Wait to see if GCC 9, Curses, LTO builds and tests pass on it

Additional context

While this resolves a bug with standard_npc which prevents the error from occurring, it does not actually address the root cause of the error. The actual error is (~[slow] ~[.],starting_items)=> (continued from above) ERROR : src/avatar.cpp:369 [void avatar::on_mission_finished(mission&)] completed mission MISSION_CAMP_LEADERSHIP_CHANGE was not in the active_missions list. But the mission should be in the active_missions list. Still, the primary purpose here is to make sure the tests pass, and if we can do that by fixing a bug with test data (standard_npc is used only for tests) then that shall suffice.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Mar 4, 2024
@RenechCDDA RenechCDDA force-pushed the mission_error_reporting branch from b3c3b6b to 9300eef Compare March 4, 2024 15:40
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 4, 2024
@RenechCDDA RenechCDDA force-pushed the mission_error_reporting branch from 9300eef to 125dc0f Compare March 4, 2024 16:17
@RenechCDDA RenechCDDA force-pushed the mission_error_reporting branch from 125dc0f to 4f21f03 Compare March 4, 2024 16:55
@RenechCDDA
Copy link
Member Author

image

image

So not only did the test I add cause this, it's a mission I added that's throwing the error. Just blame me!

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Missions Quests and missions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 4, 2024
@RenechCDDA RenechCDDA force-pushed the mission_error_reporting branch from d485718 to a203241 Compare March 4, 2024 19:40
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 4, 2024
@RenechCDDA RenechCDDA force-pushed the mission_error_reporting branch from a203241 to 557cc3a Compare March 4, 2024 22:52
@PatrikLundell
Copy link
Contributor

A question/suggestion: Wouldn't draft ensure the PR doesn't get merged prematurely? Is there something about draft mode that causes some important tests not to be performed?

@GuardianDll
Copy link
Member

iirc draft prs do not run all the tests

@RenechCDDA
Copy link
Member Author

Specifically I don't know off-hand which tests the drafts do not run, so just to be sure it's not in draft.

Yes I suppose I could check, but slapping a big warning label on it is something I know works, so eh. Keep it simple.

@RenechCDDA RenechCDDA force-pushed the mission_error_reporting branch from 557cc3a to 3fd4157 Compare March 5, 2024 18:48
@github-actions github-actions bot added the NPC / Factions NPCs, AI, Speech, Factions, Ownership label Mar 5, 2024
@RenechCDDA
Copy link
Member Author

Assuming the current commit (Assign test NPCs valid IDs) makes it through GCC9 it should be good.

@RenechCDDA RenechCDDA changed the title DO NOT MERGE(yet): Fix test errors (Fix for tests) Assign valid IDs to NPCs used during testing Mar 5, 2024
@Maleclypse
Copy link
Member

Kicked failed tests in attempt to get to GC9 Ubuntu

@RenechCDDA
Copy link
Member Author

RenechCDDA commented Mar 5, 2024

Ahhhhh the accidental ambiguity, I meant GCC 9, Curses, LTO , not GCC 9, Ubuntu, Tiles, Sound.

GCC 9, Curses, LTO is where the failure is occurring on all other PRs.

@mqrause
Copy link
Contributor

mqrause commented Mar 6, 2024

Specifically I don't know off-hand which tests the drafts do not run, so just to be sure it's not in draft.

Draft skips all the build matrix except basic build, so it would have been the wrong choice here.

@dseguin dseguin merged commit 6c060bb into CleverRaven:master Mar 6, 2024
25 of 29 checks passed
@RenechCDDA RenechCDDA deleted the mission_error_reporting branch March 6, 2024 21:45
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` [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Missions Quests and missions NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants