test(pacquet): do not tolerate the lacking of tools#11713
Merged
Conversation
Replace runtime probe-and-skip helpers with the existing `.unwrap()` calls — if `git`, `node`, or `npm` is missing the test will fail loudly rather than silently return. Tolerance defeats the purpose of testing: an under-provisioned environment is a bug in the environment, not something the suite should paper over. Git is ubiquitous and Node.js is a documented prerequisite for building pnpm. Codify the rule in pacquet/AGENTS.md so future ports don't reintroduce the pattern. Platform-locked tools are still allowed to gate via `#[cfg(...)]` or `#[cfg_attr(..., ignore = "...")]`, which surface in the test report instead of disappearing. --- Written by an agent (Claude Code, claude-opus-4-7).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
📝 WalkthroughWalkthroughThis PR adds a guidance rule forbidding runtime "tolerant" tests for missing external tools and removes PATH-probing skip helpers/guards from git-fetcher tests so those tests run unconditionally and fail loudly when tools like git/npm/node are absent. ChangesTest Tolerance Policy and Enforcement
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11713 +/- ##
==========================================
+ Coverage 88.92% 89.68% +0.76%
==========================================
Files 129 129
Lines 14845 14858 +13
==========================================
+ Hits 13201 13326 +125
+ Misses 1644 1532 -112 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Micro-Benchmark ResultsLinux |
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.43620817574,
"stddev": 0.0870583789464612,
"median": 2.43639748464,
"user": 2.69442018,
"system": 3.6541902,
"min": 2.30934284114,
"max": 2.5897108491400003,
"times": [
2.42292268514,
2.50813800314,
2.46099763914,
2.5897108491400003,
2.5022386241400003,
2.42389028914,
2.30934284114,
2.44890468014,
2.31373312514,
2.38220302114
]
},
{
"command": "pacquet@main",
"mean": 2.3608034719399997,
"stddev": 0.07982887324293203,
"median": 2.33624741814,
"user": 2.7137985799999997,
"system": 3.6281855999999997,
"min": 2.27426231514,
"max": 2.54780195314,
"times": [
2.32518226514,
2.36198060414,
2.2901866381400002,
2.31457086514,
2.27426231514,
2.4131004541400003,
2.34731257114,
2.40872466914,
2.32491238414,
2.54780195314
]
},
{
"command": "pnpm",
"mean": 4.65380335764,
"stddev": 0.029797301062884227,
"median": 4.66580558464,
"user": 7.95615108,
"system": 4.0925464,
"min": 4.60079030914,
"max": 4.68637838314,
"times": [
4.66917115414,
4.6624400151400005,
4.68550662514,
4.60079030914,
4.62380772214,
4.68637838314,
4.63216156414,
4.67239134914,
4.67601119214,
4.62937526214
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.70440574496,
"stddev": 0.062233448187051434,
"median": 0.6834164149599999,
"user": 0.37877948000000006,
"system": 1.5879343399999999,
"min": 0.66955688496,
"max": 0.87994479296,
"times": [
0.87994479296,
0.69780552996,
0.69107993796,
0.68027252196,
0.6960746639600001,
0.68318387496,
0.66955688496,
0.68034516196,
0.68364895496,
0.68214512596
]
},
{
"command": "pacquet@main",
"mean": 0.7344441802600001,
"stddev": 0.032549240880235954,
"median": 0.72160832096,
"user": 0.38396317999999996,
"system": 1.6130340399999998,
"min": 0.7032097269600001,
"max": 0.78998346296,
"times": [
0.78998346296,
0.70389381296,
0.7716880619600001,
0.71750279496,
0.7243388979600001,
0.7032097269600001,
0.7770006339600001,
0.71887774396,
0.70817389096,
0.72977277596
]
},
{
"command": "pnpm",
"mean": 2.51127179086,
"stddev": 0.09596818453758928,
"median": 2.52328249946,
"user": 3.0270620799999994,
"system": 2.2048831399999997,
"min": 2.3811284599599998,
"max": 2.6637088599600003,
"times": [
2.6637088599600003,
2.56724612896,
2.52269739796,
2.46545491096,
2.44202409796,
2.38299796196,
2.52704061496,
2.52386760096,
2.3811284599599998,
2.63655187496
]
}
]
} |
Contributor
There was a problem hiding this comment.
Pull request overview
Removes runtime “probe-and-skip” helpers from pacquet-git-fetcher tests so missing git / node / npm causes a hard test failure, and documents this testing rule in pacquet’s agent guidelines to prevent the pattern from being reintroduced.
Changes:
- Deleted
skip_if_no_git/skip_if_no_npmhelpers and the early-returnskips ingit-fetchertests. - Added an explicit “no tolerant tests for missing tools” guideline to
pacquet/AGENTS.md, recommending#[cfg]/#[ignore]for truly platform-locked tooling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pacquet/crates/git-fetcher/src/fetcher/tests.rs | Removes silent skips so tool absence fails tests loudly. |
| pacquet/AGENTS.md | Codifies a repo guideline forbidding runtime probe-and-skip test patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+230
to
+236
| are forbidden. If the test needs a tool, just call into it and let the | ||
| existing `.unwrap()` / `.expect(...)` panic when the tool is absent — a | ||
| failing test in an under-provisioned environment is the correct signal. | ||
| Tolerance defeats the purpose of testing: if the environment really | ||
| doesn't have the required tools, that's the *environment's* fault and it | ||
| needs to be fixed. | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed in c2b73e8. The entry for fetcher_runs_prepare_script_when_allowed now describes the loud-failure approach and points readers at the new pacquet/AGENTS.md rule instead of the deleted helper.
Written by an agent (Claude Code, claude-opus-4-7).
Generated by Claude Code
The TEST_PORTING.md entry for fetcher_runs_prepare_script_when_allowed still advertised the deleted `skip_if_no_npm` helper as the recommended pattern. Reword it to match the new AGENTS.md rule: rely on the existing `.unwrap()` calls and let missing tools fail the test loudly. Spotted by Copilot review on #11713. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
approved these changes
May 18, 2026
KSXGitHub
pushed a commit
that referenced
this pull request
May 18, 2026
origin/main advanced with two PRs: - #11712 (feat: report lockfile verification progress) — TS only. Adds a new `lockfile_verification` logger channel and renders its progress in `cli/default-reporter`. No pacquet code touched. - #11713 (test(pacquet): do not tolerate the lacking of tools) — hardens `git-fetcher/src/fetcher/tests.rs` so that missing host tools fail the test instead of being silently skipped, plus a paragraph in `pacquet/AGENTS.md` and a checkbox in `pacquet/plans/TEST_PORTING.md`. Both auto-merged cleanly with this branch — only one touched a file this branch also changed (`fetcher/tests.rs`, where rc.14 renamed two closure params), and git's merge driver resolved it without manual intervention. `cargo check --workspace --tests --locked`, `cargo dylint --all -- --all-targets --workspace`, and `cargo fmt --check` all clean post-merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drop the
skip_if_no_git/skip_if_no_npmruntime probes inpacquet/crates/git-fetcher/src/fetcher/tests.rs. Each affected test now relies on the existing.unwrap()/.expect(...)calls — ifgit,node, ornpmis missing, the test fails loudly instead of silently returning.Silently skipping defeats the purpose of testing. Git is ubiquitous on dev machines and Node.js is a documented prerequisite for building pnpm, so there is no realistic environment where the suite should run and those tools should be absent. If they are, that's the environment's problem.
Also codify the rule in
pacquet/AGENTS.mdso future agent-authored ports don't reintroduce the pattern. Platform-locked tools are still allowed to gate via#[cfg(...)]or#[cfg_attr(..., ignore = "...")], which surface in the test report rather than disappearing into areturn.Test plan
cargo check -p pacquet-git-fetcher --tests --lockedcargo clippy -p pacquet-git-fetcher --tests --locked -- --deny warningscargo test -p pacquet-git-fetcher— 74/74 passWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit