Uh oh!
There was an error while loading. Please reload this page.
ci: hard-fail e2e job and coverage gate - #358
Merged
Merged
Conversation
The e2e job has been gated by `continue-on-error: true` since the harness landed; the comment said "flip to false once we have ~5 consecutive green runs on main". Recent CI history shows the last 20 runs of the e2e job at 19/19 success where the job actually executed (1 skipped because build-bin failed first). The masking is no longer load-bearing. The coverage-gate's awk `exit 0` was likewise marked "soft during scaffold milestones; flip to `exit 1` at PR #5". Current main hits 92.24% line coverage against the 90% default threshold — 2.24pp margin, the gate is meaningful, the soft-fail is now a stale no-op. Three changes: 1. Remove `continue-on-error: true` from the e2e job (line 142). 2. Remove `continue-on-error: true` from the coverage-gate's download-artifact step for coverage-e2e (line 200) — the missing-artifact path is no longer reachable now that e2e is hard-fail. 3. Flip the coverage-gate awk failure branch from `exit 0` to `exit 1`. Drop the stale "flip at PR #5" comment. Stale guidance comments at lines 139-141 and 214 are also removed so reviewers don't re-add them on cargo-cult. Tracking: api7/AISIX-Cloud#398 (Tier 1 v2-HIGH × 2).
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPin ChangesCI Workflow Hard Gates
🎯 2 (Simple) | ⏱️ ~10 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
This PR tightens CI enforcement by converting the existing “soft” e2e and coverage checks into hard gates, so failures (or below-threshold coverage) will reliably fail the workflow instead of being silently tolerated.
Changes:
- Removes
continue-on-error: truefrom the e2e job so e2e failures fail CI. - Removes
continue-on-error: truefrom the coverage-gate’scoverage-e2eartifact download so missing e2e coverage blocks the gate. - Makes the coverage threshold check fail the job (
exit 1) when coverage is below the configured threshold.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
135
to
138
| e2e: | ||
| name: e2e (vitest) + coverage | ||
| needs: [build-bin] | ||
| runs-on: ubicloud-standard-2 |
Comment on lines
193
to
196
| - uses: actions/download-artifact@v4 | ||
| with: { name: coverage-unit, path: cov/unit } | ||
| - uses: actions/download-artifact@v4 | ||
| continue-on-error: true | ||
| with: { name: coverage-e2e, path: cov/e2e } |
The previous commit removed three `continue-on-error: true` instances
plus the `exit 0` soft-fail in the coverage gate. The audit agent
caught a real defect in one of those changes:
The `coverage-e2e` artifact does not actually exist on this
codebase. The e2e job invokes `pnpm test` which expands to
`vitest run` with NO `--coverage` flag (--coverage is only on the
separate `coverage` script), so `tests/e2e/coverage/lcov.info` is
never written. The e2e upload step has `if-no-files-found: ignore`
so the absence has always been silent: 3 sampled main runs
(26208667877, 26200775176, 26202404786) all show zero
coverage-e2e artifacts produced. The "92.24% combined coverage"
observed historically is in fact unit-only coverage; the merge
step's `|| cp "${files[0]}" "$merged"` fallback collapses to it.
Removing `continue-on-error: true` from the coverage-gate's
coverage-e2e download therefore would fail the gate's first step on
every run with `Artifact not found for name: coverage-e2e` — the
exact error visible in the prior soft-failed runs.
Restore the soft flag on that ONE download step with an explanatory
comment that the right fix is to make the e2e job emit coverage. The
other two changes from the previous commit stay:
- e2e job no longer has continue-on-error (this was always sound)
- coverage-gate awk failure branch stays `exit 1` (the threshold
is 90%, unit-only coverage is 92.24%, gate is meaningful)
Follow-up tracked separately: change e2e's `pnpm test` to invoke
coverage AND flip the upload's `if-no-files-found` from `ignore` to
`error` so a missing artifact fails loudly. After that lands, this
soft flag can also be removed.The lint job has been intermittently failing on `arduino/setup-protoc@v3` with `Error: unable to get latest version`. The major-only `@v3` reference makes the action query the GitHub releases API for the latest patch each run; that lookup is flaky under GitHub API contention. Pin to v3.0.0 across all four use sites to skip the latest-lookup entirely. This is the same theme as the previous two commits — make CI signal trustworthy. A lint job that fails on marketplace flake is indistinguishable from a real lint failure and erodes trust in the red signal we just made load-bearing.
Comment on lines
+198
to
+202
| # written and no `coverage-e2e` artifact is produced. The upload | ||
| # step at ~L182 has `if-no-files-found: ignore` so the absence | ||
| # is silent. Until the e2e job actually emits coverage (tracked | ||
| # follow-up), this download must remain soft — otherwise the | ||
| # coverage-gate hard-fails every run on `Artifact not found`. |
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
continue-on-error: truefrom the e2e job (hard-fail on e2e regressions)exit 0toexit 1(gate becomes load-bearing)continue-on-error: trueon the coverage-gate'scoverage-e2edownload with an explanatory comment, pending a follow-up that makes e2e actually emit coverageAudit revision (commit
fb5c415)The first commit (
57b7f03) also removedcontinue-on-error: truefrom the coverage-gate'scoverage-e2eartifact download. An independent audit caught that thecoverage-e2eartifact does not exist in this codebase: the e2e job runspnpm test(vitest run) with no--coverageflag, sotests/e2e/coverage/lcov.infois never written; the upload step'sif-no-files-found: ignoremakes the absence silent. The "92.24% combined coverage" observed historically is unit-only coverage (the merge step's|| cp "${files[0]}" "$merged"fallback collapses to it).Removing the soft flag on that download would have hard-failed every CI run on
Artifact not found for name: coverage-e2e— the same error the prior soft-failed runs were silently hiding. Commitfb5c415restores that one soft flag with a comment.The other two changes — hard-failing the e2e job and flipping the awk to
exit 1— stay. Unit coverage at 92.24% above the 90% threshold means the gate is meaningful for unit regressions.Follow-up (separate PR)
Tracked in api7/AISIX-Cloud#398:
pnpm coverage(or pass--coveragetopnpm test)if-no-files-found: ignore→errorso a missing artifact fails loudlyRisk
Low for the changes that remain. The e2e job has not actually failed in any of the last 19 real runs (the 20th was skipped because build-bin failed first). The audit confirmed those greens were real e2e successes, not soft-fails — only the coverage-gate's coverage-e2e download was relying on the soft flag.
Test plan
continue-on-errorremoved, coverage-gate awkexit 1, coverage-e2e soft flag retained with explanatory comment)origin/mainTracking
Closes 2 of 3
(v2-HIGH)items in Tier 1 of api7/AISIX-Cloud#398:ci.yml:142continue-on-error: trueon e2e job — closedci.yml:215coverage-gateexit 0→exit 1— closedci.yml:200continue-on-error: trueon coverage-e2e download — deferred to the follow-up PR (needs e2e to emit coverage first)Refs #353.
Summary by CodeRabbit