fix(noDuplicateTestHook): detect more test function defintions#7287
fix(noDuplicateTestHook): detect more test function defintions#7287ematipico merged 15 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 7a25621 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 |
WalkthroughThe PR centralises detection of "describe" call forms for the duplicate-test-hooks linter by adding an internal helper that recognises describe and chained describe variants (e.g., describe.skip, describe.each, describe.for, fdescribe, xdescribe) and uses it to push/pop hook scopes. The syntax crate API was changed: AnyJsExpression::contains_a_test_pattern now returns bool and a new contains_a_test_each_pattern helper was added. Tests for noDuplicateTestHooks were extended and no_skipped_tests updated to the new boolean API. A patch-level changeset was added. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
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. ✨ Finishing Touches
🧪 Generate unit tests
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:
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
CodSpeed Performance ReportMerging #7287 will not alter performanceComparing Summary
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
crates/biome_js_syntax/src/expr_ext.rs (3)
932-1010: Broadened test pattern matcher looks solid; consider a tiny readability pass.The finite-state style with
CalleeNamesIteratoris sound and covers the extended sets (describe.{shuffle|sequential|…}, it/test.{failing|…}, test.describe.*). For maintainability, consider hoisting the allowed-token sets into small const arrays and usingmatches!(...) || ARR.contains(...)to reduce repetition.
1999-2040: Guard with clearer control flow (minor ergonomics).The
matches!with a?in the guard is clever but a tad terse. An explicitif letreads easier and avoids early-?surprises.- if arguments.args().len() <= 3 - && (callee.contains_a_test_pattern() - || matches!(callee, AnyJsExpression::JsCallExpression(call_expression) if call_expression.callee()?.contains_a_test_each_pattern())) => + if arguments.args().len() <= 3 + && (callee.contains_a_test_pattern() + || { + if let AnyJsExpression::JsCallExpression(inner) = callee { + inner.callee()?.contains_a_test_each_pattern() + } else { + false + } + }) =>
2208-2212: Nice test coverage; add a couple of edge cases?Great to see deep chains and
.eachcalls exercised. Two quick additions would harden confidence:
describe.for([])("name", () => {})via a call-expression callee (already covered as template-less array; good).- Computed access:
describe["each"]([])("name", () => {})and optional:describe?.each?.([])("name", () => {})(after implementing computed/optional support).Also applies to: 2217-2228, 2258-2270
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js (1)
85-106: Good: validates chained describe variants as separate scopes.This neatly covers
describe.skip.each,describe.for,describe.todo, anddescribe.todo.each. Consider adding:
describe.sequential(...)anddescribe.concurrent(...)with hooks.- A
test.describe.parallel("name", () => { beforeEach(...); })case if we choose to support Playwright-styletest.describein the rule.
📜 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 (3)
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_formatter/tests/specs/prettier/js/test-declarations/test_declarations.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/puny-turtles-sniff.md(1 hunks)crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs(4 hunks)crates/biome_js_analyze/src/lint/suspicious/no_skipped_tests.rs(1 hunks)crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js(1 hunks)crates/biome_js_syntax/src/expr_ext.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: Create changesets withjust new-changeset; store them in.changeset/with correct frontmatter (package keys and change type).
In changeset descriptions, follow content conventions: user-facing changes only; past tense for what you did; present tense for current behavior; link issues for fixes; link rules/assists; include representative code blocks; end every sentence with a period.
When adding headers in a changeset, only use #### or ##### levels.
Files:
.changeset/puny-turtles-sniff.md
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/specs/suspicious/noDuplicateTestHooks/valid.jscrates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rscrates/biome_js_analyze/src/lint/suspicious/no_skipped_tests.rscrates/biome_js_syntax/src/expr_ext.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.jscrates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rscrates/biome_js_analyze/src/lint/suspicious/no_skipped_tests.rscrates/biome_js_syntax/src/expr_ext.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rscrates/biome_js_analyze/src/lint/suspicious/no_skipped_tests.rscrates/biome_js_syntax/src/expr_ext.rs
🧠 Learnings (1)
📚 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/src/lint/suspicious/no_skipped_tests.rs
🧬 Code graph analysis (2)
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs (1)
crates/biome_js_syntax/src/expr_ext.rs (2)
callee(32-37)text(1780-1785)
crates/biome_js_analyze/src/lint/suspicious/no_skipped_tests.rs (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
callee(32-37)
🔇 Additional comments (4)
crates/biome_js_syntax/src/expr_ext.rs (1)
693-699: Good call: delegate each-pattern detection to the callee.Using
tag.contains_a_test_each_pattern()removes duplicated logic and keeps “each/for” recognition in one place. LGTM.crates/biome_js_analyze/src/lint/suspicious/no_skipped_tests.rs (1)
65-65: All clear—no more.ok()?calls lurking!Great job migrating the API; I spot no stragglers. 🎉
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs (2)
124-126: Scope entry/exit on describe-like calls is the right move.Centralising the scope gate via
is_test_describe_callsimplifies the visitor and avoids per-variant checks. Nice.Also applies to: 153-156
229-234: Dynamic hook name in the diagnostic is a usability win.“Duplicate "" hook found.” is clearer for users. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js (4)
92-107: Adddescribe.sequentialand order-swapped chaining (each(...).skip)Issue #7205 calls out Vitest’s
describe.sequential. Also worth asserting that chaining works in both orders (e.g.,describe.each(...).skip(...)) to future-proof the selector logic.Apply this minimal addition near this block:
+describe.sequential("something", () => { + beforeEach(() => {}); +}); + +describe.each([]).skip("something", () => { + beforeEach(() => {}); +});
116-119: Broaden Playwright-style coverage with base and serial groupsYou’ve got
test.describe.skip.each. Consider also asserting plaintest.describeandtest.describe.serialcreate their own hook scopes.Apply this small addition:
+test.describe("something", () => { + beforeEach(() => {}); +}); + +test.describe.serial("something", () => { + beforeEach(() => {}); +});
120-126: Round out aliases with.eachonxdescribe/fdescribeIf we treat these aliases as describes, it’s useful to show they also work with
each(...).Add:
+xdescribe.each([])("something", () => { + beforeEach(() => {}); +}); + +fdescribe.each([])("something", () => { + beforeEach(() => {}); +});
92-107: Exercise other hook kinds inside a chained describeTo demonstrate hook-type agnosticism, include at least one
afterEach/beforeAll/afterAllinside a chained describe variant.For example, expand the
.todo.eachblock:describe.todo.each([])("something", () => { beforeEach(() => {}); + afterEach(() => {}); + beforeAll(() => {}); + afterAll(() => {}); });
📜 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/suspicious/noDuplicateTestHooks/valid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/puny-turtles-sniff.md(1 hunks)crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs(4 hunks)crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/puny-turtles-sniff.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs
🧰 Additional context used
📓 Path-based instructions (3)
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/specs/suspicious/noDuplicateTestHooks/valid.js
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js
🔇 Additional comments (1)
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js (1)
85-127: Good expansion of chained describe coverageCovers
describe.skip.each,describe.for,describe.todo,describe.todo.each,test.describe.skip.each, plusxdescribe/fdescribe. This aligns well with the goal of recognising chained describe variants. Nice one.
crates/biome_js_analyze/tests/specs/suspicious/noDuplicateTestHooks/valid.js
Show resolved
Hide resolved
crates/biome_js_formatter/tests/specs/prettier/js/test-declarations/test_declarations.js.snap
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| impl DuplicateHooksVisitor { | ||
| fn is_test_describe_call(node: &JsCallExpression) -> bool { |
There was a problem hiding this comment.
Could you add a small doc comment that explains the business logic of the function?
There was a problem hiding this comment.
Tried, is that good enough?
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs
Outdated
Show resolved
Hide resolved
| /// - `test.describe.serial` | ||
| /// - `test.describe.serial.only` | ||
| /// | ||
| /// - `it[.(only|skip|todo|fails|failing|concurrent|sequential)]*` |
There was a problem hiding this comment.
To be honest, I preferred the list because it was plain and simple. We changed to some sort of regular expression, and I don't even understand what the * or ? even mean. Was the list so difficult to maintain?
There was a problem hiding this comment.
If I did the math correctly, it would be around 150 cases,
if you want I can add it, but it feels a bit much
There was a problem hiding this comment.
Then I suggest rewriting the docs in a way that is more readable, or maybe explain the syntax you proposed. I can't understand it
There was a problem hiding this comment.
I tried simplified them, let me know if you feel like they are understandable now
|
|
||
| let mut rev = texts.iter().rev().flatten(); | ||
| pub fn contains_a_test_pattern(&self) -> bool { | ||
| let members = CalleeNamesIterator::new(self.clone()).collect::<Vec<_>>(); |
There was a problem hiding this comment.
I don't understand this change. Why do we collect the iterator? I think the previous code was fine.
Plus, by collecting, we don't follow the article attached to the function anymore....
There was a problem hiding this comment.
The previous code fails if there are more than 5 segments in the call
e.g
test.skip.sequential.only.skip.sequential.only();
^
this would be "first"
There was a problem hiding this comment.
I understand, thank you. Then the documentation of the function must be updated to reflect the new logic
There was a problem hiding this comment.
I updated the docs to note that it only uses the tries data structure from that article
| } | ||
|
|
||
| /// A valid pattern: | ||
| /// - Starts with a valid test pattern |
There was a problem hiding this comment.
What's a "valid test pattern"? You might want to expand it, or link the previous function.
Also, can provide some links to the original test frameworks that use this patterns?
Also, let's provide examples. Let's follow the same high standard of the other function
There was a problem hiding this comment.
Tried, is that good enough?
| /// Based on this [article] | ||
| /// | ||
| /// [article]: https://craftinginterpreters.com/scanning-on-demand.html#tries-and-state-machines | ||
| pub fn is_test_each_pattern_callee(&self) -> bool { |
There was a problem hiding this comment.
Why did we remove the doc comment?
There was a problem hiding this comment.
Since this method no longer handles the logic, I moved it to the function that does - contains_a_test_each_pattern
let me know if I should revert that
There was a problem hiding this comment.
Then the documentation must be updated, not removed :)
There was a problem hiding this comment.
Added a small doc block linking to the function containing the logic
There was a problem hiding this comment.
Actionable comments posted: 0
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_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs (1)
135-142: Emit only the first duplicate within a scopeRight now,
counter > 1will flag the 2nd, 3rd, 4th… occurrences. The issue’s requirement is to report only the first duplicate per describe scope. Gate on== 2instead.Apply:
- if counter > 1 { + if counter == 2 { ctx.match_query(DuplicateHooks(node.clone())); }
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs (1)
162-191: Broaden describe detection to any.describesegment (keeps Playwright and future chains covered)The current split-based check handles
describe.*,fdescribe,xdescribe, andtest.describe.*(second segment). To be more future-proof (e.g. deeper chains), consider accepting any segment equal todescribeonceis_test_call_expression()has gated it as a test call. This also simplifies the logic.Apply:
impl DuplicateHooksVisitor { @@ - /// 2. The test call must be a `describe.` call - /// first section = `describe` | `fdescribe` | `xdescribe` e.g. `describe.only`, `describe.skip` - /// or - /// second section = `describe` e.g. `test.describe` + /// 2. The test call’s callee chain contains a `describe` segment + /// (handles `describe.*`, `fdescribe`/`xdescribe`, and `test.describe.*`) @@ - fn is_test_describe_call(node: &JsCallExpression) -> bool { - if let Ok(callee) = node.callee() - && node.is_test_call_expression() == Ok(true) - { - let text = callee.to_trimmed_text(); - let mut split = text.split('.'); - - let first = split.next(); - let second = split.next(); - - if matches!(first, Some("describe" | "fdescribe" | "xdescribe")) - || second == Some("describe") - { - return true; - } - } - - false - } + fn is_test_describe_call(node: &JsCallExpression) -> bool { + if node.is_test_call_expression() != Ok(true) { + return false; + } + if let Ok(callee) = node.callee() { + let chain = callee.to_trimmed_text(); + // Accept any chain that contains a `describe` segment + // (e.g. describe.only, describe.each, test.describe, test.describe.parallel, etc.) + return chain.split('.').any(|seg| { + matches!(seg, "describe" | "fdescribe" | "xdescribe") + }); + } + false + }Small nit: the doc link says
[call expression]: crate::JsCallExpression; considerbiome_js_syntax::JsCallExpressionfor clarity.
📜 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 (3)
crates/biome_cli/tests/snapshots/main_cases_linter_domains/does_enable_test_rules.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_linter_domains/does_enable_test_rules_when_recommended_rules_are_disabled_but_domain_is_enabled.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_rules_via_dependencies/enables_test_globals_via_dependencies.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/puny-turtles-sniff.md(1 hunks)crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs(4 hunks)crates/biome_js_syntax/src/expr_ext.rs(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/puny-turtles-sniff.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_syntax/src/expr_ext.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.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/src/lint/suspicious/no_duplicate_test_hooks.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
callee(32-37)
🔇 Additional comments (3)
crates/biome_js_analyze/src/lint/suspicious/no_duplicate_test_hooks.rs (3)
124-126: Centralised scope entry looks goodNice consolidation via
is_test_describe_call; this keeps push logic in one place.
153-156: Symmetric scope exitPop mirrors the entry condition, so stack discipline stays correct.
243-248: Clearer diagnosticsDynamic hook name in the primary message plus a concise note reads well and is actionable.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
936-961: Docs out of sync with implementation (prefix match vs “tries”)The comment still references “tries”/state machines, but the code now does a prefix match over collected identifiers. Also worth stating explicitly that trailing qualifiers beyond the documented patterns are allowed (that’s why very long chains pass now).
Proposed doc tweak:
/// This function checks if a call expressions has one of the following members: @@ -/// Elements within parentheses `()` can be any of the listed options separated by `|`. -/// -/// Implementation first collects the tokens of callee names -/// and checks if they match any of the listed options via the tries data structure described in this [article]. -/// -/// [article]: https://craftinginterpreters.com/scanning-on-demand.html#tries-and-state-machines +/// Parenthesised groups denote alternatives. Matching is prefix-based: +/// additional chained qualifiers beyond those shown are allowed and ignored +/// (e.g. `test.skip.sequential.only.skip…` still matches `test.(skip|sequential)`). +/// +/// Implementation collects the callee identifiers and performs a direct prefix +/// match against the sequences above (no trie/state-machine involved).Also applies to: 963-966
♻️ Duplicate comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
1023-1066: Broaden.each/.fordetection to computed and optional chainsAt the moment this only matches JsStaticMemberExpression. Real suites also write
describe["each"],test?.each, etc. Reusing AnyJsMemberExpression + member_name() keeps the logic centralised and robust.Apply:
- pub fn contains_a_test_each_pattern(&self) -> bool { - let Self::JsStaticMemberExpression(member_expression) = self else { - return false; - }; - - if matches!(member_expression.object(), Ok(rest) if !rest.contains_a_test_pattern()) { - return false; - } - - match member_expression.member().ok() { - Some(AnyJsName::JsName(name)) => { - if let Some(token) = name - .value_token() - .ok() - .map(|token| token.token_text_trimmed()) - { - return matches!(token.text(), "each" | "for"); - } - false - } - _ => false, - } - } + pub fn contains_a_test_each_pattern(&self) -> bool { + use crate::{AnyJsMemberExpression, StaticValue}; + let Some(member) = AnyJsMemberExpression::cast_ref(self.syntax()) else { + return false; + }; + + // The object must itself be a recognised test/describe pattern + if matches!(member.object(), Ok(rest) if !rest.contains_a_test_pattern()) { + return false; + } + + // Accept `.each` / `.for` on static or computed names: obj.each, obj["each"], obj?.each + match member.member_name() { + Some(StaticValue::String(tok)) => matches!(tok.text(), "each" | "for"), + _ => false, + } + }Optional: add tests for:
- `describe["each"]```
- `test?.only?.each```
- `it["for"]```
🧹 Nitpick comments (2)
crates/biome_js_syntax/src/expr_ext.rs (2)
963-1021: Prefix-matching fix is sound; consider computed/optional member support nextCollecting and reversing the callee parts to do a prefix check neatly addresses >5 segment chains. One gap remains: CalleeNamesIterator skips computed members, so
describe["skip"]or optional-chained segments won’t be recognised by contains_a_test_pattern().Suggestion (non-blocking): extend CalleeNamesIterator to yield names for JsComputedMemberExpression when the index is a literal string, and to skip over optional chaining. Sketch:
// Extend Iterator::next() arm match current { // existing arms... JsComputedMemberExpression(member) => { // yield "each" for obj["each"], etc. if let Ok(expr) = member.member() { let expr = expr.omit_parentheses(); if let Some(StaticValue::String(tok)) = expr.as_static_value() { self.next = member.object().ok(); return Some(tok.token_text_trimmed()); } } None } // ... }That would make contains_a_test_pattern() cover bracketed/optional forms consistently with the new each/for helper (see next comment).
2029-2032: Minor readability: extract the “is test-like callee” guardThe guard mixes length and two separate callee-shapes. Consider extracting to a local
is_test_calleeboolean before the match for easier scanning and future extension (no behavioural change).let is_test_callee = callee.contains_a_test_pattern() || matches!(callee, AnyJsExpression::JsCallExpression(inner) if inner.callee()?.contains_a_test_each_pattern() ); match (args.next(), args.next(), args.next()) { (Some(Ok(argument)), None, None) if arguments.args().len() == 1 => { /* ... */ } (Some(Ok(AnyJsCallArgument::AnyJsExpression(_))), Some(Ok(second)), third) if arguments.args().len() <= 3 && is_test_callee => { /* ... */ } _ => Ok(false) }
📜 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 selected for processing (1)
crates/biome_js_syntax/src/expr_ext.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_syntax/src/expr_ext.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_syntax/src/expr_ext.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_syntax/src/expr_ext.rs
🧬 Code graph analysis (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
crates/biome_js_analyze/src/lint/correctness/no_self_assign.rs (2)
object(575-580)name(740-748)
🔇 Additional comments (3)
crates/biome_js_syntax/src/expr_ext.rs (3)
693-701: Delegation to central matcher looks goodNice tidy-up: the template helper now defers to AnyJsExpression::contains_a_test_each_pattern(), and the doc link points to the right place.
2237-2241: Tests: good coverage for simple, deep and chained.eachcasesThe assertions reflect the new boolean API and demonstrate the prefix-matching behaviour on long chains. The
describe.each([])(...)case is particularly helpful.Also applies to: 2245-2247, 2251-2257, 2261-2263, 2287-2299
936-936: No internal breakages detected; bump crate semver to major
- Ripgrep confirms no remaining
contains_a_test_pattern().ok()calls and all call sites have been updated to the new-> boolsignature, including inbiome_js_analyze(no_skipped_tests.rs:64).contains_a_test_each_patternalso consistently returnsbool.- Since
contains_a_test_patternis part of the public API ofbiome_js_syntax, publishing this change requires a semver-major version bump.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
1050-1073: Support computed and optional member access for .each/.for.At the moment this only matches JsStaticMemberExpression. Real suites also write describe["each"] and optional chains. You already have AnyJsMemberExpression::member_name(); use it here.
- pub fn contains_a_test_each_pattern(&self) -> bool { - let Self::JsStaticMemberExpression(member_expression) = self else { - return false; - }; - - if matches!(member_expression.object(), Ok(rest) if !rest.contains_a_test_pattern()) { - return false; - } - - match member_expression.member().ok() { - Some(AnyJsName::JsName(name)) => { - if let Some(token) = name - .value_token() - .ok() - .map(|token| token.token_text_trimmed()) - { - return matches!(token.text(), "each" | "for"); - } - false - } - _ => false, - } - } + pub fn contains_a_test_each_pattern(&self) -> bool { + use crate::AnyJsMemberExpression; + let Some(member) = AnyJsMemberExpression::cast_ref(self.syntax()) else { + return false; + }; + if matches!(member.object(), Ok(rest) if !rest.contains_a_test_pattern()) { + return false; + } + match member.member_name() { + Some(StaticValue::String(tok)) => matches!(tok.text(), "each" | "for"), + _ => false, + } + }Add tests for these cases:
#[test] fn matches_computed_and_optional_each() { let t = extract_template(r#"describe["each"]``"#); assert!(t.is_test_each_pattern_callee()); // Optional chaining (if parser supports it as a tag) // let t = extract_template(r#"describe?.each``"#); // assert!(t.is_test_each_pattern_callee()); }
🧹 Nitpick comments (1)
crates/biome_js_syntax/src/expr_ext.rs (1)
2243-2248: Broaden test coverage: computed/optional.eachand duplicate modifiers beyondonly.Please add cases for:
- describe["each"]
and test["for"].- Optional member access if supported by the parser.
- A repeated non-only modifier (e.g., test.concurrent.concurrent(name, fn)) to document intended behaviour.
I can open a follow-up with the extra tests if you like.
Also applies to: 2252-2260, 2299-2309, 2302-2309
📜 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 selected for processing (1)
crates/biome_js_syntax/src/expr_ext.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_syntax/src/expr_ext.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_syntax/src/expr_ext.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_syntax/src/expr_ext.rs
🧠 Learnings (1)
📚 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_syntax/src/expr_ext.rs
🔇 Additional comments (4)
crates/biome_js_syntax/src/expr_ext.rs (4)
694-701: Good delegation and clearer docs.Linking the tag check to contains_a_test_each_pattern() is tidy and the docstring is clear.
2036-2039: Nice: nested.eachcalls handled in is_test_call_expression.Accepting call-expression callees whose own callee matches the each/for pattern is the right shape for describe.each([])(...) and friends.
2268-2272: Targeted test for repeated.onlyis good; keep it focused.This will still pass if we narrow the duplicate check to only the "only" modifier as suggested.
964-974: Scope duplicate ban to onlyonlytokens
A ripgrep scan across JS/TS files only turned up.only.onlychains—no other modifiers repeat. Replace the broad duplicate check with anonly_count > 1guard and continue validating the full callee tail. Please run theno_skipped_testsandno_duplicate_test_hooksrules to verify there are no regressions.
…js#7287) Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
…js#7287) Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
2 participants
Todo
Summary
fixes #7205
This PR increases the amount of test functions being detected, fixing #7205 and improving anything else using these functions
Test Plan
updated tests
Note
please also let me know if the test_declarations.js.snap are valid