ci: perry-codegen integration suites can no longer be silently omitted from per-PR CI (#7708) - #7748
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe scope tool now maps all 22 ChangesCodegen E2E coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
ed5f393 to
2cbe58c
Compare
2cbe58c to
3fa5612
Compare
…d from per-PR CI (#7708)
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
860-875: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure
timeout-minutes: 450matches the platform cap.This job uses
runs-on: ubuntu-latest, so the GitHub-hosted 6-hour runner limit is the real job ceiling. A 450-minute job bound is not reachable; set the bound below the platform limit, or move this to a 5-day self-hosted runner if the scripted worst case actually needs more time.🤖 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 860 - 875, Update the job-level timeout-minutes setting in the workflow to remain below the GitHub-hosted ubuntu-latest six-hour platform limit, while preserving sufficient coverage for the documented worst-case suite duration. Do not move the job to a self-hosted runner unless the calculated worst case genuinely exceeds the hosted-runner cap, and keep the accompanying timeout rationale synchronized with the chosen bound.
🧹 Nitpick comments (2)
.github/workflows/test.yml (1)
1003-1005: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe gate and the loop use different package scopes.
Line 1003 gates the verification on
perry-codegenappearing in$SUITES. The loop at line 1004 then iterates over every entry in$EXCLUSIONS, including entries for other packages. Today all exclusions areperry-codegen, so the behavior is correct. If someone adds an exclusion for another package, that entry is verified only when aperry-codegensuite is in scope, and never otherwise.Filter each exclusion against the packages present in
$SUITESinstead of gating the whole block on one package name.🛠️ Proposed change
- if printf '%s\n' "$SUITES" | grep -q '^perry-codegen '; then - while read -r xpkg xsuite xtest; do - [ -n "$xpkg" ] || continue + while read -r xpkg xsuite xtest; do + [ -n "$xpkg" ] || continue + if printf '%s\n' "$SUITES" | grep -q "^$xpkg "; then🤖 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 1003 - 1005, Update the exclusion-processing block around the perry-codegen gate and its read loop so exclusions are filtered by the package names present in $SUITES. Remove the single-package gate, retain only entries whose package is in the active suites, and run verification for those matching exclusions regardless of which package is present.scripts/ci_e2e_scope.py (1)
521-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cap behavior, not only the split.
The comment states the cap applies to diff-named suites only. This case asserts
select_splitclassification. It never appliescap. The cap is applied inmainat lines 577-584, so a regression that capsmappedwould still pass this self-test. Extract the truncation into a small helper and assert it here, or assert directly that mapped suites survive truncation.♻️ Proposed additional assertion
mapped_pairs, named_pairs = select_split(named, root) if mapped_pairs or len(named_pairs) != 20: print( f"self-test FAILED: expected 0 mapped / 20 named, got " f"{len(mapped_pairs)}/{len(named_pairs)}", file=sys.stderr, ) return 1 + # The truncation itself: mapped is never cut, named is. + fake_mapped = [("perry-codegen", s) for s in _CODEGEN_SUITES] + capped = named_pairs[:DEFAULT_CAP] + if len(capped) != DEFAULT_CAP or len(fake_mapped) <= DEFAULT_CAP: + print("self-test FAILED: cap fixture no longer exercises truncation", + file=sys.stderr) + return 1🤖 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 521 - 540, Update the self-test around select_split to exercise the cap behavior itself, not just classification: extract the truncation logic currently in main into a small reusable helper, then apply it to the test’s mapped and named results and assert mapped suites remain intact while only diff-named suites are capped. Keep select_split classification assertions unchanged.
🤖 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 1007-1012: Update the excluded-test execution around the cargo
test command to preserve its exit status separately from captured output, while
still preventing immediate shell failure. Distinguish a completed test run that
reports one failure from infrastructure or execution failures such as
compilation errors, timeouts, or cargo errors; only report the SUITE_EXCLUSIONS
removal error when the test completed successfully or no matching test ran, and
emit an appropriate failure message for non-completed runs.
- Around line 976-997: Update the cargo test invocation in the suite loop to add
--exact whenever skips is non-empty, while preserving the current behavior when
no exclusions apply. Ensure each excluded test is matched by its exact name
rather than a substring.
In `@scripts/ci_e2e_scope.py`:
- Around line 82-104: Correct the rationale comment near the suite-map
declaration to use consistent counts: 24 suites exist, 22 are mapped, and 2 are
listed in SUITE_EXCLUSIONS, so 21 suites—not 23—are the in-process cases and 21
others were not part of the original narrow map. Ensure the prose matches the
changelog and allows the completeness invariant to be verified by hand.
---
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 860-875: Update the job-level timeout-minutes setting in the
workflow to remain below the GitHub-hosted ubuntu-latest six-hour platform
limit, while preserving sufficient coverage for the documented worst-case suite
duration. Do not move the job to a self-hosted runner unless the calculated
worst case genuinely exceeds the hosted-runner cap, and keep the accompanying
timeout rationale synchronized with the chosen bound.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 1003-1005: Update the exclusion-processing block around the
perry-codegen gate and its read loop so exclusions are filtered by the package
names present in $SUITES. Remove the single-package gate, retain only entries
whose package is in the active suites, and run verification for those matching
exclusions regardless of which package is present.
In `@scripts/ci_e2e_scope.py`:
- Around line 521-540: Update the self-test around select_split to exercise the
cap behavior itself, not just classification: extract the truncation logic
currently in main into a small reusable helper, then apply it to the test’s
mapped and named results and assert mapped suites remain intact while only
diff-named suites are capped. Keep select_split classification assertions
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e6d48df-e029-47f6-bd97-79114f11457a
📒 Files selected for processing (3)
.github/workflows/test.ymlchangelog.d/7748-codegen-suite-coverage.mdscripts/ci_e2e_scope.py
| while read -r package suite bound; do | ||
| [ -n "$package" ] || continue | ||
| skips="" | ||
| while read -r xpkg xsuite xtest; do | ||
| [ -n "$xpkg" ] || continue | ||
| if [ "$xpkg" = "$package" ] && [ "$xsuite" = "$suite" ]; then | ||
| skips="$skips --skip $xtest" | ||
| echo "::notice::skipping known failure $package::$suite::$xtest" | ||
| fi | ||
| done <<< "$EXCLUSIONS" | ||
| echo "::group::cargo test -p $package --test $suite" | ||
| # Per-suite wall-clock bound: a hung compile must not eat the whole | ||
| # job budget and hide the other suites' results. | ||
| if ! timeout 1500 cargo test -p "$package" --test "$suite"; then | ||
| # job budget and hide the other suites' results. Mapped in-process | ||
| # suites carry a much tighter bound than a diff-named one that | ||
| # shells out to `perry compile` — see ci_e2e_scope.py. | ||
| # shellcheck disable=SC2086 | ||
| if ! timeout "${bound:-1500}" cargo test -p "$package" --test "$suite" -- $skips; then | ||
| echo "::error::integration suite failed: $package --test $suite" | ||
| status=1 | ||
| fi | ||
| echo "::endgroup::" | ||
| done <<< "$SUITES" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Rust libtest --skip substring match --exact flag behavior
💡 Result:
In Rust's libtest harness, the interaction between test filtering, the --skip flag, and the --exact flag works as follows: 1. --skip Behavior The --skip flag accepts a filter string and causes the test runner to skip any tests whose full path (including module structure) contains that string as a substring [1][2]. You can provide the --skip flag multiple times to exclude multiple sets of tests [3][2]. 2. --exact Flag Behavior The --exact flag modifies how filter strings are interpreted [2][4]. By default, positional arguments and filters are matched against any substring of the test's full path [1][2]. When --exact is used, it forces the harness to match the provided filter string exactly against the full path of the test function [1][2]. 3. Combined Interaction While --exact is primarily designed to control how the primary filter (the positional argument) is matched, it fundamentally changes the matching logic used by the runner. The internal implementation of libtest uses a matches_filter function to evaluate both inclusion (positional arguments) and exclusion (--skip) [3]. The --exact flag instructs the runner to use strict equality for these comparisons rather than substring containment [3][2]. In summary: - Default: Matches occur if the filter string is a substring of the test's full path [1][2]. - With --exact: Matches occur only if the filter string is identical to the test's full path [1][2]. - The --skip flag applies this matching logic in reverse, excluding tests that meet the defined criteria [3][5]. To pass these arguments via Cargo, they must be placed after the -- separator: cargo test -- --skip --exact
Citations:
- 1: https://doc.rust-lang.org/rustc/tests/index.html
- 2: https://doc.rust-lang.org/stable/rustc/tests/index.html
- 3: https://github.com/rust-lang/libtest/blob/master/libtest/lib.rs
- 4: https://doc.rust-lang.org/nightly/rustc/tests/index.html
- 5: libtest: add a --skip flag to the test runner rust-lang/rust#36604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate workflow files:"
fd -a 'test\.ya?ml$' .github/workflows 2>/dev/null || true
echo
echo "Relevant lines around test invocation and exclusions:"
if [ -f ".github/workflows/test.yml" ]; then
nl -ba .github/workflows/test.yml | sed -n '940,1030p'
fi
echo
echo "Search for EXCLUSIONS and suite setup:"
rg -n 'EXCLUSIONS|SUITES|known-failure|timeout 300|--exact|--skip' .github/workflows/test.ymlRepository: PerryTS/perry
Length of output: 316
🌐 Web query:
Rust cargo test harness --exact flag does it affect multiple --skip filters substring match
💡 Result:
The --exact flag in Rust's libtest harness does affect how --skip filters are evaluated [1][2]. When --exact is used, it enforces that both inclusion filters (the positional arguments) and exclusion filters (the arguments provided to --skip) must match the full path of the test name exactly, rather than performing a substring match [1][3]. In standard behavior (without --exact), --skip matches any test whose name contains the provided filter string [1][2]. When --exact is active, any filter string passed to --skip must match a test's full path precisely to trigger the exclusion [1][4]. This is particularly useful when you want to avoid accidentally skipping multiple tests that share a common substring in their names [3]. Key points: - The --exact flag applies globally to the libtest harness arguments passed after the -- separator [1][4]. - It switches both inclusion and exclusion matching from substring-based to full-path-exact-matching [1][3]. - You can still pass multiple --skip filters when --exact is used, but each of those filters must now represent a full, exact test path [1][2].
Citations:
- 1: https://notriddle.com/notriddle-rustdoc-demos/help-page/rustc/tests/index.html
- 2: Running
cargo test -- --helpdoesn't produce expected output rust-lang/cargo#10392 - 3: Run tests that were not requested to be run rust-lang/cargo#11709
- 4: https://nexte.st/docs/selecting/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant lines around test invocation and exclusions:"
awk '{printf "%6d\t%s\n", NR, $0}' .github/workflows/test.yml | sed -n '940,1030p'
echo
echo "Search for EXCLUSIONS/SUITES/known-failure matching:"
grep -nE 'EXCLUSIONS|SUITES|known-failure|timeout 300|--exact|--skip' .github/workflows/test.yml || trueRepository: PerryTS/perry
Length of output: 7625
Use exact matching for suite skips
In the suite loop, --skip <test> skips any test whose full path contains that substring, while --exact is required to skip only the named test. If another test includes an excluded test name as a substring, the suite skip list can bypass it silently. Add --exact to the cargo test invocation when skips is non-empty.
🤖 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 976 - 997, Update the cargo test
invocation in the suite loop to add --exact whenever skips is non-empty, while
preserving the current behavior when no exclusions apply. Ensure each excluded
test is matched by its exact name rather than a substring.
| out="$(timeout 300 cargo test -p "$xpkg" --test "$xsuite" -- --exact "$xtest" 2>&1 || true)" | ||
| printf '%s\n' "$out" | ||
| if ! printf '%s\n' "$out" | grep -q '1 failed'; then | ||
| echo "::error::$xpkg::$xsuite::$xtest is listed in SUITE_EXCLUSIONS but did not fail (it passed, or no test matched that name). Delete its entry from SUITE_EXCLUSIONS in scripts/ci_e2e_scope.py and let the suite run it." | ||
| status=1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A build or timeout failure is reported as "did not fail".
|| true at line 1007 discards the exit status. The check then relies on the string 1 failed being present in $out. A compile error, a timeout 300 kill, or a cargo invocation error all produce output without that string. The job then fails with a message that tells the author to delete the SUITE_EXCLUSIONS entry, which is the wrong instruction.
Capture the exit status and distinguish "the test ran and passed" from "the run did not complete".
🛠️ Proposed change
- out="$(timeout 300 cargo test -p "$xpkg" --test "$xsuite" -- --exact "$xtest" 2>&1 || true)"
- printf '%s\n' "$out"
- if ! printf '%s\n' "$out" | grep -q '1 failed'; then
+ out="$(timeout 300 cargo test -p "$xpkg" --test "$xsuite" -- --exact "$xtest" 2>&1)" && rc=0 || rc=$?
+ printf '%s\n' "$out"
+ if [ "$rc" -eq 124 ] || ! printf '%s\n' "$out" | grep -qE '^test result:'; then
+ echo "::error::$xpkg::$xsuite::$xtest known-failure check did not complete (build error or timeout). This is not an exclusion bookkeeping problem."
+ status=1
+ elif ! printf '%s\n' "$out" | grep -q '1 failed'; then
echo "::error::$xpkg::$xsuite::$xtest is listed in SUITE_EXCLUSIONS but did not fail (it passed, or no test matched that name). Delete its entry from SUITE_EXCLUSIONS in scripts/ci_e2e_scope.py and let the suite run it."
status=1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| out="$(timeout 300 cargo test -p "$xpkg" --test "$xsuite" -- --exact "$xtest" 2>&1 || true)" | |
| printf '%s\n' "$out" | |
| if ! printf '%s\n' "$out" | grep -q '1 failed'; then | |
| echo "::error::$xpkg::$xsuite::$xtest is listed in SUITE_EXCLUSIONS but did not fail (it passed, or no test matched that name). Delete its entry from SUITE_EXCLUSIONS in scripts/ci_e2e_scope.py and let the suite run it." | |
| status=1 | |
| fi | |
| out="$(timeout 300 cargo test -p "$xpkg" --test "$xsuite" -- --exact "$xtest" 2>&1)" && rc=0 || rc=$? | |
| printf '%s\n' "$out" | |
| if [ "$rc" -eq 124 ] || ! printf '%s\n' "$out" | grep -qE '^test result:'; then | |
| echo "::error::$xpkg::$xsuite::$xtest known-failure check did not complete (build error or timeout). This is not an exclusion bookkeeping problem." | |
| status=1 | |
| elif ! printf '%s\n' "$out" | grep -q '1 failed'; then | |
| echo "::error::$xpkg::$xsuite::$xtest is listed in SUITE_EXCLUSIONS but did not fail (it passed, or no test matched that name). Delete its entry from SUITE_EXCLUSIONS in scripts/ci_e2e_scope.py and let the suite run it." | |
| status=1 | |
| fi |
🤖 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 1007 - 1012, Update the
excluded-test execution around the cargo test command to preserve its exit
status separately from captured output, while still preventing immediate shell
failure. Distinguish a completed test run that reports one failure from
infrastructure or execution failures such as compilation errors, timeouts, or
cargo errors; only report the SUITE_EXCLUSIONS removal error when the test
completed successfully or no matching test ran, and emit an appropriate failure
message for non-completed runs.
| # What changed in #7708: this map listed THREE suites. `crates/perry-codegen/ | ||
| # tests/` holds 24, and 23 of them are the same kind of thing — in-process | ||
| # compiles of hand-built HIR with `emit_ir_only: true`. The narrowness was not a | ||
| # judgment about the other 20; it was what one commit had time to verify. Nothing | ||
| # said so, and nothing could: a suite that is in neither the map nor any rule is | ||
| # indistinguishable from a suite nobody needed to map. | ||
| # | ||
| # So the map is now COMPLETE-BY-CONSTRUCTION for this crate. `_assert_map_covers_ | ||
| # codegen_suites` (run by `--self-test`, which the `e2e-scoped` job executes | ||
| # before it does anything else) requires every `crates/perry-codegen/tests/*.rs` | ||
| # on disk to be either mapped here or named in `SUITE_EXCLUSIONS` below. A new | ||
| # suite fails CI until someone classifies it. That is the "cannot silently omit | ||
| # a suite" property #7708 asked for; the map is still hand-written, but an | ||
| # omission is no longer silent. | ||
| # | ||
| # Measured on arm64 macOS, debug, `--test-threads=1`, one `cargo test -p | ||
| # perry-codegen --test <suite>` per row: every one of the 22 mapped suites | ||
| # completes in 2.2-10.4 s wall clock, and nearly all of that is cargo's | ||
| # freshness check — the test time itself is sub-second (e.g. shadow_slot_hygiene | ||
| # 12 tests / 0.10 s, native_proof_buffer_views 36 / 0.2 s). The cost is the | ||
| # build, which `e2e-scoped` already pays whenever any perry-codegen suite is in | ||
| # scope. They are NOT like the 163 `perry` suites the general refusal protects | ||
| # against: no `perry compile` subprocess, no link, no runtime. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the suite counts in the rationale comment.
Line 83 states 24 suites on disk and that 23 are in-process compiles. The file maps 22 suites and excludes 2, which totals 24. The "23" and the "other 20" on line 85 do not match those numbers. The changelog fragment states "The other 21 were dark". Align the comment with the actual counts so a future reader can verify the completeness invariant by hand.
📝 Proposed comment correction
-# What changed in `#7708`: this map listed THREE suites. `crates/perry-codegen/
-# tests/` holds 24, and 23 of them are the same kind of thing — in-process
-# compiles of hand-built HIR with `emit_ir_only: true`. The narrowness was not a
-# judgment about the other 20; it was what one commit had time to verify. Nothing
+# What changed in `#7708`: this map listed THREE suites. `crates/perry-codegen/
+# tests/` holds 24, and all of them are the same kind of thing — in-process
+# compiles of hand-built HIR with `emit_ir_only: true`. The narrowness was not a
+# judgment about the other 21; it was what one commit had time to verify. Nothing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # What changed in #7708: this map listed THREE suites. `crates/perry-codegen/ | |
| # tests/` holds 24, and 23 of them are the same kind of thing — in-process | |
| # compiles of hand-built HIR with `emit_ir_only: true`. The narrowness was not a | |
| # judgment about the other 20; it was what one commit had time to verify. Nothing | |
| # said so, and nothing could: a suite that is in neither the map nor any rule is | |
| # indistinguishable from a suite nobody needed to map. | |
| # | |
| # So the map is now COMPLETE-BY-CONSTRUCTION for this crate. `_assert_map_covers_ | |
| # codegen_suites` (run by `--self-test`, which the `e2e-scoped` job executes | |
| # before it does anything else) requires every `crates/perry-codegen/tests/*.rs` | |
| # on disk to be either mapped here or named in `SUITE_EXCLUSIONS` below. A new | |
| # suite fails CI until someone classifies it. That is the "cannot silently omit | |
| # a suite" property #7708 asked for; the map is still hand-written, but an | |
| # omission is no longer silent. | |
| # | |
| # Measured on arm64 macOS, debug, `--test-threads=1`, one `cargo test -p | |
| # perry-codegen --test <suite>` per row: every one of the 22 mapped suites | |
| # completes in 2.2-10.4 s wall clock, and nearly all of that is cargo's | |
| # freshness check — the test time itself is sub-second (e.g. shadow_slot_hygiene | |
| # 12 tests / 0.10 s, native_proof_buffer_views 36 / 0.2 s). The cost is the | |
| # build, which `e2e-scoped` already pays whenever any perry-codegen suite is in | |
| # scope. They are NOT like the 163 `perry` suites the general refusal protects | |
| # against: no `perry compile` subprocess, no link, no runtime. | |
| # What changed in `#7708`: this map listed THREE suites. `crates/perry-codegen/ | |
| # tests/` holds 24, and all of them are the same kind of thing — in-process | |
| # compiles of hand-built HIR with `emit_ir_only: true`. The narrowness was not a | |
| # judgment about the other 21; it was what one commit had time to verify. Nothing | |
| # said so, and nothing could: a suite that is in neither the map nor any rule is indistinguishable from a suite nobody needed to map. | |
| # | |
| # So the map is now COMPLETE-BY-CONSTRUCTION for this crate. `_assert_map_covers_ | |
| # codegen_suites` (run by `--self-test`, which the `e2e-scoped` job executes | |
| # before it does anything else) requires every `crates/perry-codegen/tests/*.rs` | |
| # on disk to be either mapped here or named in `SUITE_EXCLUSIONS` below. A new | |
| # suite fails CI until someone classifies it. That is the "cannot silently omit | |
| # a suite" property `#7708` asked for; the map is still hand-written, but an | |
| # omission is no longer silent. | |
| # | |
| # Measured on arm64 macOS, debug, `--test-threads=1`, one `cargo test -p | |
| # perry-codegen --test <suite>` per row: every one of the 22 mapped suites | |
| # completes in 2.2-10.4 s wall clock, and nearly all of that is cargo's | |
| # freshness check — the test time itself is sub-second (e.g. shadow_slot_hygiene | |
| # 12 tests / 0.10 s, native_proof_buffer_views 36 / 0.2 s). The cost is the | |
| # build, which `e2e-scoped` already pays whenever any perry-codegen suite is in | |
| # scope. They are NOT like the 163 `perry` suites the general refusal protects | |
| # against: no `perry compile` subprocess, no link, no runtime. |
🤖 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 82 - 104, Correct the rationale comment
near the suite-map declaration to use consistent counts: 24 suites exist, 22 are
mapped, and 2 are listed in SUITE_EXCLUSIONS, so 21 suites—not 23—are the
in-process cases and 21 others were not part of the original narrow map. Ensure
the prose matches the changelog and allows the completeness invariant to be
verified by hand.
3fa5612 to
e570cff
Compare
Merging as v0.5.1438 — this closes #7708 properly rather than adding three more suitesThe triage cross-checks exactly"Four of the six are fixed, by unrelated work, with nothing recording it" matches what I measured independently today: #7730 took And the observation that matters more than the count — four entries silently stopped being true — is the same bookkeeping failure #797 recorded for the parity skip-list. Letting that shape the design was the right call. 3 → 22, complete-by-constructionThe old map wasn't a judgment about the other 20; it was what one commit had time to verify, and nothing could tell you that. A suite in neither the map nor any rule was indistinguishable from a suite nobody needed to map.
Naming the issue in the failure text is what makes the next person's five minutes cheap. Two design decisions I'd have got wrongPer-test exclusions, not per-suite. Holding out a whole 262-test suite for one failure is exactly how 261 tests' worth of coverage went dark in the first place. And they're self-invalidating in both directions — the job runs the held-out tests and fails if one passes, so a fix cannot land leaving its exclusion behind. That is the property the parity skip-list lacked. The On the validationExecuting the workflow bash with a stubbed The honest note that this PR touches no Gates 19/19, |
1 participant
Fixes #7708. Supersedes #7507 (which asked for six of these suites and was blocked on them being green).
1. Triage of the six
Re-ran
cargo test -p perry-codegensuite-by-suite onmain(v0.5.1432). Four of the six are fixed, by unrelated work, with nothing recording it:integer_arithmetic_array_push_omits_inbounds_layout_note_and_barrierinteger_modulo::i32_counter_mod_unsafe_or_nonliteral_divisors_keep_fremproven_buffer_and_typed_array_reads_are_numeric_operandsreassigned_typed_array_store_records_runtime_fallbacklarge_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barriernative_proof_regressions::typed_f64_receiver_method_clone_raw_loads_after_composed_guardsFour entries silently stopped being true, which is exactly the bookkeeping failure #797 recorded for the parity skip-list. That shaped the design below more than the count did.
2. The map was three suites; the crate has twenty-four
SOURCE_SUITE_MAPmappedcrates/perry-codegen/src/toshadow_slot_hygiene,scalar_replaced_slot_roots,temp_root_operand_temporaries. 23 of the 24 suites in that crate are the same kind of thing — in-process compiles of hand-built HIR withemit_ir_only: true, noperry compilesubprocess, no link, no runtime. The narrowness was never a judgment about the other 20; it was what one commit had time to verify. And nothing could tell you: a suite in neither the map nor any rule is indistinguishable from a suite nobody needed to map.Measured, one
cargo test -p perry-codegen --test <suite>per row, arm64 macOS / debug:Nearly all of each number is cargo's freshness check; the test time is sub-second (
shadow_slot_hygiene12 tests / 0.10 s,native_proof_buffer_views36 / 0.2 s). The cost is the build, whiche2e-scopedalready pays whenever any perry-codegen suite is in scope.The map is now complete-by-construction: 22 suites, up from 3.
3. Omission can no longer be silent
_assert_map_covers_codegen_suites— run by--self-test, whiche2e-scopedexecutes before it installs a toolchain — requires everycrates/perry-codegen/tests/*.rson disk to be either mapped or in the newSUITE_EXCLUSIONS. A new suite fails CI until someone classifies it. The map stays hand-written (there is still no coverage data), but an omission is no longer indistinguishable from a decision.The sabotage case is in the self-test: it plants an unclassified suite and requires the raise. A rule that has never failed is a rule nobody has verified — CLAUDE.md hazard 4.
4. Exclusions are per-test and die when fixed
Holding out a whole 262-test suite for one failure is how 261 tests' worth of coverage went dark. So the two known failures are excluded by test name; the suites still run with
--skip.Self-invalidating in both directions:
--self-testrejects an entry whose suite is gone, or which is also mapped.5. The cap would have re-created the bug one layer up
--cap(default 12) exists so a mass suite rename can't turn one PR into a multi-hourperry compilerun. Applied to 22 mapped suites it would have silently truncated the set the map just completed. So the cap now applies to diff-named suites only, and the two tiers carry different per-suite bounds (MAPPED_SUITE_TIMEOUT_S=300vsNAMED_SUITE_TIMEOUT_S=1500). The job'stimeout-minutesmath is updated in the same commit and the comment states the new arithmetic.Validation
python3 scripts/ci_e2e_scope.py --self-test— passes, including the two new sabotage cases (unclassified suite must raise; cap must not touch mapped suites).cargoacross five scenarios: (1) all-as-recorded → exit 0, 22 suites, 2 known-failure checks, 0 errors; (2) an excluded test now passes → exit 1 with the delete-the-entry error; (3) a mapped suite regresses → exit 1; (4) a diff that names an excluded suite directly still gets--skip; (5) a non-codegen diff runs no known-failure checks.cargoagainst a built tree: 22 suites all green, both known-failure checks still failing as recorded, exit 0.Note that this PR touches no
crates/perry-codegen/src/file, soe2e-scopedon this PR will select nothing — hence the local end-to-end run above. The first PR that touches codegen source is the first live exercise.No version bump (maintainer bumps at merge).
Summary by CodeRabbit
Bug Fixes
Tests