ci(e2e): Make E2E/Override actually unblock the PR - #3970
Conversation
The e2e/* contexts are required status checks on master, so a required status only passes on `success`. E2E/Override wrote `error` to all five, which turned "E2E pending" into "E2E permanently blocked" — the opposite of what the label exists to do. PR #3955 sat unmergeable with a green approved review and an E2E suite that had already passed on that SHA before the override overwrote the results. Give markE2EStatusesCancelled a state parameter so the two callers can express different intent. Applying E2E/Override waives E2E and writes `success`, with a description recording that no tests ran. Pulling E2E/Run mid-flight withdraws coverage without waiving it, so it keeps `error` and the gate stays closed. Removing E2E/Override now re-arms a real run. Previously there was no unlabel handler at all, which was harmless when the statuses were red but would otherwise leave a waived-green required check standing with no way to reset it short of pushing a new commit. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@esarafianou: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 📝 WalkthroughWalkthroughThe PR updates E2E override handling. It restores ChangesE2E override waivers
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change makes the E2E override unblock required checks while preserving the blocked state when E2E coverage is withdrawn; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant E2ETriggerWorkflow
participant GitHubStatuses
PullRequest->>E2ETriggerWorkflow: remove E2E/Override on non-draft pull request
E2ETriggerWorkflow->>GitHubStatuses: remove and re-add E2E/Run
E2ETriggerWorkflow->>GitHubStatuses: report successful waiver status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
The
E2E/Overridelabel is documented as "Cancels/skips E2E runs for this PR and marks the check as skipped", but applying it currently makes the PR permanently unmergeable.All five
e2e/*contexts are required status checks onmaster:A required status passes only on
success— unlike check runs, commit statuses have noskippedstate.markE2EStatusesCancelledwroteerrorto all five, so the override converted "E2E pending" into "E2E permanently blocked", exactly the state the label exists to clear.#3955 is the live case
Changes
markE2EStatusesCancelledtakes astateparameter, so the two callers can express different intent:E2E/Overrideapplied →success, described asE2E waived (E2E/Override label applied) — no tests ran. A maintainer explicitly waived E2E, so the gate should open, and the description keeps the check list honest about the fact that nothing ran.E2E/Runremoved mid-flight → stayserror. That withdraws coverage without waiving it, so the gate should stay closed. Unchanged behaviour.E2E/Overridenow re-arms a real run. There was no unlabel handler forE2E/Overrideat all before — harmless while the statuses were red, but with a green waiver it would leave a passing required check that never ran tests and no way to reset it short of pushing a new commit.Test plan
npx eslint e2e/utils/github-actions.js— cleane2e/utilssuites still pass (node --test e2e/utils/*.test.js)E2E/Run, so E2E should go green normallyE2E/Override, then re-add it. Thelabeledevent re-fires the job, which reads the helper frommasterat run time (the step checks outbase.ref), so Attribute Sentry events to a persisted install ID #3955 gets the fix without rebasing. The fivee2e/*statuses should flip to green and auto-merge should proceed.Note on the alternative
The cleaner long-term shape is migrating
e2e/*from commit statuses to check runs, since branch protection accepts askippedcheck run conclusion as satisfying a required check — no need to writesuccessfor something that didn't run. That touchese2e-functional.ymland needschecks: write, so it's left out of this fix. Happy to file a follow-up if there's appetite.Release Note
Summary by CodeRabbit