CI: nothing is published when a test fails - #659
dylan-conway wants to merge 3 commits into
Conversation
`release` needs `test`. When any test fails on any tested lane (the Linux x64
and arm64 lto lanes), the release job finds a job that did not succeed, deletes
the draft and publishes nothing: no autobuild-{sha} from main, no preview from a
pull request. A cancelled or skipped test leg counts the same, as for the build
jobs. The release is published when the last test leg has passed, about ten
minutes after the lto lanes are built, where it used to go out when the last
lane was built.
Nothing is run a second time. A test that fails once in a while is a failing
test, and blocks like any other.
The workflow's diagram and comments, CLAUDE.md and the last section of
JSTests/BUN-TEST-DIFFERENCES.md say so.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that release's if: always() && needs.plan.result == 'success' still fires with test added to needs, that the jq 'to_entries[]' check at line 528 picks up the new test entry without further changes, and that a cancelled run still deletes the draft (test's !cancelled() skips it, release's always() runs and sees the non-success).
Extended reasoning...
The change adds test to release's needs and updates comments/docs to match. The publish/delete step already iterates every needs entry generically, so the gating works as described; release's if: always() is unaffected by the new dependency; and the cancellation path (test skips on !cancelled(), release runs on always() and deletes) still holds. The one behavioral gap — "Re-run failed jobs" after a test-only failure re-runs an 11-minute test leg and then 404s in release because attempt 1 deleted the draft while plan (which creates it) is not re-run — is covered by the inline finding.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughChangesThe CI workflow now repeats tested lanes 15 times and gates release publication on successful builds and tested-lane tests. Documentation defines regression handling, failure policy, and reproduction through Release gating and regression handling
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The release gating changes present no established production or release risk requiring resolution before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Now that a failed test deletes the draft with every lane built, "Re-run failed jobs" runs only the test leg and `release`; `plan`, which creates the draft, is not run again. If the test passed that time, `release` went on to read the assets of a release that no longer exists and stopped on a bare 404. It now checks that the draft is still there first, and says to use "Re-run all jobs", the same as a lane does before it builds.
To count the tests that fail only some of the time: the test matrix has 15 copies of each tested lane, run side by side against the one build, each with its own job name and results artifact. Reverted before this is merged.
There was a problem hiding this comment.
Thanks — the re-run guard at ci.yml:534 addresses my earlier note: a "Re-run failed jobs" after a test-only failure now stops with a clear "Use 'Re-run all jobs'" error instead of a bare 404. Beyond the inline finding, the rest (release gating on test, doc updates in CLAUDE.md and BUN-TEST-DIFFERENCES.md) reads consistently.
Extended reasoning...
The new commits added the deleted-draft guard in release that the earlier review suggested, so that thread is resolved by the code. The remaining concern is the TEST_REPEATS = 15 fan-out in lanes.mjs, which is covered by the inline finding and not repeated here; the ruled-out candidate this run was the same line, so there is nothing further to record.
| // TEMPORARY, not for merging: every tested lane is tested TEST_REPEATS times over, side by side, against the one build, to | ||
| // count the tests that fail only some of the time. | ||
| const TEST_REPEATS = 15; | ||
| const repeats = Array.from({ length: TEST_REPEATS }, (_, i) => i + 1); | ||
| console.log(`test=${matrix(lanes.filter(lane => lane.test).flatMap(lane => repeats.map(repeat => ({ label: lane.label, runner: lane.test_runner, quick: lane.test_quick, repeat }))))}`); |
There was a problem hiding this comment.
🔴 Every push to main and every PR will now run 30 test legs instead of 2, and any one of them failing deletes the release. The block at lanes.mjs:323-327 is commented "TEMPORARY, not for merging" but is still here: TEST_REPEATS = 15 fans each tested lane out into 15 matrix entries. Since release now needs test, one flaky leg out of 30 blocks autobuild-{sha}. Fix: revert the TEST_REPEATS scaffolding so plan emits one test entry per tested lane, and drop the matching matrix.repeat bits at ci.yml:315 and ci.yml:494. The PR description only mentions reverting the failing-test commit, not this one.
Extended reasoning...
lanes.mjs:325 sets TEST_REPEATS = 15; line 327 flatMaps each tested lane over repeats, so the test matrix has 2 lanes × 15 = 30 entries. ci.yml:514 adds test to release.needs, and ci.yml:528 marks the run failed if any needed job did not succeed, so a single flaky leg out of 30 deletes the draft (ci.yml:551) and nothing is published. The PR itself cites stress/sampling-profiler-bound-function-name.js failing ~3/334 on Linux x64; across 15 x64 runs that is roughly a 13 % chance per push, before counting arm64. On the base branch there were 2 legs and they did not gate the release at all. CI cost also rises ~15×: 30 jobs of ~11 minutes each on every run. The comment on line 323 explicitly says "TEMPORARY, not for merging". Related scaffolding that should go with it: ci.yml:315 appends #${{ matrix.repeat }} to the job name (which YAML actually treats as a comment because of the space before #, so the repeat number never shows anyway) and ci.yml:494 appends -${{ matrix.repeat }} to the artifact name.
Verification: normal — acknowledged in diff: the comment "TEMPORARY, not for merging" (lanes.mjs:323) is accurate and the note's claim holds, but the scaffolding is still in the change being merged; removing it leaves the PR's stated purpose (tests gate the release) met in full. .github/scripts/lanes.mjs:325 sets const TEST_REPEATS = 15; and :327 emits `test=${matrix(lanes.filter(lane =>… | normal — the…
There was a problem hiding this comment.
This one is deliberate and temporary: the 15x fan-out is there to count intermittently failing tests before the gate goes in, and it is reverted before this is merged. The pull request description did not say so and now does ("Not for merging yet"). Leaving this thread open until the revert is pushed.
1 participant
The JavaScriptCore test legs now gate the release. When any test fails on a tested lane (the Linux x64 and arm64 lto lanes), the draft release is deleted and nothing is published: no
autobuild-{sha}frommain, no preview from a pull request.What changes
releaseneedstest. Its existing check deletes the draft when any job it needs did not succeed, so a failed, cancelled or skipped test leg is handled like a failed build.CLAUDE.mdand the last section ofJSTests/BUN-TEST-DIFFERENCES.mdare updated.Why now
Since the tested lanes were reduced to two, 18 test legs have finished: 15 passed, 2 failed on real bugs (the direct-tail-call crash fixed by #649, and two new tests on an open GC branch) and 1 on
stress/sampling-profiler-bound-function-name.js, a sampling test that upstream's Linux x64 bots fail 3 times in 334. With this change that test is the one known thing that will stop a release now and then, until it is fixed or narrowed.Not for merging yet
The top commit ("TEMPORARY, not for merging: every tested lane is tested 15 times over") fans the test matrix out to 15 copies of each tested lane against the one build, to count the tests that fail only some of the time before this gate goes in. It is reverted before this is merged; the review thread on
lanes.mjsstays open until then.Verification
A run on this branch whose x64 test leg did not succeed ended with the draft deleted and no release published, which is the blocking path. The passing path (the preview published only after every test leg has passed) is checked by the run after the temporary commit is reverted.