fix(noPlaywrightMissingAwait): fix false positives with jest-dom matchers#9154
Conversation
🦋 Changeset detectedLatest commit: a2b72fc 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 |
WalkthroughSplits Playwright matchers into Possibly related PRs
Suggested labels
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 |
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_analyze/src/lint/nursery/no_playwright_missing_await.rs (1)
542-554:⚠️ Potential issue | 🟡 MinorAdd invalid fixtures for the remaining overlapping matchers with Playwright locators.
The jest-dom valid fixture is excellent — it confirms all 12 matchers work synchronously. However, only
toBeVisibleandtoBeEnabledappear in invalid fixtures with Playwright locators. The other 10 matchers inOVERLAPPING_MATCHERS(toBeChecked,toBeDisabled,toBeEmpty,toHaveAccessibleDescription,toHaveAccessibleErrorMessage,toHaveAccessibleName,toHaveAttribute,toHaveClass,toHaveRole,toHaveValue) lack negative test coverage, leaving the true-positive path untested for those cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs` around lines 542 - 554, Tests currently only include invalid fixtures for Playwright locators for toBeVisible and toBeEnabled, leaving the other 10 matchers in OVERLAPPING_MATCHERS untested; add negative (invalid) Playwright locator fixtures for each of the remaining matchers (toBeChecked, toBeDisabled, toBeEmpty, toHaveAccessibleDescription, toHaveAccessibleErrorMessage, toHaveAccessibleName, toHaveAttribute, toHaveClass, toHaveRole, toHaveValue) so the lint rule's true-positive path is covered; locate the test module under #[cfg(test)] mod tests and the existing fixtures used for Playwright invalid cases and add analogous invalid-case fixtures for each matcher name in OVERLAPPING_MATCHERS, ensuring they mirror the structure of the existing Playwright invalid examples and are included in test runs alongside PLAYWRIGHT_ONLY_MATCHERS/OVERLAPPING_MATCHERS checks.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs (1)
338-419:find_expect_first_argduplicateshas_expect_in_chain's chain-walking logic.Both functions traverse the identical
JsCallExpression/JsStaticMemberExpressionstructure to locateexpect. Consider replacing the bool-returninghas_expect_in_chainwith a thin wrapper:fn has_expect_in_chain(expr: &AnyJsExpression) -> bool { find_expect_first_arg(expr).is_some() || matches!(expr, AnyJsExpression::JsCallExpression(c) if /* direct expect() at this level */ ...) }Or, more cleanly, just inline
find_expect_first_argat all three call sites inget_async_expect_matcherand drophas_expect_in_chainentirely, since every call tohas_expect_in_chainthat isn't theis_overlappingbranch can be replaced by checkingfind_expect_first_arg(...).is_some().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs` around lines 338 - 419, The two functions duplicate chain-walking logic; replace has_expect_in_chain's implementation with a thin wrapper that reuses find_expect_first_arg by returning find_expect_first_arg(expr).is_some(), or alternatively inline find_expect_first_arg at the three call sites in get_async_expect_matcher and remove has_expect_in_chain entirely (keep the is_overlapping branch logic intact if it needs a boolean). Update references to has_expect_in_chain to use the wrapper or direct find_expect_first_arg(...).is_some() and remove the duplicated pattern-matching code from has_expect_in_chain.
🤖 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_analyze/src/lint/nursery/no_playwright_missing_await.rs`:
- Around line 328-333: The false-negative occurs because
is_playwright_call_chain only recognizes direct chains on identifiers like
page/frame and find_expect_first_arg may return an Identifier (e.g., btn) whose
initializer is a playwright chain; update the logic so when
find_expect_first_arg returns an Identifier you resolve that identifier to its
initializer and re-check that initializer with is_playwright_call_chain.
Concretely: in the branch around find_expect_first_arg/if
!is_playwright_call_chain, add a resolution step (e.g., a helper
resolve_identifier_initializer or using the existing scope/resolver) that finds
the VariableDeclarator/assignment for the Identifier and retrieves its init
expression (follow simple aliasing like const a = b.getBy...; const c = a;
once). Then call is_playwright_call_chain on the resolved init expression and
only return None if neither the original arg nor its resolved initializer is a
playwright call chain. Ensure this uses the existing function names
is_playwright_call_chain and find_expect_first_arg so the change is localized.
In
`@crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/valid/jest-dom-matchers.js`:
- Around line 2-6: The test uses the global "screen" variable (screen.getByRole)
but never imports it; either add the appropriate import (e.g., import { screen }
from "@testing-library/dom") at the top of the file so the "screen" symbol is
defined for the test, or replace the call to screen.getByRole(...) with a
DOM-native lookup such as document.querySelector("button") to avoid adding a
third‑party import; update the code referencing screen (the getByRole call)
accordingly.
---
Outside diff comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs`:
- Around line 542-554: Tests currently only include invalid fixtures for
Playwright locators for toBeVisible and toBeEnabled, leaving the other 10
matchers in OVERLAPPING_MATCHERS untested; add negative (invalid) Playwright
locator fixtures for each of the remaining matchers (toBeChecked, toBeDisabled,
toBeEmpty, toHaveAccessibleDescription, toHaveAccessibleErrorMessage,
toHaveAccessibleName, toHaveAttribute, toHaveClass, toHaveRole, toHaveValue) so
the lint rule's true-positive path is covered; locate the test module under
#[cfg(test)] mod tests and the existing fixtures used for Playwright invalid
cases and add analogous invalid-case fixtures for each matcher name in
OVERLAPPING_MATCHERS, ensuring they mirror the structure of the existing
Playwright invalid examples and are included in test runs alongside
PLAYWRIGHT_ONLY_MATCHERS/OVERLAPPING_MATCHERS checks.
---
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_playwright_missing_await.rs`:
- Around line 338-419: The two functions duplicate chain-walking logic; replace
has_expect_in_chain's implementation with a thin wrapper that reuses
find_expect_first_arg by returning find_expect_first_arg(expr).is_some(), or
alternatively inline find_expect_first_arg at the three call sites in
get_async_expect_matcher and remove has_expect_in_chain entirely (keep the
is_overlapping branch logic intact if it needs a boolean). Update references to
has_expect_in_chain to use the wrapper or direct
find_expect_first_arg(...).is_some() and remove the duplicated pattern-matching
code from has_expect_in_chain.
Merging this PR will improve performance by 12.83%
Performance Changes
Comparing Footnotes
|
…hers
For matchers shared between Playwright (async) and jest-dom (sync) —
such as `toBeVisible`, `toBeChecked`, and `toHaveAttribute` — the rule
now checks whether `expect()`'s argument is a Playwright locator or page
object before flagging. Playwright-only matchers like `toHaveTitle` are
still flagged unconditionally.
Additionally, add semantic variable resolution so that extracted
Playwright locators are still correctly detected:
```js
const loc = page.locator('.item');
expect(loc).toBeVisible(); // still flagged
```
Fixes biomejs#9115
Co-Authored-By: Claude Opus 4.6 <[email protected]>
716376d to
b28db4b
Compare
… reassigned When a variable initialized with a Playwright locator is later reassigned, the rule can no longer trust the initializer. Bail out via `binding.all_writes()` to avoid false positives on reassigned variables. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/valid/reassigned-variable.js (1)
9-13: Intentional false negative for simplicity — worth a brief comment?This test verifies that the rule bails out when a variable has multiple writes, even when reassigned to another Playwright locator. Whilst this avoids false positives, it introduces a false negative:
expect(loc).toBeVisible()on Line 12 technically still needs anawait.The conservative approach is reasonable, but a brief inline comment explaining this tradeoff would help future maintainers understand why this is in the valid folder rather than invalid.
📝 Suggested comment
test("reassigned to another playwright locator", async ({ page }) => { + // Still a Playwright locator, but rule bails out on any reassignment to avoid + // complexity in tracking writes — accepted false negative. let loc = page.locator(".old"); loc = page.locator(".new"); expect(loc).toBeVisible(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/valid/reassigned-variable.js` around lines 9 - 13, Add a brief inline comment explaining why this case is placed in the valid folder: inside the test callback passed to test(...) the variable loc is reassigned (loc = page.locator(".new")) so the rule conservatively bails out on multiple writes even though expect(loc).toBeVisible() still requires an await; add a one-line comment above the reassignment or above the expect describing this intentional false negative tradeoff (mentioning loc and page.locator) so future maintainers understand why the test is valid.crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/invalid/reassigned-still-playwright.js (1)
1-6: Consider renaming the test file for clarity.The filename
reassigned-still-playwright.jsis a bit misleading since this test specifically validates thatconstbindings (which cannot be reassigned) are correctly flagged. A name likeconst-locator-still-flagged.jsorextracted-const-locator.jsmight better reflect the test's intent — i.e., that extracted locators in non-reassignable bindings should still trigger the rule.Otherwise, the test case itself is spot-on for validating the semantic resolution path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/invalid/reassigned-still-playwright.js` around lines 1 - 6, Rename the test file to better reflect its intent: it verifies that extracting a locator into a non-reassignable const binding still triggers the rule. Change the filename from reassigned-still-playwright.js to something like const-locator-still-flagged.js (or extracted-const-locator.js) and keep the test body (test("const playwright locator", async ({ page }) => { const loc = page.locator(".item"); expect(loc).toBeVisible(); })); update any test harness references to the new filename so the spec still runs.
🤖 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_analyze/tests/specs/nursery/noPlaywrightMissingAwait/invalid/reassigned-still-playwright.js`:
- Around line 1-6: Rename the test file to better reflect its intent: it
verifies that extracting a locator into a non-reassignable const binding still
triggers the rule. Change the filename from reassigned-still-playwright.js to
something like const-locator-still-flagged.js (or extracted-const-locator.js)
and keep the test body (test("const playwright locator", async ({ page }) => {
const loc = page.locator(".item"); expect(loc).toBeVisible(); })); update any
test harness references to the new filename so the spec still runs.
In
`@crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/valid/reassigned-variable.js`:
- Around line 9-13: Add a brief inline comment explaining why this case is
placed in the valid folder: inside the test callback passed to test(...) the
variable loc is reassigned (loc = page.locator(".new")) so the rule
conservatively bails out on multiple writes even though
expect(loc).toBeVisible() still requires an await; add a one-line comment above
the reassignment or above the expect describing this intentional false negative
tradeoff (mentioning loc and page.locator) so future maintainers understand why
the test is valid.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/invalid/reassigned-still-playwright.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/valid/reassigned-variable.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
crates/biome_js_analyze/src/frameworks/playwright.rscrates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/invalid/reassigned-still-playwright.jscrates/biome_js_analyze/tests/specs/nursery/noPlaywrightMissingAwait/valid/reassigned-variable.js
|
@dyc3 would you mind reviewing? thnx |
|
Thanks @dyc3 !! |
Summary
Fixes #9115
For matchers shared between Playwright (async) and jest-dom (sync) — such as
toBeVisible,toBeChecked, andtoHaveAttribute— the rule now checks whetherexpect()'s argument is a Playwright locator or page object before flagging. Playwright-only matchers liketoHaveTitleare still flagged unconditionally.The matchers are now split into two categories:
toBeAttached,toBeHidden,toHaveTitle,toHaveURL, etc.) — always flagged.toBeVisible,toBeChecked,toHaveAttribute, etc.) — only flagged whenexpect()'s argument is a Playwright locator/page.Additionally, semantic variable resolution was added so that extracted Playwright locators are still correctly detected:
Variables that have been reassigned after their Playwright initializer are not flagged, since the value at the
expect()call site may no longer be a Playwright object:Test Plan
valid/jest-dom-matchers.js— verifies no false positives on jest-dom matchers.valid/extracted-non-playwright.js— verifies no false positives when extracted variables come from non-Playwright sources (testing-library, DOM queries, function calls).valid/reassigned-variable.js— verifies no false positives when a Playwright-initialized variable is later reassigned.invalid/extracted-locator-overlapping.js— verifies overlapping matchers are still flagged when the argument is an extracted Playwright locator.invalid/reassigned-still-playwright.js— verifiesconstbindings (never reassigned) are still correctly flagged.Docs
No documentation changes needed — this is a bugfix for an existing nursery rule.