fix(noDuplicateTestHooks): check for options argument#8934
Merged
ematipico merged 14 commits intobiomejs:mainfrom Feb 26, 2026
Merged
fix(noDuplicateTestHooks): check for options argument#8934ematipico merged 14 commits intobiomejs:mainfrom
ematipico merged 14 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: ecc0c6c 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 |
github-actions
bot
added
A-Parser
tim-we
force-pushed
the
fix-duplicate-test-hooks-rule
branch
from
eef298d to
888fa41
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
tim-we
force-pushed
the
fix-duplicate-test-hooks-rule
branch
from
7a6b2f8 to
2a7ca69
Compare
Member
|
This code is also used by our formatter, so make sure to add a test for it, and track it in the changelog |
tim-we
force-pushed
the
fix-duplicate-test-hooks-rule
branch
2 times, most recently
from
38d85c2 to
d8b9652
Compare
Contributor
Author
|
@ematipico thanks for the reminder. I added a tests and added this to the changelog. Sorry for taking so long, I did not find much time for this. The PR is ready for review now. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates test-call detection in the syntax crate to recognise TestOptions-style 3-argument patterns (e.g., Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2372-2378: Good positive test cases for the new pattern. Consider adding a couple of negative cases to guard against false positives — e.g.test('foo', { retry: 3 })(options without callback) andtest('foo', { retry: 3 }, 42)(options with non-function third arg) should both returnOk(false).Suggested negative test cases
let call_expression = extract_call_expression("describe.only('bar', { timeout: 42*1000 }, () => {})"); assert_eq!(call_expression.is_test_call_expression(), Ok(true)); + + // Negative cases for TestOptions pattern + let call_expression = extract_call_expression("test('foo', { retry: 3 })"); + assert_eq!(call_expression.is_test_call_expression(), Ok(false)); + + let call_expression = extract_call_expression("test('foo', { retry: 3 }, 42)"); + assert_eq!(call_expression.is_test_call_expression(), Ok(false));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_syntax/src/expr_ext.rs` around lines 2372 - 2378, Add negative unit tests to ensure is_test_call_expression doesn't false-positive: call extract_call_expression with "test('foo', { retry: 3 })" and with "test('foo', { retry: 3 }, 42)" and assert that call_expression.is_test_call_expression() returns Ok(false) for both; place these alongside the existing positive tests near the extract_call_expression usages so the tests cover options-without-callback and options-with-non-function-third-arg cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Around line 2372-2378: Add negative unit tests to ensure
is_test_call_expression doesn't false-positive: call extract_call_expression
with "test('foo', { retry: 3 })" and with "test('foo', { retry: 3 }, 42)" and
assert that call_expression.is_test_call_expression() returns Ok(false) for
both; place these alongside the existing positive tests near the
extract_call_expression usages so the tests cover options-without-callback and
options-with-non-function-third-arg cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2025-2025:⚠️ Potential issue | 🟡 MinorPre-existing doc-link typo — easy fix while you're here.
The link
[function expression]: crate::JsCallArgumentListresolves toJsCallArgumentListrather thanJsFunctionExpression.📝 One-line fix
-/// [function expression]: crate::JsCallArgumentList +/// [function expression]: crate::JsFunctionExpression🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_syntax/src/expr_ext.rs` at line 2025, The doc-link for the item incorrectly points to JsCallArgumentList; update the markdown reference label so `[function expression]` resolves to `crate::JsFunctionExpression` instead of `crate::JsCallArgumentList` in expr_ext.rs (replace the target symbol `JsCallArgumentList` with `JsFunctionExpression` for the `[function expression]: ...` link).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Around line 2372-2387: Add two edge-case tests to exercise the TestOptions
branch: one where the callback is an expression-body arrow function (e.g.,
test('x', {retry:1}, () => 1)) and assert is_test_call_expression() returns
Ok(false) to cover the is_function_with_block_body arrow-body path, and another
where the callback is a regular function expression (e.g., test('x', {retry:1},
function() {})) and assert is_test_call_expression() returns Ok(true) to hit the
JsFunctionExpression branch used by is_function_with_block_body; place these
near the existing TestOptions positive/negative cases so they exercise
is_test_call_expression and its is_function_with_block_body logic.
- Around line 2085-2095: The change restricts the 3-arg pattern by using
is_function_with_block_body(&second) which now rejects arrow-expression bodies
and multi-parameter functions (e.g., test("x", x => x, 1000)) causing a
behavioral break; add a brief inline comment next to the
is_function_with_block_body check explaining that the 3-argument form
intentionally requires a block-bodied single-param function (and that the 2-arg
form remains permissive to match Prettier), and add a negative unit test that
asserts the pattern returns false for cases like an expression-bodied arrow or
multi-param function passed as the second argument with a numeric third to lock
the new semantics (reference the is_function_with_block_body check and the
matching branch that returns Ok(matches!(...)) to locate where to add the
comment and where to add the test).
---
Outside diff comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Line 2025: The doc-link for the item incorrectly points to JsCallArgumentList;
update the markdown reference label so `[function expression]` resolves to
`crate::JsFunctionExpression` instead of `crate::JsCallArgumentList` in
expr_ext.rs (replace the target symbol `JsCallArgumentList` with
`JsFunctionExpression` for the `[function expression]: ...` link).
tim-we
force-pushed
the
fix-duplicate-test-hooks-rule
branch
from
6dd7f2d to
a20bc5c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Line 2373: Typo in the test comment: change the misspelled "TesOptions" to
"TestOptions" in the comment that precedes the positive cases for the
TestOptions pattern (search for the comment text "Positive cases for TesOptions
pattern" near the tests for TestOptions). Update only the comment text to the
correct spelling to avoid confusion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_formatter/tests/specs/js/module/declarations/test_declaration.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/grumpy-zebras-sort.mdcrates/biome_js_formatter/tests/specs/js/module/declarations/test_declaration.jscrates/biome_js_syntax/src/expr_ext.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/grumpy-zebras-sort.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2373-2373:⚠️ Potential issue | 🟡 Minor
TesOptions→TestOptions(typo still present).✏️ One-character fix
- // Positive cases for TesOptions pattern + // Positive cases for TestOptions pattern🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_syntax/src/expr_ext.rs` at line 2373, Fix the one-character typo in the comment that reads "TesOptions" so it correctly reads "TestOptions" (the comment near the "Positive cases for TestOptions pattern" section in expr_ext.rs). Locate the comment containing the misspelled identifier "TesOptions" and update the text to "TestOptions" to match the actual type/name used elsewhere.
🧹 Nitpick comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2086-2095:Nonearm in the innermatches!is unreachable.After the
len == 2early-return on line 2082, the code that follows only runs whenlen == 3, sothirdis alwaysSome(…)here. TheNonebranch is dead.✂️ Optional cleanup
- return Ok(matches!( - third, - None | Some(Ok(AnyJsCallArgument::AnyJsExpression( - AnyJsLiteralExpression( - self::AnyJsLiteralExpression::JsNumberLiteralExpression(_) - ) - ))) - )); + return Ok(matches!( + third, + Some(Ok(AnyJsCallArgument::AnyJsExpression( + AnyJsLiteralExpression( + self::AnyJsLiteralExpression::JsNumberLiteralExpression(_) + ) + ))) + ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_syntax/src/expr_ext.rs` around lines 2086 - 2095, The inner matches! includes a redundant None arm because the surrounding logic (after the len == 2 early return) guarantees len == 3, so third is always Some(...); update the check in the block guarded by is_function_with_block_body(&second)? to only match the Some(Ok(...)) case (e.g., replace the matches! that allows None with a direct Some(Ok(...)) pattern or an if let Some(Ok(...)) check) so the unreachable None branch is removed; touch the matches! invocation that references third and the AnyJsCallArgument/AnyJsLiteralExpression/JsNumberLiteralExpression pattern to perform this simplification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Line 2373: Fix the one-character typo in the comment that reads "TesOptions"
so it correctly reads "TestOptions" (the comment near the "Positive cases for
TestOptions pattern" section in expr_ext.rs). Locate the comment containing the
misspelled identifier "TesOptions" and update the text to "TestOptions" to match
the actual type/name used elsewhere.
---
Nitpick comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Around line 2086-2095: The inner matches! includes a redundant None arm
because the surrounding logic (after the len == 2 early return) guarantees len
== 3, so third is always Some(...); update the check in the block guarded by
is_function_with_block_body(&second)? to only match the Some(Ok(...)) case
(e.g., replace the matches! that allows None with a direct Some(Ok(...)) pattern
or an if let Some(Ok(...)) check) so the unreachable None branch is removed;
touch the matches! invocation that references third and the
AnyJsCallArgument/AnyJsLiteralExpression/JsNumberLiteralExpression pattern to
perform this simplification.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2373-2373:⚠️ Potential issue | 🟡 MinorTypo in test comment —
TesOptions→TestOptions.✏️ Proposed fix
- // Positive cases for TesOptions pattern + // Positive cases for TestOptions pattern🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_syntax/src/expr_ext.rs` at line 2373, Update the inline test comment that currently reads "TesOptions" to the correct "TestOptions" so the comment correctly describes the positive cases for the TestOptions pattern; locate the comment containing "Positive cases for TesOptions pattern" (near the TestOptions test cases in expr_ext.rs) and correct the typo.
🧹 Nitpick comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2372-2394: Consider adding a test for the single-binding unparenthesised arrow callback form.The existing positive cases only exercise
() => {}(zero params). Adding a case for a single unparenthesised param (x => {}) would directly exercise theAnyJsBindingpath inis_function_with_block_bodyand confirm thelen()concern raised above.📝 Suggested additional assertion
let call_expression = extract_call_expression("it('bar', { timeout: 5000 }, function() {})"); assert_eq!(call_expression.is_test_call_expression(), Ok(true)); + +// Single-binding (unparenthesised) arrow with block body +let call_expression = + extract_call_expression("test('foo', { retry: 1 }, x => {})"); +assert_eq!(call_expression.is_test_call_expression(), Ok(true));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_syntax/src/expr_ext.rs` around lines 2372 - 2394, Add a positive test that covers the single-binding unparenthesised arrow callback to exercise the AnyJsBinding path in is_function_with_block_body: create a call_expression using extract_call_expression with a third argument like an unparenthesised arrow (e.g. x => { }) passed to test(...) and assert that call_expression.is_test_call_expression() returns Ok(true); place this assertion alongside the other positive TestOptions pattern cases to validate the len() handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Line 2373: Update the inline test comment that currently reads "TesOptions" to
the correct "TestOptions" so the comment correctly describes the positive cases
for the TestOptions pattern; locate the comment containing "Positive cases for
TesOptions pattern" (near the TestOptions test cases in expr_ext.rs) and correct
the typo.
---
Nitpick comments:
In `@crates/biome_js_syntax/src/expr_ext.rs`:
- Around line 2372-2394: Add a positive test that covers the single-binding
unparenthesised arrow callback to exercise the AnyJsBinding path in
is_function_with_block_body: create a call_expression using
extract_call_expression with a third argument like an unparenthesised arrow
(e.g. x => { }) passed to test(...) and assert that
call_expression.is_test_call_expression() returns Ok(true); place this assertion
alongside the other positive TestOptions pattern cases to validate the len()
handling.
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Right now biome does not correctly identify this pattern
where the second argument is an options object.
This is where you put options in Vitest so it should be supported by biome.
This PR fixes issue #8265.
This is still work in progress.
AI assistance notice
I used Claude Code to help me understand the codebase and help me where my Rust knowledge got a little bit rusty. It also helped me updating the comments. The solution is my own.
Test Plan
I added tests in
crates/biome_js_syntax/src/expr_ext.rs.Docs
Not a new rule or an option so no documentation changes (so far)