test(ci): make a missing pyyaml a skip, not a failure - #2058
Merged
Merged
Conversation
The guard printed SKIP and exited 1. Today that is harmless -- the suite runs in the `links` job, which installs pyyaml one step earlier -- but it is wrong the moment anything else decides where the suite runs, and something is about to: the scripts restructure replaces CI's hand-maintained test list with discovery over `scripts/ci/tests/`, and `scripts/test-*.sh` is in its move list. Under that runner a non-zero exit reports FAIL, so an absent optional dependency would have reddened CI. Exiting 0 with a loud SKIP line is the behaviour the environment actually warrants. Also adds the `# requires: python3` header the runner reads. It cannot express the real dependency -- the header is tested with `command -v`, and pyyaml is a python module, so `command -v python3` succeeds without it -- so the header covers the interpreter and the in-script guard covers the module. Known gap, recorded in the script at the point of failure: exit 0 makes a skipped run indistinguishable from a passed one in that runner's summary, which is the conflation its own header comment warns against. Closing it needs a skip exit code the runner understands (77 is the usual spelling); that belongs in the runner rather than here. Verified: normal run passes, the skip path exits 0 against a python3 whose `import yaml` fails, and removing GH_REPO from ci.yaml still fails the suite.
Contributor
🔍 Rendered manifest diff — this PR vs
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🔍 test
📝 Summary
scripts/test-ci-notify-main-broken.sh(#2057) printedSKIPand exited 1 when pyyamlwas missing. Harmless today — the suite runs in the
linksjob, which installs pyyaml onestep earlier — but wrong as soon as anything else decides where it runs, and something is
about to: the
scripts/restructure onworktree-scripts-restructurereplaces CI'shand-maintained test list with discovery over
scripts/ci/tests/, and its plan movesscripts/test-*.sh(20 files, now 21) into that directory.Under
scripts/ci/tests/run.sha non-zero exit reports FAIL, so an absent optionaldependency would have turned CI red.
🎯 Changes
SKIPline# requires: python3header that runner reads🗂️ Files
scripts/test-ci-notify-main-broken.shrequiresheaderWhy the header alone cannot fix this
run.shtests a# requires:entry withcommand -v. pyyaml is a python module, not abinary —
command -v python3succeeds on a machine that lacks it. So the header coversthe interpreter and the in-script guard covers the module. Both are needed.
Known gap, for whoever lands the restructure
exit 0makes a skipped run indistinguishable from a passed one inrun.sh's summary— the exact conflation its own header comment warns against:
Closing that needs a skip exit code the runner understands — 77 is the usual spelling —
and that belongs in
run.sh, not in each suite. This PR picks the lesser of the twowrong answers and says so at the point of failure, rather than leaving a suite that
reddens CI over an optional dependency.
Verification
Three paths, all fresh:
shellcheckclean.🏷️ Labels
Tests, configuration changes