Repository navigation
Conversation
github-actions
Bot
added
review/needs-review
|
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Splitting the Node and Cloudflare E2E targets into independent jobs is a sound way to stop every shard from running both targets in series. Disabling fail-fast also fits the goal, since "Re-run failed jobs" now re-runs only the shards that failed. Both reviewers approved and found no actionable issues.
The reviewers checked the gating of each job on its test-plan.mjs output and the needs list and check_lane handling in the Tests aggregate. They also checked the per-target fixture builds, the artifact naming and upload condition, and the retained E2E Tests and Tests job names. One caveat remains: it could not be confirmed statically that no branch ruleset still requires the old E2E shard (n/8) check names. CI execution and the reported timing improvements were not verified. No changeset is needed, because only the CI workflow changes.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
emdashbot
Bot
added
review/needs-review
…ling shard Each E2E shard ran the Node target and then the Cloudflare target, so the slowest shard paid for both and set the run's wall clock. #3277 paired them to cut the job count while the org could run only 20 jobs at a time, and that limit no longer applies. Run test-e2e-node and test-e2e-cloudflare as separate 8-shard matrices, each gated on its own plan output and building only its own fixture, and have the Tests job check both. Set fail-fast: false on the vitest and E2E matrices again, so a red run shows every failing shard and 'Re-run failed jobs' repeats only those instead of every shard that fail-fast cancelled.
8842da7 to
8a15eb6
Compare
github-actions
Bot
added
query-count changed
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://daniel-ci-e2e-split-targets.try.emdashcms.com, https://daniel-ci-e2e-split-targets-emdash-playground.emdash-cms.workers.dev (commit 8a15eb6)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
There was a problem hiding this comment.
Splitting the E2E targets remains a sound approach to reducing serial CI work while letting every failing shard finish and report.
For this follow-up, I checked the current workflow against the PR diff and the updated fixture setup: target selection, per-target builds, matrix failure handling, artifact uploads, and both lanes’ inclusion in the Tests aggregate remain consistent. The previous review had no actionable findings, and I found no new issues to fix.
Static review only; CI execution, timing improvements, and external branch-ruleset requirements were not independently verified.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
What does this PR do?
Runs Node and Cloudflare E2E as separate jobs. A red run now shows every failing shard.
Each E2E shard ran the Node tests and then the Cloudflare tests, so the slowest shard paid for both. #3277 paired them to cut the job count while the org could run only 20 jobs at a time. That limit is gone.
test-e2e-nodeandtest-e2e-cloudflarenow run 8 shards each. Each starts only when the test plan selects its target and builds only its own fixture. Each gets a 25-minute timeout instead of 40, about twice the slowest run measured. The Tests job checks both, and required check names don't change.Once #4447 and a separate playground job have merged, E2E is the slowest job again, and this split takes a full run from about 13 to about 10 minutes. Each extra job pays its own install and build, so a full run takes about 15 more runner minutes: the 16 E2E jobs took 110.5 runner minutes on this PR's CI run, against about 95 for the 8 combined shards on
main. 12 combined shards would cost about the same (about +14). So the reason for the split isn't runner minutes. It's the shorter E2E job: each one ends about 1.5 minutes sooner, which the later steps in #4449 need, and each target gets its own job and timeout.The core unit test shards and both E2E jobs go back to
fail-fast: false. With fail-fast, "Re-run failed jobs" also re-ran every shard that fail-fast had cancelled. In run 39123937452, one flaky shard failed and all 8 ran again. The cost is that a failing run turns red only when its slowest shard finishes, not as soon as the first one fails. That run went red 51 seconds after its failure. Without fail-fast, it would have waited for the other shards, and the split makes them shorter.mainruns CI one at a time, with one waiting slot. From 2026-10-17 to 10-20, 25 of 70 CI runs onmainwere replaced by a newer push before any job started. Shorter runs mean moremaincommits get tested.#4447 changes the same E2E step, so this PR gets rebased onto it once it merges.
Part of #4449
Type of change
Your connection to this change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. (n/a: no admin UI change)AI assistance disclosure
Screenshots / test output
Test output:
mainruns after ci: build the E2E fixtures once instead of two or three times per job #4317 merged (2026-10-27 and 10-28): the 8 combined E2E shards took 92 to 102 runner minutes (median 96), and the slowest shard took 16.0 to 19.4 min..github/zizmor.yml: no findings in the default persona, and the same counts asmainin the pedantic and auditor personas.