refactor(render-result-naming-convention): refine checks to decide if coming from Testing Library (#282)

* feat(render-result-naming-convention): detect render calls from wrappers

* fix: check imported node properly when specifiers are renamed

ImportNamespaceSpecifier had to be checked properly in order to detect rename imports properly like:
import { a as b } from 'foo'

* refactor: split checks for import matching node name in different methods

* test(render-result-naming-convention): add extra invalid case for wrapped function

* fix(render-result-naming-convention): cover more renamed imports

Author: Belco90

lib/detect-testing-library-utils.ts

@@ -8,6 +8,7 @@ import {
   getImportModuleName,
   getPropertyIdentifierNode,
   getReferenceNode,
+  hasImportMatch,
   ImportModuleNode,
   isImportDeclaration,
   isImportNamespaceSpecifier,
@@ -127,22 +128,41 @@ export function detectTestingLibraryUtils<
     /**
      * Small method to extract common checks to determine whether a node is
      * related to Testing Library or not.
+     *
+     * To determine whether a node is a valid Testing Library util, there are
+     * two conditions to match:
+     * - it's named in a particular way (decided by given callback)
+     * - it's imported from valid Testing Library module (depends on aggressive
+     *    reporting)
      */
     function isTestingLibraryUtil(
       node: TSESTree.Identifier,
-      isUtilCallback: (identifierNode: TSESTree.Identifier) => boolean
+      isUtilCallback: (
+        identifierNodeName: string,
+        originalNodeName?: string
+      ) => boolean
     ): boolean {
-      if (!isUtilCallback(node)) {
+      const referenceNode = getReferenceNode(node);
+      const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode);
+      const importedUtilSpecifier = getImportedUtilSpecifier(
+        referenceNodeIdentifier
+      );
+
+      const originalNodeName =
+        isImportSpecifier(importedUtilSpecifier) &&
+        importedUtilSpecifier.local.name !== importedUtilSpecifier.imported.name
+          ? importedUtilSpecifier.imported.name
+          : undefined;
+
+      if (!isUtilCallback(node.name, originalNodeName)) {
         return false;
       }
 
-      const referenceNode = getReferenceNode(node);
-      const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode);
+      if (isAggressiveModuleReportingEnabled()) {
+        return true;
+      }
 
-      return (
-        isAggressiveModuleReportingEnabled() ||
-        isNodeComingFromTestingLibrary(referenceNodeIdentifier)
-      );
+      return isNodeComingFromTestingLibrary(referenceNodeIdentifier);
     }
 
     /**
@@ -272,8 +292,8 @@ export function detectTestingLibraryUtils<
      * coming from Testing Library will be considered as valid.
      */
     const isAsyncUtil: IsAsyncUtilFn = (node) => {
-      return isTestingLibraryUtil(node, (identifierNode) =>
-        ASYNC_UTILS.includes(identifierNode.name)
+      return isTestingLibraryUtil(node, (identifierNodeName) =>
+        ASYNC_UTILS.includes(identifierNodeName)
       );
     };
 
@@ -347,13 +367,27 @@ export function detectTestingLibraryUtils<
      * only those nodes coming from Testing Library will be considered as valid.
      */
     const isRenderUtil: IsRenderUtilFn = (node) => {
-      return isTestingLibraryUtil(node, (identifierNode) => {
-        if (isAggressiveRenderReportingEnabled()) {
-          return identifierNode.name.toLowerCase().includes(RENDER_NAME);
+      return isTestingLibraryUtil(
+        node,
+        (identifierNodeName, originalNodeName) => {
+          if (isAggressiveRenderReportingEnabled()) {
+            return identifierNodeName.toLowerCase().includes(RENDER_NAME);
+          }
+
+          return [RENDER_NAME, ...customRenders].some((validRenderName) => {
+            let isMatch = false;
+
+            if (validRenderName === identifierNodeName) {
+              isMatch = true;
+            }
+
+            if (!!originalNodeName && validRenderName === originalNodeName) {
+              isMatch = true;
+            }
+            return isMatch;
+          });
         }
-
-        return [RENDER_NAME, ...customRenders].includes(identifierNode.name);
-      });
+      );
     };
 
     /**
@@ -402,25 +436,34 @@ export function detectTestingLibraryUtils<
       specifierName
     ) => {
       const node = getCustomModuleImportNode() ?? getTestingLibraryImportNode();
+
       if (!node) {
         return null;
       }
+
       if (isImportDeclaration(node)) {
-        const namedExport = node.specifiers.find(
-          (n) => isImportSpecifier(n) && n.imported.name === specifierName
-        );
+        const namedExport = node.specifiers.find((n) => {
+          return (
+            isImportSpecifier(n) &&
+            [n.imported.name, n.local.name].includes(specifierName)
+          );
+        });
+
         // it is "import { foo [as alias] } from 'baz'""
         if (namedExport) {
           return namedExport;
         }
+
         // it could be "import * as rtl from 'baz'"
         return node.specifiers.find((n) => isImportNamespaceSpecifier(n));
       } else {
         const requireNode = node.parent as TSESTree.VariableDeclarator;
+
         if (ASTUtils.isIdentifier(requireNode.id)) {
           // this is const rtl = require('foo')
           return requireNode.id;
         }
+
         // this should be const { something } = require('foo')
         const destructuring = requireNode.id as TSESTree.ObjectPattern;
         const property = destructuring.properties.find(
@@ -429,27 +472,48 @@ export function detectTestingLibraryUtils<
             ASTUtils.isIdentifier(n.key) &&
             n.key.name === specifierName
         );
+        if (!property) {
+          return undefined;
+        }
         return (property as TSESTree.Property).key as TSESTree.Identifier;
       }
     };
 
+    const getImportedUtilSpecifier = (
+      node: TSESTree.MemberExpression | TSESTree.Identifier
+    ): TSESTree.ImportClause | TSESTree.Identifier | undefined => {
+      const identifierName: string | undefined = getPropertyIdentifierNode(node)
+        .name;
+
+      return findImportedUtilSpecifier(identifierName);
+    };
+
     /**
      * Determines if file inspected meets all conditions to be reported by rules or not.
      */
     const canReportErrors: CanReportErrorsFn = () => {
       return isTestingLibraryImported() && isValidFilename();
     };
+
     /**
-     * Takes a MemberExpression or an Identifier and verifies if its name comes from the import in TL
-     * @param node a MemberExpression (in "foo.property" it would be property) or an Identifier
+     * Determines whether a node is imported from a valid Testing Library module
+     *
+     * This method will try to find any import matching the given node name,
+     * and also make sure the name is a valid match in case it's been renamed.
      */
     const isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn = (
       node
     ) => {
+      const importNode = getImportedUtilSpecifier(node);
+
+      if (!importNode) {
+        return false;
+      }
+
       const identifierName: string | undefined = getPropertyIdentifierNode(node)
         .name;
 
-      return !!findImportedUtilSpecifier(identifierName);
+      return hasImportMatch(importNode, identifierName);
     };
 
     const helpers: DetectionHelpers = {

lib/node-utils.ts

@@ -620,3 +620,14 @@ export function getInnermostReturningFunction(
 
   return functionScope.block;
 }
+
+export function hasImportMatch(
+  importNode: TSESTree.ImportClause | TSESTree.Identifier,
+  identifierName: string
+): boolean {
+  if (ASTUtils.isIdentifier(importNode)) {
+    return importNode.name === identifierName;
+  }
+
+  return importNode.local.name === identifierName;
+}

lib/rules/render-result-naming-convention.ts

@@ -1,6 +1,11 @@
 import { createTestingLibraryRule } from '../create-testing-library-rule';
-import { getDeepestIdentifierNode, isObjectPattern } from '../node-utils';
-import { ASTUtils } from '@typescript-eslint/experimental-utils';
+import {
+  getDeepestIdentifierNode,
+  getFunctionName,
+  getInnermostReturningFunction,
+  isObjectPattern,
+} from '../node-utils';
+import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils';
 
 export const RULE_NAME = 'render-result-naming-convention';
 export type MessageIds = 'renderResultNamingConvention';
@@ -30,15 +35,33 @@ export default createTestingLibraryRule<Options, MessageIds>({
   defaultOptions: [],
 
   create(context, _, helpers) {
+    const renderWrapperNames: string[] = [];
+
+    function detectRenderWrapper(node: TSESTree.Identifier): void {
+      const innerFunction = getInnermostReturningFunction(context, node);
+
+      if (innerFunction) {
+        renderWrapperNames.push(getFunctionName(innerFunction));
+      }
+    }
+
     return {
+      'CallExpression Identifier'(node: TSESTree.Identifier) {
+        if (helpers.isRenderUtil(node)) {
+          detectRenderWrapper(node);
+        }
+      },
       VariableDeclarator(node) {
         const initIdentifierNode = getDeepestIdentifierNode(node.init);
 
         if (!initIdentifierNode) {
           return;
         }
 
-        if (!helpers.isRenderUtil(initIdentifierNode)) {
+        if (
+          !helpers.isRenderUtil(initIdentifierNode) &&
+          !renderWrapperNames.includes(initIdentifierNode.name)
+        ) {
           return;
         }
 

tests/create-testing-library-rule.test.ts

@@ -126,6 +126,19 @@ ruleTester.run(RULE_NAME, rule, {
       const utils = render()
       `,
     },
+    {
+      settings: {
+        'testing-library/utils-module': 'test-utils',
+      },
+      code: `
+      // case (render util): aggressive reporting disabled - method with same name
+      // as TL method but not coming from TL module is valid
+      import { render as testingLibraryRender } from 'test-utils'
+      import { render } from 'somewhere-else'
+
+      const utils = render()
+      `,
+    },
 
     // Test Cases for presence/absence assertions
     // cases: asserts not related to presence/absence
@@ -287,6 +300,21 @@ ruleTester.run(RULE_NAME, rule, {
       waitFor()
       `,
     },
+    {
+      settings: {
+        'testing-library/utils-module': 'test-utils',
+      },
+      code: `
+      // case (async util): aggressive reporting disabled - method with same name
+      // as TL method but not coming from TL module is valid
+      import { waitFor as testingLibraryWaitFor } from 'test-utils'
+      import { waitFor } from 'somewhere-else'
+
+        test('this should not be reported', () => {
+        waitFor()
+      });
+      `,
+    },
 
     // Test Cases for all settings mixed
     {
@@ -642,26 +670,6 @@ ruleTester.run(RULE_NAME, rule, {
         },
       ],
     },
-    {
-      settings: {
-        'testing-library/utils-module': 'test-utils',
-      },
-      code: `
-        // case: waitFor from object property shadowed name is checked correctly
-        import * as tl from 'test-utils'
-        const obj = { tl }
-        
-        obj.module.waitFor(() => {})
-      `,
-      errors: [
-        {
-          line: 6,
-          column: 20,
-          messageId: 'asyncUtilError',
-          data: { utilName: 'waitFor' },
-        },
-      ],
-    },
     {
       settings: {
         'testing-library/utils-module': 'test-utils',

tests/lib/rules/prefer-wait-for.test.ts

@@ -218,6 +218,21 @@ ruleTester.run(RULE_NAME, rule, {
       cy.wait();
       `,
     },
+    {
+      settings: {
+        'testing-library/utils-module': 'test-utils',
+      },
+      code: `
+      // case: aggressive reporting disabled - method named same as invalid method
+      // but not coming from Testing Library is valid
+      import { wait as testingLibraryWait } from 'test-utils'
+      import { wait } from 'somewhere-else'
+      
+      async () => {
+        await wait();
+      }
+      `,
+    },
     {
       // https://github.com/testing-library/eslint-plugin-testing-library/issues/145
       code: `import * as foo from 'imNoTestingLibrary';