-
Notifications
You must be signed in to change notification settings - Fork 3
E2E Gates reports success when E2E shards are skipped by an upstream failure #2043
Description
Problem
E2E Gates — the main-only required check whose entire purpose is to block a beta → main promotion when E2E fails — reports success when no E2E test has run at all.
Observed on the promotion PR #2041 (run 31104570445):
| Check | Result |
|---|---|
| E2E Cache Warmup | ❌ failure |
| E2E Tests (Shard 1..16/16) | ⏭️ skipped |
| E2E Smoke Tests | ⏭️ skipped |
| Merge E2E Reports | ⏭️ skipped |
| E2E Gates | ✅ success |
Mechanism
-
e2edeclaresneeds: [detect-changes, docker, e2e-warmup](.github/workflows/ci.yml:523). -
When
e2e-warmupfails, GitHub skips every dependent job — so all 16 shards becomeskipped, notfailure. -
e2e-gatesevaluates results with (.github/workflows/ci.yml:219):if [ "$result" != "success" ] && [ "$result" != "skipped" ]; then
skippedis explicitly accepted, so the gate passes.
The skipped allowance is itself legitimate — it exists for the path-filter case, where e2e is skipped via if: needs.detect-changes.outputs.app == 'true' || needs.detect-changes.outputs.e2e == 'true' because nothing testable changed. The defect is that the check conflates two very different meanings of skipped: "there was nothing to test" and "an upstream job failed, so we never tested."
Impact
This is the last gate before code reaches main, and it can pass vacuously. It compounds an existing property of the setup: E2E Gates is required on main only, so commits merged to beta are never full-E2E-gated either. A promotion could therefore reach main with zero E2E tests executed against the range, with a green gate.
Concretely, on #2041 the warmup failed for an unrelated infrastructure reason (a competing apt-get held /var/cache/apt/archives/lock, so playwright install-deps exited 100 on all three retry attempts). The gate went green anyway. It was caught only by manually inspecting shard-level conclusions; re-running the failed jobs then produced a genuine 16/16 pass.
Suggested fix
Make the gate distinguish the two skip reasons:
- Add
e2e-warmup(anddocker) toe2e-gates'sneedsand fail the gate when either did not succeed. - Only accept
skippedfore2e/e2e-merge-reportswhendetect-changesreports nothing testable changed; otherwise requiresuccess.
Acceptance criteria
- When
e2e-warmupfails,E2E Gatesfails. - When any E2E shard is skipped due to a failed upstream dependency,
E2E Gatesfails. - When
detect-changesreports no app/e2e changes and the shards are legitimately skipped,E2E Gatesstill passes (no regression to the path-filter path). - A green
E2E Gateson amain-targeted PR implies every shard actually reportedsuccess.
Notes
Found during /release for promotion PR #2041. Not fixed there deliberately — a CI-gate change does not belong bundled into a promotion.
Activity
Metadata
Metadata
Assignees
Projects
- StatusTodo