Repository: playwright-community/eslint-plugin-playwright
Pull request #292 (2024-05-16 01:56:13)

Fix prefer-to-have-count rule for expect arguments that need dereferencing  
ISSUE #293: 
The current prefer-to-have-count rule has a problem where any expect checks that take a variable argument will not check to make sure the value is not originating from a .count call. 

For example the following code would not be flagged as an error:

```ts
   const filesCount = await files.count();
   expect(filesCount).toBe(2);
```

The current code misses this case because we are not dereferencing the last assigned value of `filesCount` at the moment of verifying the expect argument. The rule should flag the error and return the following as a fix:

```ts
   const filesCount = files;
   await expect(filesCount).toHaveCount(2);
```

SOLUTION:

This PR updates the `prefer-to-have-count` code to re-use the `dereference` method previously used by the `prefer-web-first-assertion` rule. To make this method available to other files I moved it, along with all its related methods, to the `ast.ts` file which looked to me like the best place to centralize it (curious to know if there is a better place for these methods though).

I have added multiple new tests to check for other edge cases like variable re-assignment.


Patch:
diff --git a/src/rules/prefer-to-have-count.ts b/src/rules/prefer-to-have-count.ts
index 4230719..ba0513a 100644
--- a/src/rules/prefer-to-have-count.ts
+++ b/src/rules/prefer-to-have-count.ts
@@ -1,4 +1,4 @@
-import { equalityMatchers, isPropertyAccessor } from '../utils/ast'
+import { dereference, equalityMatchers, isPropertyAccessor } from '../utils/ast'
 import { createRule } from '../utils/createRule'
 import { replaceAccessorFixer } from '../utils/fixer'
 import { parseFnCall } from '../utils/parseFnCall'
@@ -15,7 +15,7 @@ export default createRule({
           return
         }
 
-        const [argument] = call.args
+        const argument = dereference(context, call.args[0])
         if (
           argument?.type !== 'AwaitExpression' ||
           argument.argument.type !== 'CallExpression' ||
diff --git a/src/rules/prefer-web-first-assertions.ts b/src/rules/prefer-web-first-assertions.ts
index 2d7adc6..1c295e4 100644
--- a/src/rules/prefer-web-first-assertions.ts
+++ b/src/rules/prefer-web-first-assertions.ts
@@ -1,6 +1,5 @@
-import { Rule } from 'eslint'
-import ESTree, { AssignmentExpression } from 'estree'
 import {
+  dereference,
   findParent,
   getRawValue,
   getStringValue,
@@ -8,7 +7,6 @@ import {
 } from '../utils/ast'
 import { createRule } from '../utils/createRule'
 import { parseFnCall } from '../utils/parseFnCall'
-import { TypedNodeWithParent } from '../utils/types'
 
 type MethodConfig = {
   inverse?: string
@@ -60,77 +58,6 @@ 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 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') {
-    return node
-  }
-
-  const scope = context.sourceCode.getScope(node)
-  const parents = scope.references
-    .map((ref) => ref.identifier as Rule.Node)
-    .map((ident) => ident.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)
-
-  // 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({
   create(context) {
     return {
diff --git a/src/utils/ast.ts b/src/utils/ast.ts
index db423b9..f6d3ee5 100644
--- a/src/utils/ast.ts
+++ b/src/utils/ast.ts
@@ -1,5 +1,5 @@
 import { Rule } from 'eslint'
-import ESTree from 'estree'
+import ESTree, { AssignmentExpression } from 'estree'
 import { isSupportedAccessor } from './parseFnCall'
 import { NodeWithParent, TypedNodeWithParent } from './types'
 
@@ -158,3 +158,88 @@ export function getNodeName(node: ESTree.Node): string | null {
 
   return null
 }
+
+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 node argument is a variable reference, finds the variable initializer
+ * or last variable assignment and returns the assigned value.
+ *
+ * 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.
+ *
+ * @example <caption>Dereference a `const` initialized node:</caption>
+ *   // returns 1
+ *   const variable = 1
+ *   console.log(variable) // dereferenced value of the 'variable' node is 1
+ *
+ * @example <caption>Dereference a `let` re-assigned node:</caption>
+ *   // returns 1
+ *   let variable = 0
+ *   variable = 1
+ *   console.log(variable) // dereferenced value of the 'variable' node is 1
+ */
+export function dereference(
+  context: Rule.RuleContext,
+  node: ESTree.Node | undefined,
+) {
+  if (node?.type !== 'Identifier') {
+    return node
+  }
+
+  const scope = context.sourceCode.getScope(node)
+  const parents = scope.references
+    .map((ref) => ref.identifier as Rule.Node)
+    .map((ident) => ident.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)
+
+  // 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
+}


Test patch:
diff --git a/src/rules/prefer-to-have-count.test.ts b/src/rules/prefer-to-have-count.test.ts
index 3fce87f..13302d2 100644
--- a/src/rules/prefer-to-have-count.test.ts
+++ b/src/rules/prefer-to-have-count.test.ts
@@ -72,6 +72,83 @@ runRuleTester('prefer-to-have-count', rule, {
         },
       },
     },
+    {
+      code: `
+        const filesCount = await files.count();
+        expect(filesCount).toBe(1)
+      `,
+      errors: [
+        { column: 28, endColumn: 32, line: 3, messageId: 'useToHaveCount' },
+      ],
+      output: `
+        const filesCount = files;
+        await expect(filesCount).toHaveCount(1)
+      `,
+    },
+    {
+      code: `
+        const filesCount = await files.count();
+        const unrelatedConst = unrelated;
+        expect(filesCount).toBe(1)
+      `,
+      errors: [
+        { column: 28, endColumn: 32, line: 4, messageId: 'useToHaveCount' },
+      ],
+      output: `
+        const filesCount = files;
+        const unrelatedConst = unrelated;
+        await expect(filesCount).toHaveCount(1)
+      `,
+    },
+    {
+      code: `
+        let filesCount = 3;
+        filesCount = await files.count();
+        expect(filesCount).toBe(1);
+      `,
+      errors: [
+        { column: 28, endColumn: 32, line: 4, messageId: 'useToHaveCount' },
+      ],
+      output: `
+        let filesCount = 3;
+        filesCount = files;
+        await expect(filesCount).toHaveCount(1);
+      `,
+    },
+    {
+      code: `
+        let filesCount = 3;
+        filesCount = await files.count();
+        let unrelatedVar = unrelated;
+        expect(filesCount).toBe(1);
+      `,
+      errors: [
+        { column: 28, endColumn: 32, line: 5, messageId: 'useToHaveCount' },
+      ],
+      output: `
+        let filesCount = 3;
+        filesCount = files;
+        let unrelatedVar = unrelated;
+        await expect(filesCount).toHaveCount(1);
+      `,
+    },
+    {
+      code: `
+        let filesCount = 3;
+        filesCount = await files.count();
+        expect(filesCount).toBe(1);
+        filesCount = 0;
+      `,
+      errors: [
+        { column: 28, endColumn: 32, line: 4, messageId: 'useToHaveCount' },
+      ],
+      output: `
+        let filesCount = 3;
+        filesCount = files;
+        await expect(filesCount).toHaveCount(1);
+        filesCount = 0;
+      `,
+    },
   ],
   valid: [
     { code: 'await expect(files).toHaveCount(1)' },
diff --git a/src/rules/prefer-web-first-assertions.test.ts b/src/rules/prefer-web-first-assertions.test.ts
index 871d398..862af5b 100644
--- a/src/rules/prefer-web-first-assertions.test.ts
+++ b/src/rules/prefer-web-first-assertions.test.ts
@@ -39,11 +39,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -53,6 +48,11 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const unrelatedAssignment = 'unrelated'
+        const isTweetVisible = page.locator(".tweet")
+        await expect(isTweetVisible).toBeVisible()
+      `),
     },
     {
       code: test(`
@@ -60,11 +60,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -74,6 +69,11 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const unrelatedAssignment = 'unrelated'
+        const isTweetVisible = page.locator(".tweet")
+        await expect(isTweetVisible).toBeHidden()
+      `),
     },
     {
       code: test(`
@@ -81,11 +81,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -95,6 +90,11 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const locatorFoo = page.locator(".foo")
+        const isBarVisible = locatorFoo.locator(".bar")
+        await expect(isBarVisible).toBeHidden()
+      `),
     },
     {
       code: test(`
@@ -102,11 +102,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -116,6 +111,11 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const locatorFoo = page.locator(".foo")
+        const isBarVisible = locatorFoo.locator(".bar")
+        await expect(isBarVisible).toBeVisible()
+      `),
     },
     {
       code: test(
@@ -391,11 +391,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -405,6 +400,11 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const fooLocator = page.locator('.fooClass');
+        const fooLocatorText = fooLocator;
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
     },
     {
       code: test(`
@@ -414,13 +414,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -430,6 +423,13 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const fooLocator = page.locator('.fooClass');
+        let fooLocatorText = fooLocator;
+        await expect(fooLocatorText).toHaveText('foo');
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+      `),
     },
     {
       code: test(`
@@ -439,13 +439,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -455,6 +448,13 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        let fooLocatorText;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = 'Unrelated';
+        fooLocatorText = fooLocator;
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
     },
     {
       code: test(`
@@ -465,14 +465,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -482,6 +474,14 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        let fooLocatorText;
+        let fooLocatorText2;
+        const fooLocator = page.locator('.fooClass');
+        fooLocatorText = fooLocator;
+        fooLocatorText2 = await fooLocator.textContent();
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
     },
     {
       code: test(`
@@ -491,13 +491,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -507,6 +500,13 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        let fooLocatorText;
+        fooLocatorText = 'foo';
+        expect(fooLocatorText).toEqual('foo');
+        fooLocatorText = page.locator('.fooClass');
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
     },
     {
       code: test(`
@@ -514,11 +514,6 @@ runRuleTester('prefer-web-first-assertions', rule, {
         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,
@@ -528,6 +523,11 @@ runRuleTester('prefer-web-first-assertions', rule, {
           messageId: 'useWebFirstAssertion',
         },
       ],
+      output: test(`
+        const unrelatedAssignment = "unrelated";
+        const fooLocatorText = page.locator('.foo');
+        await expect(fooLocatorText).toHaveText('foo');
+      `),
     },
 
     // isChecked