ci(e2e): run the end-to-end suite on every code PR (#1420 §1) - #1421
Conversation
`e2e.yml` was gated `if: github.event_name == 'push' || contains(...labels..., 'e2e')`. So end-to-end coverage ran on `main` after a change merged, or on a PR only when someone remembered a label. That gap is not redundant with the unit suite. `pytest tests/` imports from the source tree; e2e exercises the three *installed* paths (`uv-tool`, `pipx`, `venv-pip`). A regression in packaging, entry points or the installed CLI is invisible to the unit suite by construction, and had no pre-merge gate at all. The label opt-in is removed and the trigger is path-filtered to the same globs as ci.yml's `code` filter. `opened` is added to the event types — with only `[labeled, synchronize, reopened]` a freshly-opened PR never fired the workflow, so even the label escape hatch needed a second push to work. Trigger-level `paths` is safe here specifically because `e2e` is NOT one of the branch's required contexts. A path-filtered *required* check never reports on a PR it skips and leaves that PR permanently pending, which is why ci.yml deliberately has no `paths` and filters inside the job. The comment says so and a test pins it, so this pattern cannot be copied into a required workflow by someone reading only the diff. Three tests appended to the existing `tests/test_ci_path_filter.py` rather than a new module, matching how that file already derives its expectations instead of hand-maintaining them: - the label gate is absent, and `opened` is present - e2e's paths cover ci.yml's `code` filter, so the two lists cannot drift - ci.yml filters inside the job while e2e filters at the trigger Mutation-checked on a cleared `__pycache__`, one per gate: - restore the label `if:` -> test_e2e_runs_on_code_prs_without_an_opt_in_label fails - drop `benchmarks/**` from e2e's paths -> test_e2e_paths_cover_the_ci_code_filter fails Restored: 7 passed. Refs #1420 (§1).
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe E2E workflow now runs for code-related pull requests without an ChangesE2E trigger alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideEnsures the GitHub Actions end-to-end workflow runs on every code-touching pull request (not just main or label-gated PRs), keeps its path filter aligned with the main CI code filter, and adds tests that pin this behavior and document why trigger-level path filtering is safe only for non-required checks, with a brief changelog entry describing the fix. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_ci_path_filter.py`:
- Around line 149-166: Update the path-list parsing in the test to locate the
`paths:` key specifically under `on.pull_request`, rather than accepting any
unique `paths:` key in the workflow. Preserve the existing indentation-based
glob collection and assertions after scoping the scan to the pull-request
configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb4717c6-b685-4f3d-8cfd-c625303297ae
📒 Files selected for processing (3)
.github/workflows/e2e.ymlCHANGELOG/v4.mdtests/test_ci_path_filter.py
|
[claim:review:Kulili:2026-08-06T20:25:22Z] |
Three gaps, each demonstrated by a mutation that passed the suite as it
stood.
AC2 had no guard at all. `test_e2e_paths_cover_the_ci_code_filter`
asserts `ci - e2e == {}`, i.e. e2e's list is a superset of ci's, and a
superset assertion is blind to additions. Appending `docs/**` and
`CHANGELOG/**` to the e2e trigger kept all seven tests green while
putting the 3-leg install matrix on every docs-only PR — AC2 reversed
with nothing red. `test_e2e_still_skips_docs_only_prs` mirrors the
guard ci.yml already has, and is what makes that mutation fail.
AC1 was pinned as the absence of one literal, so any re-gate with a
different spelling survived: `if: github.event_name == 'push'` on the
job negates AC1 completely and leaves `labels.*.name, 'e2e'` absent.
The assertion is now the invariant — the job carries no job-level `if:`
at all — with the label literal kept as a secondary check.
The paths parser searched the whole file for `paths:` and asserted
exactly one, so it could not tell the pull_request trigger's list from
the push trigger's, and a legitimate `push: paths:` would have failed an
unrelated test with a message about parser trust. It now walks
`on:` -> `pull_request:` -> `paths:` by indentation. Block extraction
returns index ranges rather than line contents because sibling blocks
hold byte-identical lines (`branches: [main]` appears under both
triggers) and content matching would bind to the wrong one.
Mutations: docs/** added -> red; job re-gated with a different
expression -> red; benchmarks/** dropped -> red; a legitimate
`push: paths:` added -> still green, no false failure.
The entry opened a second `### Fixed` inside `[Unreleased]`, above `### Added`, while `[Unreleased]` already had a `### Fixed` further down. Two same-named sections in one release block: a reader scanning for fixes finds the first, sees one entry, and misses the four below, and whoever assembles the release notes has to merge the sections by hand — which is where an entry gets dropped. Every released section in the file runs Added -> Changed -> Fixed (4.2.0, 4.1.0, 4.0.0), and across the last 60 commits touching this file no `[Unreleased]` block has ever carried a duplicate heading. Hand-moved, not resolved by a script; the diff against main is a pure insertion with zero removed lines.
The doc still described the `e2e` label opt-in as how the suite runs on a PR, which this branch removes. It now states the real trigger — every PR whose diff matches the `paths` list mirroring ci.yml's `code` filter, no label — and says plainly that the `e2e` label no longer triggers anything, since a label that silently stopped working is worse than one that was deleted.
Review — the change is right; 6 findings fixed on this branch (
|
| n | median | p90 | max | |
|---|---|---|---|---|
e2e full 3-leg matrix (push runs) |
100 | 47s | — | 81s |
ci.yml on pull_request |
83 | 220s | 255s | 321s |
The distributions are disjoint: e2e's slowest observed run is 81s against a median
CI settle of 220s. The three legs run in parallel with everything else, so the
expected delta to median PR settle time is ~0, and e2e cannot become the
critical path unless it regresses by 3x.
The caveat that makes this an estimate rather than a measurement: there are zero
completed e2e pull_request runs in the workflow's entire history — 2,668 runs,
all skipped bar three that were cancelled. The figures above are the push regime
substituted for the PR regime, and the PR regime has more contention (a code PR
already fires ~25 non-skipped check-runs). AC3 as written is not fully answerable
until this change lands and produces the first real sample. Worth saying so in the
body rather than leaving the AC silently unaddressed.
Two things the review turned up that are not defects in this PR
Both reproduce, both are pre-existing, and neither is claimed here — but they
change how this PR should be read, so they should not stay buried in a review.
e2e has not executed against any change in 541 commits. The newest
push-event run is 2026-07-21T01:30:43Z at 3421cd73, pushed by a human.
git rev-list --count 3421cd73..github/main = 541. The reason is that
merge-train FF-pushes with secrets.GITHUB_TOKEN, and GitHub does not trigger
workflows on pushes made with that token — so the "post-merge regression catch"
half of e2e's trigger has been inert since the merge train took over. Combined
with the label opt-in nobody used, the installed-package path has had no gate at
all, on either side.
This makes #1420 §1's premise understated: a regression there is not "caught on
main instead of on the PR" — it is caught nowhere. It also means this PR will be
the first time e2e has actually run in 541 commits, which is worth knowing before
merging: if it comes back red, that is 16 days of accumulated packaging drift
surfacing at once, and under merge-train's all-checks gate it would block every
labelled PR simultaneously. The good news is that this PR triggers e2e on itself
(.github/workflows/e2e.yml is in its own paths list), so its own run is the
validation — hold the merge until it is observed green rather than merging on
the unit suite alone. I'd file the push-side deadness separately; say the word and
I will.
A failing e2e will block merge-train — but that is not new. I verified
merge-train.yml gates on the full check-runs rollup minus two names, so every
check-run gates, required or not. The PR's "e2e is not a required context" argument
is therefore about branch protection only. It is still correct that nothing new
enters the rollup: e2e already emits check-runs on unlabelled PRs today (verified
on merged head aae8126a: two e2e check-runs, both skipped), because
synchronize fires and the job-level if skips. This PR changes an existing
member's conclusion from skipped to success/failure. Worth one sentence in
the body, since "not required" reads as "cannot block" and that is not the case here.
Verification
- FF on
github/main, 4 commits, all signed (G) uv run --extra archive pytest -q -p no:randomly: 7389 passed, 70 skipped,
71 xfailed, 0 failed- Note for anyone re-running: without
--extra archivesix uninstall tests fail
locally. That is the documented false negative from a venv missing CI's deps,
not a regression — they pass ongithub/mainin isolation and the full suite is
clean with the extra. tests/test_ci_path_filter.py: 8 passed- discretion grep on added lines: clean
AC1 ✅ AC2 ✅ (now guarded) AC3
Approving on the merged state, with the one hold: watch this PR's own e2e run
before labelling ready-to-merge. Actions is in a major outage (started
15:22Z), so nothing can run yet.
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:Garsecg:2026-08-06T22:55:07Z] |
|
[release:review:Garsecg:2026-08-06T22:55:13Z] |
|
[claim:review:Gylf:2026-08-06T22:59:53Z] |
|
[release:review:Gylf:2026-08-06T22:59:58Z] |
|
merge-train: merged 2d1e4ae → |
|
merge-train: merged 2d1e4ae → |
Refs #1420 §1. Does not close the issue — §2 (perf), §3 (bench-gate corpus)
and §4 (mutation) remain open, so there is no
Closeslink.The gap
e2e.yml:26was:End-to-end coverage therefore ran on
mainafter a change merged, or on a PRonly if someone remembered to add an
e2elabel.This is not redundant with the unit suite.
pytest tests/imports from thesource tree; e2e exercises the three installed paths —
uv-tool,pipx,venv-pip. A regression in packaging, entry points, or the installed CLI isinvisible to the unit suite by construction and had no pre-merge gate at all.
A second, smaller defect fell out of reading it: the trigger was
types: [labeled, synchronize, reopened], which does not includeopened. Soeven the label escape hatch did not work on a fresh PR without a second push.
What ships
openedadded to the event types.ci.yml'scodefilter, sodocs-only PRs stay off it.
Why trigger-level
pathsis safe here and must not be copied.e2eis notone of the branch's required contexts (those are
history-scan,pattern-scan,pytest (3.12),pytest (3.13),secrets-scan). A path-filtered requiredcheck never reports on a PR it skips and leaves that PR permanently pending —
which is exactly why
ci.ymlhas nopathsand filters inside the job withdorny/paths-filterinstead. The workflow comment says this andtest_e2e_is_path_filtered_at_the_trigger_not_inside_the_jobpins it, so thepattern cannot be copied into a required workflow by someone reading only the diff.
Tests
Appended to the existing
tests/test_ci_path_filter.pyrather than a new module— that file already derives its expectations by scanning, instead of comparing a
hand-maintained list against the constants it was copied from, and these follow
the same shape.
test_e2e_runs_on_code_prs_without_an_opt_in_labelopenedis presenttest_e2e_paths_cover_the_ci_code_filtertest_e2e_is_path_filtered_at_the_trigger_not_inside_the_jobThe second one is the load-bearing one:
e2e.ymlcarries a "keep these in sync"comment, and a comment cannot enforce that. A path added to
ci.ymlandforgotten in
e2e.ymlmeans a PR that changes installed behaviour runs the unitsuite, skips e2e, and shows nothing red.
Verification
Mutation-checked, cleared
__pycache__, one per gate:if:test_e2e_runs_on_code_prs_without_an_opt_in_labelfailsbenchmarks/**from e2e's pathstest_e2e_paths_cover_the_ci_code_filterfailsEach turns exactly one test red. Restored: 7 passed.
YAML validated; the
e2ejob now carries noif:, andsurface-failure'sgithub.event_name == 'pull_request'branch is unchanged and still labels afailing PR
attn:e2e-failure.Cost
e2e is
timeout-minutes: 8across a 3-way install matrix, running in parallelwith the rest. Median PR settle time before this change was ~4 min (measured on
runs created before the 2026-08-06 Actions incident — runs inside that window
report retry and queue time as duration and are not a usable baseline). The
post-merge number should be re-measured once Actions is healthy and quoted on
#1420 §1's AC3 rather than estimated here.
Out of scope
§2 perf gating, §3 the 36 corpus-dependent bench-gate tests, §4 mutation on PRs.
Each is a separate change with its own constraints — §3 in particular is blocked
on a security decision about self-hosted runners on a public repo, not on effort.
Summary by Sourcery
Ensure the end-to-end workflow runs on every code pull request and keep its path filter aligned with the main CI configuration.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Documentation