fix(lint/useExpect): reject asymmetric matchers and utilities as assertions#9455
Conversation
…rtions is_expect_expression treated any expect.XYZ() pattern as a valid assertion because the JsStaticMemberExpression branch unconditionally recursed on the object. This caused false negatives for expect.stringContaining(), expect.extend(), expect.not.objectContaining(), and similar constructs that are not assertions. When the object is a bare expect identifier, check the member name against an allowlist of assertion-qualifying members (soft, poll, assertions, hasAssertions). Everything else on bare expect is now correctly rejected. Closes biomejs#9174
🦋 Changeset detectedLatest commit: ee548ac 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 |
ematipico
left a comment
There was a problem hiding this comment.
Changeset is missing. The link to how to create was in the PR template, which was wiped out. I leave that to you
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTightens detection in the useExpect lint rule: static 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.
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/nursery/useExpect/invalid/asymmetric-matchers.js (1)
3-17: Consider adding a few more asymmetric matchers for completeness (optional).The fix rejects all non-allowlisted members generically, so exhaustive testing isn't essential. However, adding one or two more cases (e.g.,
expect.anything(),expect.arrayContaining()) would strengthen confidence and serve as documentation of the intended behaviour.🤖 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/useExpect/invalid/asymmetric-matchers.js` around lines 3 - 17, Add one or two extra test cases that use other asymmetric matchers to broaden coverage: e.g., create a test similar to "only asymmetric matcher, not an assertion" that assigns expect.anything() to a variable and another that assigns expect.arrayContaining([1,2]) (or expect.objectContaining with nested asymmetric) to exercise arrayContaining; place them near the existing tests (functions test/it using matcher variables) so names like "only asymmetric matcher, not an assertion" / "negated asymmetric matcher is not an assertion" are extended with new cases and the new matcher variables mirror the existing pattern.
🤖 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/useExpect/invalid/asymmetric-matchers.js`:
- Around line 3-17: Add one or two extra test cases that use other asymmetric
matchers to broaden coverage: e.g., create a test similar to "only asymmetric
matcher, not an assertion" that assigns expect.anything() to a variable and
another that assigns expect.arrayContaining([1,2]) (or expect.objectContaining
with nested asymmetric) to exercise arrayContaining; place them near the
existing tests (functions test/it using matcher variables) so names like "only
asymmetric matcher, not an assertion" / "negated asymmetric matcher is not an
assertion" are extended with new cases and the new matcher variables mirror the
existing pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2c01fad-b148-49f2-9340-d672981fa972
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/nursery/useExpect/invalid/asymmetric-matchers.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useExpect/valid/assertion-counts.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/useExpect/invalid/asymmetric-matchers.jscrates/biome_js_analyze/tests/specs/nursery/useExpect/valid/assertion-counts.js
|
Addressed both review items:
|
omar-y-abdi
left a comment
There was a problem hiding this comment.
Happy to help out
Merging this PR will not alter performance
Comparing Footnotes
|
Co-authored-by: Naoki Ikeguchi <me@s6n.jp>
|
@ematipico Hey, this is ready to merge whenever you get a chance. |
|
@omar-y-abdi there is still failing CI |
Summary
Closes #9174
useExpectproduced false negatives for Vitest constructs likeexpect.stringContaining(),expect.extend(), andexpect.not.objectContaining(). These are asymmetric matchers and utilities, not assertions, but the rule counted them as valid assertions and did not flag the test.The root cause was in
is_expect_expressioninplaywright.rs. TheJsStaticMemberExpressionbranch unconditionally recursed on the object expression, so anyexpect.anything()pattern was treated as a valid assertion chain.Fix
When the object of a static member expression is a bare
expectidentifier (not a call likeexpect(value)), the member name is now checked against an allowlist of assertion-qualifying members:soft,poll,assertions,hasAssertions. All other members on bareexpect(asymmetric matchers,extend,notprefix for negated matchers) are correctly rejected.Chained assertions like
expect(value).not.toBe(1)andexpect.soft(page).toBeVisible()are unaffected because their object is a call expression, not a bare identifier.Test plan
invalid/asymmetric-matchers.jswith 3 cases:expect.stringContaining(),expect.extend(),expect.not.objectContaining()— all now correctly flaggedvalid/assertion-counts.jswith 5 cases:expect.assertions(),expect.hasAssertions(),expect.assertions(0),expect.soft(),expect.poll()— all correctly passuse_expectspec tests passis_expect_callunit tests pass (no regression)no_playwright_*spec tests pass (no regression)no_conditional_expectspec tests pass (no regression)Closes #9174
AI disclosure
This PR was written with assistance from Claude Code (code comprehension, tracing the bug through the AST traversal logic, and drafting the implementation). All code was reviewed and verified by the author.