perf(ci): make per-PR cargo-test fast (<10 min) — unit tests for affected crates; integration → nightly/tags#5411
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ 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 (5)
📝 WalkthroughWalkthroughAdds ChangesPR-scoped CI test selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 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 @.github/workflows/test.yml:
- Around line 315-327: The gh pr view command execution on line 316 lacks error
handling, which means if the command fails due to authentication or transient
errors, the changed_files variable becomes empty or contains error text and the
script silently proceeds. Add error checking immediately after the gh pr view
command invocation to detect and fail fast when the command fails, ensuring that
transient CLI failures do not result in silently skipped tests. Use set -e or an
explicit error check like if [ $? -ne 0 ] to guard against the gh pr view
command failure and prevent the script from continuing with invalid data.
🪄 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: a06aa730-f59c-4b97-a116-0bcfa30761b8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/test.ymlCHANGELOG.mdCLAUDE.mdCargo.tomlscripts/ci_test_scope.py
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| changed_files="$(gh pr view "${{ github.event.pull_request.number }}" \ | ||
| --json files --jq '.files[].path')" | ||
| echo "Changed files in PR:"; printf '%s\n' "$changed_files" | ||
| scope="$(printf '%s\n' "$changed_files" | python3 scripts/ci_test_scope.py)" | ||
| else | ||
| scope="$(python3 scripts/ci_test_scope.py --full </dev/null)" | ||
| fi | ||
| echo "Packages in test scope:"; printf '%s\n' "$scope" | ||
| if [ -z "$scope" ]; then | ||
| echo "No crates affected by this diff — nothing to test." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: compare gh PR-view file count vs paginated REST count.
# Usage:
# REPO=owner/name PR_NUMBER=123 bash verify_pr_file_count.sh
set -euo pipefail
: "${REPO:?set REPO=owner/name}"
: "${PR_NUMBER:?set PR_NUMBER=<number>}"
echo "gh pr view files count:"
gh pr view "$PR_NUMBER" --repo "$REPO" --json files --jq '.files | length'
echo "Paginated REST files count:"
gh api "repos/$REPO/pulls/$PR_NUMBER/files?per_page=100" --paginate --jq '.[].filename' | wc -lRepository: PerryTS/perry
Length of output: 102
🏁 Script executed:
# First, locate and read the workflow file
git ls-files ".github/workflows/*.yml" | head -20Repository: PerryTS/perry
Length of output: 514
🏁 Script executed:
# Read the specific section being reviewed
cat -n .github/workflows/test.yml | sed -n '310,330p'Repository: PerryTS/perry
Length of output: 1385
🏁 Script executed:
# Check if there are any other references to this script or error handling patterns
rg "gh pr view|changed_files|ci_test_scope" --type yaml -A 2 -B 2Repository: PerryTS/perry
Length of output: 39
🌐 Web query:
GitHub CLI gh pr view pagination limit file listing behavior
💡 Result:
The gh pr view command, when used with the --json files flag, historically suffered from a silent truncation of results at 100 items [1][2]. This behavior occurred because the GitHub GraphQL API, which the command relies upon, has a 100-node limit per request, and the command previously did not implement pagination to fetch subsequent pages [1][3]. As of May 2026, this behavior has been updated [1]. A pull request (cli/cli#13340) was merged to introduce pagination for files and commits in gh pr view --json [1]. This update adds PageInfo to the Files and Commits structs and implements concurrent preloading to fetch results beyond the initial 100-node limit [1]. Before this change, users requiring a complete list of files for pull requests with more than 100 changed files were advised to use the gh api command with the --paginate flag to correctly handle GraphQL pagination [3][4]. For simple listings, gh pr diff --name-only is also a common alternative that avoids this specific GraphQL limitation [5].
Citations:
- 1: Paginate files and commits in
gh pr view --jsoncli/cli#13340 - 2: The
pr viewcommand with the--json filesflag omits some of the files changed cli/cli#9916 - 3: Remove restrictions of
gh pr view --json filescli/cli#5368 - 4: Add option for gh pr diff to list the filenames that has changes on the PR cli/cli#1393
- 5: https://stackoverflow.com/questions/25071579/list-all-files-changed-in-a-pull-request-in-git-github
🏁 Script executed:
# Check if GitHub CLI version is pinned in the workflow
cat -n .github/workflows/test.yml | sed -n '1,100p' | grep -i "actions/github-script\|github-cli\|gh"Repository: PerryTS/perry
Length of output: 311
🏁 Script executed:
# Look for the full context of where this code runs to understand the shell environment
cat -n .github/workflows/test.yml | sed -n '280,340p'Repository: PerryTS/perry
Length of output: 3976
🏁 Script executed:
# Check if set -euo pipefail is used anywhere in this workflow script
cat .github/workflows/test.yml | sed -n '280,340p'Repository: PerryTS/perry
Length of output: 3549
🌐 Web query:
GitHub Actions runner GitHub CLI version June 2026 update pagination files
💡 Result:
As of June 2026, the GitHub CLI (gh) is at version 2.94.0 [1], which is the version included in current GitHub Actions runner images [2][3]. Regarding pagination and updates related to GitHub CLI files and commands in June 2026: 1. GitHub CLI 2.94.0 Release: This release, published on June 10, 2026, introduced several new commands, such as the gh discussion set and gh skill list [1]. It does not contain major breaking changes to general pagination logic but maintains standard behavior for existing commands [1]. 2. Pagination in gh pr view: Recent development has addressed silent truncation issues for specific pull request sub-resources [4]. Specifically, pull requests now support improved pagination for files and commits when using gh pr view --json, ensuring that results exceeding the standard 100-item limit are correctly fetched rather than truncated [4]. 3. General Pagination Behavior: - The --paginate flag remains the primary method for handling automatic pagination in gh api calls [5][6]. - For commands like gh issue list or gh pr list, users should continue to use the -L (--limit) flag to define result caps (defaulting to 30) [5]. - Users are advised that --paginate can only handle pagination for a single connection at a time; nested pagination (e.g., paginating comments within an issue) requires custom logic [6]. - Documentation notes that combining --paginate with --jq or --slurp can sometimes lead to complex or unexpected output behavior [7][5]. GitHub Actions runner images have been updated to include GitHub CLI 2.94.0 across all major OS platforms (Ubuntu 26.04, macOS-26, etc.) as of the mid-June 2026 image updates [2][8][9][10][11].
Citations:
- 1: https://github.com/cli/cli/releases/tag/v2.94.0
- 2: https://github.com/actions/runner-images/releases/tag/ubuntu26/20260615.44
- 3: https://github.com/actions/runner-images/releases
- 4: Paginate files and commits in
gh pr view --jsoncli/cli#13340 - 5: https://github.com/github/gh-cli/blob/v2.94.0/skills/gh/SKILL.md
- 6: https://github.blog/developer-skills/github/exploring-github-cli-how-to-interact-with-githubs-graphql-api-endpoint/
- 7: gh api --paginate is unreliable or confusing for /notifications responses cli/cli#13270
- 8: https://github.com/actions/runner-images/releases/tag/macos-26/20260608.0215
- 9: https://github.com/actions/runner-images/releases/tag/macos-15-arm64/20260610.0126
- 10: https://github.com/actions/runner-images/releases/tag/macos-15/20260609.0193
- 11: https://github.com/actions/runner-images/releases/tag/macos-26-arm64/20260610.0145
🌐 Web query:
cli/cli#13340 GitHub Actions deployment release date
💡 Result:
The GitHub issue referenced as cli/cli#13340 is titled Paginate files and commits in gh pr view --json [1]. This pull request was created on May 4, 2026, and is not related to a GitHub Actions deployment release date [1]. The repository cli/cli does maintain a deployment workflow located at.github/workflows/deployment.yml [2][3], but this file is part of the repository's internal infrastructure and is not associated with a public GitHub Actions release date [3]. There is no specific public release date for a GitHub Actions deployment feature under this issue number [1].
Citations:
- 1: Paginate files and commits in
gh pr view --jsoncli/cli#13340 - 2: fix: using variable interpolation `${{ in deployment.yml... cli/cli#13258
- 3: https://github.com/cli/cli/blob/537a22228cd6b42b740d7f1c09f47c45bb1dab30/.github/workflows/deployment.yml
Add error handling for gh pr view failures in PR test scope logic.
Line 316 calls gh pr view without guarding against command failures. If the CLI call fails (authentication issue, transient error, etc.), changed_files becomes empty or contains error text, and the script silently proceeds to line 324 where an empty scope exits with success, skipping all tests without alerting.
While GitHub CLI 2.94.0 (deployed to Actions runners mid-June 2026) fixed the 100-item truncation issue for PR files, the lack of error handling remains a availability risk. Tests can be silently skipped due to transient CLI failures.
Suggested hardening
+ set -euo pipefail
if [ "${{ github.event_name }}" = "pull_request" ]; then
- changed_files="$(gh pr view "${{ github.event.pull_request.number }}" \
- --json files --jq '.files[].path')"
- echo "Changed files in PR:"; printf '%s\n' "$changed_files"
- scope="$(printf '%s\n' "$changed_files" | python3 scripts/ci_test_scope.py)"
+ if ! changed_files="$(gh api \
+ "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files?per_page=100" \
+ --paginate --jq '.[].filename')"; then
+ echo "::warning::Failed to fetch PR files; falling back to full workspace scope."
+ scope="$(python3 scripts/ci_test_scope.py --full </dev/null)"
+ elif [ -z "$changed_files" ]; then
+ echo "::warning::Empty PR file list; falling back to full workspace scope."
+ scope="$(python3 scripts/ci_test_scope.py --full </dev/null)"
+ else
+ echo "Changed files in PR:"; printf '%s\n' "$changed_files"
+ scope="$(printf '%s\n' "$changed_files" | python3 scripts/ci_test_scope.py)"
+ fi
else
scope="$(python3 scripts/ci_test_scope.py --full </dev/null)"
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 315 - 327, The gh pr view command
execution on line 316 lacks error handling, which means if the command fails due
to authentication or transient errors, the changed_files variable becomes empty
or contains error text and the script silently proceeds. Add error checking
immediately after the gh pr view command invocation to detect and fail fast when
the command fails, ensuring that transient CLI failures do not result in
silently skipped tests. Use set -e or an explicit error check like if [ $? -ne 0
] to guard against the gh pr view command failure and prevent the script from
continuing with invalid data.
proggeramlug
force-pushed
the
perf/affected-crate-tests
branch
from
38873c7 to
1903916
Compare
proggeramlug
changed the title
…cted crates cargo-test took ~90 min: full workspace, serial debug build (CARGO_BUILD_JOBS=1 + clean-between-packages disk workaround), and 30/40 perry integration tests each auto-optimize-compile via perry (4-6 min each, unparallelizable — concurrent auto-opt builds thrash a shared target). Per-PR cargo-test is now (a) scoped to the diff (scripts/ci_test_scope.py: changed crates + reverse-dep closure, perry edge for runtime-linked stdlib/ffi/ext; infra/unknown -> full; metadata-only -> nothing) and (b) unit/lib/bin tests only (cargo test --lib --bins) — the slow auto-optimize integration tests are NOT run per-PR. Unit binaries are small so builds parallelize (no serialization/clean churn) and no staticlib is needed. Full suite (every integration test) runs on release tags, a new nightly schedule (04:00 UTC), workflow_dispatch, and the run-extended-tests label. test.yml branches the step on github.event_name.
proggeramlug
force-pushed
the
perf/affected-crate-tests
branch
from
1903916 to
0f7a728
Compare
…dules (#5389 Tier 2) A computed require(expr) in a compiled external/compilePackages module whose specifier const-folds to a finite set (ternary of literals, module-const, or a directory-anchored template glob) now resolves synchronously to the target compiled-module namespace, reusing the dynamic import() path resolver. - New synchronous: bool field on Expr::DynamicImport (false for import(), true for require()). - try_dynamic_require lowers a bare non-literal require in external modules to a synchronous DynamicImport; literals stay on try_require_literal. - collect_modules const-folds/globs paths unchanged; an unresolved synchronous node records an empty path set (ambient fallback) instead of import()'s deferred-reject/strict error. - lower_dynamic_require emits the same string_equals dispatch as import() but returns the namespace value directly (no Promise) with the Tier 1 ambient require (js_module_ambient_require_apply) as the no-match fallthrough. Genuinely runtime-computed package strings remain unsupported by design (same AOT boundary as dynamic import()). Also restores the v0.5.1184 changelog entry dropped from main during the #5411 merge (its code is on main).
…p zero-unit-test crates (#5413) The #5411 fast per-PR path built every affected crate's unit-test binary in one unbounded-parallel invocation. Each links the whole runtime statically, so a wide diff (~50 crates for a perry-runtime change) OOMed the runner at link ('linking with cc failed') — the failure the FULL path avoids via CARGO_BUILD_JOBS=1. Surfaced on #5402. Fast path now: CARGO_BUILD_JOBS=1 (serialize heavy links) + filter scope through ci_test_scope.py --with-tests (drop crates with no src/ unit tests, whose lib test binary links the runtime for zero tests). Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
…dules (#5389 Tier 2) A computed require(expr) in a compiled external/compilePackages module whose specifier const-folds to a finite set (ternary of literals, module-const, or a directory-anchored template glob) now resolves synchronously to the target compiled-module namespace, reusing the dynamic import() path resolver. - New synchronous: bool field on Expr::DynamicImport (false for import(), true for require()). - try_dynamic_require lowers a bare non-literal require in external modules to a synchronous DynamicImport; literals stay on try_require_literal. - collect_modules const-folds/globs paths unchanged; an unresolved synchronous node records an empty path set (ambient fallback) instead of import()'s deferred-reject/strict error. - lower_dynamic_require emits the same string_equals dispatch as import() but returns the namespace value directly (no Promise) with the Tier 1 ambient require (js_module_ambient_require_apply) as the no-match fallthrough. Genuinely runtime-computed package strings remain unsupported by design (same AOT boundary as dynamic import()). Also restores the v0.5.1184 changelog entry dropped from main during the #5411 merge (its code is on main).
…ims (#5415) Two issues made #5411's fast path fail on foundational diffs (#5402): 1. Feature unification: one multi-package 'cargo test -p A -p B' invocation unifies perry-runtime features; a crate enabling e.g. fetch turns on perry-runtime's js_fetch_with_options reference, defined in a separate crate the other test binaries don't link -> undefined reference. Fix: run each crate in its own invocation (per-package), isolating feature sets. 2. Over-broad fan-out: a perry-runtime change closured into ~40 FFI shims (perry-ext-*/perry-stdlib), each a perry-runtime feature rebuild, for unit tests that don't exercise runtime internals. Fix: ci_test_scope.py no longer fans into perry-ext-*/perry-stdlib (_is_fanout_leaf); direct changes still select them. perry-runtime change now -> 4 crates, not ~50. Validated locally end-to-end: green, ~2min warm. Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
…dules (#5389 Tier 2) A computed require(expr) in a compiled external/compilePackages module whose specifier const-folds to a finite set (ternary of literals, module-const, or a directory-anchored template glob) now resolves synchronously to the target compiled-module namespace, reusing the dynamic import() path resolver. - New synchronous: bool field on Expr::DynamicImport (false for import(), true for require()). - try_dynamic_require lowers a bare non-literal require in external modules to a synchronous DynamicImport; literals stay on try_require_literal. - collect_modules const-folds/globs paths unchanged; an unresolved synchronous node records an empty path set (ambient fallback) instead of import()'s deferred-reject/strict error. - lower_dynamic_require emits the same string_equals dispatch as import() but returns the namespace value directly (no Promise) with the Tier 1 ambient require (js_module_ambient_require_apply) as the no-match fallthrough. Genuinely runtime-computed package strings remain unsupported by design (same AOT boundary as dynamic import()). Also restores the v0.5.1184 changelog entry dropped from main during the #5411 merge (its code is on main).
…dules (#5389 Tier 2) A computed require(expr) in a compiled external/compilePackages module whose specifier const-folds to a finite set (ternary of literals, module-const, or a directory-anchored template glob) now resolves synchronously to the target compiled-module namespace, reusing the dynamic import() path resolver. - New synchronous: bool field on Expr::DynamicImport (false for import(), true for require()). - try_dynamic_require lowers a bare non-literal require in external modules to a synchronous DynamicImport; literals stay on try_require_literal. - collect_modules const-folds/globs paths unchanged; an unresolved synchronous node records an empty path set (ambient fallback) instead of import()'s deferred-reject/strict error. - lower_dynamic_require emits the same string_equals dispatch as import() but returns the namespace value directly (no Promise) with the Tier 1 ambient require (js_module_ambient_require_apply) as the no-match fallthrough. Genuinely runtime-computed package strings remain unsupported by design (same AOT boundary as dynamic import()). Also restores the v0.5.1184 changelog entry dropped from main during the #5411 merge (its code is on main).
…dules (#5389 Tier 2) (#5402) A computed require(expr) in a compiled external/compilePackages module whose specifier const-folds to a finite set (ternary of literals, module-const, or a directory-anchored template glob) now resolves synchronously to the target compiled-module namespace, reusing the dynamic import() path resolver. - New synchronous: bool field on Expr::DynamicImport (false for import(), true for require()). - try_dynamic_require lowers a bare non-literal require in external modules to a synchronous DynamicImport; literals stay on try_require_literal. - collect_modules const-folds/globs paths unchanged; an unresolved synchronous node records an empty path set (ambient fallback) instead of import()'s deferred-reject/strict error. - lower_dynamic_require emits the same string_equals dispatch as import() but returns the namespace value directly (no Promise) with the Tier 1 ambient require (js_module_ambient_require_apply) as the no-match fallthrough. Genuinely runtime-computed package strings remain unsupported by design (same AOT boundary as dynamic import()). Also restores the v0.5.1184 changelog entry dropped from main during the #5411 merge (its code is on main). Co-authored-by: Ralph Küpper <ralph2@skelpo.com>
1 participant
Summary
cargo-testtakes ~90 min because it tests the entire workspace in debug, serially, and 30 of 40 perry integration test files compile via the slow auto-optimize path (whole-program optimized rebuilds — individual tests run 4–6 min each). Most PRs touch a handful of crates, so this is mostly wasted work.This scopes per-PR
cargo-testto only the crates the diff can affect. Release tags, a new nightly cron, andworkflow_dispatchstill run the full workspace as the safety net.How
scripts/ci_test_scope.py— reads the PR's changed files and prints the packages to test:crates/<dir>plus its reverse-dependency closure (a change to a foundational crate likeperry-runtime/perry-hirstill fans out to all dependents);perry-stdlib,perry-ffi,perry-ext-*) add aperryedge, since the driver links those archives into compiled output at runtime (not via cargo) andperry's integration tests exercise them;.github/,scripts/,rust-toolchain*) or any unrecognized path → full workspace (conservative);CHANGELOG.md,CLAUDE.md,*.md,docs/, rootCargo.toml/Cargo.lock) → nothing (a version-bump PR is instantly green);.github/workflows/test.yml— the cargo-test step computes scope fromgh pr view --json files(PR) or--full(tag/cron/dispatch),exit 0s when nothing is affected, and only runs the single-threaded perry-runtime pass / builds the staticlibs when those are in scope. Adds a nightlyschedule(04:00 UTC) full-workspace run — main otherwise never runs the suite (pushes don't trigger Tests), so this is the cross-crate-regression backstop.Effect
perry-ext-*): tests that crate +perryonly.perry-hir,perry-runtime): still fans out (correct)..github/+scripts/→ runs the full workspace (validates the full path).Trade-off (deliberate)
A cross-crate regression a scoped PR misses is caught by the nightly full run rather than at PR time.
Verified
scripts/ci_test_scope.pyagainst representative inputs: metadata-only → 0;perry-stdlib→perry perry-stdlib;perry-ext-http→perry perry-ext-http;.github/→ full (66); foundationalperry-types→ 11 dependents.test.ymlvalidates as YAML with triggerspush, pull_request, schedule, workflow_dispatch.Summary by CodeRabbit