test(pacquet): coverage#11710
Conversation
…friends Adds ~44 unit and integration tests to close the easier targets in the coverage analysis at #339-style boundaries. Touches: - `lockfile::resolved_dependency`: cover `as_alias` / `ver_peer` for non-matching variants, alias / parse error variants, `TryFrom<Cow>`, serialize-alias / serialize-link, `From<PkgVerPeer> / From<PkgNameVerPeer>`, and a Display vs. `String` round-trip. - `lockfile::freshness`: cover the plural arms of `SpecDiff::Display` and the comma separator inside the removed/modified loops. - `lockfile::pkg_name`: cover `TryFrom<String>` and `TryFrom<Cow>` happy and empty-input paths. - `lockfile::snapshot_dep_ref`: cover `ver_peer` and `From<PkgVerPeer>`. - `lockfile::save_lockfile`: cover the `CreateDir` / `RemoveFile` / `RenameFile` error classifications by planting a regular file or directory where the writer expects a file or a writable path. - `registry::package`: cover `PartialEq`, `latest`, and `pinned_version` happy and no-match paths. - `graph-hasher::engine_name`: cover `detect_node_version` / `detect_node_major` when `node` is on PATH; skip cleanly otherwise. - `graph-hasher::object_hasher`: cover the null / bool / array arms of the bytestream serializer. - `patching::apply`: cover non-NotFound read-patch error, partial-delete non-empty result, unsupported rename/copy operation, and `Create` with an unwritable parent. - `workspace-state`: cover the `CreateDir` / `ReadFile` / `ParseJson` error variants. - `package-manifest`: cover `from_path` ENOENT, `add_dependency` on a non-object field, `safe_read_package_json_from_dir` non-NotFound IO error, and `convert_engines_runtime_to_dependencies` for unsupported shapes. - `fs::file_mode`: cover `EXEC_MASK` / `EXEC_MODE` constants, `is_executable` for all positions, and `make_file_executable` on Unix. - `executor`: cover `execute_shell` happy and non-zero-exit paths. - `cli/tests/run.rs`: new integration tests for `pacquet run`, including argument forwarding and `--if-present`. Also bumps `hex_decode` in `graph-hasher::tests` to use `is_multiple_of` so test-time clippy stays clean under Rust 1.95. https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
|
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous 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). (7)
🧰 Additional context used📓 Path-based instructions (2)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
pacquet/{**/*.rs,**/*.md}📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughAdds comprehensive test coverage across crates for CLI/script execution, shell helpers, file-mode semantics, graph hashing, lockfile behaviors, manifest edge cases, patch application errors, registry version selection, and workspace-state error classification. All edits are tests or test wiring; no public APIs changed. ChangesTest Coverage Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 #11710 +/- ##
==========================================
+ 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:
|
Micro-Benchmark ResultsLinux |
Dylint's perfectionist::macro-trailing-comma rule rejected the single- line `assert!` formatting I'd accidentally inherited from the multi- line shape — remove the trailing comma so the lint clears. https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0100859038400003,
"stddev": 0.06004223045597236,
"median": 2.01422144374,
"user": 2.8184472599999997,
"system": 2.2121591799999996,
"min": 1.88147232124,
"max": 2.10210939724,
"times": [
2.01724755424,
2.00530829824,
2.10210939724,
1.98902079924,
2.01119533324,
1.95631765224,
2.03660937124,
1.88147232124,
2.04869928424,
2.05287902724
]
},
{
"command": "pacquet@main",
"mean": 1.9663229356400003,
"stddev": 0.05485434752336871,
"median": 1.95505900274,
"user": 2.8870892599999998,
"system": 2.2106699799999996,
"min": 1.89641118224,
"max": 2.08373155224,
"times": [
1.95545242124,
1.9763053512400002,
1.89641118224,
2.00871130624,
1.90580935424,
2.08373155224,
1.95466558424,
1.93498997324,
1.94610630624,
2.00104632524
]
},
{
"command": "pnpm",
"mean": 4.20045113994,
"stddev": 0.06207072591916954,
"median": 4.19779768824,
"user": 8.052286059999998,
"system": 2.56517238,
"min": 4.12061781324,
"max": 4.27743916424,
"times": [
4.12061781324,
4.27743916424,
4.25511461124,
4.195088773239999,
4.26876372824,
4.261643009239999,
4.20050660324,
4.128285539239999,
4.13536831324,
4.16168384424
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.5077869803,
"stddev": 0.05923998550880405,
"median": 0.4879180275,
"user": 0.38898498000000004,
"system": 0.92126786,
"min": 0.47410696450000006,
"max": 0.6727930525,
"times": [
0.6727930525,
0.4986193575,
0.49535841550000004,
0.48755333050000005,
0.47410696450000006,
0.5168629885,
0.48496497450000003,
0.47746373450000007,
0.48186426050000003,
0.48828272450000004
]
},
{
"command": "pacquet@main",
"mean": 0.49512300940000004,
"stddev": 0.011313619461233429,
"median": 0.4924195545000001,
"user": 0.37465428000000006,
"system": 0.9530475599999999,
"min": 0.4810207355000001,
"max": 0.5146770245,
"times": [
0.5146770245,
0.48733939150000005,
0.5073454205,
0.48989077850000007,
0.48200619450000004,
0.4970298765,
0.49220905150000005,
0.4810207355000001,
0.49263005750000005,
0.5070815635
]
},
{
"command": "pnpm",
"mean": 2.2482425582,
"stddev": 0.1226795518322691,
"median": 2.209213748,
"user": 2.9856997799999996,
"system": 1.2813889600000001,
"min": 2.0859454025,
"max": 2.5441991135,
"times": [
2.3095257865,
2.2058105475,
2.1985624695,
2.0859454025,
2.1710465445000002,
2.2126169485,
2.5441991135,
2.3063687085,
2.2536031805000003,
2.1947468805
]
}
]
} |
The two `detect_node_*` tests previously skipped silently when `node`
was missing from `PATH`. `node` is a documented prerequisite of the
test suite (see `pacquet/CONTRIBUTING.md`'s setup section), so a
missing binary is a test-env bug to surface, not a condition to paper
over. Switch the `let Some(...) = ... else { return }` skips to
`expect` so a missing `node` fails loudly with a clear message.
https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
…ame (#11708) PR #11708 renamed the dependency-injection seam (`Api` → `Sys`, `RealApi` → `Host`, `FsReadString` → `FsReadToString`, fakes drop the `*Api` suffix) and documented the convention in `pacquet/CODE_STYLE_GUIDE.md`. My tests on this branch use real fixtures only (no DI fakes), so the rename has no source-level impact on the tests themselves; the documentation references in the PR description and the per-file commit message get updated separately. https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
Addresses six review comments on #11710: - `cli/tests/run.rs`: the three `package.json` fixtures used `format!("{{ ... }}", marker = …)` with literal-brace escapes, which is awkward and brittle when path strings contain JSON-special characters. Switched to `json!({...}).to_string()` so serde handles escaping and the shape reads as JSON, not as a brace-escaped template. - `patching/src/apply/tests.rs`: the three patch fixtures I added used `"\<newline>...\n"` raw strings. Converted them to `text_block_fnl!`, matching the convention the rest of the workspace (lockfile, package-manager, testing-utils) uses for multi-line fixture text. The `_fnl` variant keeps the trailing newline that git-diff parsers expect. Pre-existing fixtures in the same file (`IS_POSITIVE_PATCH`, etc.) were not touched — keeping the scope to the lines reviewers flagged. https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
Idiomatic Rust and produces a better failure message when the assertion trips. Caught during a self-review pass over #11710. https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
wasn't the convention to always put tests in a separate file?
There was a problem hiding this comment.
Right — fixed in 50c39ca. Moved the executor and fs::file_mode tests into their conventional src/<parent>/tests.rs locations as documented in CODE_STYLE_GUIDE.md ("Unit test file layout"). My mistake on the inline mod tests { ... } block.
Generated by Claude Code
There was a problem hiding this comment.
Pull request overview
Adds pacquet test coverage across lockfile, package manifest, patching, graph hashing, filesystem, executor, workspace-state, registry package logic, and CLI run behavior.
Changes:
- Adds unit tests for serialization/parsing helpers, error classification paths, hashing dispatch, file modes, and shell execution.
- Adds CLI integration tests for
pacquet run. - Adds
text-block-macrosas a patching dev-dependency for multiline patch fixtures.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.lock |
Records the new patching test dependency. |
pacquet/crates/cli/tests/run.rs |
Adds integration tests for pacquet run. |
pacquet/crates/executor/src/lib.rs |
Adds executor unit tests. |
pacquet/crates/fs/src/file_mode.rs |
Adds file mode unit tests. |
pacquet/crates/graph-hasher/src/engine_name.rs |
Adds Node version detection tests. |
pacquet/crates/graph-hasher/src/tests.rs |
Adds object hasher variant coverage and clippy cleanup. |
pacquet/crates/lockfile/src/freshness/tests.rs |
Adds plural diff display coverage. |
pacquet/crates/lockfile/src/pkg_name/tests.rs |
Adds conversion-path coverage. |
pacquet/crates/lockfile/src/resolved_dependency/tests.rs |
Adds importer dependency parsing/serialization coverage. |
pacquet/crates/lockfile/src/save_lockfile/tests.rs |
Adds save error classification coverage. |
pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs |
Adds snapshot dependency helper coverage. |
pacquet/crates/package-manifest/src/tests.rs |
Adds manifest error and engines-runtime edge-case coverage. |
pacquet/crates/patching/Cargo.toml |
Adds text-block-macros for test fixtures. |
pacquet/crates/patching/src/apply/tests.rs |
Adds patch application failure-path coverage. |
pacquet/crates/registry/src/package/tests.rs |
Adds package equality/latest/pinned-version coverage. |
pacquet/crates/workspace-state/src/tests.rs |
Adds workspace-state load/update error coverage. |
Comments suppressed due to low confidence (2)
pacquet/crates/cli/tests/run.rs:48
- The redirection target is an unquoted temp path inside a shell command. This becomes invalid or redirects to the wrong location when the temp directory contains spaces or shell metacharacters, making the integration test environment-dependent.
"echo-args": format!(r#"sh -c 'printf %s "$1" > {}' --"#, marker_path.display()),
pacquet/crates/graph-hasher/src/engine_name.rs:258
- This test also hard-requires a host
nodebinary even though the detection API is expected to tolerate its absence. In Rust-only environments it will fail before exercising the parser; consider using a controlled fixture or skipping whennode --versioncannot be spawned.
let major = detect_node_major().expect("`node` must be on PATH for the test suite");
let version = detect_node_version().expect("`node` must be on PATH for the test suite");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn execute_shell_propagates_nonzero_exit_as_ok() { | ||
| execute_shell("exit 1").expect("wait succeeds even on nonzero exit"); |
There was a problem hiding this comment.
Good catch. Dropped this test in 50c39ca. The "non-zero exit → Ok" behavior in execute_shell is indeed a bug relative to pnpm/npm's run semantics (which propagate the script's status as the CLI's exit code), and pinning it as a regression test would block the eventual fix. The fix itself belongs in its own PR — outside the scope of a tests-only change.
Generated by Claude Code
| /// bug, so we `expect` rather than skip. | ||
| #[test] | ||
| fn detect_node_version_strips_leading_v() { | ||
| let version = detect_node_version().expect("`node` must be on PATH for the test suite"); |
There was a problem hiding this comment.
Intentionally left as-is. A human reviewer (KSXGitHub) explicitly asked in [r3255978...] for these tests to fail loudly when node is missing rather than skip silently, on grounds that node is a documented prerequisite of the test suite (see pacquet/CONTRIBUTING.md "Setup → Prerequisites"). That choice landed as 5d0987c. Reverting to a tolerant skip would undo it.
Generated by Claude Code
There was a problem hiding this comment.
Yes. Tolerant tests are kinda pointless.
I wonder. Amongst the existing AI-generated tests, how many are mistakenly "tolerant" like the one Copilot suggested.
| let manifest = json!({ | ||
| "name": "test", | ||
| "version": "0.0.0", | ||
| "scripts": { "touch-marker": format!("touch {}", marker_path.display()) }, |
There was a problem hiding this comment.
Fixed in 50c39ca. Wrapped the temp-path interpolation in "..." in both cli/tests/run.rs shell fixtures so a path with a space (the common macOS /var/folders/... shape) doesn't split the redirect/touch argument. Stopped short of a full shell-escape because tempfile::tempdir() paths can't contain ' or " in practice — quoting handles the realistic case.
Generated by Claude Code
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::execute_shell; |
There was a problem hiding this comment.
Fixed in 50c39ca by gating mod tests itself on #[cfg(all(test, unix))] at the parent's declaration site. Since both tests in the module spawn sh and are Unix-only, the cleanest fix is to compile the whole module out on Windows rather than to scatter #[cfg(unix)] across each test.
Generated by Claude Code
- Move executor + fs::file_mode unit tests from inline modules to the project's standard `src/<parent>/tests.rs` layout. The convention is documented in `CODE_STYLE_GUIDE.md` under "Unit test file layout"; zkochan flagged the violation in the executor file. - Gate `pacquet-executor::tests` on `cfg(all(test, unix))` at the declaration site so the `use super::execute_shell` import isn't dead on Windows. Copilot flagged that `clippy --tests --deny warnings` would fail there. - Drop `execute_shell_propagates_nonzero_exit_as_ok`. Copilot flagged that it regression-pinned a behavior that *should* change: pnpm/npm `run` exits with the script's status, but pacquet's wrapper currently returns `Ok(())` regardless. A fix belongs in its own PR; until then we should not add tests that block that fix. - Quote the temp-path redirect targets in the `cli/tests/run.rs` shell fixtures so a tempdir path containing a space (`/var/folders/...` on macOS) doesn't split the `touch` / redirect argument. Copilot flagged the unquoted paths. The `fs::file_mode::make_file_executable_sets_exec_bits` test keeps `#[cfg(unix)]` at the test site (and moves its `make_file_executable` import inside) rather than gating the whole `tests` module — the other two tests (`exec_constants_pin_pnpm_layout`, `is_executable_matches_any_exec_bit`) are platform-neutral and should still run on Windows. Copilot's concern about `detect_node_version`'s `expect` (in `engine_name.rs`) is intentionally not addressed: KSXGitHub asked for the hard-fail in 5d0987c on grounds that `node` is a documented prerequisite of the test suite, and the human reviewer's call wins over the bot's. https://claude.ai/code/session_01D8WBTfQzTpsZsRknrzwNKL
… in new test fixtures origin/main advanced with four PRs touching files this branch also changes: - #11679 (fix: silence verify-deps auto-install output) — TS only, no conflict with pacquet/. - #11705 (feat: tighten minimumReleaseAge) — TS only, no conflict. - #11708 (refactor(pacquet): optional DI pattern + documentation) — renames the test-DI seam (`Api` generic → `Sys`, `RealApi` → `Host`, `FsReadString` → `FsReadToString`, fakes drop `*Api` suffix) across modules-yaml / cmd-shim / config / reporter / package-manager / cli, and adds a "Dependency injection for tests" section to `pacquet/CODE_STYLE_GUIDE.md` plus a corresponding rule 7 in `pacquet/AGENTS.md`. Most files auto-merged cleanly with this branch's single-letter renames; only `pacquet/crates/patching/Cargo.toml` needed a manual resolve where this branch added `[lints] workspace = true` and main added `text-block-macros` to dev-dependencies — combined. - #11710 (test(pacquet): coverage) — adds 44 unit + integration tests. Two of the new closures (`|v|` in `registry/package/tests.rs:84` and `|e|` in `lockfile/save_lockfile/tests.rs:337`) tripped `perfectionist::single_letter_closure_param` because they predate this branch's enablement of the rule; renamed to `|version|` / `|entry|` to keep dylint at zero warnings post-merge. Verified: `cargo check --workspace --tests --locked` clean, `cargo dylint --all -- --all-targets --workspace` clean, `cargo fmt --check` clean.
Adds 44 unit + integration tests to close the easier targets from a
cargo llvm-covpass over thepacquet/workspace, lifting line coverage on the touched files without refactoring any production code.What's covered now
lockfile::resolved_dependency(From/Display/Serialize/TryFrom<Cow>arms),lockfile::{freshness, pkg_name, snapshot_dep_ref},registry::package(PartialEq/latest/pinned_version),graph-hasher::{engine_name, object_hasher}(Node-version detection + null / bool / array dispatch),package-manifest(engines-runtime conversion edge cases),fs::file_mode,executor::execute_shell.lockfile::save_lockfile,workspace-state,patching::apply, andpackage-manifest::safe_read_package_json_from_dirget coverage for their typedCreateDir/RemoveFile/RenameFile/ReadFile/ParseJsonarms by planting a regular file or directory where the writer expects something else. No DI fakes needed — these branches are reachable through real filesystem state, which matches the "prefer real fixtures" rule the new style guide added in refactor(pacquet): optional DI pattern + documentation #11708.pacquet run <script>(happy path, extra-arg forwarding, missing-script with and without--if-present).Out of scope
The bigger gaps that do need the
Sys: <FsCapability + …>seam from the DI section ofCODE_STYLE_GUIDE.md—fs::ensure_filerename retry / exhausted temp paths,install_package_by_snapshot.rsbinary / git / directory resolution,cmd-shim::link_binsremaining error arms — are left for follow-ups. They need newFs*capabilities onHostor fetcher fakes, which is more invasive than a tests-only PR should be.Drive-by
hex_decodeingraph-hasher::teststois_multiple_ofso test-time clippy stays clean on Rust 1.95.assert!inpackage-manifest/src/tests.rsto satisfy Dylint'sperfectionist::macro-trailing-comma.mainto pick up refactor(pacquet): optional DI pattern + documentation #11708.Test plan
cargo nextest runon every touched crate after mergingmain— 321 tests pass, including all the new ones.cargo clippy --locked --testson touched crates passes with--deny warnings.cargo fmt --checkclean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Tests
Chores