Skip to content

Conversation

@siketyan
Copy link
Member

Summary

Fixed the failing tests on CI. tsconfig.json has some paths based on where the project exists to resolve path aliases (#7597). TSConfigJson::initialize_paths needs to be called before inserting into the project layer. In this pull request, I changed the project_layout_for_test_file function to use TSConfigJson::read_manifest (impl of Manifest::read_manifest) to parse, deserialise and insert the tsconfig.json into the layout.

Test Plan

CI should back green.

Docs

N/A

@siketyan siketyan requested review from arendjr and ematipico October 23, 2025 14:21
@siketyan siketyan self-assigned this Oct 23, 2025
@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2025

⚠️ No Changeset found

Latest commit: 6a50cb9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge once (if) it's green 🙌

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

This change refactors the manifest reading approach in the test utilities. Instead of directly deserialising JSON for package.json and tsconfig.json files, the code now uses a manifest-based reading pattern. An OsFileSystem instance is created to provide filesystem context, and the read_manifest method is delegated to handle parsing for both PackageJson and TsConfigJson types. The public API exports are updated to include Manifest alongside the existing types, whilst error handling logic remains intact through has_errors checks on manifest results.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "test: use Manifest::read_manifest to insert tsconfig" directly describes the main change in the changeset, which replaces direct JSON deserialization with manifest-based reads by calling read_manifest on TsConfigJson. The title is specific, concise, and clearly indicates the primary modification to the test utilities. It uses the conventional "test:" prefix appropriate for test-related changes and provides enough detail for a reviewer to understand the key change without ambiguity.
Description Check ✅ Passed The description is directly related to the changeset and provides relevant context for the fix. It clearly explains the problem (tsconfig paths depend on project location and initialize_paths must be called), the solution (using TSConfigJson::read_manifest), and the affected function (project_layout_for_test_file). The description connects the change to the broader context of fixing CI failures, making it contextually meaningful rather than generic or off-topic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
crates/biome_test_utils/src/lib.rs (2)

217-242: LGTM! Consider optional refactor to avoid double-reading.

The change correctly uses read_manifest to ensure initialize_paths is called. However, the file is read twice: once at line 218 and again inside read_manifest at line 219.

For a minor optimisation, you could call read_manifest directly and only read the file again for diagnostics if errors occur, avoiding double-reading in the success case.

Apply this diff for the optional refactor:

-    let package_json_file = input_file.with_extension("package.json");
-    if let Ok(json) = std::fs::read_to_string(&package_json_file) {
-        let deserialized = PackageJson::read_manifest(&fs, &package_json_file);
+    let package_json_file = input_file.with_extension("package.json");
+    let deserialized = PackageJson::read_manifest(&fs, &package_json_file);
+    if deserialized.has_errors() || deserialized.into_deserialized().is_some() {
         if deserialized.has_errors() {
+            let json = std::fs::read_to_string(&package_json_file).unwrap_or_default();
             diagnostics.extend(
                 deserialized
                     .into_diagnostics()
                     .into_iter()
                     .map(|diagnostic| {
                         diagnostic_to_string(
                             package_json_file.file_stem().unwrap(),
                             &json,
                             diagnostic,
                         )
                     }),
             );
         } else {
             project_layout.insert_node_manifest(
                 input_file
                     .parent()
                     .map(|dir_path| dir_path.to_path_buf())
                     .unwrap_or_default(),
                 deserialized.into_deserialized().unwrap_or_default(),
             );
         }
     }

244-265: LGTM! Same optional refactor applies here.

The tsconfig.json handling correctly uses read_manifest to ensure initialize_paths is called, which was the root cause of the failing CI tests. The same optional refactor to avoid double-reading (suggested for package.json) applies here as well.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ea8cbd and 6a50cb9.

📒 Files selected for processing (1)
  • crates/biome_test_utils/src/lib.rs (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place core crates under /crates/biome_*/

Files:

  • crates/biome_test_utils/src/lib.rs
**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Format Rust files before committing (e.g., via just f which formats Rust)
Document rules, assists, and options with inline rustdoc in source

Files:

  • crates/biome_test_utils/src/lib.rs
🧠 Learnings (1)
📚 Learning: 2025-10-15T09:23:33.055Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:23:33.055Z
Learning: Applies to crates/biome_js_type_info/src/{type_info,local_inference,resolver,flattening}.rs : Avoid recursive type structures and cross-module Arcs; represent links between types using TypeReference and TypeData::Reference.

Applied to files:

  • crates/biome_test_utils/src/lib.rs
🧬 Code graph analysis (1)
crates/biome_test_utils/src/lib.rs (1)
crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
  • read_manifest (55-63)
⏰ 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). (8)
  • GitHub Check: Test (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Check Dependencies
  • GitHub Check: Lint project (depot-windows-2022)
  • GitHub Check: Documentation
  • GitHub Check: Test (depot-windows-2022-16)
  • GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Test Node.js API
  • GitHub Check: autofix
🔇 Additional comments (2)
crates/biome_test_utils/src/lib.rs (2)

19-19: LGTM!

The Manifest trait import is necessary for calling read_manifest on PackageJson and TsConfigJson.


215-215: LGTM!

Creating an OsFileSystem for the test file's parent directory provides the necessary filesystem context for read_manifest.

@siketyan
Copy link
Member Author

It's still red, but at least this resolved issues in biome_js_analyze. I'll merge this first and will investigate the next issue

@ematipico
Copy link
Member

I'll merge this first and will investigate the next issue

Probably a test we couldn't catch because of this failure. I can help with that. Feel free to merge this PR anyway

@siketyan siketyan merged commit b6d2eba into biomejs:main Oct 23, 2025
10 of 12 checks passed
Jagget pushed a commit to Jagget/biome that referenced this pull request Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants