ci: run the integration suites a PR's diff names (#5960)#6338
ci: run the integration suites a PR's diff names (#5960)#6338proggeramlug wants to merge 2 commits into
Conversation
The per-PR `cargo-test` gate runs `--lib --bins` only, so no `tests/*.rs` integration suite has ever executed on a pull request — including the PR's own new acceptance suite. #5938 landed with `capture_rereg_renamed_class.rs` red at the very commit that introduced it, through green required checks. New `e2e-scoped` job (PR-only) + `scripts/ci_e2e_scope.py`: map the changed file list to the suites it names and run exactly those. crates/<pkg>/tests/<suite>.rs -> cargo test -p <pkg> --test <suite> crates/<pkg>/tests/<suite>/mod.rs -> the parent suite crates/<pkg>/tests/common/... -> every suite in that crate (capped) Deleted suites are skipped, cross-host UI crates are excluded (shared `EXCLUDED` with ci_test_scope), and the selection is capped at 12 so a mass rename can't turn one PR into a multi-hour run. Manifest names are parsed straight out of Cargo.toml, not `cargo metadata`, so the scope step needs no toolchain: a PR that names no suite finishes in ~20-30s and installs nothing. When a suite IS named, the build runs in parallel with cargo-test (the longer pole), so added wall-clock on the critical path is ~0 for most PRs. Also: the concurrency group now keys on the event and only cancels in-progress runs for `pull_request`. The nightly full run is the only backstop for a regression in a suite the diff doesn't name (#6037 class), so it must always reach a conclusion; keeping it cancellable by a manual dispatch on main was a loose end. Superseded PR pushes still cancel, which is where the minutes are. Self-test: `python3 scripts/ci_e2e_scope.py --self-test` (runs in the job).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds diff-based integration-suite selection and a dedicated pull-request E2E job. It also updates workflow concurrency behavior and documents that scoped acceptance suites run outside the ChangesScoped E2E CI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant ScopeScript
participant Cargo
PullRequest->>GitHubActions: provide changed paths
GitHubActions->>ScopeScript: compute scoped suites
ScopeScript-->>GitHubActions: return package-suite pairs
GitHubActions->>Cargo: prepare and run selected tests
Cargo-->>GitHubActions: return aggregate suite status
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
515-543: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftToolchain/sccache/cache/eviction setup is duplicated verbatim between
cargo-testande2e-scoped.Steps at Lines 515-543 (install toolchain, install sccache, cache sccache objects, rust-cache, evict stale auto-opt archives) mirror the existing
cargo-testjob's steps almost line-for-line. Given the eviction step exists specifically to avoid a repeat of a hard-to-diagnose bug (#5892), duplicating it across two jobs risks the fix drifting out of sync if it's ever updated in only one place. A composite action (.github/actions/setup-perry-build/action.yml) would let both jobs share this logic.🤖 Prompt for 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. In @.github/workflows/test.yml around lines 515 - 543, Extract the duplicated Rust build setup steps—Install Rust toolchain, Install sccache, Cache sccache objects, rust-cache, and Evict stale auto-opt archives—into a composite action at .github/actions/setup-perry-build/action.yml. Update both the cargo-test and e2e-scoped jobs to invoke that action with equivalent conditions and inputs, preserving the existing cache keys, save behavior, and eviction command so the shared setup cannot drift.
519-536: 🔒 Security & Privacy | 🔵 TrivialCache-restore steps flagged for cache-poisoning risk (sccache-action, actions/cache, Swatinem/rust-cache).
zizmor flags all three as always-restoring/enabled-by-default caches. This is the same pattern already used by the
cargo-testjob (unchanged), so it's an inherited risk rather than a new regression — but since it's now duplicated into a second job that also compiles PR-controlled code, it's worth revisiting as a pair: e.g. confirmsave-ifscoping is sufficient (this job'ssave-if: ${{ github.ref == 'refs/heads/main' }}is trivially always false here since the job only runs onpull_request, so it only ever restores — which is arguably the safe outcome, just worth being intentional about rather than incidental).🤖 Prompt for 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. In @.github/workflows/test.yml around lines 519 - 536, Make the cache configuration for Install sccache, Cache sccache objects, and Swatinem/rust-cache explicitly safe for pull_request runs by preventing restoration of caches for untrusted PR-controlled code, rather than relying only on save-if. Preserve cache use for trusted main-branch builds where appropriate, and apply the same intentional restore policy consistently across all three steps.Source: Linters/SAST tools
🤖 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 @.github/workflows/test.yml:
- Around line 489-490: Update the actions/checkout step in the affected test job
to set persist-credentials to false. Apply the same checkout hardening to the
existing cargo-test job checkout as well, without changing the build or test
steps.
- Around line 476-578: Increase the e2e-scoped job’s timeout-minutes to cover
the maximum serial runtime of all suites allowed by DEFAULT_CAP, using the
1500-second per-suite timeout plus sufficient setup and build buffer. Keep the
per-suite timeout and failure reporting in “Run scoped integration suites”
unchanged, and ensure the job-level timeout remains synchronized with those
limits.
In `@scripts/ci_e2e_scope.py`:
- Around line 78-123: The suite discovery in _suites_of currently misses Cargo
integration-test targets using tests/<name>/main.rs, causing changes there to
select all suites while omitting <name>. Update _suites_of to include
directory-style targets whose tests/<name>/main.rs exists, and extend _self_test
to verify discovery and selection of that suite without changing existing
top-level-file or shared-helper behavior.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 515-543: Extract the duplicated Rust build setup steps—Install
Rust toolchain, Install sccache, Cache sccache objects, rust-cache, and Evict
stale auto-opt archives—into a composite action at
.github/actions/setup-perry-build/action.yml. Update both the cargo-test and
e2e-scoped jobs to invoke that action with equivalent conditions and inputs,
preserving the existing cache keys, save behavior, and eviction command so the
shared setup cannot drift.
- Around line 519-536: Make the cache configuration for Install sccache, Cache
sccache objects, and Swatinem/rust-cache explicitly safe for pull_request runs
by preventing restoration of caches for untrusted PR-controlled code, rather
than relying only on save-if. Preserve cache use for trusted main-branch builds
where appropriate, and apply the same intentional restore policy consistently
across all three steps.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d696092-7ab3-405b-95e8-b3d10dab8a50
📒 Files selected for processing (2)
.github/workflows/test.ymlscripts/ci_e2e_scope.py
| def _suites_of(root: str, crate_dir: str): | ||
| """Every `tests/*.rs` integration target of a crate, by suite name.""" | ||
| tests_dir = os.path.join(root, "crates", crate_dir, "tests") | ||
| try: | ||
| entries = os.listdir(tests_dir) | ||
| except OSError: | ||
| return [] | ||
| return sorted( | ||
| e[:-3] | ||
| for e in entries | ||
| if e.endswith(".rs") and os.path.isfile(os.path.join(tests_dir, e)) | ||
| ) | ||
|
|
||
|
|
||
| def select(changed, root: str): | ||
| """-> sorted list of (package, suite) named by the changed paths.""" | ||
| selected = set() | ||
| for path in changed: | ||
| m = _TESTS_PATH.match(path.strip()) | ||
| if not m: | ||
| continue | ||
| crate_dir, rest = m.group(1), m.group(2) | ||
| pkg = _package_name(root, crate_dir) | ||
| if pkg is None or pkg in EXCLUDED: | ||
| continue | ||
|
|
||
| if "/" not in rest: | ||
| if not rest.endswith(".rs"): | ||
| continue | ||
| suite = rest[:-3] | ||
| # Skip deletions: the target no longer exists. | ||
| if os.path.isfile(os.path.join(root, "crates", crate_dir, "tests", rest)): | ||
| selected.add((pkg, suite)) | ||
| continue | ||
|
|
||
| head = rest.split("/", 1)[0] | ||
| sibling = os.path.join(root, "crates", crate_dir, "tests", head + ".rs") | ||
| if os.path.isfile(sibling): | ||
| # Module directory of `<head>.rs`. | ||
| selected.add((pkg, head)) | ||
| else: | ||
| # Shared helper / fixture dir (`common/`, `fixtures/`): any suite in | ||
| # the crate can depend on it. | ||
| for suite in _suites_of(root, crate_dir): | ||
| selected.add((pkg, suite)) | ||
| return sorted(selected) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby tests without executing repo code.
git ls-files scripts/ci_e2e_scope.py
wc -l scripts/ci_e2e_scope.py
sed -n '1,220p' scripts/ci_e2e_scope.py
# Look for self-tests or references to the suite selection logic.
rg -n "_self_test|select\\(|_suites_of\\(|tests/<name>/main\\.rs|main\\.rs" scripts tests .github -g '!**/node_modules/**'Repository: PerryTS/perry
Length of output: 12214
🌐 Web query:
Cargo Book integration tests tests/<name>/main.rs standalone target directory-style main.rs
💡 Result:
In Cargo, integration tests are defined in files located within the tests/ directory at the root of your project [1][2][3]. When you run cargo test, Cargo automatically discovers these files and compiles each one as a separate, standalone crate that links against your library's public API [1][4][3]. Regarding the specific structure tests//main.rs, while the standard convention is to place integration tests directly in the tests/ directory (e.g., tests/my_test.rs), Cargo also supports using subdirectories for complex integration tests [4][5]. If you create a file at tests//main.rs, Cargo will treat it as a distinct integration test target [1][4]. Key details about this structure include: 1. Standalone Compilation: Each file in the tests/ directory (including those in subdirectories like tests//main.rs) is compiled as an independent crate [1][4][3]. Because these act as separate crates, they can only access the public items of your library [1][4][6]. 2. Directory-Style Integration Tests: Placing a main.rs inside a subdirectory (e.g., tests/my_integration_test/main.rs) is a valid way to organize tests that require their own local modules or supporting files [5][6]. This allows you to keep supporting code within that subdirectory, preventing it from being incorrectly treated as a separate, standalone integration test by Cargo [5][6][3]. 3. Target Directory: Cargo typically builds all targets in a central target/ directory [7][1]. The term standalone target directory usually refers to the isolation mechanism where each integration test is compiled independently, or sometimes to specific build patterns used in advanced CI scenarios or distribution packaging to avoid workspace-level interference [8][9][3]. It is important to note that integration tests require a library crate (src/lib.rs) to function effectively [4][3]. If your project is a pure binary crate (only src/main.rs), it does not expose a public API for other crates to use, which is why integration tests are generally not supported for projects lacking a library component [4][3].
Citations:
- 1: https://doc.rust-lang.org/cargo/reference/cargo-targets.html?highlight=test
- 2: https://doc.rust-lang.org/book/ch11-03-test-organization.html
- 3: https://learnrust.net/chapter-14/integration-tests/
- 4: https://doc.rust-lang.org/1.51.0/book/ch11-03-test-organization.html
- 5: https://doc.rust-lang.org/stable/book/ch11-03-test-organization.html
- 6: https://doc.rust-lang.org/stable/rust-by-example/testing/integration_testing.html
- 7: https://myrrlyn.net/blog/rfcs/cargo-book
- 8: Book CI jobs are broken async-graphql/async-graphql#1794
- 9: How to verify individual crates build (and ideally pass tests) for distro packaging rust-lang/cargo#3732
🌐 Web query:
site:doc.rust-lang.org cargo integration tests subdirectory main.rs tests/<name>/main.rs
💡 Result:
Yes, Cargo supports organizing integration tests into subdirectories within the tests directory. If an integration test consists of multiple source files, you can place a main.rs file (along with any necessary additional modules) inside a subdirectory of the tests directory, such as tests//main.rs [1]. In this structure, Cargo automatically treats the directory name () as the name of the integration test target [1]. When you run cargo test, Cargo compiles the files in this directory as an individual crate [1][2]. This approach allows you to organize complex integration tests into multiple files while maintaining the standard Cargo testing workflow [1].
Citations:
- 1: https://doc.rust-lang.org/stable/cargo/guide/project-layout.html
- 2: https://doc.rust-lang.org/stable/book/ch11-03-test-organization.html
Handle tests/<name>/main.rs as a suite target
tests/<name>/main.rs is a valid Cargo integration-test target named after the directory, but _suites_of() only sees top-level tests/*.rs files. A change under that layout currently falls into the shared-dir branch, selects every suite in the crate, and still omits <name> itself. Add the directory-style target to _suites_of() and cover it in _self_test().
🤖 Prompt for 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.
In `@scripts/ci_e2e_scope.py` around lines 78 - 123, The suite discovery in
_suites_of currently misses Cargo integration-test targets using
tests/<name>/main.rs, causing changes there to select all suites while omitting
<name>. Update _suites_of to include directory-style targets whose
tests/<name>/main.rs exists, and extend _self_test to verify discovery and
selection of that suite without changing existing top-level-file or
shared-helper behavior.
The job capped out at 90 minutes while allowing up to 12 suites (DEFAULT_CAP) each bounded by `timeout 1500` (25 min) — a 300-minute worst case. A legitimately-slow selection would hit the job wall clock and GitHub would cancel the whole job, losing every suite's result behind an uninformative "cancelled" instead of the per-suite ::error:: — reintroducing, one level up, exactly the failure mode the per-suite timeout exists to prevent. Derived the job timeout from the two bounds it has to cover (12 x 25 + 30 for toolchain/cache/staticlib build = 330) and documented the invariant so the three numbers can't drift apart. It's a backstop, not a budget: each suite is independently bounded and the common case selects zero suites and exits in ~20-30s. Also drop credential persistence from this job's checkout — it compiles third-party crates (build scripts run) and nothing in it pushes back to the repo. Found by review.
1 participant
Fixes #5960 (suggestion 3, plus a scoped version of suggestion 2).
The hole
The per-PR
cargo-testgate runs--lib --bins. That means nocrates/*/tests/*.rsintegration suite has ever executed on a pull request — not perry's 163, not perry-hir's 30, not perry-codegen's 17. The suites appear in the scope listing (their crate is in scope), they are compiled, and they are never run.The sharp consequence, in the issue's words: a PR's own new acceptance suite cannot fail its own CI. #5938 landed with
capture_rereg_renamed_class.rs— the suite the PR itself added — red at the very commit that introduced it, through green required checks.What this adds
A new PR-only
e2e-scopedjob plusscripts/ci_e2e_scope.py, which maps a diff to the suites it names and runs exactly those:crates/<pkg>/tests/<suite>.rscargo test -p <pkg> --test <suite>crates/<pkg>/tests/<suite>/<file>.rs(module dir, e.g.native_proof_regressions/)crates/<pkg>/tests/common/…,tests/fixtures/…(no sibling<name>.rs)src/changes)Details: deleted suites are skipped (the target is gone); cross-host UI crates are excluded (shares
EXCLUDEDwithci_test_scope.py); the selection is capped at 12 so a mass rename of suites can't turn one PR into a multi-hour run (overflow is reported and left to the nightly); each suite gets a 25-mintimeoutso one hungperry compilecan't eat the budget and hide the other suites' results;libperry_{runtime,stdlib}.aare built up front when aperry/perry-stdlibsuite is selected, becausecargo testnever builds thestaticlibcrate-type and thePERRY_NO_AUTO_OPTIMIZE=1suites link those archives directly.--self-testcovers the mapping table above and runs as the first line of the job.Cost
The scope step parses
crates/<dir>/Cargo.tomldirectly instead of shelling out tocargo metadata, specifically so it can run before any toolchain is installed:if: steps.scope.outputs.suites != ''.cargo-test(50–60 min warm) remains the longer pole. Added wall-clock on the PR's critical path: ~0 for most PRs; roughly 0–30 min in the worst case of a suite-heavy diff whose e2e build outrunscargo-test.The job always produces a check run on PRs (it exits early rather than being
if-skipped on the diff), so it is safe to add to the required-checks list — worth doing, otherwise the gate is advisory.What this would and would not have caught
Honest accounting, since the point is to close a specific hole and not to claim more:
crates/perry/tests/capture_rereg_renamed_class.rs; the suite would have run and failed on the PR.crates/perry-codegen/tests/{native_proof_regressions,typed_feedback}.rsandcrates/perry/tests/local_bound_loop_semantics.rs(verified: pipinggit show --name-only ce0117a60into the new script selects exactly those three), so its own new suites would have been gated. The behavioural regression it shipped, however, showed up in existing fixtures and incompile-smoke, which only runs on tags/labeled PRs — that is why PR fix(codegen): scalar replacement ignored own-method writes and prototype mutation (#5872) #6324 had to put its acceptance coverage in unit tests to get it gated at all. With this job, an acceptance suite undercrates/perry/tests/is a first-class option again for that kind of fix.issue_cjs_destructured_var_forward_captureregressed on main by an unrelated source change) — not caught, by design. Same for the current nightly red (issue_5024_class_prototype_assignment_enumeration::assignment_registered_prototype_methods_are_enumerable, failing since at least 2026-07-05): the regressing PR's diff names neither suite. There is no coverage data to map source → suite with, and the only mapping available a priori is crate-level —perry-codegen→ all of perry's suites — which is precisely the full run this scoping exists to avoid. The nightly stays the backstop for that class, which is what motivates the second change:Concurrency (issue suggestion 2)
The starvation vector described in the issue is no longer live: pushes to
maindon't trigger this workflow, so there is no main-push run to be cancelled by a merge train, and PR runs live underrefs/pull/N/merge(their own group). The residual vector is narrow but real — a manualworkflow_dispatchonmainsharesrefs/heads/mainwith the cron and would cancel a nightly mid-flight (the last nightlies ran 1h0m–1h53m).So: keep
cancel-in-progressfor PR pushes (that's where the minutes are) and turn it off everywhere else, with the event in the group key.Two lines, no behaviour change for PRs, and the nightly + release-tag runs can no longer be cancelled. Given the nightly is now formally the only backstop for the #6037 class, it must always reach a conclusion.
Notes
lintis also red, on the Address-classification audit step — unrelated to this change, but it means the nightly is red on two independent counts right now.Validation
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/test.yml'))"— parses.python3 scripts/ci_e2e_scope.py --self-test— 8 cases: direct suite, source-only diff, deleted suite, module dir, sharedcommon/dir, excluded UI crate, dedup, unknown crate.ce0117a60(Representation-aware type lowering + native-ABI material evidence gate #5466) → the 3 suites it added; a synthetic fix(hir): capture-snapshot P0 pair — resolved-name re-registration + assignment tracking #5938-shaped diff →perry capture_rereg_renamed_class..github/andscripts/, so it names no integration suite —e2e-scopedwill report "nothing to run" here. The first PR to add or edit a suite is the one that exercises it end to end.Summary by CodeRabbit
e2e-scopedjob to run only the end-to-end suites affected by pull request changes.