fix(rules): added more use cases for prefer-in-document (#121)

benmonro · web-flow · commit a912e689f519 · 2020-12-05T13:01:13.000-08:00
* fix(rules): more use cases for prefer-in-document

* added notes about .length

* covg

* fix cwd issue in husky on CI

* fix cwd issue in husky on CI

* fix cwd issue in husky on CI
diff --git a/.github/workflows/eslint-plugin-jest-dom.yml b/.github/workflows/eslint-plugin-jest-dom.yml
@@ -19,7 +19,7 @@ jobs:
         with:
           node-version: ${{ matrix.node }}
       - uses: actions/checkout@v1
-      - run: npm install
+      - run: INIT_CWD="$(pwd)" npm install
       - run: npm run validate
       - run: npx codecov@3
   release:
diff --git a/docs/rules/prefer-in-document.md b/docs/rules/prefer-in-document.md
@@ -28,6 +28,7 @@ expect(bar).toHaveLength(0);
 Examples of **correct** code for this rule:
 
 ```js
+expect(screen.getByText("foo").length).toBe(1);
 expect(screen.queryByText("foo")).toBeInTheDocument();
 expect(await screen.findByText("foo")).toBeInTheDocument();
 expect(queryByText("foo")).toBeInTheDocument();
@@ -50,6 +51,11 @@ expect(baz).toBeDefined();
 Don't use this rule if you don't care about the added readability and
 improvements that `toBeInTheDocument` offers to your expects.
 
+Note, that `expect(screen.getByText("foo").length).toBe(1)` is valid. If you want to report on that please use [jest/prefer-to-have-length](https://github.com/jest-community/eslint-plugin-jest/blob/HEAD/docs/rules/prefer-to-have-length.md)
+![fixable][] which will first convert those use cases to use `.toHaveLength`.
+
 ## Further Reading
 
 - [Docs on toBeInTheDocument](https://github.com/testing-library/jest-dom#tobeinthedocument)
+
+[fixable]: https://img.shields.io/badge/-fixable-green.svg
diff --git a/src/__tests__/lib/rules/prefer-in-document.js b/src/__tests__/lib/rules/prefer-in-document.js
@@ -74,14 +74,28 @@ const valid = [
   `expect(await screen.findAllByRole("button")).not.toHaveLength(
       NUM_BUTTONS
     )`,
+  `expect( screen.getAllByRole("link") ).not.toHaveLength(0);`,
+
+  `import {NUM_BUTTONS} from "./foo";
+     expect(screen.getByText('foo')).toHaveLength(NUM_BUTTONS)`,
+  `expect(screen.getAllByText("foo")).toHaveLength(getLength())`,
 ];
 const invalid = [
-  // Invalid cases that applies to all variants
-
   invalidCase(
     `expect(screen.getByText('foo')).toHaveLength(1)`,
     `expect(screen.getByText('foo')).toBeInTheDocument()`
   ),
+  invalidCase(
+    `const NUM_BUTTONS=1;
+     expect(screen.getByText('foo')).toHaveLength(NUM_BUTTONS)`,
+    `const NUM_BUTTONS=1;
+     expect(screen.getByText('foo')).toBeInTheDocument()`
+  ),
+
+  invalidCase(
+    `expect(screen.queryAllByTestId("foo")).toHaveLength(0)`,
+    `expect(screen.queryByTestId("foo")).not.toBeInTheDocument()`
+  ),
   invalidCase(
     `expect(getByText('foo')).toHaveLength(1)`,
     `expect(getByText('foo')).toBeInTheDocument()`
@@ -205,54 +219,101 @@ const invalid = [
   ),
 
   // Invalid cases that applies to queryBy* and queryAllBy*
-  ...["queryByText", "queryAllByText"].map((q) => [
-    invalidCase(
-      `expect(${q}('foo')).toHaveLength(0)`,
-      `expect(${q}('foo')).not.toBeInTheDocument()`
-    ),
-    invalidCase(
-      `expect(${q}('foo')).toBeNull()`,
-      `expect(${q}('foo')).not.toBeInTheDocument()`
-    ),
-    invalidCase(
-      `expect(${q}('foo')).not.toBeNull()`,
-      `expect(${q}('foo')).toBeInTheDocument()`
-    ),
-    invalidCase(
-      `expect(${q}('foo')) .not .toBeNull()`,
-      `expect(${q}('foo')).toBeInTheDocument()`
-    ),
-    invalidCase(
-      `expect(${q}('foo')).toBeDefined()`,
-      `expect(${q}('foo')).toBeInTheDocument()`
-    ),
-    invalidCase(
-      `expect(${q}('foo')) .not .toBeDefined()`,
-      `expect(${q}('foo')) .not .toBeInTheDocument()`
-    ),
-    invalidCase(
-      `let foo;
-      foo = screen.${q}('foo');
+
+  invalidCase(
+    `expect(queryByText('foo')).toHaveLength(0)`,
+    `expect(queryByText('foo')).not.toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryByText('foo')).toBeNull()`,
+    `expect(queryByText('foo')).not.toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryByText('foo')).not.toBeNull()`,
+    `expect(queryByText('foo')).toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryByText('foo')) .not .toBeNull()`,
+    `expect(queryByText('foo')).toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryByText('foo')).toBeDefined()`,
+    `expect(queryByText('foo')).toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryByText('foo')) .not .toBeDefined()`,
+    `expect(queryByText('foo')) .not .toBeInTheDocument()`
+  ),
+  invalidCase(
+    `let foo;
+      foo = screen.queryByText('foo');
       expect(foo).toHaveLength(0);`,
-      `let foo;
-      foo = screen.${q}('foo');
+    `let foo;
+      foo = screen.queryByText('foo');
       expect(foo).not.toBeInTheDocument();`
-    ),
-    invalidCase(
-      `let foo;
-      foo = screen.${q}('foo');
+  ),
+  invalidCase(
+    `let foo;
+      foo = screen.queryByText('foo');
       expect(foo) .not.toBeNull();`,
-      `let foo;
-      foo = screen.${q}('foo');
+    `let foo;
+      foo = screen.queryByText('foo');
+      expect(foo).toBeInTheDocument();`
+  ),
+  invalidCase(
+    `let foo = screen.queryByText('foo');
+      expect(foo).not.toBeNull();`,
+    `let foo = screen.queryByText('foo');
+      expect(foo).toBeInTheDocument();`
+  ),
+
+  invalidCase(
+    `expect(queryAllByText('foo')).toHaveLength(0)`,
+    `expect(queryByText('foo')).not.toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryAllByText('foo')).toBeNull()`,
+    `expect(queryByText('foo')).not.toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryAllByText('foo')).not.toBeNull()`,
+    `expect(queryByText('foo')).toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryAllByText('foo')) .not .toBeNull()`,
+    `expect(queryByText('foo')).toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryAllByText('foo')).toBeDefined()`,
+    `expect(queryByText('foo')).toBeInTheDocument()`
+  ),
+  invalidCase(
+    `expect(queryAllByText('foo')) .not .toBeDefined()`,
+    `expect(queryByText('foo')) .not .toBeInTheDocument()`
+  ),
+  invalidCase(
+    `let foo;
+      foo = screen.queryAllByText('foo');
+      expect(foo).toHaveLength(0);`,
+    `let foo;
+      foo = screen.queryByText('foo');
+      expect(foo).not.toBeInTheDocument();`
+  ),
+  invalidCase(
+    `let foo;
+      foo = screen.queryAllByText('foo');
+      expect(foo) .not.toBeNull();`,
+    `let foo;
+      foo = screen.queryByText('foo');
       expect(foo).toBeInTheDocument();`
-    ),
-    invalidCase(
-      `let foo = screen.${q}('foo');
+  ),
+  invalidCase(
+    `let foo = screen.queryAllByText('foo');
       expect(foo).not.toBeNull();`,
-      `let foo = screen.${q}('foo');
+    `let foo = screen.queryByText('foo');
       expect(foo).toBeInTheDocument();`
-    ),
-  ]),
+  ),
+  //END
   invalidCase(
     `it("foo", async () => {
       expect(await findByRole("button")).toBeDefined();
diff --git a/src/rules/prefer-in-document.js b/src/rules/prefer-in-document.js
@@ -23,26 +23,56 @@ export const meta = {
 function isAntonymMatcher(matcherNode, matcherArguments) {
   return (
     matcherNode.name === "toBeNull" ||
-    (matcherNode.name === "toHaveLength" && matcherArguments[0].value === 0)
+    usesToHaveLengthZero(matcherNode, matcherArguments)
   );
 }
 
+function usesToHaveLengthZero(matcherNode, matcherArguments) {
+  return matcherNode.name === "toHaveLength" && matcherArguments[0].value === 0;
+}
+
 export const create = (context) => {
   const alternativeMatchers = /(toHaveLength|toBeDefined|toBeNull)/;
+  function getLengthValue(matcherArguments) {
+    let lengthValue;
+
+    if (matcherArguments[0].type === "Identifier") {
+      const assignment = getAssignmentForIdentifier(matcherArguments[0].name);
+      if (!assignment) {
+        return;
+      }
+      lengthValue = assignment.value;
+    } else if (matcherArguments[0].type === "Literal") {
+      lengthValue = matcherArguments[0].value;
+    }
+
+    return lengthValue;
+  }
   function check({
     queryNode,
     matcherNode,
     matcherArguments,
     negatedMatcher,
     expect,
   }) {
+    // only report on dom nodes which we can resolve to RTL queries.
     if (!queryNode || (!queryNode.name && !queryNode.property)) return;
+
     // toHaveLength() is only invalid with 0 or 1
-    if (
-      matcherNode.name === "toHaveLength" &&
-      (matcherArguments[0].type !== "Literal" || matcherArguments[0].value > 1)
-    ) {
-      return;
+    if (matcherNode.name === "toHaveLength") {
+      const lengthValue = getLengthValue(matcherArguments);
+      // isNotToHaveLengthZero represents .not.toHaveLength(0) which is a valid use of toHaveLength
+      const isNotToHaveLengthZero =
+        usesToHaveLengthZero(matcherNode, matcherArguments) && negatedMatcher;
+      const isValidUseOfToHaveLength =
+        isNotToHaveLengthZero ||
+        !["Literal", "Identifier"].includes(matcherArguments[0].type) ||
+        lengthValue === undefined ||
+        lengthValue > 1;
+
+      if (isValidUseOfToHaveLength) {
+        return;
+      }
     }
 
     const query = queryNode.name || queryNode.property.name;
@@ -59,18 +89,14 @@ export const create = (context) => {
           for (const argument of Array.from(matcherArguments)) {
             operations.push(fixer.remove(argument));
           }
-          if (
-            matcherNode.name === "toHaveLength" &&
-            matcherArguments[0].value === 1 &&
-            query.indexOf("All") > 0
-          ) {
-            operations.push(
-              fixer.replaceText(
-                queryNode.property || queryNode,
-                query.replace("All", "")
-              )
-            );
-          }
+
+          // AllBy should not be used with toBeInTheDocument
+          operations.push(
+            fixer.replaceText(
+              queryNode.property || queryNode,
+              query.replace("All", "")
+            )
+          );
           // Flip the .not if necessary
           if (isAntonymMatcher(matcherNode, matcherArguments)) {
             if (negatedMatcher) {
@@ -96,36 +122,38 @@ export const create = (context) => {
     }
   }
 
-  function getQueryNodeFrom(expression) {
+  function getAssignmentFrom(expression) {
     return expression.type === "TSAsExpression"
-      ? getQueryNodeFrom(expression.expression)
+      ? getAssignmentFrom(expression.expression)
       : expression.type === "AwaitExpression"
-      ? getQueryNodeFrom(expression.argument)
-      : expression.callee;
+      ? getAssignmentFrom(expression.argument)
+      : expression.callee
+      ? expression.callee
+      : expression;
   }
 
-  function getQueryNodeFromAssignment(identifierName) {
+  function getAssignmentForIdentifier(identifierName) {
     const variable = context.getScope().set.get(identifierName);
+
     if (!variable) return;
     const init = variable.defs[0].node.init;
 
-    let queryNode;
+    let assignmentNode;
     if (init) {
-      // let foo = screen.<query>();
-      queryNode = getQueryNodeFrom(init);
+      // let foo = bar;
+      assignmentNode = getAssignmentFrom(init);
     } else {
       // let foo;
-      // foo = screen.<query>();
+      // foo = bar;
       const assignmentRef = variable.references
         .reverse()
         .find((ref) => !!ref.writeExpr);
       if (!assignmentRef) {
         return;
       }
-      const assignment = assignmentRef.writeExpr;
-      queryNode = getQueryNodeFrom(assignment);
+      assignmentNode = getAssignmentFrom(assignmentRef.writeExpr);
     }
-    return queryNode;
+    return assignmentNode;
   }
   return {
     // expect(<query>).not.<matcher>
@@ -151,7 +179,7 @@ export const create = (context) => {
     [`MemberExpression[object.object.callee.name=expect][object.property.name=not][property.name=${alternativeMatchers}][object.object.arguments.0.type=Identifier]`](
       node
     ) {
-      const queryNode = getQueryNodeFromAssignment(
+      const queryNode = getAssignmentForIdentifier(
         node.object.object.arguments[0].name
       );
       const matcherNode = node.property;
@@ -172,7 +200,7 @@ export const create = (context) => {
     [`MemberExpression[object.callee.name=expect][property.name=${alternativeMatchers}][object.arguments.0.type=Identifier]`](
       node
     ) {
-      const queryNode = getQueryNodeFromAssignment(
+      const queryNode = getAssignmentForIdentifier(
         node.object.arguments[0].name
       );
       const matcherNode = node.property;
