-
-
Notifications
You must be signed in to change notification settings - Fork 157
ci: perry-codegen integration suites can no longer be silently omitted from per-PR CI (#7708) #7748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -837,7 +837,8 @@ jobs: | |||||||||||||||||||||||||||||||
| # changed `crates/<pkg>/tests/<suite>.rs` -> cargo test -p <pkg> --test <suite> | ||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||
| # (plus suite module dirs and `common/` helper dirs — see | ||||||||||||||||||||||||||||||||
| # scripts/ci_e2e_scope.py, capped at 12 suites). | ||||||||||||||||||||||||||||||||
| # scripts/ci_e2e_scope.py — diff-named suites capped at 12; the perry-codegen | ||||||||||||||||||||||||||||||||
| # source map added in #7708 is uncapped and in-process). | ||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||
| # Cost: PRs that touch no integration suite — the large majority — finish in | ||||||||||||||||||||||||||||||||
| # ~20-30s of checkout + scope computation and never install a toolchain or | ||||||||||||||||||||||||||||||||
|
|
@@ -860,13 +861,18 @@ jobs: | |||||||||||||||||||||||||||||||
| # clock kills legitimately-running suites and we get an uninformative | ||||||||||||||||||||||||||||||||
| # "cancelled" instead of the per-suite `::error::` — reintroducing, one | ||||||||||||||||||||||||||||||||
| # level up, exactly what `timeout 1500` below exists to prevent. | ||||||||||||||||||||||||||||||||
| # DEFAULT_CAP (ci_e2e_scope.py) = 12 suites | ||||||||||||||||||||||||||||||||
| # per-suite bound below = timeout 1500s = 25 min | ||||||||||||||||||||||||||||||||
| # 12 x 25 = 300, + 30 min for toolchain/cache/staticlib build = 330 | ||||||||||||||||||||||||||||||||
| # Keep these three in sync. This is a backstop, not a budget: each suite is | ||||||||||||||||||||||||||||||||
| # DEFAULT_CAP (ci_e2e_scope.py) = 12 diff-named suites | ||||||||||||||||||||||||||||||||
| # NAMED_SUITE_TIMEOUT_S = 1500s = 25 min -> 12 x 25 = 300 | ||||||||||||||||||||||||||||||||
| # #7708 mapped perry-codegen = 22 suites, uncapped, but in-process: | ||||||||||||||||||||||||||||||||
| # MAPPED_SUITE_TIMEOUT_S = 300s = 5 min -> 22 x 5 = 110 | ||||||||||||||||||||||||||||||||
| # + 2 known-failure checks at 5 min = 10 | ||||||||||||||||||||||||||||||||
| # + 30 min for toolchain/cache/staticlib build | ||||||||||||||||||||||||||||||||
| # total = 450 | ||||||||||||||||||||||||||||||||
| # Keep these in sync with ci_e2e_scope.py. This is a backstop, not a budget: | ||||||||||||||||||||||||||||||||
| # every mapped suite measured 2.2-10.4 s wall clock, each suite is | ||||||||||||||||||||||||||||||||
| # independently bounded, and the common case selects zero suites and exits | ||||||||||||||||||||||||||||||||
| # in ~20-30s. | ||||||||||||||||||||||||||||||||
| timeout-minutes: 330 | ||||||||||||||||||||||||||||||||
| timeout-minutes: 450 | ||||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||||
| RUSTC_WRAPPER: sccache | ||||||||||||||||||||||||||||||||
| SCCACHE_GHA_ENABLED: "false" | ||||||||||||||||||||||||||||||||
|
|
@@ -891,6 +897,13 @@ jobs: | |||||||||||||||||||||||||||||||
| changed_files="$(gh pr view "${{ github.event.pull_request.number }}" \ | ||||||||||||||||||||||||||||||||
| --json files --jq '.files[].path')" | ||||||||||||||||||||||||||||||||
| suites="$(printf '%s\n' "$changed_files" | python3 scripts/ci_e2e_scope.py)" | ||||||||||||||||||||||||||||||||
| # #7708: the held-out tests, so the run step can skip them AND assert | ||||||||||||||||||||||||||||||||
| # each one still fails. | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| echo 'exclusions<<PERRY_EOF' | ||||||||||||||||||||||||||||||||
| python3 scripts/ci_e2e_scope.py --exclusions | ||||||||||||||||||||||||||||||||
| echo 'PERRY_EOF' | ||||||||||||||||||||||||||||||||
| } >> "$GITHUB_OUTPUT" | ||||||||||||||||||||||||||||||||
| if [ -z "$suites" ]; then | ||||||||||||||||||||||||||||||||
| echo "No integration suite named by this diff — nothing to run." | ||||||||||||||||||||||||||||||||
| echo "suites=" >> "$GITHUB_OUTPUT" | ||||||||||||||||||||||||||||||||
|
|
@@ -945,6 +958,7 @@ jobs: | |||||||||||||||||||||||||||||||
| # Bound the heavy per-binary runtime link so the runner doesn't OOM. | ||||||||||||||||||||||||||||||||
| CARGO_BUILD_JOBS: "1" | ||||||||||||||||||||||||||||||||
| SUITES: ${{ steps.scope.outputs.suites }} | ||||||||||||||||||||||||||||||||
| EXCLUSIONS: ${{ steps.scope.outputs.exclusions }} | ||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||
| # `cargo test` never builds the `staticlib` crate-type, so | ||||||||||||||||||||||||||||||||
| # libperry_{runtime,stdlib}.a don't exist unless built explicitly — | ||||||||||||||||||||||||||||||||
|
|
@@ -956,17 +970,49 @@ jobs: | |||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| status=0 | ||||||||||||||||||||||||||||||||
| while read -r package suite; do | ||||||||||||||||||||||||||||||||
| # #7708: every suite runs with its known-failing tests skipped, in | ||||||||||||||||||||||||||||||||
| # whichever tier selected it. Holding out a TEST instead of a SUITE is | ||||||||||||||||||||||||||||||||
| # what lets `native_proof_regressions` contribute its other 261. | ||||||||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # #7708: the exclusions are self-invalidating. A held-out test that | ||||||||||||||||||||||||||||||||
| # now PASSES (or that no longer exists under that name) fails the job, | ||||||||||||||||||||||||||||||||
| # so a fix cannot land while leaving its entry behind — the failure | ||||||||||||||||||||||||||||||||
| # mode #797 recorded for the parity skip-list. | ||||||||||||||||||||||||||||||||
| if printf '%s\n' "$SUITES" | grep -q '^perry-codegen '; then | ||||||||||||||||||||||||||||||||
| while read -r xpkg xsuite xtest; do | ||||||||||||||||||||||||||||||||
| [ -n "$xpkg" ] || continue | ||||||||||||||||||||||||||||||||
| echo "::group::known-failure check $xpkg::$xsuite::$xtest" | ||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||
|
Comment on lines
+1007
to
+1012
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win A build or timeout failure is reported as "did not fail".
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| echo "::endgroup::" | ||||||||||||||||||||||||||||||||
| done <<< "$EXCLUSIONS" | ||||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||||
| exit "$status" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # --------------------------------------------------------------------------- | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 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:
🏁 Script executed:
Repository: 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:
cargo test -- --helpdoesn't produce expected output rust-lang/cargo#10392🏁 Script executed:
Repository: 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--exactis 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--exactto thecargo testinvocation whenskipsis non-empty.🤖 Prompt for AI Agents