chore: add test case for pnpm monorepo#8162
Conversation
|
WalkthroughThis PR promotes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (4)
crates/biome_module_graph/src/module_graph/fs_proxy.rs (1)
8-12: MakingModuleGraphFsProxypublic matches its new test usageExposing this struct is consistent with the new resolver tests and doesn’t change behaviour. Since it’s now part of the public surface, a brief doc comment explaining that it adapts
ModuleGraph/ProjectLayouttoResolverFsProxywould be a nice follow‑up.crates/biome_module_graph/tests/spec_tests.rs (3)
126-142:insert_node_manifestnicely DRYs up manifest handlingCentralising the JSON read + insert logic removes a lot of noisy boilerplate from the tests. If you ever touch this again, you might consider passing
package_json_path’s string todeserialize_from_json_strfor slightly richer error messages, but it’s not essential for test code.
223-274: Reusinginsert_node_manifestin the monorepo fixture test improves claritySwapping the inline JSON parsing for calls to
insert_node_manifestmakes the setup intest_resolve_package_import_in_monorepo_fixturesmuch easier to scan. If you feel like tidying further,test_resolve_swr_typescould be migrated to the same helper for consistency, but that can wait.
2120-2181: pnpm symlink regression test is thorough, but tightly coupled to the store pathThis test walks the full pnpm
.pnpmpath, wires it intoModuleGraphFsProxy::new, and then checks both resolution and the inferred return type ofgetDocument. That’s a very direct guard against regressions in the resolver. The only mildly brittle bit is the hard-coded store directory name; as long as the committed fixture stays in sync, that’s fine, but if the pnpm lock gets regenerated in future expect to update this string too.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crates/biome_module_graph/tests/fixtures/pnpm-mono/node_modules/.pnpm/@typed-firestore+server@2.0.0_firebase-admin@13.4.0_encoding@0.1.13__firebase-functions_8d5bf6c1ffe6eb686c52361f663d4298/node_modules/@typed-firestore/server/dist/documents/get-document.d.tsis excluded by!**/dist/**,!**/node_modules/**and included by**crates/biome_module_graph/tests/fixtures/pnpm-mono/node_modules/.pnpm/@typed-firestore+server@2.0.0_firebase-admin@13.4.0_encoding@0.1.13__firebase-functions_8d5bf6c1ffe6eb686c52361f663d4298/node_modules/@typed-firestore/server/dist/documents/index.d.tsis excluded by!**/dist/**,!**/node_modules/**and included by**crates/biome_module_graph/tests/fixtures/pnpm-mono/node_modules/.pnpm/@typed-firestore+server@2.0.0_firebase-admin@13.4.0_encoding@0.1.13__firebase-functions_8d5bf6c1ffe6eb686c52361f663d4298/node_modules/@typed-firestore/server/dist/index.d.tsis excluded by!**/dist/**,!**/node_modules/**and included by**crates/biome_module_graph/tests/fixtures/pnpm-mono/node_modules/.pnpm/@typed-firestore+server@2.0.0_firebase-admin@13.4.0_encoding@0.1.13__firebase-functions_8d5bf6c1ffe6eb686c52361f663d4298/node_modules/@typed-firestore/server/dist/index.jsis excluded by!**/dist/**,!**/node_modules/**and included by**crates/biome_module_graph/tests/fixtures/pnpm-mono/node_modules/.pnpm/@typed-firestore+server@2.0.0_firebase-admin@13.4.0_encoding@0.1.13__firebase-functions_8d5bf6c1ffe6eb686c52361f663d4298/node_modules/@typed-firestore/server/package.jsonis excluded by!**/node_modules/**and included by**crates/biome_module_graph/tests/fixtures/pnpm-mono/services/api/node_modules/@typed-firestore/serveris excluded by!**/node_modules/**and included by**
📒 Files selected for processing (7)
crates/biome_module_graph/src/js_module_info.rs(1 hunks)crates/biome_module_graph/src/lib.rs(1 hunks)crates/biome_module_graph/src/module_graph.rs(1 hunks)crates/biome_module_graph/src/module_graph/fs_proxy.rs(1 hunks)crates/biome_module_graph/tests/fixtures/pnpm-mono/services/api/package.json(1 hunks)crates/biome_module_graph/tests/fixtures/pnpm-mono/services/api/src/index.ts(1 hunks)crates/biome_module_graph/tests/spec_tests.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
crates/biome_module_graph/tests/fixtures/pnpm-mono/services/api/package.json
📚 Learning: 2025-08-20T16:24:59.781Z
Learnt from: arendjr
Repo: biomejs/biome PR: 7266
File: crates/biome_js_type_info/src/type.rs:94-102
Timestamp: 2025-08-20T16:24:59.781Z
Learning: In crates/biome_js_type_info/src/type.rs, the flattened_union_variants() method returns TypeReference instances that already have the correct module IDs applied to them. These references should be used directly with resolver.resolve_reference() without applying additional module ID transformations, as variant references may originate from nested unions in different modules.
Applied to files:
crates/biome_module_graph/tests/spec_tests.rs
🧬 Code graph analysis (2)
crates/biome_module_graph/src/lib.rs (1)
crates/biome_js_analyze/src/services/module_graph.rs (1)
module_graph(15-17)
crates/biome_module_graph/tests/spec_tests.rs (5)
crates/biome_project_layout/src/project_layout.rs (1)
insert_node_manifest(91-120)crates/biome_module_graph/src/module_graph/fs_proxy.rs (1)
new(15-25)crates/biome_test_utils/src/lib.rs (1)
get_added_paths(169-190)crates/biome_module_graph/src/js_module_info/module_resolver.rs (1)
for_module(76-91)crates/biome_js_syntax/src/file_source.rs (1)
ts(179-186)
⏰ 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). (10)
- GitHub Check: autofix
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_module_graph)
🔇 Additional comments (7)
crates/biome_module_graph/src/js_module_info.rs (1)
173-181: Doc comment now correctly referencesblanket_reexportsGood catch aligning the documentation with the actual field; this keeps the exports/re‑exports story consistent with the implementation.
crates/biome_module_graph/tests/fixtures/pnpm-mono/services/api/src/index.ts (1)
1-1: Import is sufficient to surfacegetDocumentin the module graphThis import gives the tests a named binding to resolve without otherwise changing the fixture’s behaviour, which is exactly what you need here.
crates/biome_module_graph/tests/fixtures/pnpm-mono/services/api/package.json (1)
1-52: Fixture manifest is coherent with the pnpm monorepo setupThe dependency set and the pinned
@typed-firestore/serverversion line up with the paths used in the new pnpm test, so this should be a stable, realistic fixture.crates/biome_module_graph/src/lib.rs (1)
11-16: Re-exportingModuleGraphFsProxykeeps the façade consistentForwarding
ModuleGraphFsProxyfrom the crate root matches the underlying module’s visibility and simplifies consumer/test imports.crates/biome_module_graph/src/module_graph.rs (1)
27-27: Public re-export ofModuleGraphFsProxyis in line with the new APIThis ties the internal fs proxy module into the public module graph API cleanly and matches the usage from
spec_tests.rs.crates/biome_module_graph/tests/spec_tests.rs (2)
12-25: Additional imports are well-scoped to the new testsBringing in the parser, extra AST enums,
ModuleGraphFsProxy, and resolver utilities is justified by the new tests and keeps this file self-contained from a test harness point of view.
2195-2302: CORS call-expression test andget_expressionhelper are focused and robustUsing a tiny in-memory file plus
parseandresolved_type_of_expressiongives a sharp check that acors()call is inferred as a function returningvoid. Theget_expressionhelper does exactly what it says—first top-level expression statement—and the assertion on diagnostics keeps it honest. Looks like a good pattern for future expression-level resolution tests.
CodSpeed Performance ReportMerging #8162 will not alter performanceComparing Summary
Footnotes
|
|
Bloody Windows getting in the way again 😓 I'll just close this then. |
1 participant
Summary
I was looking into an issue with PNPM symlinks in the past and created a test to try to identify the issue. I don't think I ever managed to reproduce it, but I found the test and figured I might as well push it. Having an extra test can only be a good thing to avoid breaking things in the future.
Test Plan
This PR.
Docs
N/A