Add json checking for more mods on all PRs #35995
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Would be nice if this script could considers the mod dependencies, e.g. if mod A requires mod B, the script should only test mod A and it can skip testing mod B as it was already loaded in the test for mod A. |
|
We're testing all the mods at once, so dependencies are not the issue -- conflicts between mods could be, but I'll leave that for a future PR. |
ifreund
added
<Enhancement / Feature>
jbytheway
force-pushed
the
json_checking_more_mods
branch
2 times, most recently
from
ae6f982 to
e46bf0d
Compare
jbytheway
force-pushed
the
json_checking_more_mods
branch
3 times, most recently
from
b76efc4 to
0079cdb
Compare
jbytheway
changed the title
|
Appears to be working properly now. |
The test executable had custom string splitting code. Use the generic string_split instead.
Currently our CI doesn't check mod json except for a couple of cases, and in one of those cases (Magiclysm) it doesn't check it on json-only PRs, which is unhelpful. Add an additional test run, called as part of the TEST_STAGE CI job (i.e. the first one, which runs in full on json-only changes). This runs the test executable with every mod enabled (except those from a blacklist), without actually running any tests. The purposes is simply to load all of the mod json, enabling us to catch any errors therein.
jbytheway
force-pushed
the
json_checking_more_mods
branch
from
0079cdb to
f122583
Compare
51 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Infrastructure "Add json checking for more mods on all PRs"
Purpose of change
Currently our CI doesn't check mod json except for a couple of cases, and in one of those cases (Magiclysm) it doesn't check it on json-only PRs, which is unhelpful.
This makes it easy for more json errors to creep in (as has happened recently a couple of times with Magiclysm).
Describe the solution
Add an additional test run, called as part of the
TEST_STAGECI job (i.e. the first one, which runs in full on json-only changes). This runs the test executable with every mod enabled (except those from a blacklist), without actually running any tests.The purposes is simply to load all of the mod json, enabling us to catch any errors therein.
At the same time, I took the opportunity to simplify the code which parses the
--modsargument to the test executable.Describe alternatives you've considered
Fixing all the mods first, so we wouldn't need the blacklist, but that is problematic.
Testing
Have done some local testing, but to truly test I need to see Travis run this, which is why this is marked [WIP].