fix(biome-js-analyze): add tests for useReadonlyClassProperties#7315
Conversation
🦋 Changeset detectedLatest commit: ecfab31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds two new test classes to valid.ts under the useReadonlyClassProperties rule tests: one validating a nested UpdateExpression case (obj.prop = this.thing++;) and another validating an assignment inside a nested callback. Also adds a patch changeset documenting the fix for nested assignments related to issue #7310. No public APIs or exports are modified. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(No out-of-scope functional changes identified.) Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
github-actions
bot
added
A-Linter
![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 (5)
crates/biome_js_analyze/tests/quick_test.rs (2)
28-29: Quick test is enabled but has no assertion — either assert or re-ignore.As written, the test always passes and only prints diagnostics. Either make it meaningful by asserting no diagnostics, or keep it opt-in via #[ignore] to avoid CI noise.
Apply one of the following:
Option A — make it assertive:
-// #[ignore] +// #[ignore] @@ - // assert_eq!(error_ranges.as_slice(), &[]); + assert_eq!(error_ranges.as_slice(), &[]);Option B — keep it opt-in:
-// #[ignore] +#[ignore]
32-43: Optionally broaden coverage to private fields and pre-increment.If you want this ad-hoc to mirror the spec coverage, consider adding a variant with a class field and pre-increment:
class Test { - private thing: any; + private thing: any; + #field: number = 0; @@ callback: () => { this.thing = x; }, }); } } + +class Test2 { + #n: number = 0; + bump(): void { + const t = { x: 0 }; + t.x = ++this.#n; + } +}crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts (3)
667-675: Add mirrored cases for private fields and pre/compound assignments.To fully cover the patterns mentioned in the issue, add:
- A mirror using a class private field (#field): temp.x = this.#thing++;
- A pre-increment variant: temp.x = ++this.thing;
- A compound-assignment-as-expression: temp.x = (this.thing += 1);
Example additions (outside this hunk):
class TestIncrementInAssignmentPrivateField { #thing: number = 0; incrementThing(): void { const temp = { x: 0 }; temp.x = this.#thing++; } } class TestPreIncrementInAssignment { private thing: number = 0; incrementThing(): void { const temp = { x: 0 }; temp.x = ++this.thing; } } class TestCompoundAssignmentInAssignment { private thing: number = 0; incrementThing(): void { const temp = { x: 0 }; temp.x = (this.thing += 1); } }
677-687: Typo: rename TesAssignmentInNestedCallback → TestAssignmentInNestedCallback.Keeps naming consistent with the rest of the file.
-class TesAssignmentInNestedCallback { +class TestAssignmentInNestedCallback {
677-687: Optional: declare ui and x to make the snippet self-contained.Not required for this rule, but avoids surprises if other checks ever run over this file.
Add near the top (outside this hunk):
declare const ui: { showText(msg: string, opts: { callback: () => void }): void }; declare const x: unknown;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/tests/quick_test.rs(2 hunks)crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/tests/quick_test.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/quick_test.rscrates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/quick_test.rscrates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/quick_test.rscrates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts
🧠 Learnings (4)
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/tests/quick_test.rs : Use `biome_js_analyze/tests/quick_test.rs` for quick, ad-hoc testing; un-ignore the test and adjust the rule filter as needed
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : For new JavaScript lint rules generated by `just new-js-lintrule`, implement the rule in the generated file under `biome_js_analyze/lib/src/lint/nursery/`
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/src/**/*.rs : Use TypeData::Unknown for unimplemented inference and TypeData::UnknownKeyword for the explicit TypeScript unknown keyword; treat them semantically the same but keep them distinct for measurement
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/**/tests/specs/**/{invalid*,valid*}.{js,jsx,ts,tsx,css,graphql,jsonc} : Place snapshot test cases under `tests/specs/<group>/<ruleName>/` using files prefixed with `invalid` and `valid`
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/tests/quick_test.rs (3)
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts (6)
test(323-325)test(333-335)test(409-411)test(419-421)test(431-433)test(443-445)crates/biome_js_analyze/src/suppressions.tests.rs (1)
quick_test(12-60)crates/biome_service/src/file_handlers/mod.rs (1)
rule_filter(1135-1135)
⏰ 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). (24)
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (3)
crates/biome_js_analyze/tests/quick_test.rs (2)
32-43: Good regression snippet for nested-callback assignment.This neatly exercises the “assignment inside an arrow callback” case for the rule. Nice and focused.
60-60: Rule filter points to the right rule.Targeting style/useReadonlyClassProperties is correct for this PR’s objective.
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/valid.ts (1)
667-675: Solid case: post-increment used as RHS of a property assignment.This directly addresses the regression in #7310 and should prevent the readonly false positive.
CodSpeed Performance ReportMerging #7315 will not alter performanceComparing Summary
|
Did we add a changeset for that? |
no, because it only adds test and no changes in code? my understanding is we should not do one in this instance. Please confirm |
|
From the description, I understood that the issue was fixed in #7015 Here you added tests to confirm the fix. This means we fixed the issue, but I think we didn't add a changeset for the fix. |
ok, yes that makes perfect sense now. thanks for the explanation. let me add a changeset. |
3 participants
Summary
Adds tests to confirm already fixed issues in pull request: #7015
Closes #7310