fix(release): skip_tests silently skipped smoke and soak too - #1350
Merged
Conversation
Run 30504814537 built all eight platforms successfully with skip_tests=true, and
reported:
test: skipped (intended)
smoke: skipped (NOT intended)
soak: skipped (NOT intended)
release-draft: running
GitHub propagates "skipped" TRANSITIVELY down the needs graph. `build` carried
`!cancelled() && !failure()` so it overrode the skipped `test` and ran; `smoke`
had no `if:` and `soak` only checked soak_level, so both inherited the skip.
Nothing failed. Nothing announced it. The run then proceeded toward publishing
binaries that had never been smoke-tested or soaked -- the exact opposite of what
skip_tests was for, which was to skip a phase already proven green, not to
quietly stop testing the artifacts.
The second half of the defect is worse than the first: `release-draft` guarded
itself with a bare `!cancelled() && !failure()`, which is fail-OPEN. A skipped
job is neither cancelled nor failed, so a skipped smoke sails straight through
it. The draft was one gate away from being cut from unverified binaries, and the
only reason it was not is that the run was cancelled by hand.
Fixes, both directions:
- smoke and soak get `!cancelled() && !failure()`, so a deliberately skipped
ancestor no longer skips them;
- release-draft now requires `needs.smoke.result == 'success'` and enumerates
`needs.soak.result` explicitly, so a SKIPPED smoke BLOCKS the draft, and a
legitimately skipped soak (soak_level=none) stays distinguishable from a soak
that never ran.
tests/test_release_gate_chain_contract.sh pins both properties: every job
downstream of the optional phase must carry the override, and the draft must
demand smoke success rather than smoke non-failure. Verified in both directions
-- it passes on this workflow and fails with 2 violations when the previous
smoke/soak conditions are restored.
Nothing was published: the release remained a draft (isDraft=true,
publishedAt=null) and the run is cancelled.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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.
What happened
Run 30504814537 built all eight platforms successfully with
skip_tests=true, and reported:GitHub propagates "skipped" transitively down the
needsgraph.buildcarried!cancelled() && !failure(), so it overrode the skippedtestand ran — butsmokehad noif:andsoakonly checkedsoak_level, so both inherited the skip. Nothing failed; nothing announced it. The run then proceeded toward publishing binaries that had never been smoke-tested or soaked, which is the opposite of whatskip_testswas for: skipping a phase already proven green, not quietly stopping testing of the artifacts.The worse half
release-draftguarded itself with a bare!cancelled() && !failure(), which is fail-open: a skipped job is neither cancelled nor failed, so a skipped smoke sails straight through. The draft was one gate away from being cut from unverified binaries. The only reason it wasn't is that I cancelled the run by hand after noticing the twoskippedlines.Fix
smokeandsoakget!cancelled() && !failure(), so a deliberately skipped ancestor no longer skips them.release-draftnow requiresneeds.smoke.result == 'success'and enumeratesneeds.soak.resultexplicitly — a skipped smoke blocks the draft, while a legitimately skipped soak (soak_level=none) stays distinguishable from a soak that never ran.Contract
tests/test_release_gate_chain_contract.shpins both properties: every job downstream of the optional phase must carry the override, and the draft must demand smoke success rather than smoke non-failure. Verified in both directions — passes on this workflow, fails with 2 violations when the previous conditions are restored.State
Nothing was published: the release remained a draft (
isDraft=true,publishedAt=null) and the run is cancelled. 17/17 local contracts green.