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

Canonical test ordering - lexicographic #78570

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RenechCDDA
Copy link
Member

@RenechCDDA RenechCDDA commented Dec 14, 2024

Summary

Infrastructure "Tests now run in a guaranteed order (lexicographic) by default (can be overriden with cli argument)"

Purpose of change

  • Fix vehicle_turret test #78567 (comment) exposed some good questions. What is the ordering in how our tests are run? Apparently it's partly determined by linking order, which makes reproducing some test fails impossible without using the same link order (e.g. being on the same commit and using the same build method).

This is an unnecessary impediment when we're trying to diagnose test failures.

Describe the solution

Run the tests in lexicographical order instead.

Note that this works across subsets of tests. As an example I used the tests from #78524 which are DECLARED in the order
NPC rules (avoid doors)
NPC rules (close doors)
NPC rules (avoid locks)

By default that is the order they are run in (declared order). Running the tests with the arguments of "[npc_rules]" --wait-for-keypress exit --order lex instead gets us the following order:
NPC rules (avoid doors)
NPC rules (avoid locks)
NPC rules (close doors)

So the desired order(lexicographical) remains consistent even when additional arguments are provided, or subsets of tests are run.

Describe alternatives you've considered

We could run in the provided rand order instead. I initially thought this would be useful if we needed to specify subsets of tests to run, but lexicographical has us covered there already.

Since the random order was made subset stable, we promise that given the same random seed, the order of test cases will be the same across different platforms, as long as the tests were compiled against identical version of Catch2. We reserve the right to change the relative order of tests cases between Catch2 versions, but it is unlikely to happen often.

I thought random order might also be useful if we needed to shuffle the order of test cases for some reason, but the more I thought about it I failed to find any 'some reason'.

Testing

Works on my machine

Would like to see github CI all green before this is considered mergeable

Additional context

There is a second, mostly-unrelated commit on this branch, intentionally.

It enables --wait-for-keypress exit on MSVC builds, because it is really annoying that the default is to close the tests after being run.

This should only be the case when a human being manually runs the test (and only when using MSVC). It should NOT run when github tests does its auto magic, otherwise they'll get stuck waiting for a keypress that never comes.

If it turns out to be problematic... well that's why it's in a separate commit, we can just get rid of it.

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Dec 14, 2024
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Dec 14, 2024
@PatrikLundell
Copy link
Contributor

The only reason I can see to run tests in a random order would be to find poorly designed tests that rely on conditions they aren't setting up properly, but then we'd run into the problem of tracking the cause down when it happens. I'd say it's better not to detect such errors or encounter them deterministically, especially since the tests aren't intended to test that the tests are good, but that the tested code does what it's intended to do.

@RenechCDDA
Copy link
Member Author

The only reason I can see to run tests in a random order would be to find poorly designed tests that rely on conditions they aren't setting up properly, but then we'd run into the problem of tracking the cause down when it happens. I'd say it's better not to detect such errors or encounter them deterministically, especially since the tests aren't intended to test that the tests are good, but that the tested code does what it's intended to do.

Agree on all points! :)

@RenechCDDA
Copy link
Member Author

...Oh goodness it looks like we're gonna have some fixing to do

@github-actions github-actions bot added Vehicles Vehicles, parts, mechanics & interactions [C++] Changes (can be) made in C++. Previously named `Code` labels Dec 14, 2024
@mqrause
Copy link
Contributor

mqrause commented Dec 15, 2024

There is a second, mostly-unrelated commit on this branch, intentionally.

It enables --wait-for-keypress exit on MSVC builds, because it is really annoying that the default is to close the tests after being run.

I'm a bit confused by this. I don't think that does anything when you put it there? Plus it doesn't just close for me. I don't think it ever did and I don't remember ever changing some setting. Though it's been too long since I used VS2019 honestly.

@github-actions github-actions bot added the NPC / Factions NPCs, AI, Speech, Factions, Ownership label Dec 17, 2024
@RenechCDDA RenechCDDA force-pushed the canonical_test_order branch 2 times, most recently from 20d729f to fbb058d Compare December 17, 2024 20:42
@github-actions github-actions bot added the Melee Melee weapons, tactics, techniques, reach attack label Dec 17, 2024
@RenechCDDA RenechCDDA force-pushed the canonical_test_order branch 2 times, most recently from a351191 to a1b5ada Compare December 17, 2024 23:35
@github-actions github-actions bot added the Items: Containers Things that hold other things label Dec 20, 2024
@github-actions github-actions bot added the EOC: Effects On Condition Anything concerning Effects On Condition label Dec 20, 2024
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: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. Code: Tooling Tooling that is not part of the main game but is part of the repo. EOC: Effects On Condition Anything concerning Effects On Condition Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions Melee Melee weapons, tactics, techniques, reach attack NPC / Factions NPCs, AI, Speech, Factions, Ownership Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants