fix: pnp global cache should be supported #129
Conversation
WalkthroughA new fixture named Changes
Sequence Diagram(s)sequenceDiagram
participant TestScript as test.js
participant Yarn as Yarn Berry
participant GlobalCache as Global Cache
participant Resolver as Resolver
TestScript->>Yarn: require.resolve('source-map-support')
Yarn->>GlobalCache: Look up 'source-map-support' in global cache
GlobalCache-->>Yarn: Return path to 'source-map-support'
Yarn-->>TestScript: Return resolved path
Note over Resolver,GlobalCache: In test, Resolver checks module resolution<br>from global cache for correctness
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to cc6bfaf in 43 seconds. Click for details.
- Reviewed
88lines of code in6files - Skipped
1files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. fixtures/global-pnp/.gitignore:1
- Draft comment:
Good ignore patterns: local Yarn cache is excluded while important subdirectories are kept. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. fixtures/global-pnp/.yarnrc.yml:1
- Draft comment:
Enables global cache; consider adding a brief comment for future maintainers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. fixtures/global-pnp/package.json:1
- Draft comment:
Package file looks fine with pinned yarn version and dependency defined. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. fixtures/global-pnp/test.js:1
- Draft comment:
Simple test logging the resolved path; seems appropriate for verifying resolution. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. justfile:18
- Draft comment:
Added global-pnp installation step; ensure this fixture is needed in all environments. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/tests/pnp.rs:138
- Draft comment:
New 'resolve_global_cache' test is useful. Note: using std::env::home_dir() (deprecated) and hardcoded cache paths may be brittle in different environments. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_NLXIcZsF861bKMAt
You can customize 
There was a problem hiding this comment.
Pull Request Overview
Adds support verification for resolving packages from the global Yarn PnP cache by introducing a new test and corresponding fixtures, and updating the build/install recipe.
- Introduce a
resolve_global_cachetest inpnp.rsto validate resolution from~/.yarn/berry/cache. - Extend the
justfileinstall step to set up and install the newglobal-pnpfixture. - Add fixture directory
fixtures/global-pnpwith a simple test script,package.json, Yarn config, and.gitignore.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/pnp.rs | Added resolve_global_cache test using env::home_dir |
| justfile | Added cd fixtures/global-pnp && yarn to install step |
| fixtures/global-pnp/test.js | Basic script to log resolved path of source-map-support |
| fixtures/global-pnp/package.json | Declares source-map-support dependency |
| fixtures/global-pnp/.yarnrc.yml | Enables Yarn global cache for the fixture |
| fixtures/global-pnp/.gitignore | Excludes PnP and Yarn cache artifacts |
Comments suppressed due to low confidence (1)
fixtures/global-pnp/test.js:1
- This script only logs the resolved path without an assertion, so it can’t fail on a wrong resolution; consider adding an assertion or exiting with a non-zero code when the output doesn't match expectations.
console.log(require.resolve('source-map-support'))
![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
🧹 Nitpick comments (2)
fixtures/global-pnp/test.js (1)
1-1: Enhance test script robustness and add a shebang. Currently this script only logs the resolved path and will exit zero even on failure. Consider making it an executable test by adding#!/usr/bin/env nodeat the top and wrappingrequire.resolvein a try/catch so it exits non-zero when resolution fails. For example:+#!/usr/bin/env node + try { - console.log(require.resolve('source-map-support')) + console.log(require.resolve('source-map-support')); } catch (err) { - // no explicit error handling + console.error('Module resolution failed:', err); + process.exit(1); }fixtures/global-pnp/package.json (1)
1-7: Add atestscript to automate resolution checks. Without a"test"entry, invokingyarn testwon’t executetest.js. Consider adding:"dependencies": { "source-map-support": "^0.5.21" }, + "scripts": { + "test": "node test.js" + } }This enables
cd fixtures/global-pnp && yarn testin CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
fixtures/global-pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
fixtures/global-pnp/.gitignore(1 hunks)fixtures/global-pnp/.yarnrc.yml(1 hunks)fixtures/global-pnp/package.json(1 hunks)fixtures/global-pnp/test.js(1 hunks)justfile(1 hunks)src/tests/pnp.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
justfile (3)
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Learnt from: JounQin
PR: unrs/unrs-resolver#115
File: tests/resolve_test.rs:251-263
Timestamp: 2025-05-28T02:21:41.547Z
Learning: In pnpm workspaces, `pnpm install` must be run first to initialize dependencies and create the proper node_modules/.pnpm structure before tests can pass. Tests that rely on resolving packages will fail if dependencies haven't been installed yet.
🪛 GitHub Actions: CI
src/tests/pnp.rs
[warning] 139-139: Use of deprecated function std::env::home_dir. Consider using a crate from crates.io instead.
[error] 143-143: Test failure in 'tests::pnp::resolve_global_cache': assertion left == right failed. Expected Ok path but got Err(NotFound("source-map")).
🪛 GitHub Actions: Code Coverage
src/tests/pnp.rs
[warning] 139-139: Use of deprecated function std::env::home_dir. This function's behavior may be unexpected on Windows. Consider using a crate from crates.io instead.
[error] 143-143: Test failure in tests::pnp::resolve_global_cache: assertion left == right failed. Expected Ok path but got Err(NotFound("source-map")).
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
fixtures/global-pnp/.yarnrc.yml (1)
1-1: Enable Yarn global cache. The settingenableGlobalCache: truecorrectly activates the global cache for this fixture under Yarn 4.9.1.justfile (1)
18-18: Install global-pnp fixture dependencies. The addedcd fixtures/global-pnp && yarnensures that the newglobal-pnpfixture’s dependencies are installed during setup.fixtures/global-pnp/.gitignore (1)
1-7: Yarn PnP ignore patterns are correct. Ignoring.pnp.*and most of.yarn/while whitelisting essential subdirectories aligns with Yarn Berry best practices.src/tests/pnp.rs (1)
137-154:❓ Verification inconclusive
Refactor test to use fixture-based approach instead of relying on user's global cache.
The current test implementation has several critical issues:
- Environment dependency: Accesses the user's actual global cache, making it unreliable in CI/CD environments
- Hard-coded assumptions: Assumes specific packages and versions exist in the global cache
- Test failure: Currently failing because the expected cache files don't exist
- Inconsistent pattern: Other tests use fixture directories, but this test accesses real system directories
Based on the AI summary mentioning a new
global-pnpfixture, the test should use that fixture instead:-#[test] -fn resolve_global_cache() { - let global_cache = env::home_dir().unwrap().join(".yarn/berry/cache"); - - let resolver = Resolver::default(); - - assert_eq!( - resolver - .resolve( - global_cache.join("source-map-support-npm-0.5.21-09ca99e250-9ee09942f4.zip/node_modules/source-map-support/source-map-support.js"), - "source-map" - ) - .map(|r| r.full_path()), - Ok(global_cache.join( - "source-map-npm-0.6.1-1a3621db16-ab55398007.zip/node_modules/source-map/source-map.js" - )), - ); -} +#[test] +fn resolve_global_cache() { + let fixture = super::fixture_root().join("global-pnp"); + + let resolver = Resolver::new(ResolveOptions { + extensions: vec![".js".into()], + ..ResolveOptions::default() + }); + + // Test resolution within the global-pnp fixture + // Update with actual expected paths based on the fixture setup + assert_eq!( + resolver.resolve(&fixture, "source-map-support").map(|r| r.full_path()), + Ok(fixture.join("expected/path/to/source-map-support")) + ); +}Please verify that the
global-pnpfixture contains the necessary dependencies and update the test accordingly:
🏁 Script executed:
#!/bin/bash # Check if global-pnp fixture exists and contains the expected structure fd "global-pnp" --type d cat fixtures/global-pnp/package.json 2>/dev/null || echo "global-pnp fixture not found" ls -la fixtures/global-pnp/ 2>/dev/null || echo "Directory contents not accessible"Length of output: 654
🏁 Script executed:
#!/bin/bash echo "=== fixtures/global-pnp/test.js ===" sed -n '1,200p' fixtures/global-pnp/test.js echo echo "=== fixtures/global-pnp/.yarnrc.yml ===" sed -n '1,200p' fixtures/global-pnp/.yarnrc.ymlLength of output: 344
#!/bin/bash echo "=== Searching for source-map-support entry in yarn.lock ===" grep -R "source-map-support@" -n fixtures/global-pnp/yarn.lock -A5 echo echo "=== Searching for source-map entry in yarn.lock ===" grep -R "source-map@" -n fixtures/global-pnp/yarn.lock -A5
Refactor
resolve_global_cachetest to use the committedglobal-pnpfixtureSwitch this test from reading the user’s real home‐dir cache to your checked-in
fixtures/global-pnpdirectory so it’s hermetic and CI-friendly.Key changes:
- Replace
env::home_dir()withsuper::fixture_root().join("global-pnp")- Drive the expected resolution from the fixture’s own
test.js(runnode fixtures/global-pnp/test.jsand copy its output)- Keep using your existing
ResolverAPIProposed diff:
#[test] fn resolve_global_cache() { - let global_cache = env::home_dir().unwrap().join(".yarn/berry/cache"); - - let resolver = Resolver::default(); - - assert_eq!( - resolver - .resolve( - global_cache.join("source-map-support-npm-0.5.21-09ca99e250-9ee09942f4.zip/node_modules/source-map-support/source-map-support.js"), - "source-map" - ) - .map(|r| r.full_path()), - Ok(global_cache.join( - "source-map-npm-0.6.1-1a3621db16-ab55398007.zip/node_modules/source-map/source-map.js" - )), - ); + // point at our fixture + let fixture = super::fixture_root().join("global-pnp"); + let resolver = Resolver::default(); + + // derive expected path by running: + // cd fixtures/global-pnp && node test.js + // then paste the printed path here: + let expected = fixture.join("<<output-of-node-test.js>>"); + + assert_eq!( + resolver + .resolve(&fixture.join("test.js"), "source-map-support") + .map(|r| r.full_path()), + Ok(expected), + ); }Next steps:
- Run
node fixtures/global-pnp/test.jsto get the real path- Replace
<<output-of-node-test.js>>with that valueThis will remove the external dependency and make the test reproducible.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 139-139: Use of deprecated function
std::env::home_dir. Consider using a crate from crates.io instead.
[error] 143-143: Test failure in 'tests::pnp::resolve_global_cache': assertion
left == rightfailed. Expected Ok path but got Err(NotFound("source-map")).🪛 GitHub Actions: Code Coverage
[warning] 139-139: Use of deprecated function
std::env::home_dir. This function's behavior may be unexpected on Windows. Consider using a crate from crates.io instead.
[error] 143-143: Test failure in
tests::pnp::resolve_global_cache: assertionleft == rightfailed. Expected Ok path but got Err(NotFound("source-map")).
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 3ecded6 in 1 minute and 54 seconds. Click for details.
- Reviewed
202lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_85kC4yakNFc0yI6o
You can customize
CodSpeed Performance ReportMerging #129 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/lib.rs(2 hunks)src/tests/pnp.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/pnp.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
🧬 Code Graph Analysis (1)
src/lib.rs (5)
src/fs_cache.rs (1)
path(319-321)src/tsconfig_serde.rs (2)
path(42-44)path(252-254)src/package_json_serde.rs (1)
path(47-49)src/specifier.rs (2)
path(13-15)path(71-74)src/resolution.rs (1)
path(54-56)
🪛 GitHub Actions: CI
src/lib.rs
[error] 879-879: Clippy lint 'needless_borrow': This expression creates a reference which is immediately dereferenced by the compiler. Suggested fix: change pnp::find_pnp_manifest(&root).unwrap() to pnp::find_pnp_manifest(root).unwrap().
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Benchmark
JounQin
force-pushed
the
fix/pnp_global_cache
branch
2 times, most recently
from
d4eb5d9 to
a8ab76b
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed a8ab76b in 2 minutes and 19 seconds. Click for details.
- Reviewed
215lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib.rs:912
- Draft comment:
Consider refactoring the nested control flow in load_pnp for better readability and maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_RlMa8TVx82fYt7E6
You can customize
JounQin
force-pushed
the
fix/pnp_global_cache
branch
from
a8ab76b to
0dc60e8
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 0dc60e8 in 2 minutes and 10 seconds. Click for details.
- Reviewed
232lines of code in6files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:137
- Draft comment:
In tests, using dirs::home_dir().unwrap() may lead to unclear panic messages. Consider using expect() with a clear error message for better diagnostics. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a test file, so error handling is less critical. The test will fail clearly if home_dir() returns None. The change from env to dirs is the main focus. While better error messages are nice, this seems like a minor improvement that doesn't warrant a comment. Test code often uses unwrap() as a convention. The comment does have a valid point about improving error messages. In a large codebase, clearer test failures can save debugging time. While clearer errors are good, this is a minor improvement in test code. The failure would already be clear enough with unwrap() since the line number would be shown. Delete the comment. While technically correct, suggesting expect() over unwrap() in test code is too minor of an improvement to warrant a PR comment.
Workflow ID: wflow_npIoa3zxIC2viW9H
You can customize
|
@arcanis Would you like to help here? It seems I'll try to add a failing test case for https://github.com/yarnpkg/pnp-rs. Hmmm... It turns out the test case path is wrong at But yarnpkg/pnp-rs#10 for adding a new test case is still valuable. yarnpkg/pnp-rs#10 is failing on Windows. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
=======================================
Coverage 93.40% 93.41%
=======================================
Files 13 13
Lines 2897 2899 +2
=======================================
+ Hits 2706 2708 +2
Misses 191 191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cbc33a7 in 1 minute and 4 seconds. Click for details.
- Reviewed
98lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:19
- Draft comment:
In the 'pnp_basic' test, the assertions use hard‐coded Yarn cache paths. Consider adding a comment to clarify these versioned paths and note that they might change with Yarn updates. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/tests/pnp.rs:90
- Draft comment:
The test 'resolve_pnp_pkg_should_failed_while_disable_pnp_mode' correctly verifies that resolution fails when PnP mode is disabled. Consider using pattern matching on the error variant rather than a direct equality check to make the test more resilient to minor changes in error messages. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/tests/pnp.rs:135
- Draft comment:
In 'resolve_global_cache', the global cache path is derived from the user’s home directory. For improved test isolation and portability, consider using a temporary directory or a mock global cache path rather than relying on dirs::home_dir(). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Ra2UF6PJZXQ1HoPY
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
49-49: Enable full test output with--nocaptureIncluding
-- --nocaptureoncargo test --all-featuresimproves visibility into test failures, which is especially helpful for debugging the newresolve_global_cachetest. Consider applying the same flag to thecargo test --docstep for consistency in diagnosing documentation tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/actions/pnpm/action.yml(1 hunks).github/workflows/ci.yml(1 hunks)Cargo.toml(1 hunks)benches/resolver.rs(1 hunks)examples/resolver.rs(2 hunks)src/fs_cache.rs(1 hunks)src/lib.rs(1 hunks)src/tests/pnp.rs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- Cargo.toml
- benches/resolver.rs
- examples/resolver.rs
- src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/pnp.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
.github/actions/pnpm/action.yml (1)
Learnt from: JounQin
PR: unrs/unrs-resolver#75
File: fixtures/pnp/package.json:11-14
Timestamp: 2025-04-21T08:11:41.469Z
Learning: The project uses a justfile with an `install` task that includes running yarn in the fixtures/pnp directory to install dependencies with the command: `cd fixtures/pnp && yarn`.
🪛 GitHub Check: codecov/patch
src/fs_cache.rs
[warning] 253-254: src/fs_cache.rs#L253-L254
Added lines #L253 - L254 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/fs_cache.rs (1)
253-254: LGTM! Improved path formatting in debug assertion.The change from
{:?}to{}with.display()provides cleaner, more readable path output in debug messages. This follows Rust best practices for user-friendly path formatting.The static analysis hint about test coverage is expected and acceptable since this is a debug assertion for an error condition that shouldn't occur during normal operation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 253-254: src/fs_cache.rs#L253-L254
Added lines #L253 - L254 were not covered by tests.github/actions/pnpm/action.yml (2)
35-35: Approve installation of PnP fixture dependenciesAdding
yarn installinfixtures/pnpensures the existing PnP fixture’s dependencies are correctly installed during CI. This aligns with theinstallrecipe in the justfile.
37-37: Install dependencies for the newglobal-pnpfixtureThe new
cd ../global-pnpstep supports the added global PnP fixture by navigating into its directory before runningyarn install. This correctly mirrors the directory structure and sets up the global cache test.
JounQin
force-pushed
the
fix/pnp_global_cache
branch
from
cbc33a7 to
5555a1f
Compare
JounQin
force-pushed
the
fix/pnp_global_cache
branch
from
5555a1f to
51ebcd6
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5555a1f in 1 minute and 41 seconds. Click for details.
- Reviewed
108lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:90
- Draft comment:
Consider renaming the test function 'resolve_pnp_pkg_should_failed_while_disable_pnp_mode' for clarity and grammatical correctness (e.g. 'resolve_pnp_pkg_should_fail_when_pnp_disabled'). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. src/tests/pnp.rs:137
- Draft comment:
Good use of chained .join() for constructing the global cache path for Windows compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/tests/pnp.rs:20
- Draft comment:
These tests rely on hard-coded expected paths with specific version/hash values. Ensure the fixture data remains consistent or consider abstracting expected path generation to reduce brittleness. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. benches/resolver.rs:151
- Draft comment:
The format string in theassert!macro appears to have a mistake. It currently uses "{:?} {request}" but only one argument (path.display()) is provided. If the intention was to print bothpath.display()andrequest, consider using two appropriate format specifiers (e.g. "{:?} {:?}") with both values. Otherwise, remove or correct the{request}placeholder. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The original code used {path:?} which would debug print the path, while the new code uses path.display() which returns a Display impl. The {request} in both versions refers to the request variable which is in scope. The format string actually looks correct - the first argument uses {:?} for path.display() and the second placeholder {request} automatically uses Display for request. I could be wrong about the {request} syntax - maybe explicit format specifiers are always required. The comment author seems confident there's an issue. No, I'm confident that in Rust, you can use {variable} as shorthand for {variable:?} or {:?} when the variable implements Display. The code as written will work correctly. The comment is incorrect - the format string syntax is valid Rust and will work as intended. The comment should be deleted.
5. src/tests/pnp.rs:137
- Draft comment:
Typographical error: Consider changing "can not" to "cannot" in the comment for improved clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct that "cannot" is the more formal version, this is an extremely minor stylistic issue in a code comment. Code comments don't need to follow strict formal English rules. This kind of nitpick creates noise without adding value. The existing comment is perfectly clear and understandable. Perhaps maintaining consistent style across the codebase is important for professionalism and readability? The benefit of consistent comment style is far outweighed by the cost of noise in the PR review. This level of nitpicking can discourage contribution and slow down development. Delete this comment as it's an extremely minor stylistic issue that doesn't improve code quality or clarity.
Workflow ID: wflow_e9AwsuTGlOl30hvC
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 51ebcd6 in 2 minutes and 41 seconds. Click for details.
- Reviewed
121lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:137
- Draft comment:
Using separate .join() calls for constructing the global cache path improves cross‐platform compatibility. Consider adding a brief comment on why this is necessary. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/tests/pnp.rs:145
- Draft comment:
This test verifies proper resolution within Yarn’s global cache. Ensure that the expected archive names remain stable across Yarn versions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure something, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue with the code.
3. src/lib.rs:874
- Draft comment:
Avoid using unwrap() when looking up the PnP manifest. Consider handling errors gracefully to prevent potential panics in production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/lib.rs:912
- Draft comment:
Clarify the intent behind the export_resolution check after load_package_self in the PnP path. A comment explaining why returning this value resolves the global cache redirect would aid maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/lib.rs:904
- Draft comment:
Consider adding debug logging for the intermediate PnP resolution (via pnp::resolve_to_unqualified_via_manifest) to facilitate troubleshooting of global cache scenarios. - Reason this comment was not posted:
Comment was on unchanged code.
6. src/tests/pnp.rs:137
- Draft comment:
Typo: In the comment, consider using "cannot" instead of "can not" for improved clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct that "cannot" is the more common form, this is an extremely minor stylistic issue in a code comment. The existing "can not" is perfectly understandable and changing it provides negligible value. Comments should focus on substantive issues that require code changes. Perhaps maintaining consistent style in comments helps readability and professionalism of the codebase? While consistency is good, this level of nitpicking on comment wording is excessive and distracts from more important review feedback. Delete this comment as it's an extremely minor stylistic suggestion about comment wording that provides little value.
Workflow ID: wflow_46iqU3zyAHiAjWwb
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 510aff0 in 1 minute and 47 seconds. Click for details.
- Reviewed
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_F8MKJdYtq8LXeuPe
You can customize
JounQin
force-pushed
the
fix/pnp_global_cache
branch
from
510aff0 to
c220ff6
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed c220ff6 in 1 minute and 45 seconds. Click for details.
- Reviewed
42lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:142
- Draft comment:
Possible typo: '.yar/berry/cache' likely should be '.yarn/berry/cache' for consistency with Yarn's naming. - Reason this comment was not posted:
Marked as duplicate.
2. src/tests/pnp.rs:156
- Draft comment:
Redundant use of join("") – this call does not alter the path and can be removed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_dpLUKTnVqdRb3fZ1
You can customize
JounQin
force-pushed
the
fix/pnp_global_cache
branch
from
c220ff6 to
61423df
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 61423df in 1 minute and 14 seconds. Click for details.
- Reviewed
42lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:141
- Draft comment:
Consider splitting the non-Windows path into separate join calls for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/tests/pnp.rs:156
- Draft comment:
The use of .join("") is redundant; consider removing it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct that .join("") is redundant, this seems like an extremely minor issue. The code would work exactly the same with or without it. The comment doesn't point out any actual problems or risks. Path manipulation code often includes seemingly redundant separators for clarity or consistency. The comment is technically accurate - the join is redundant. Maybe there's a good reason for the empty join, like ensuring consistent behavior across platforms? Even if technically correct, this is too minor of an issue to warrant a comment. It doesn't affect functionality and the code is clear as is. This comment should be removed as it points out an extremely minor stylistic issue that doesn't impact functionality or readability.
Workflow ID: wflow_7Tj3NTux6XXj0zWL
You can customize
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2fc70c7 in 50 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/pnp.rs:137
- Draft comment:
The #[cfg(not(windows))] attribute correctly skips the test on Windows as intended. Note that the inner #[cfg(windows)] branch (lines 142-143) becomes unreachable in this configuration. Consider removing it or adding a TODO comment to clarify future re-enablement plans. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_OidzETWccqvKRWCr
You can customize
Important
Add support for global Yarn PnP cache, improve error message formatting, and update dependencies and scripts for testing.
src/lib.rsandsrc/tests/pnp.rs.global-pnpwith configuration files for testing global cache.benches/resolver.rs,examples/resolver.rs, andsrc/fs_cache.rs.Cargo.tomlandCargo.lockto includedirsas a development dependency..github/actions/pnpm/action.ymlandjustfileto include new fixture setup.This description was created by
for 2fc70c7. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit