feat(lint): add Playwright ESLint rules#8960
Conversation
🦋 Changeset detectedLatest commit: b577e37 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-CLI
|
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:
WalkthroughThis PR adds comprehensive support for 11 Playwright ESLint rules to Biome, introducing a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @.changeset/no-playwright-element-handle.md:
- Line 5: Change the wording for the new rule description to use the imperative
form: update the sentence "Prefers locators over element handles." to "Prefer
locators over element handles." in the
.changeset/no-playwright-element-handle.md entry for the
noPlaywrightElementHandle rule.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_element_handle.rs`:
- Around line 109-111: The markup! invocation is using double braces
"{{method}}" which renders the literal "{method}" instead of interpolating the
variable; update the markup so the method variable is passed/interpolated
correctly (replace the literal "{{method}}" with the actual interpolation
{method}) inside the Emphasis node in the note—look for the markup! block that
builds the note with "Element handles like" and variables receiver and method
and change the method token to use {method} interpolation.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_eval.rs`:
- Around line 96-98: The diagnostic string in the markup! macro is using
"{{method}}" which emits the literal "{method}" instead of the method variable;
update the interpolation to use a single-brace expression (replace "{{method}}"
with {method}) inside the Emphasis element so the displayed message uses the
actual method value (refer to the markup! invocation where "Unexpected use of
"<Emphasis>{receiver}"."{{method}}"()"</Emphasis>" is constructed).
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_force_option.rs`:
- Around line 57-70: The METHODS_WITH_FORCE array is missing Playwright's
"clear" method; add "clear" into the slice alphabetically (i.e., between "check"
and "click") in the const METHODS_WITH_FORCE so the list remains sorted for the
binary search-based lookup in no_playwright_force_option.rs.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs`:
- Around line 374-414: The is_call_awaited_or_returned function fails to detect
awaits wrapped in parenthesized expressions; update its parent traversal so that
before checking for JS_AWAIT_EXPRESSION (and when comparing the arrow function
body) you unwrap any JS_PARENTHESIZED_EXPRESSION nodes by walking up through
parenthesised-expression ancestors to find the real enclosing node, and when
comparing the arrow body use the unwrapped body expression's text range (or
unwrap the call_expr's parent chain similarly) so constructs like await
(expect(...).toBeVisible()) are recognized as awaited.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_page_pause.rs`:
- Around line 33-39: Update the documentation example in
no_playwright_page_pause.rs so it no longer uses the banned page.waitForSelector
pattern; replace the waitForSelector line with a locator-based assertion such as
using page.locator('.result') combined with expect(...).toBeVisible() to match
the new noPlaywrightWaitForSelector rule and keep the "Valid" snippet consistent
with the lint rule.
In `@crates/biome_rule_options/src/no_playwright_element_handle.rs`:
- Around line 1-6: Add an inline rustdoc comment for the
NoPlaywrightElementHandleOptions struct: insert a concise /// doc comment
directly above the pub struct NoPlaywrightElementHandleOptions describing what
the options configure (e.g., options for the NoPlaywrightElementHandle rule),
any default behavior, and note that the struct is currently empty if applicable;
mention usage context (rule/assist) and feature flags only if relevant so
documentation follows the project's guideline for rules, assists, and options.
In `@crates/biome_rule_options/src/no_playwright_missing_await.rs`:
- Around line 1-6: Add an inline rustdoc comment above the
NoPlaywrightMissingAwaitOptions struct that includes a fenced rust code block
doctest showing usage and asserting the default equals an empty struct (e.g.,
create options via NoPlaywrightMissingAwaitOptions::default() and
assert_eq!(options, NoPlaywrightMissingAwaitOptions {})); ensure the docblock
follows rustdoc/doctest format (/// ```rust ... /// ```), references the type
name NoPlaywrightMissingAwaitOptions, and leaves the existing derives and serde
attributes unchanged so the doctest runs in CI.
In `@crates/biome_rule_options/src/no_playwright_page_pause.rs`:
- Around line 1-6: Add a rustdoc comment with a small doctest for the
NoPlaywrightPagePauseOptions type: above the struct NoPlaywrightPagePauseOptions
add an /// doc block that explains the options and includes a fenced Rust code
example that constructs NoPlaywrightPagePauseOptions::default() and asserts
equality with NoPlaywrightPagePauseOptions {} (using the fully qualified path
biome_rule_options::no_playwright_page_pause::NoPlaywrightPagePauseOptions or
relative path), so the doc test compiles and verifies the Default/Eq behavior;
keep the serde and derive attributes unchanged.
In `@crates/biome_rule_options/src/no_playwright_wait_for_selector.rs`:
- Around line 1-6: Add an inline rustdoc comment above the
NoPlaywrightWaitForSelectorOptions struct that includes a fenced doctest code
block showing how to construct the options and asserting equality (e.g., use
biome_rule_options::no_playwright_wait_for_selector::NoPlaywrightWaitForSelectorOptions;
let options = NoPlaywrightWaitForSelectorOptions::default(); assert_eq!(options,
NoPlaywrightWaitForSelectorOptions {})); ensure the comment uses proper rustdoc
triple-slash format and the code block is marked for doctests so it runs during
cargo test; place the doc immediately above the pub struct
NoPlaywrightWaitForSelectorOptions declaration.
In `@crates/biome_rule_options/src/no_playwright_wait_for_timeout.rs`:
- Around line 1-6: Add an inline rustdoc comment above the
NoPlaywrightWaitForTimeoutOptions struct that includes a doctest demonstrating
default construction and equality: include a code block that imports
biome_rule_options::no_playwright_wait_for_timeout::NoPlaywrightWaitForTimeoutOptions,
creates let options = NoPlaywrightWaitForTimeoutOptions::default(); and asserts
assert_eq!(options, NoPlaywrightWaitForTimeoutOptions {}); so the doc test
compiles and validates the Default/Eq impls for
NoPlaywrightWaitForTimeoutOptions.
🧹 Nitpick comments (4)
.changeset/no-playwright-useless-await.md (1)
7-9: Consider labelling the example as incorrect or adding the fix.The example shows invalid code but doesn't explicitly indicate this. Users reading the changelog might benefit from either a comment marking it as incorrect or showing the correct alternative.
📝 Suggested enhancement
Option 1: Label as incorrect
+// Incorrect: const loc = await page.locator('.btn'); + +// Correct: +const loc = page.locator('.btn');Option 2: Add a brief inline note
-const loc = await page.locator('.btn'); +// Incorrect - locator() is synchronous +const loc = await page.locator('.btn');crates/biome_js_analyze/src/lint/nursery/no_playwright_useless_await.rs (1)
140-215: Reuse the shared Playwright helper for page/frame detection.
is_page_or_frameduplicates logic already exposed inframeworks::playwright. Leaning on the shared helper keeps heuristics consistent across rules and trims maintenance.♻️ Suggested refactor
-use crate::frameworks::playwright::{LOCATOR_METHODS, is_page_or_frame_name}; +use crate::frameworks::playwright::{LOCATOR_METHODS, get_page_or_frame_name}; @@ if SYNC_PAGE_METHODS.binary_search(&method_name).is_ok() { // Verify it's called on page/frame let object = member_expr.object().ok()?; - if is_page_or_frame(&object) { + if get_page_or_frame_name(&object).is_some() { return Some(()); } } @@ -fn is_page_or_frame(expr: &AnyJsExpression) -> bool { - match expr { - AnyJsExpression::JsIdentifierExpression(id) => { - if let Ok(name) = id.name() - && let Ok(token) = name.value_token() - { - return is_page_or_frame_name(token.text_trimmed()); - } - false - } - AnyJsExpression::JsStaticMemberExpression(member) => { - if let Ok(member_name) = member.member() - && let Some(name) = member_name.as_js_name() - && let Ok(token) = name.value_token() - { - return is_page_or_frame_name(token.text_trimmed()); - } - false - } - _ => false, - } -}crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs (1)
56-61: Consider adding rustdoc to the public enum.The
InvalidReasonenum is public but lacks documentation. A brief rustdoc comment would help consumers understand each variant.📝 Suggested documentation
+/// Reasons why a describe callback is invalid. pub enum InvalidReason { + /// The callback is async, which is not allowed for describe blocks. Async, + /// The callback has parameters, which describe callbacks should not have. HasParameters, + /// No callback function was provided. MissingCallback, + /// The provided argument is not a function. NotFunction, }crates/biome_js_analyze/src/lint/nursery/no_playwright_element_handle.rs (1)
60-63: Inconsistent field visibility with sibling rule.
ElementHandleCallhaspubfields while the analogousEvalMethodCallinno_playwright_eval.rsdoes not. Consider aligning them for consistency—either both private (if only used internally) or both public.
crates/biome_js_analyze/src/lint/nursery/no_playwright_element_handle.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_playwright_force_option.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/expect_playwright_expect.rs`:
- Around line 126-144: get_test_callback currently returns the first function
argument it finds, which yields false positives for calls like test("name", {
retry: ... }, async () => ...) — update get_test_callback (which accepts
&biome_js_syntax::JsCallArguments and returns Option<AnyJsExpression>) to choose
the last argument that is a function/arrow function instead of the first:
iterate the args in reverse (or collect and scan from the end), check
arg.as_any_js_expression() and match AnyJsExpression::JsArrowFunctionExpression
| AnyJsExpression::JsFunctionExpression, and return the last matching
expr.clone(); ensure non-function args are skipped so the actual test callback
is selected.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_force_option.rs`:
- Around line 100-110: The loop over call_expr.arguments() currently only
matches AnyJsExpression::JsObjectExpression directly, missing parenthesised
object literals; update the argument handling to unwrap
JsParenthesizedExpression (recursively if needed) before checking for object
expressions so invocations like click(({ force: true })) are detected; you can
either modify the matching in the loop to handle
AnyJsExpression::JsParenthesizedExpression and extract its inner JsExpression or
add a small helper (used by the loop and/or has_force_true) that returns the
inner unparenthesized JsObjectExpression for inspection.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs`:
- Around line 285-302: The has_poll_in_chain check currently flags any .poll
chain; wrap that detection with a guard that ensures the chain actually starts
with expect by also calling has_expect_in_chain(&object) before returning
MissingAwaitType::ExpectPoll—i.e., change the logic around
has_poll_in_chain(&object) so it only returns Some(MissingAwaitType::ExpectPoll)
when both has_poll_in_chain(&object) and has_expect_in_chain(&object) are true,
keeping the existing behavior and enums (MissingAwaitType::ExpectPoll,
has_poll_in_chain, has_expect_in_chain, ExpectMatcher).
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_wait_for_selector.rs`:
- Around line 162-168: The code uses mutation.replace_node(call_expr.clone(),
wait_for_call) which discards trivia (comments/whitespace); change it to
mutation.replace_node_transfer_trivia(call_expr.clone(), wait_for_call) and
handle the Result/Option it returns (propagate or log/convert to JsRuleAction
error) so the fixer preserves comments and formatting; update the surrounding
block that constructs the JsRuleAction (where JsRuleAction::new is created) to
use the transfer-trivia call and react to any error accordingly.
🧹 Nitpick comments (3)
crates/biome_js_analyze/src/lint/nursery/no_playwright_skipped_test.rs (2)
62-75: Consider derivingDebugforSkippedType.Adding
#[derive(Debug)]would assist with debugging and usingdbg!()during development.♻️ Suggested improvement
/// The type of skipped annotation detected +#[derive(Debug)] pub enum SkippedType { Skip, Fixme, }
177-206: Minor: Double iteration over arguments.The code calls
args.args().iter().count()on line 179, thenargs.args().iter().next()on line 186, iterating twice. This is negligible for typical argument lists, but you could collect once if you fancy.♻️ Optional simplification
// Check if call has arguments (conditional skip like test.skip(condition)) if let Ok(args) = call_expr.arguments() { - let arg_count = args.args().iter().count(); + let args_list: Vec<_> = args.args().iter().collect(); + let arg_count = args_list.len(); // test.skip(condition) - one argument is a conditional skip // test.skip(condition, "reason") - two arguments is also conditional // test.skip("name", callback) - two arguments with string first is not conditional if arg_count == 1 || arg_count == 2 { // Check if first arg is NOT a string literal (would be test name) - if let Some(first_arg) = args.args().iter().next() + if let Some(first_arg) = args_list.first() && let Ok(first_arg) = first_argcrates/biome_js_analyze/src/lint/nursery/no_playwright_conditional_expect.rs (1)
106-143: Reduce duplication in expect identifier checks.
The identifier token extraction is repeated; a tiny helper keeps it DRY and easier to tweak later.♻️ Suggested refactor
-use biome_js_syntax::{AnyJsExpression, JsCallExpression, JsSyntaxKind}; +use biome_js_syntax::{AnyJsExpression, JsCallExpression, JsIdentifierExpression, JsSyntaxKind}; @@ - AnyJsExpression::JsIdentifierExpression(id) => { - if let Ok(name) = id.name() - && let Ok(token) = name.value_token() - { - return token.text_trimmed() == "expect"; - } - false - } + AnyJsExpression::JsIdentifierExpression(id) => is_expect_identifier(id), @@ - AnyJsExpression::JsIdentifierExpression(id) => { - if let Ok(name) = id.name() - && let Ok(token) = name.value_token() - { - return token.text_trimmed() == "expect"; - } - } + AnyJsExpression::JsIdentifierExpression(id) => { + if is_expect_identifier(id) { + return true; + } + } @@ } +fn is_expect_identifier(id: &JsIdentifierExpression) -> bool { + if let Ok(name) = id.name() + && let Ok(token) = name.value_token() + { + return token.text_trimmed() == "expect"; + } + false +}
crates/biome_js_analyze/src/lint/nursery/expect_playwright_expect.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_playwright_wait_for_selector.rs
Outdated
Show resolved
Hide resolved
Review Feedback Status UpdateAll CodeRabbit review feedback has been addressed: Already Fixed (6 items)
Newly Fixed (4 nitpicks)
Skipped with Justification (5 items)
All tests pass and linting/formatting checks complete successfully. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_useless_await.rs`:
- Around line 194-260: is_sync_expect_call currently only recognizes expect()
directly as the object of the matcher, so chains like await
expect(x).not.toBe(1) are missed; update is_sync_expect_call to also handle when
member_expr.object() is a JsStaticMemberExpression (e.g. the `.not` modifier)
whose object is the JsCallExpression for expect() and whose member token text is
"not" (treat `.not` as a synchronous modifier), then apply the same checks
(including calling has_async_modifier(&expect_call, call_expr)) so `.not` chains
are flagged as useless awaits; reference is_sync_expect_call,
member_expr.object(), JsStaticMemberExpression, and has_async_modifier when
making this change.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs (2)
56-66: NarrowInvalidReasonvisibility if it’s not part of the public API.
Keeping itpubwidens the crate surface for little gain;pub(crate)(or private) is plenty unless external users rely on it.🔧 Suggested tweak
-pub enum InvalidReason { +pub(crate) enum InvalidReason {
68-94: Trim an allocation inis_playwright_describe_call.
You can match directly onnames.as_slice()and drop the intermediateVec<&str>; same behaviour, fewer allocations.♻️ Suggested tweak
- // Convert to &str slice for pattern matching - let names_ref: Vec<&str> = names.iter().map(String::as_str).collect(); - - match names_ref.as_slice() { - // describe() - ["describe"] => Some(true), - // test.describe() - ["test", "describe"] => Some(true), - // test.describe.only() / test.describe.skip() - ["test", "describe", modifier] if is_describe_modifier(modifier) => Some(true), - // test.describe.parallel() / test.describe.serial() - ["test", "describe", mode] if is_describe_mode(mode) => Some(true), - // test.describe.parallel.only() / test.describe.serial.only() - ["test", "describe", mode, modifier] - if is_describe_mode(mode) && is_describe_modifier(modifier) => - { - Some(true) - } - _ => Some(false), - } + match names.as_slice() { + [name] if name == "describe" => Some(true), + [test, describe] if test == "test" && describe == "describe" => Some(true), + [test, describe, modifier] + if test == "test" && describe == "describe" && is_describe_modifier(modifier) => + { + Some(true) + } + [test, describe, mode] + if test == "test" && describe == "describe" && is_describe_mode(mode) => + { + Some(true) + } + [test, describe, mode, modifier] + if test == "test" + && describe == "describe" + && is_describe_mode(mode) + && is_describe_modifier(modifier) => + { + Some(true) + } + _ => Some(false), + }
crates/biome_js_analyze/src/lint/nursery/no_playwright_useless_await.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/expect_playwright_expect.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/expect_playwright_expect.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_playwright_expect.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_playwright_skipped_test.rs
Outdated
Show resolved
Hide resolved
- Rename expectPlaywrightExpect → usePlaywrightExpect (rules must start with use/no) - Extract shared functions to frameworks/playwright.rs: - collect_member_names() with TokenText to avoid allocations - is_test_call() for detecting test/it calls - get_test_callback() returning LAST function argument - Fix noPlaywrightForceOption to handle parenthesized objects - Fix noPlaywrightMissingAwait to guard expect.poll with has_expect_in_chain - Fix noPlaywrightWaitForSelector to use replace_node_transfer_trivia - Fix noPlaywrightUselessAwait to handle .not modifier - Fix noPlaywrightConditionalExpect to avoid duplicate diagnostics - Refactor rules to use shared code from playwright.rs - Add #[derive(Debug)] to state enums Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/biome_js_analyze/src/lint/nursery/no_playwright_conditional_expect.rs`:
- Around line 131-167: is_part_of_expect_chain currently fails to detect chained
member patterns like expect(...).not.toHaveTitle because it only checks the
immediate member.object for an identifier or call; update
is_part_of_expect_chain to recursively traverse member chains by handling
AnyJsExpression::JsStaticMemberExpression as well as
AnyJsExpression::JsCallExpression when encountered as member.object(), so that
encountering a nested member (e.g., member.object() -> JsStaticMemberExpression)
will recurse into that member's object/property and ultimately detect the
identifier "expect" (refer to is_part_of_expect_chain,
AnyJsExpression::JsStaticMemberExpression, AnyJsExpression::JsCallExpression).
crates/biome_js_analyze/src/lint/nursery/no_playwright_conditional_expect.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/frameworks/playwright.rs`:
- Around line 120-141: The is_test_call function incorrectly treats member
expressions like test.describe() as tests; modify the
AnyJsExpression::JsStaticMemberExpression arm in is_test_call to first inspect
the member/property identifier (e.g., via member.property() -> JsName or token
text) and return false if that name is "describe" (and its modifiers like
"describe.skip" / "describe.only"), before recursing into member.object(); keep
existing logic for JsIdentifierExpression and recursion after this early check
so only true test identifiers ("test" or "it") are accepted.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/frameworks/playwright.rs (1)
279-307: Add a regression test fortest.describe.
A small test here would lock in the intended behaviour foris_test_calland prevent future false positives.🧪 Suggested test snippet
#[test] fn locator_methods_sorted() { assert!(LOCATOR_METHODS.is_sorted()); } + +#[test] +fn is_test_call_excludes_describe() { + let expr = biome_js_syntax::parse_js_expression("test.describe").unwrap(); + let callee = expr.syntax().first_child().unwrap(); + let callee = AnyJsExpression::cast(callee).unwrap(); + assert!(!is_test_call(&callee)); +}
Add comprehensive test coverage for all Playwright expect modifiers per PR biomejs#8960 review feedback about handling .not/.resolves chains. Test cases added: - .resolves, .rejects modifiers - expect.poll(), expect.soft() variants - Chained modifiers: .resolves.not, .rejects.not, .soft().not Both invalid (conditional) and valid (unconditional) scenarios covered with unit tests verifying is_expect_call handles all modifier patterns. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
There's a few changesets that are invalid. The only changesets that should exist should be ones that say they added a rule. The changesets that say they added a code fix or something don't make sense to have because the rules haven't been released yet.
This is kind of a big PR. I haven't reviewed the actual behavior of all the rules in depth, but at a glance it looks fine. Made a few notes that you should apply generally to all the rules.
crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/frameworks/playwright.rs`:
- Around line 156-177: Update the rustdoc above get_test_callback to use a
fenced code block for the JS example: replace the inline example `test("name", {
retry: () => 2 }, async () => { ... })` with a fenced block starting with
```javascript, the example line, and closing ``` so the comment becomes a proper
rustdoc code block; target the doc comment immediately above the
get_test_callback function.
- Delete 4 invalid changesets (fix/note changesets for unreleased rules) - Reorder diagnostic notes: WHAT → WHY → HOW across all 14 rules - Move helper structs/consts/fns below `impl Rule` blocks in all rules - Change `&TokenText` → `&str` in helper function parameters - Refactor `is_sync_expect_call` to use idiomatic let-else guards - Add missing WHY/HOW diagnostic notes to noPlaywrightMissingAwait - Remove unused `method` variable in noPlaywrightElementHandle - Format with rustfmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs`:
- Around line 406-432: The return-statement walk currently only stops at
JS_ARROW_FUNCTION_EXPRESSION, allowing inner non-arrow functions to see outer
returns and causing false negatives; modify the loop matching node.kind() so
that when encountering JS_FUNCTION_DECLARATION or JS_FUNCTION_EXPRESSION it does
NOT keep searching outward (i.e., treat them as function boundaries and break
the loop rather than continuing), while keeping the existing special-case logic
for JsArrowFunctionExpression (which may still return true if the call_expr
exactly matches the arrow body); update the match to include arms for
biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION and
::JS_FUNCTION_EXPRESSION that simply break (or otherwise stop the walk) so calls
inside those inner functions cannot match an outer return.
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_skipped_test.rs`:
- Around line 161-164: Add handling for the describe.fixme pattern alongside the
existing describe.skip branch: in the same function where you check names[0] ==
"describe" && names[1] == "skip" (in no_playwright_skipped_test.rs), add a
parallel check for names[0] == "describe" && names[1] == "fixme" and return the
appropriate SkippedType variant (e.g., SkippedType::Fixme) so describe.fixme is
treated consistently with describe.skip.
In
`@crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs`:
- Around line 211-213: The helper function is_describe_modifier currently only
recognizes "only" and "skip", so add "fixme" to its checks; update the
is_describe_modifier function (the one returning s == "only" || s == "skip") to
also return true for "fixme" so that calls like test.describe.fixme() are
treated as a describe modifier and validated accordingly.
🧹 Nitpick comments (5)
crates/biome_js_analyze/src/lint/nursery/no_playwright_wait_for_selector.rs (1)
104-170: Consider handling>2arguments defensively.The action gracefully handles 0, 1, and 2 arguments. However, if a user somehow passes three or more arguments (e.g. through a misunderstanding or future API change), args beyond the second are silently dropped by the fix. Since the fix kind is
Unsafethis is broadly acceptable, but a brief comment noting the assumption (max 2 args) would aid future maintainers.Also — minor nit —
opts.clone()on line 147 is redundant sinceoptsis already an ownedAnyJsCallArgument. You can pass it directly.♻️ Suggested tweak
- let options_arg = arg_list.get(1).and_then(|a| a.clone().ok()); + // waitForSelector accepts at most 2 args: selector and an optional options object. + let options_arg = arg_list.get(1).and_then(|a| a.clone().ok());- make::js_call_arguments(make::token(T!['(']), make_arg_list([opts.clone()]), make::token(T![')'])) + make::js_call_arguments(make::token(T!['(']), make_arg_list([opts]), make::token(T![')']))crates/biome_js_analyze/src/lint/nursery/no_playwright_useless_await.rs (1)
224-287: Consider extracting the duplicated expect-callee matching into a helper.The logic that checks whether a
JsCallExpressionisexpect(...)orexpect.soft(...)(and then callshas_async_modifier) is repeated nearly verbatim in both the direct branch (lines 225–253) and the.notbranch (lines 255–284). A small helper would reduce duplication and make future changes (e.g., supporting additional modifiers) less error-prone.♻️ Sketch of a possible extraction
+/// Returns `true` if `expect_call` is a plain `expect(…)` or `expect.soft(…)` invocation +/// (i.e. not `expect.poll(…)`) and the chain up to `final_call` has no async modifiers. +fn is_sync_expect_or_soft(expect_call: &JsCallExpression, final_call: &JsCallExpression) -> bool { + let Some(expect_callee) = expect_call.callee().ok() else { + return false; + }; + match expect_callee { + AnyJsExpression::JsIdentifierExpression(id) => { + if let Ok(name) = id.name() + && let Ok(token) = name.value_token() + && token.text_trimmed() == "expect" + { + return !has_async_modifier(expect_call, final_call); + } + } + AnyJsExpression::JsStaticMemberExpression(expect_member) => { + if let Ok(member) = expect_member.member() + && let Some(name) = member.as_js_name() + && let Ok(token) = name.value_token() + && token.text_trimmed() == "soft" + { + return !has_async_modifier(expect_call, final_call); + } + } + _ => {} + } + false +}Then both branches become one-liners calling
is_sync_expect_or_soft.crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs (1)
176-209: Baredescribe.only()/describe.skip()not matched.The
len == 2branch only matchestest.describe(). If someone writesdescribe.only('suite', () => {})ordescribe.skip(...), this rule won't fire. Whether that's intentional depends on whether you consider baredescribeimports valid Playwright usage — thelen == 1branch already matches baredescribe(), so it seems consistent to also handledescribe.only/skip/fixme.Proposed addition for len==2
2 => { - // test.describe() - Some(names[0] == "test" && names[1] == "describe") + // test.describe() or describe.only() / describe.skip() / describe.fixme() + Some( + (names[0] == "test" && names[1] == "describe") + || (names[0] == "describe" + && (is_describe_modifier(names[1].text()) + || is_describe_mode(names[1].text()))), + ) }crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs (2)
151-202: Minor duplication between the two fix paths.The Promise combinator and normal fix paths share nearly identical mutation logic. Could extract a small helper, but it's readable as-is and the messages differ, so this is just a thought for later.
240-245: Consider adding brief rustdoc toMissingAwaitTypevariants.The enum is public and the variants could benefit from a one-liner each (e.g. what
ExpectPollvsExpectMatchermeans for consumers). Low priority given the names are fairly self-documenting. As per coding guidelines: "Use inline rustdoc documentation for rules, assists, and their options."
crates/biome_js_analyze/src/lint/nursery/no_playwright_skipped_test.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_playwright_valid_describe_callback.rs
Outdated
Show resolved
Hide resolved
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/frameworks/playwright.rs`:
- Around line 165-167: The loop over arg_list.iter() uses arg.ok()? which
returns early on the first Err and discards any previously captured callback;
change the error handling so a parse error for a trailing argument does not
return None for the whole function — e.g., replace the arg.ok()? usage with
explicit handling (match or if let Ok(arg) = arg { ... } else { continue }) or
pre-filter the iterator (filter_map) so you only inspect successfully parsed
args before calling as_any_js_expression(), ensuring any previously found
callback remains valid; update the block around arg_list.iter(), the local
variable arg and the as_any_js_expression() branch accordingly.
- Around line 88-98: The static member arm pushes the member name even when
member.object() fails, producing a truncated chain; change the logic in the
AnyJsExpression::JsStaticMemberExpression arm so that if member.object() returns
Err you do not continue (e.g., return None or propagate the error) instead of
just skipping recursion, and only push the member name after a successful
object() and recursive collect_member_names_rec call; apply the same fix to the
AnyJsExpression::JsComputedMemberExpression arm so both arms consistently
require a successful object() before adding the member to names.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/frameworks/playwright.rs (2)
275-297: Redundant locator-method check — both branches recurse identically.Lines 280–290 check
is_locator_methodthen recurse onmember.object(). Lines 291–294 unconditionally do the same recurse. The locator check never changes the outcome, making it dead logic that misleads readers into thinking locator methods are treated specially.You could simplify this entire arm:
Simplified version
AnyJsExpression::JsCallExpression(call) => { if let Ok(callee) = call.callee() && let Some(member) = biome_js_syntax::JsStaticMemberExpression::cast_ref(callee.syntax()) { - // Check if it's a locator method - if let Ok(member_name) = member.member() - && let Some(name) = member_name.as_js_name() - && let Ok(token) = name.value_token() - && is_locator_method(token.text_trimmed()) - { - // Verify the chain continues to page/frame - if let Ok(object) = member.object() { - return is_playwright_call_chain(&object); - } - } - // Also recurse for any call on page/frame + // Recurse through the call chain to find a page/frame root if let Ok(object) = member.object() { return is_playwright_call_chain(&object); } } false }If you do intend to restrict matching to only locator methods in the future, add a
TODOcomment and drop the fallback.
303-383: Good unit tests for the core helpers — consider adding coverage for the remaining public functions.
get_test_callback,collect_member_names,is_playwright_call_chain,is_expect_call, andcontains_expect_callare allpub(crate)and used by multiple lint rules. Direct unit tests here would help catch regressions independently of rule-level fixtures.Not blocking — just a nice safety net for future maintenance.
- Fix error handling in playwright.rs: use `member.object().ok()?` and
`let Ok(arg) = arg else { continue }` patterns (C22, C23)
- Add function boundary guard in no_playwright_missing_await (C19)
- Support `describe.fixme(...)` in no_playwright_skipped_test (C20)
- Handle `test.describe.fixme` in use_playwright_valid_describe_callback (C21)
- Fix domains.json.snap to include `playwright` domain (CI fix)
- Add test cases for all new patterns
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename expectPlaywrightExpect → usePlaywrightExpect (rules must start with use/no) - Extract shared functions to frameworks/playwright.rs: - collect_member_names() with TokenText to avoid allocations - is_test_call() for detecting test/it calls - get_test_callback() returning LAST function argument - Fix noPlaywrightForceOption to handle parenthesized objects - Fix noPlaywrightMissingAwait to guard expect.poll with has_expect_in_chain - Fix noPlaywrightWaitForSelector to use replace_node_transfer_trivia - Fix noPlaywrightUselessAwait to handle .not modifier - Fix noPlaywrightConditionalExpect to avoid duplicate diagnostics - Refactor rules to use shared code from playwright.rs - Add #[derive(Debug)] to state enums Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…mejs#8960 Add catch clause detection to noPlaywrightConditionalExpect rule so that expect() inside catch blocks is flagged as conditional. Add test coverage for bare test.skip() with no arguments and test.describe.fixme() pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pedTests Merge the nursery `noPlaywrightSkippedTest` rule into the existing `suspicious/noSkippedTests` rule, adding Playwright-specific patterns (.fixme, test.describe, test.step, bracket notation, bare calls) and an `allowConditional` option. Remove the now-redundant nursery rule, its tests, options, and codegen references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove allowConditional option from noSkippedTests (can't add options in patch release) - Rename noPlaywrightConditionalExpect → noConditionalExpect with Test domain and Jest/Vitest sources - Rename usePlaywrightExpect → useExpect with Test domain and Jest/Vitest sources - Update changesets and diagnostic categories Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…omejs#8960 Add toMatchAriaSnapshot to ASYNC_PLAYWRIGHT_MATCHERS and remove stale #[allow(dead_code)] on SkipState::annotation field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mejs#8960 - Add `domains: &[RuleDomain::Test]` to NoSkippedTests metadata - Remove unreachable JS_SWITCH_STATEMENT arm in no_conditional_expect - Extract `annotation_for` helper to deduplicate skip/fixme logic - Add || and ?? test cases for noConditionalExpect Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix clippy collapsible_if warning in expr_ext.rs and update CLI snapshot for should_enable_domain_via_cli to account for useExpect rule firing on test domain fixtures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
abossenbroek
force-pushed
the
feat/playwright-eslint-rules
branch
from
2412ebc to
2ca5ad7
Compare
|
@dyc3 had to fix some linting issues and merge main back onto branch. |
…rrow/as_ref, and map_or Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thank you @abossenbroek for the awesome work! I trust @dyc3 judgement. There's one thing missing that isn't covered, probably because your agent wiped out the template: docs for the new domain We can't merge this PR without documentation for it. It should be very easy, and the PR must be sent to the website repository |
Good catch @ematipico! I'll cover it. (My agent helps but I keep reviewing and learning) Keep up the amazing work!
how do I go about doing that @ematipico? Thanks |
Merge origin/main, keeping Playwright additions alongside main's new entries (Types domain, EslintE18e, EslintBetterTailwindcss) and dropping rules that main removed from nursery (useMaxParams, useQwikMethodUsage, useQwikValidLexicalScope). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…k/biome into feat/playwright-eslint-rules
We have our website in another repo: https://github.com/biomejs/website There's some codegen that generates the pages for the domain documentation. Basically, just open a PR that updates this CI will fail at first because we have to wait for the domain introduced here to sync over to the website repo when this is merged. We don't add new domains that often. This is a one-time thing for the playwright stuff. |
Companion to biomejs/biome#8960. Adds the Playwright domain description to the codegen match so the domains page includes it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Website companion PR for the Playwright domain documentation: biomejs/website#3954 |
Companion to biomejs/biome#8960. Adds the Playwright domain description to the codegen match so the domains page includes it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
thanks @dyc3 and @ematipico !! Glad to have added first playwright support to biome! |
3 participants
Summary
Implements Playwright lint rules from eslint-plugin-playwright, adding comprehensive linting support for Playwright test code.
This is a fresh implementation that addresses feedback from #7775:
expect.poll()detection for sync matchers (e.g.,expect.poll(...).toBe(42))Promise.all/allSettled/race/anyhandling to wrap the outer combinator instead of serializing inner callsTokenTextover heap-allocating.to_string()where applicablepage.)noPlaywrightSkippedTestinto the existingnoSkippedTestsrule, enhancing it with Playwright-specific patterns (.fixme(),test.describe.skip(),test.step(), bracket notation)Rules
noPlaywrightElementHandlenoPlaywrightEvalnoPlaywrightForceOptionnoPlaywrightMissingAwaitnoPlaywrightNetworkidlenoPlaywrightPagePausenoSkippedTests(enhanced)noPlaywrightUselessAwaitnoPlaywrightWaitForNavigationnoPlaywrightWaitForSelectornoPlaywrightWaitForTimeoutusePlaywrightValidDescribeCallbackusePlaywrightExpectnoPlaywrightConditionalExpectAuto-fixes
noPlaywrightElementHandle$()/$$()withlocator()noPlaywrightWaitForSelectorwaitForSelector(sel)withlocator(sel).waitFor()Diagnostic improvements
noPlaywrightWaitForTimeoutAll rules target the nursery group with corresponding ESLint migration mappings for
eslint-plugin-playwright.Test plan
cargo t -p biome_js_analyze playwright)just gen-analyzer)Closes #7796
Tracks #8961
Related: #7775
🤖 Generated with Claude Code