refactor(prefer-presence-queries): use custom rule creator (#252)

Belco90 · web-flow · commit b48b28647505 · 2020-11-10T09:25:17.000+01:00
* test(prefer-presence-queries): improve existing invalid tests

* refactor(prefer-presence-queries): use custom rule creator

* feat(prefer-presence-queries): use aggressive query reporting

* refactor(prefer-presence-queries): rename message ids

* test: add fake rule tests for queries

* refactor(extract helpers for detecting presence/absence assets): add fake rule tests for queries

* refactor(prefer-presence-queries): use presence/absence helpers

* refactor: simplify negated matcher condition

* style: format files after rebase
diff --git a/.lintstagedrc b/.lintstagedrc
@@ -1,6 +1,6 @@
 {
   "*.{js,ts}": [
-    "eslint --fix",
+    "eslint --max-warnings 0 --fix",
     "prettier --write",
     "jest --findRelatedTests"
   ],
diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts
@@ -1,14 +1,19 @@
-import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
+import {
+  ASTUtils,
+  TSESLint,
+  TSESTree,
+} from '@typescript-eslint/experimental-utils';
 import {
   getImportModuleName,
+  getAssertNodeInfo,
   isLiteral,
   ImportModuleNode,
   isImportDeclaration,
   isImportNamespaceSpecifier,
   isImportSpecifier,
-  isIdentifier,
   isProperty,
 } from './node-utils';
+import { ABSENCE_MATCHERS, PRESENCE_MATCHERS } from './utils';
 
 export type TestingLibrarySettings = {
   'testing-library/module'?: string;
@@ -41,6 +46,11 @@ export type DetectionHelpers = {
   getCustomModuleImportName: () => string | undefined;
   getIsTestingLibraryImported: () => boolean;
   getIsValidFilename: () => boolean;
+  isGetByQuery: (node: TSESTree.Identifier) => boolean;
+  isQueryByQuery: (node: TSESTree.Identifier) => boolean;
+  isSyncQuery: (node: TSESTree.Identifier) => boolean;
+  isPresenceAssert: (node: TSESTree.MemberExpression) => boolean;
+  isAbsenceAssert: (node: TSESTree.MemberExpression) => boolean;
   canReportErrors: () => boolean;
   findImportedUtilSpecifier: (
     specifierName: string
@@ -85,7 +95,8 @@ export function detectTestingLibraryUtils<
         return getImportModuleName(importedCustomModuleNode);
       },
       /**
-       * Gets if Testing Library is considered as imported or not.
+       * Determines whether Testing Library utils are imported or not for
+       * current file being analyzed.
        *
        * By default, it is ALWAYS considered as imported. This is what we call
        * "aggressive reporting" so we don't miss TL utils reexported from
@@ -105,17 +116,75 @@ export function detectTestingLibraryUtils<
       },
 
       /**
-       * Gets if filename being analyzed is valid or not.
-       *
-       * This is based on "testing-library/filename-pattern" setting.
+       * Determines whether filename is valid or not for current file
+       * being analyzed based on "testing-library/filename-pattern" setting.
        */
       getIsValidFilename() {
         const fileName = context.getFilename();
         return !!fileName.match(filenamePattern);
       },
 
       /**
-       * Wraps all conditions that must be met to report rules.
+       * Determines whether a given node is `getBy*` or `getAllBy*` query variant or not.
+       */
+      isGetByQuery(node) {
+        return !!node.name.match(/^get(All)?By.+$/);
+      },
+
+      /**
+       * Determines whether a given node is `queryBy*` or `queryAllBy*` query variant or not.
+       */
+      isQueryByQuery(node) {
+        return !!node.name.match(/^query(All)?By.+$/);
+      },
+
+      /**
+       * Determines whether a given node is sync query or not.
+       */
+      isSyncQuery(node) {
+        return this.isGetByQuery(node) || this.isQueryByQuery(node);
+      },
+
+      /**
+       * Determines whether a given MemberExpression node is a presence assert
+       *
+       * Presence asserts could have shape of:
+       *  - expect(element).toBeInTheDocument()
+       *  - expect(element).not.toBeNull()
+       */
+      isPresenceAssert(node) {
+        const { matcher, isNegated } = getAssertNodeInfo(node);
+
+        if (!matcher) {
+          return false;
+        }
+
+        return isNegated
+          ? ABSENCE_MATCHERS.includes(matcher)
+          : PRESENCE_MATCHERS.includes(matcher);
+      },
+
+      /**
+       * Determines whether a given MemberExpression node is an absence assert
+       *
+       * Absence asserts could have shape of:
+       *  - expect(element).toBeNull()
+       *  - expect(element).not.toBeInTheDocument()
+       */
+      isAbsenceAssert(node) {
+        const { matcher, isNegated } = getAssertNodeInfo(node);
+
+        if (!matcher) {
+          return false;
+        }
+
+        return isNegated
+          ? PRESENCE_MATCHERS.includes(matcher)
+          : ABSENCE_MATCHERS.includes(matcher);
+      },
+
+      /**
+       * Determines if file inspected meets all conditions to be reported by rules or not.
        */
       canReportErrors() {
         return (
@@ -144,7 +213,7 @@ export function detectTestingLibraryUtils<
           return node.specifiers.find((n) => isImportNamespaceSpecifier(n));
         } else {
           const requireNode = node.parent as TSESTree.VariableDeclarator;
-          if (isIdentifier(requireNode.id)) {
+          if (ASTUtils.isIdentifier(requireNode.id)) {
             // this is const rtl = require('foo')
             return requireNode.id;
           }
@@ -153,7 +222,7 @@ export function detectTestingLibraryUtils<
           const property = destructuring.properties.find(
             (n) =>
               isProperty(n) &&
-              isIdentifier(n.key) &&
+              ASTUtils.isIdentifier(n.key) &&
               n.key.name === specifierName
           );
           return (property as TSESTree.Property).key as TSESTree.Identifier;
diff --git a/lib/node-utils.ts b/lib/node-utils.ts
@@ -1,5 +1,6 @@
 import {
   AST_NODE_TYPES,
+  ASTUtils,
   TSESLint,
   TSESTree,
 } from '@typescript-eslint/experimental-utils';
@@ -253,3 +254,41 @@ export function getImportModuleName(
     return node.arguments[0].value;
   }
 }
+
+type AssertNodeInfo = {
+  matcher: string | null;
+  isNegated: boolean;
+};
+/**
+ * Extracts matcher info from MemberExpression node representing an assert.
+ */
+export function getAssertNodeInfo(
+  node: TSESTree.MemberExpression
+): AssertNodeInfo {
+  const emptyInfo = { matcher: null, isNegated: false } as AssertNodeInfo;
+
+  if (
+    !isCallExpression(node.object) ||
+    !ASTUtils.isIdentifier(node.object.callee)
+  ) {
+    return emptyInfo;
+  }
+
+  if (node.object.callee.name !== 'expect') {
+    return emptyInfo;
+  }
+
+  let matcher = ASTUtils.getPropertyName(node);
+  const isNegated = matcher === 'not';
+  if (isNegated) {
+    matcher = isMemberExpression(node.parent)
+      ? ASTUtils.getPropertyName(node.parent)
+      : null;
+  }
+
+  if (!matcher) {
+    return emptyInfo;
+  }
+
+  return { matcher, isNegated };
+}
diff --git a/lib/rules/prefer-presence-queries.ts b/lib/rules/prefer-presence-queries.ts
@@ -1,29 +1,12 @@
-import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
-import {
-  getDocsUrl,
-  ALL_QUERIES_METHODS,
-  PRESENCE_MATCHERS,
-  ABSENCE_MATCHERS,
-} from '../utils';
-import {
-  findClosestCallNode,
-  isMemberExpression,
-  isIdentifier,
-} from '../node-utils';
+import { TSESTree } from '@typescript-eslint/experimental-utils';
+import { findClosestCallNode, isMemberExpression } from '../node-utils';
+import { createTestingLibraryRule } from '../create-testing-library-rule';
 
 export const RULE_NAME = 'prefer-presence-queries';
-export type MessageIds = 'presenceQuery' | 'absenceQuery' | 'expectQueryBy';
+export type MessageIds = 'wrongPresenceQuery' | 'wrongAbsenceQuery';
 type Options = [];
 
-const QUERIES_REGEXP = new RegExp(
-  `^(get|query)(All)?(${ALL_QUERIES_METHODS.join('|')})$`
-);
-
-function isThrowingQuery(node: TSESTree.Identifier) {
-  return node.name.startsWith('get');
-}
-
-export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
+export default createTestingLibraryRule<Options, MessageIds>({
   name: RULE_NAME,
   meta: {
     docs: {
@@ -33,62 +16,47 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
       recommended: 'error',
     },
     messages: {
-      presenceQuery:
+      wrongPresenceQuery:
         'Use `getBy*` queries rather than `queryBy*` for checking element is present',
-      absenceQuery:
+      wrongAbsenceQuery:
         'Use `queryBy*` queries rather than `getBy*` for checking element is NOT present',
-      expectQueryBy:
-        'Use `getBy*` only when checking elements are present, otherwise use `queryBy*`',
     },
     schema: [],
     type: 'suggestion',
     fixable: null,
   },
   defaultOptions: [],
 
-  create(context) {
+  create(context, _, helpers) {
     return {
-      [`CallExpression Identifier[name=${QUERIES_REGEXP}]`](
-        node: TSESTree.Identifier
-      ) {
+      'CallExpression Identifier'(node: TSESTree.Identifier) {
         const expectCallNode = findClosestCallNode(node, 'expect');
 
-        if (expectCallNode && isMemberExpression(expectCallNode.parent)) {
-          const expectStatement = expectCallNode.parent;
-          const property = expectStatement.property as TSESTree.Identifier;
-          let matcher = property.name;
-          let isNegatedMatcher = false;
+        if (!expectCallNode || !isMemberExpression(expectCallNode.parent)) {
+          return;
+        }
 
-          if (
-            matcher === 'not' &&
-            isMemberExpression(expectStatement.parent) &&
-            isIdentifier(expectStatement.parent.property)
-          ) {
-            isNegatedMatcher = true;
-            matcher = expectStatement.parent.property.name;
-          }
+        // Sync queries (getBy and queryBy) are corresponding ones used
+        // to check presence or absence. If none found, stop the rule.
+        if (!helpers.isSyncQuery(node)) {
+          return;
+        }
 
-          const validMatchers = isThrowingQuery(node)
-            ? PRESENCE_MATCHERS
-            : ABSENCE_MATCHERS;
+        const isPresenceQuery = helpers.isGetByQuery(node);
+        const expectStatement = expectCallNode.parent;
+        const isPresenceAssert = helpers.isPresenceAssert(expectStatement);
+        const isAbsenceAssert = helpers.isAbsenceAssert(expectStatement);
 
-          const invalidMatchers = isThrowingQuery(node)
-            ? ABSENCE_MATCHERS
-            : PRESENCE_MATCHERS;
+        if (!isPresenceAssert && !isAbsenceAssert) {
+          return;
+        }
 
-          const messageId = isThrowingQuery(node)
-            ? 'absenceQuery'
-            : 'presenceQuery';
+        if (isPresenceAssert && !isPresenceQuery) {
+          return context.report({ node, messageId: 'wrongPresenceQuery' });
+        }
 
-          if (
-            (!isNegatedMatcher && invalidMatchers.includes(matcher)) ||
-            (isNegatedMatcher && validMatchers.includes(matcher))
-          ) {
-            return context.report({
-              node,
-              messageId,
-            });
-          }
+        if (isAbsenceAssert && isPresenceQuery) {
+          return context.report({ node, messageId: 'wrongAbsenceQuery' });
         }
       },
     };
diff --git a/lib/utils.ts b/lib/utils.ts
@@ -24,6 +24,7 @@ const LIBRARY_MODULES = [
   '@testing-library/svelte',
 ];
 
+// TODO: should be deleted after all rules are migrated to v4
 const hasTestingLibraryImportModule = (
   node: TSESTree.ImportDeclaration
 ): boolean => {
diff --git a/package.json b/package.json
@@ -27,7 +27,7 @@
   "scripts": {
     "build": "tsc",
     "postbuild": "cpy README.md ./dist && cpy package.json ./dist && cpy LICENSE ./dist",
-    "lint": "eslint . --ext .js,.ts",
+    "lint": "eslint . --max-warnings 0 --ext .js,.ts",
     "lint:fix": "npm run lint -- --fix",
     "format": "prettier --write README.md \"{lib,docs,tests}/**/*.{js,ts,md}\"",
     "format:check": "prettier --check README.md \"{lib,docs,tests}/**/*.{js,json,yml,ts,md}\"",
diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts
diff --git a/tests/fake-rule.ts b/tests/fake-rule.ts
diff --git a/tests/lib/rules/prefer-presence-queries.test.ts b/tests/lib/rules/prefer-presence-queries.test.ts

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"*.{js,ts}": [`
`3`		`- "eslint --fix",`
	`3`	`+ "eslint --max-warnings 0 --fix",`
`4`	`4`	`"prettier --write",`
`5`	`5`	`"jest --findRelatedTests"`
`6`	`6`	`],`