fix: support load_as_directory for pnp mode#75
Conversation
WalkthroughThis change removes the entire Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Resolver
participant PnPFixture
TestRunner->>Resolver: initialize with ResolveOptions (main_fields)
TestRunner->>Resolver: resolve("@atlaskit/pragmatic-drag-and-drop/combine", pnp_fixture_dir)
Resolver->>PnPFixture: Locate package in PnP zip cache
PnPFixture-->>Resolver: Return path to module file
Resolver-->>TestRunner: Return resolved path
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
💤 Files with no reviewable changes (8)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ 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:
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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a failing test case for a specific PnP scenario for the import-js/eslint-import-resolver-typescript#409 issue while removing an older nested-package-json test and its corresponding action step.
- Removed the outdated test "resolve_nested_dir_with_package_json" from resolve.rs
- Renamed a PnP test from "pnp1" to "pnp_basic" and added a new test "resolve_pnp_nested_package_json" in pnp.rs
- Removed the nested-package-json installation step in the GitHub action configuration
Reviewed Changes
Copilot reviewed 9 out of 16 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/tests/resolve.rs | Removed obsolete nested-package-json test |
| src/tests/pnp.rs | Renamed test for clarity and added a new failing PnP test |
| .github/actions/pnpm/action.yml | Removed installation step for nested-package-json fixtures |
Files not reviewed (7)
- fixtures/misc/nested-package-json/.gitignore: Language not supported
- fixtures/misc/nested-package-json/.yarnrc.yml: Language not supported
- fixtures/misc/nested-package-json/package.json: Language not supported
- fixtures/misc/nested-package-json/package/nested/package.json: Language not supported
- fixtures/misc/nested-package-json/package/package.json: Language not supported
- fixtures/pnp/package.json: Language not supported
- justfile: Language not supported
Comments suppressed due to low confidence (2)
src/tests/resolve.rs:120
- The removal of the 'resolve_nested_dir_with_package_json' test may reduce coverage for nested package JSON resolution. Please ensure that this behavior is sufficiently tested elsewhere.
-#[test]
-fn resolve_nested_dir_with_package_json() {
.github/actions/pnpm/action.yml:34
- The removal of the GitHub Action step for 'nested-package-json' might affect validation of installation steps for nested package scenarios. Confirm that any required steps for nested package JSON handling are covered by other tests.
- - name: nested-package-json
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to aa885d3 in 1 minute and 57 seconds. Click for details.
- Reviewed
172lines of code in14files - Skipped
2files when reviewing. - Skipped posting
10draft 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. .github/actions/pnpm/action.yml:11
- Draft comment:
Removal of the 'nested-package-json' step is consistent with the deletion of its fixtures. Ensure no CI dependencies remain on this fixture. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. justfile:16
- Draft comment:
The command 'cd fixtures/misc/nested-package-json && yarn' has been removed. Verify that removal of nested-package-json is intentional and not used elsewhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/tests/pnp.rs:117
- Draft comment:
New test 'resolve_pnp_nested_package_json' added. Confirm that the expected full path for '@atlaskit/pragmatic-drag-and-drop/combine' is correct for the current cache naming scheme. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/tests/resolve.rs:118
- Draft comment:
Removed test 'resolve_nested_dir_with_package_json'. Ensure that its covered functionality is either deprecated or migrated to appropriate tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. .github/actions/pnpm/action.yml:31
- Draft comment:
Removed the 'nested-package-json' step. Confirm that any related fixture or test references are no longer needed. - 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.
6. fixtures/misc/nested-package-json/package.json:1
- Draft comment:
Removed entire nested-package-json fixture. Verify that its removal does not affect any still-active tests or documentation. - 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.
7. justfile:16
- Draft comment:
Removed the nested-package-json yarn install command. Ensure that cleaning up this outdated reference is intentional. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment is asking the PR author to ensure that the removal of a command is intentional. This falls under the rule of not asking the author to confirm their intention. However, it does point out a specific change, which is the removal of a command, and could be useful if rephrased to confirm the intention of the removal without asking the author to ensure it.
8. src/tests/pnp.rs:116
- Draft comment:
New test 'resolve_pnp_nested_package_json' added. Note that the expected path directly includes a specific yarn cache hash. Consider whether a more resilient check might be preferable if the hash changes. - 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% The comment suggests making the test more resilient to hash changes, but: 1) All other tests in the file use the same pattern with specific hashes 2) This appears to be intentional test design 3) If hash changes were a problem, it would affect all tests equally 4) The comment doesn't provide a concrete alternative 5) This might be testing exact path resolution which requires precise matching Maybe there's a real issue with yarn cache hashes changing frequently in practice, making tests brittle. Maybe there's a better pattern I'm not aware of. The consistent use of exact hashes across all tests suggests this is intentional design. If hash changes were a practical problem, it would have been addressed globally, not just for this one test. Delete the comment. It questions an established pattern used throughout the test suite without providing actionable alternatives.
9. src/tests/resolve.rs:118
- Draft comment:
Removed the 'resolve_nested_dir_with_package_json' test. Confirm that its functionality is now adequately covered by other tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the functionality of a removed test is covered by other tests. This falls under the rule of not asking the author to confirm or ensure things. Therefore, this comment should be removed.
10. src/tests/pnp.rs:91
- Draft comment:
Typographical issue: Consider renaming the test function fromresolve_pnp_pkg_should_failed_while_disable_pnp_modeto something likeresolve_pnp_pkg_should_fail_when_pnp_mode_is_disabledfor clarity and grammatical correctness. - 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_enVrRVBCSm6SQzVe
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
fixtures/misc/nested-package-json/yarn.lockis excluded by!**/yarn.lock,!**/*.lockfixtures/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/actions/pnpm/action.yml(0 hunks)fixtures/misc/nested-package-json/.gitignore(0 hunks)fixtures/misc/nested-package-json/.yarnrc.yml(0 hunks)fixtures/misc/nested-package-json/package.json(0 hunks)fixtures/misc/nested-package-json/package/nested/package.json(0 hunks)fixtures/misc/nested-package-json/package/package.json(0 hunks)fixtures/pnp/package.json(1 hunks)justfile(0 hunks)src/tests/pnp.rs(2 hunks)src/tests/resolve.rs(0 hunks)
💤 Files with no reviewable changes (8)
- fixtures/misc/nested-package-json/package.json
- fixtures/misc/nested-package-json/package/package.json
- .github/actions/pnpm/action.yml
- justfile
- src/tests/resolve.rs
- fixtures/misc/nested-package-json/.yarnrc.yml
- fixtures/misc/nested-package-json/.gitignore
- fixtures/misc/nested-package-json/package/nested/package.json
🧰 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.
src/tests/pnp.rs (1)
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.
🪛 GitHub Actions: CI
src/tests/pnp.rs
[error] 127-127: Test failure in 'tests::pnp::resolve_pnp_nested_package_json': assertion left == right failed. Expected Ok path but got Err NotFound for '@atlaskit/pragmatic-drag-and-drop/combine'.
🪛 GitHub Actions: Code Coverage
src/tests/pnp.rs
[error] 127-127: Test failure in 'tests::pnp::resolve_pnp_nested_package_json': assertion left == right failed. Expected Ok path but got Err(NotFound) for '@atlaskit/pragmatic-drag-and-drop/combine'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
src/tests/pnp.rs (1)
10-10: Function rename improves clarity.Renaming from
pnp1topnp_basicmakes the test purpose clearer and follows a more descriptive naming convention.
aa885d3 to
1093e80
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
+ Coverage 93.22% 93.24% +0.01%
==========================================
Files 13 13
Lines 2850 2858 +8
==========================================
+ Hits 2657 2665 +8
Misses 193 193 ☔ 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 1093e80 in 1 minute and 9 seconds. Click for details.
- Reviewed
41lines of code in1files - 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/lib.rs:892
- Draft comment:
Avoid using unwrap() on the result of strip_prefix(pkg_name). Consider handling the None case explicitly. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/lib.rs:899
- Draft comment:
Document the rationale behind constructing the inner_request and its impact on directory resolution. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. src/lib.rs:892
- Draft comment:
Avoid using unwrap() on specifier.strip_prefix(pkg_name) as it may panic if pkg_name is not a prefix. Consider handling this case gracefully. - Reason this comment was not posted:
Comment was on unchanged code.
4. src/lib.rs:885
- Draft comment:
The logic computing 'inner_request' is fairly complex. Consider refactoring this block into a helper function or adding more inline comments for improved clarity and 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:876
- Draft comment:
Typo: The comment "symbol linked package doesn't have node_modules structure" should be corrected to "symlinked package doesn't have node_modules structure" for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_vmW9OZON23OrvzXX
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
CodSpeed Performance ReportMerging #75 will not alter performanceComparing Summary
|
close import-js/eslint-import-resolver-typescript#409
Important
Add test for
@atlaskit/pragmatic-drag-and-dropresolution and remove nested package fixtures.pnp1()topnp_basic()inpnp.rsfor clarity.resolve_pnp_nested_package_json()test inpnp.rsfor@atlaskit/pragmatic-drag-and-dropresolution.resolve_nested_dir_with_package_json()test inresolve.rs.nested-package-jsonfixture files and related setup in.github/actions/pnpm/action.ymlandjustfile.@atlaskit/pragmatic-drag-and-droptodevDependenciesinpnp/package.json.ResolverGeneric::load_pnp()inlib.rsto handle directory resolution withpackage.jsonredirection.This description was created by
for 1093e80. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
pnpfixture.