Repository: playwright-community/eslint-plugin-playwright
Pull request #287 (2024-04-18 00:33:42)

Fix prefer web first assertion rule
Fixes the following issue:
#286

I have also added new tests to verify this scenario. 

An631: @mskelton I see you have made most of the changes to this rule. 
Do you mind taking a looking whenever you have time? Thanks!
mskelton: Please fix the formatting errors with Prettier before submitting. I'll take a look once the formatting and linting is fixed.
An631: @mskelton I have formatted the files with the command `pnpm fmt`. Although I did notice that prettier was also formatting two more files unrelated to my changes and I couldn't make prettier run without them. I figured it wouldn't harm anyone having those properly formatted as well in this PR so just kept them in there. Looks like the checks are passing now but please let me know if I missed something and thanks for taking a look.

Patch:
diff --git a/src/rules/prefer-web-first-assertions.ts b/src/rules/prefer-web-first-assertions.ts
index 537c85c..2d7adc6 100644
--- a/src/rules/prefer-web-first-assertions.ts
+++ b/src/rules/prefer-web-first-assertions.ts
@@ -1,5 +1,5 @@
 import { Rule } from 'eslint'
-import ESTree from 'estree'
+import ESTree, { AssignmentExpression } from 'estree'
 import {
   findParent,
   getRawValue,
@@ -8,6 +8,7 @@ import {
 } from '../utils/ast'
 import { createRule } from '../utils/createRule'
 import { parseFnCall } from '../utils/parseFnCall'
+import { TypedNodeWithParent } from '../utils/types'
 
 type MethodConfig = {
   inverse?: string
@@ -59,9 +60,50 @@ const supportedMatchers = new Set([
   'toBeFalsy',
 ])
 
+const isVariableDeclarator = (
+  node: ESTree.Node,
+): node is TypedNodeWithParent<'VariableDeclarator'> =>
+  node.type === 'VariableDeclarator'
+
+const isAssignmentExpression = (
+  node: ESTree.Node,
+): node is TypedNodeWithParent<'AssignmentExpression'> =>
+  node.type === 'AssignmentExpression'
+
+/**
+ * Given a Node and an assignment expression, finds out if the assignment
+ * expression happens before the node identifier (based on their range
+ * properties) and if the assignment expression left side is of the same name as
+ * the name of the given node.
+ *
+ * @param node The node we are comparing the assignment expression to.
+ * @param assignment The assignment that will be verified to see if its left
+ *   operand is the same as the node.name and if it happens before it.
+ * @returns True if the assignment left hand operator belongs to the node and
+ *   occurs before it, false otherwise. If either the node or the assignment
+ *   expression doesn't contain a range array, this will also return false
+ *   because their relative positions cannot be calculated.
+ */
+function isNodeLastAssignment(
+  node: ESTree.Identifier,
+  assignment: AssignmentExpression,
+) {
+  if (node.range && assignment.range && node.range[0] < assignment.range[1]) {
+    return false
+  }
+
+  return (
+    assignment.left.type === 'Identifier' && assignment.left.name === node.name
+  )
+}
+
 /**
  * If the expect call argument is a variable reference, finds the variable
- * initializer.
+ * initializer or last variable assignment.
+ *
+ * If a variable is assigned after initialization we have to look for the last
+ * time it was assigned because it could have been changed multiple times. We
+ * then use its right hand assignment operator as the dereferenced node.
  */
 function dereference(context: Rule.RuleContext, node: ESTree.Node | undefined) {
   if (node?.type !== 'Identifier') {
@@ -69,15 +111,24 @@ function dereference(context: Rule.RuleContext, node: ESTree.Node | undefined) {
   }
 
   const scope = context.sourceCode.getScope(node)
+  const parents = scope.references
+    .map((ref) => ref.identifier as Rule.Node)
+    .map((ident) => ident.parent)
 
-  // Find the variable declaration and return the initializer
-  for (const ref of scope.references) {
-    const refParent = (ref.identifier as Rule.Node).parent
+  // Look for any variable declarators in the scope references that match the
+  // dereferenced node variable name
+  const decl = parents
+    .filter(isVariableDeclarator)
+    .find((p) => p.id.type === 'Identifier' && p.id.name === node.name)
 
-    if (refParent.type === 'VariableDeclarator') {
-      return refParent.init
-    }
-  }
+  // Look for any variable assignments in the scope references and pick the last
+  // one that matches the dereferenced node variable name
+  const expr = parents
+    .filter(isAssignmentExpression)
+    .reverse()
+    .find((assignment) => isNodeLastAssignment(node, assignment))
+
+  return expr?.right ?? decl?.init
 }
 
 export default createRule({


Test patch:
diff --git a/src/rules/prefer-web-first-assertions.test.ts b/src/rules/prefer-web-first-assertions.test.ts
index 0c238fc..871d398 100644
--- a/src/rules/prefer-web-first-assertions.test.ts
+++ b/src/rules/prefer-web-first-assertions.test.ts
@@ -33,6 +33,90 @@ runRuleTester('prefer-web-first-assertions', rule, {
       ],
       output: test('await expect(page.locator(".tweet")).toBeHidden()'),
     },
+    {
+      code: test(`
+        const unrelatedAssignment = 'unrelated'
+        const isTweetVisible = await page.locator(".tweet").isVisible()
+        expect(isTweetVisible).toBe(true)
+      `),
+      output: test(`
+        const unrelatedAssignment = 'unrelated'
+        const isTweetVisible = page.locator(".tweet")
+        await expect(isTweetVisible).toBeVisible()
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toBeVisible', method: 'isVisible' },
+          endColumn: 31,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        const unrelatedAssignment = 'unrelated'
+        const isTweetVisible = await page.locator(".tweet").isVisible()
+        expect(isTweetVisible).toBe(false)
+      `),
+      output: test(`
+        const unrelatedAssignment = 'unrelated'
+        const isTweetVisible = page.locator(".tweet")
+        await expect(isTweetVisible).toBeHidden()
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toBeHidden', method: 'isVisible' },
+          endColumn: 31,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        const locatorFoo = page.locator(".foo")
+        const isBarVisible = await locatorFoo.locator(".bar").isVisible()
+        expect(isBarVisible).toBe(false)
+      `),
+      output: test(`
+        const locatorFoo = page.locator(".foo")
+        const isBarVisible = locatorFoo.locator(".bar")
+        await expect(isBarVisible).toBeHidden()
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toBeHidden', method: 'isVisible' },
+          endColumn: 29,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        const locatorFoo = page.locator(".foo")
+        const isBarVisible = await locatorFoo.locator(".bar").isVisible()
+        expect(isBarVisible).toBe(true)
+      `),
+      output: test(`
+        const locatorFoo = page.locator(".foo")
+        const isBarVisible = locatorFoo.locator(".bar")
+        await expect(isBarVisible).toBeVisible()
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toBeVisible', method: 'isVisible' },
+          endColumn: 29,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
     {
       code: test(
         'expect(await page.locator(".tweet").isVisible()).toEqual(true)',
@@ -301,6 +385,150 @@ runRuleTester('prefer-web-first-assertions', rule, {
       ],
       output: test('await expect(foo).not.toHaveText("bar")'),
     },
+    {
+      code: test(`
+        const fooLocator = page.locator('.fooClass');
+        const fooLocatorText = await fooLocator.textContent();
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      output: test(`
+        const fooLocator = page.locator('.fooClass');
+        const fooLocatorText = fooLocator;
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toHaveText', method: 'textContent' },
+          endColumn: 31,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        const fooLocator = page.locator('.fooClass');
+        let fooLocatorText = await fooLocator.textContent();
+        expect(fooLocatorText).toEqual('foo');
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      output: test(`
+        const fooLocator = page.locator('.fooClass');
+        let fooLocatorText = fooLocator;
+        await expect(fooLocatorText).toHaveText('foo');
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toHaveText', method: 'textContent' },
+          endColumn: 31,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        let fooLocatorText;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = 'Unrelated';
+        fooLocatorText = await fooLocator.textContent();
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      output: test(`
+        let fooLocatorText;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = 'Unrelated';
+        fooLocatorText = fooLocator;
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toHaveText', method: 'textContent' },
+          endColumn: 31,
+          line: 6,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        let fooLocatorText;
+        let fooLocatorText2;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = await fooLocator.textContent();
+        fooLocatorText2 = await fooLocator.textContent();
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      output: test(`
+        let fooLocatorText;
+        let fooLocatorText2;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = fooLocator;
+        fooLocatorText2 = await fooLocator.textContent();
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toHaveText', method: 'textContent' },
+          endColumn: 31,
+          line: 7,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        let fooLocatorText;
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+        fooLocatorText = await page.locator('.fooClass').textContent();
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      output: test(`
+        let fooLocatorText;
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+        fooLocatorText = page.locator('.fooClass');
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toHaveText', method: 'textContent' },
+          endColumn: 31,
+          line: 6,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
+    {
+      code: test(`
+        const unrelatedAssignment = "unrelated";
+        const fooLocatorText = await page.locator('.foo').textContent();
+        expect(fooLocatorText).toEqual('foo');
+      `),
+      output: test(`
+        const unrelatedAssignment = "unrelated";
+        const fooLocatorText = page.locator('.foo');
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
+      errors: [
+        {
+          column: 9,
+          data: { matcher: 'toHaveText', method: 'textContent' },
+          endColumn: 31,
+          line: 4,
+          messageId: 'useWebFirstAssertion',
+        },
+      ],
+    },
 
     // isChecked
     {
@@ -736,5 +964,44 @@ runRuleTester('prefer-web-first-assertions', rule, {
         expect(myValue).toBeVisible();
       `),
     },
+    {
+      code: test(`
+        let fooLocatorText;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = await fooLocator.textContent();
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+      `),
+    },
+    {
+      code: test(`
+        let fooLocatorText;
+        let fooLocatorText2;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = 'foo';
+        fooLocatorText2 = await fooLocator.textContent();
+        expect(fooLocatorText).toEqual('foo');
+      `),
+    },
+    {
+      code: test(`
+        let fooLocatorText;
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo')
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = fooLocator;
+        expect(fooLocatorText).toHaveText('foo');
+      `),
+    },
+    {
+      code: test(`
+        const fooLocator = page.locator('.fooClass');
+        let fooLocatorText;
+        fooLocatorText = fooLocator;
+        expect(fooLocatorText).toHaveText('foo');
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo')
+      `),
+    },
   ],
 })