Skip to content

Conversation

@harakka
Copy link
Member

@harakka harakka commented Oct 9, 2025

Summary

Build "Run tests in CI for multiple sets of mods"

Purpose of change

We might want to run tests against not just magiclysm but also against XE, MoM etc, but so that they're run on their own instead of all together. This enables that.

Describe the solution

Update gha_tests_only.sh to handle MODS env variable contents so that:

  • MODS=a,b,c: run as previously, so that a, b and c are loaded
  • MODS=a,b|c: run once with a and b loaded, and once with c loaded

Update matrix.yml so that there's no unnecessary "--mods=" passed along, we can add that at the actual use site

Describe alternatives you've considered

Testing

  • Running my own test script locally to see how the string splitting etc works
  • XE enabled for clang++-13, looks like it's running the tests on XE the same way it dos for Magiclysm

Additional context

Maleclypse wanted this for XE

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments Code: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Oct 9, 2025
@harakka harakka marked this pull request as draft October 10, 2025 16:43
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 13, 2025
@harakka harakka marked this pull request as ready for review October 13, 2025 16:25
@harakka
Copy link
Member Author

harakka commented Oct 18, 2025

To me it looks like it's working, now the missing bit is either Maleclypse fixing those XE test errors, or turning the switch off and letting Maleclypse do that in a separate PR. Will happily wait on this for a while.

@Maleclypse
Copy link
Member

Sorry for the extra commits but I didn't want to ask you to rebase this just for it to tell me what else is I need to fix now

@Maleclypse
Copy link
Member

Is this an XE error or an RNG error?

@harakka
Copy link
Member Author

harakka commented Oct 29, 2025

Good question. It's failing in

Mods-(~[slow] [.],starting_items)=> Having_all_mutations_give_correct_highest_category
Mods-(
[slow] ~[.],starting_items)=> Given: The player has all mutations for FAIR_FOLK_COMMONER_POOKA

but the stack trace is from

cata_test: src/uilist.cpp:464: void uilist::init(): Assertion `!test_mode' failed.

Dunno if something uilist related has been merged recently that could cause this

@harakka
Copy link
Member Author

harakka commented Oct 29, 2025

From some quick discussion on discord, it seems likely this is caused by some EOC triggering in the " Given: The player has all mutations for FAIR_FOLK_COMMONER_POOKA" test that calls run_eoc_selector, which displays a dialog using uilist. I guess we've never had anything in vanilla or magiclysm that does this during tests.

I guess uilist needs to be updated to be test compatible somehow, but it's bit outside my wheelhouse since I've never looked into it.

Edit: well, I just looked into it, I think in npctalk.cpp:6394 we need to check for test_mode, and if it's true we return a lambda that resolves to one of the list options, instead of returning the uilist lambda that's currently there.

If no-one gets to it earlier I can try to kick that forward over the weekend at latest.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership [C++] Changes (can be) made in C++. Previously named `Code` labels Oct 29, 2025
@harakka harakka force-pushed the test_modsets branch 4 times, most recently from 89d7387 to 0b671ee Compare October 30, 2025 16:48
@harakka
Copy link
Member Author

harakka commented Oct 30, 2025

@Maleclypse I think there's a new set of XE errors for you 😅

@Maleclypse
Copy link
Member

@Maleclypse I think there's a new set of XE errors for you 😅

Error: Mods-(~[slow] ~[.],starting_items)=>   io::enum_to_string( Type ) := "character_kills_monster"
Mods-(~[slow] ~[.],starting_items)=>   ref := "Killed a Kevlar hulk."
Mods-(~[slow] ~[.],starting_items)=>   SIGSEGV - Segmentation violation signal
Mods-(~[slow] ~[.],starting_items)=> 
Mods-(~[slow] ~[.],starting_items)=> Log messages during failed test:

This one isn't mine though right?

@harakka
Copy link
Member Author

harakka commented Oct 30, 2025

I don't think so, but not 100% sure.

@Maleclypse
Copy link
Member

Oh crap I missed you force pushed and reset the tests

@harakka
Copy link
Member Author

harakka commented Nov 8, 2025

Looks like more density errors, not sure about the other stuff

@harakka
Copy link
Member Author

harakka commented Nov 9, 2025

Accidentally re-ran the tests again q_q I need to run this set locally at some point so you can have the full list without messing around in CI

@Maleclypse
Copy link
Member

#83697

@GuardianDll
Copy link
Member

Can be updated now

@harakka
Copy link
Member Author

harakka commented Nov 15, 2025

@Maleclypse I ran the tests locally to hopefully get a more complete list, there's still a bunch of things failing with density checks xe_tests_38ea1.txt

The c++ error in memorial_test is consistent as well so it needs to be tackled some way. I don't think it's a XE bug as such though, but something about how the test triggers the monster death (to check for memorial message) results in EOC_WEREWOLF_GET_MANA_ON_NETHER_KILLS not having a beta talker. Maybe the test setup can be improved to get rid of it.

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 [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments Code: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions NPC / Factions NPCs, AI, Speech, Factions, Ownership

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants