refactor: improve logic to detect if testing librar imported (#239)

Belco90 · web-flow · commit 63c9d7b3048e · 2020-10-25T20:21:02.000+01:00
* refactor: check testing library imports automatically

* feat: add new shared setting for testing library module

* test: increase coverage for create testing library rule

* refactor: extract common enhanced rule create types

* docs: add jsdoc to detection helpers

* docs: update old comments related to ImportDeclaration

* test: check rule listener are combined properly
diff --git a/jest.config.js b/jest.config.js
@@ -11,12 +11,6 @@ module.exports = {
       statements: 100,
     },
     // TODO drop this custom threshold in v4
-    './lib/detect-testing-library-utils.ts': {
-      branches: 50,
-      functions: 90,
-      lines: 90,
-      statements: 90,
-    },
     './lib/node-utils.ts': {
       branches: 90,
       functions: 90,
diff --git a/lib/create-testing-library-rule.ts b/lib/create-testing-library-rule.ts
@@ -2,7 +2,7 @@ import { ESLintUtils, TSESLint } from '@typescript-eslint/experimental-utils';
 import { getDocsUrl } from './utils';
 import {
   detectTestingLibraryUtils,
-  DetectionHelpers,
+  EnhancedRuleCreate,
 } from './detect-testing-library-utils';
 
 // These 2 types are copied from @typescript-eslint/experimental-utils
@@ -20,13 +20,9 @@ export function createTestingLibraryRule<
     name: string;
     meta: CreateRuleMeta<TMessageIds>;
     defaultOptions: Readonly<TOptions>;
-    create: (
-      context: Readonly<TSESLint.RuleContext<TMessageIds, TOptions>>,
-      optionsWithDefault: Readonly<TOptions>,
-      detectionHelpers: Readonly<DetectionHelpers>
-    ) => TRuleListener;
+    create: EnhancedRuleCreate<TOptions, TMessageIds, TRuleListener>;
   }>
-): TSESLint.RuleModule<TMessageIds, TOptions, TRuleListener> {
+): TSESLint.RuleModule<TMessageIds, TOptions> {
   const { create, ...remainingConfig } = config;
 
   return ESLintUtils.RuleCreator(getDocsUrl)({
diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts
@@ -1,7 +1,31 @@
 import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
 
+export type TestingLibrarySettings = {
+  'testing-library/module'?: string;
+};
+
+export type TestingLibraryContext<
+  TOptions extends readonly unknown[],
+  TMessageIds extends string
+> = Readonly<
+  TSESLint.RuleContext<TMessageIds, TOptions> & {
+    settings: TestingLibrarySettings;
+  }
+>;
+
+export type EnhancedRuleCreate<
+  TOptions extends readonly unknown[],
+  TMessageIds extends string,
+  TRuleListener extends TSESLint.RuleListener = TSESLint.RuleListener
+> = (
+  context: TestingLibraryContext<TOptions, TMessageIds>,
+  optionsWithDefault: Readonly<TOptions>,
+  detectionHelpers: Readonly<DetectionHelpers>
+) => TRuleListener;
+
 export type DetectionHelpers = {
-  getIsImportingTestingLibrary: () => boolean;
+  getIsTestingLibraryImported: () => boolean;
+  canReportErrors: () => boolean;
 };
 
 /**
@@ -11,50 +35,91 @@ export function detectTestingLibraryUtils<
   TOptions extends readonly unknown[],
   TMessageIds extends string,
   TRuleListener extends TSESLint.RuleListener = TSESLint.RuleListener
->(
-  ruleCreate: (
-    context: Readonly<TSESLint.RuleContext<TMessageIds, TOptions>>,
-    optionsWithDefault: Readonly<TOptions>,
-    detectionHelpers: Readonly<DetectionHelpers>
-  ) => TRuleListener
-) {
+>(ruleCreate: EnhancedRuleCreate<TOptions, TMessageIds, TRuleListener>) {
   return (
-    context: Readonly<TSESLint.RuleContext<TMessageIds, TOptions>>,
+    context: TestingLibraryContext<TOptions, TMessageIds>,
     optionsWithDefault: Readonly<TOptions>
-  ): TRuleListener => {
-    let isImportingTestingLibrary = false;
+  ): TSESLint.RuleListener => {
+    let isImportingTestingLibraryModule = false;
+    let isImportingCustomModule = false;
 
-    // TODO: init here options based on shared ESLint config
+    // Init options based on shared ESLint settings
+    const customModule = context.settings['testing-library/module'];
 
-    // helpers for Testing Library detection
+    // Helpers for Testing Library detection.
     const helpers: DetectionHelpers = {
-      getIsImportingTestingLibrary() {
-        return isImportingTestingLibrary;
+      /**
+       * Gets if Testing Library is considered as imported or not.
+       *
+       * By default, it is ALWAYS considered as imported. This is what we call
+       * "aggressive reporting" so we don't miss TL utils reexported from
+       * custom modules.
+       *
+       * However, there is a setting to customize the module where TL utils can
+       * be imported from: "testing-library/module". If this setting is enabled,
+       * then this method will return `true` ONLY IF a testing-library package
+       * or custom module are imported.
+       */
+      getIsTestingLibraryImported() {
+        if (!customModule) {
+          return true;
+        }
+
+        return isImportingTestingLibraryModule || isImportingCustomModule;
+      },
+
+      /**
+       * Wraps all conditions that must be met to report rules.
+       */
+      canReportErrors() {
+        return this.getIsTestingLibraryImported();
       },
     };
 
-    // instructions for Testing Library detection
+    // Instructions for Testing Library detection.
     const detectionInstructions: TSESLint.RuleListener = {
+      /**
+       * This ImportDeclaration rule listener will check if Testing Library related
+       * modules are loaded. Since imports happen first thing in a file, it's
+       * safe to use `isImportingTestingLibraryModule` and `isImportingCustomModule`
+       * since they will have corresponding value already updated when reporting other
+       * parts of the file.
+       */
       ImportDeclaration(node: TSESTree.ImportDeclaration) {
-        isImportingTestingLibrary = /testing-library/g.test(
-          node.source.value as string
-        );
+        if (!isImportingTestingLibraryModule) {
+          // check only if testing library import not found yet so we avoid
+          // to override isImportingTestingLibraryModule after it's found
+          isImportingTestingLibraryModule = /testing-library/g.test(
+            node.source.value as string
+          );
+        }
+
+        if (!isImportingCustomModule) {
+          // check only if custom module import not found yet so we avoid
+          // to override isImportingCustomModule after it's found
+          const importName = String(node.source.value);
+          isImportingCustomModule = importName.endsWith(customModule);
+        }
       },
     };
 
     // update given rule to inject Testing Library detection
     const ruleInstructions = ruleCreate(context, optionsWithDefault, helpers);
-    const enhancedRuleInstructions = Object.assign({}, ruleInstructions);
+    const enhancedRuleInstructions: TSESLint.RuleListener = {};
+
+    const allKeys = new Set(
+      Object.keys(detectionInstructions).concat(Object.keys(ruleInstructions))
+    );
 
-    Object.keys(detectionInstructions).forEach((instruction) => {
-      (enhancedRuleInstructions as TSESLint.RuleListener)[instruction] = (
-        node
-      ) => {
+    // Iterate over ALL instructions keys so we can override original rule instructions
+    // to prevent their execution if conditions to report errors are not met.
+    allKeys.forEach((instruction) => {
+      enhancedRuleInstructions[instruction] = (node) => {
         if (instruction in detectionInstructions) {
           detectionInstructions[instruction](node);
         }
 
-        if (ruleInstructions[instruction]) {
+        if (helpers.canReportErrors() && ruleInstructions[instruction]) {
           return ruleInstructions[instruction](node);
         }
       };
diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts
@@ -25,11 +25,10 @@ export default createTestingLibraryRule<Options, MessageIds>({
   },
   defaultOptions: [],
 
-  create: (context, _, helpers) => {
+  create(context) {
     function showErrorForNodeAccess(node: TSESTree.MemberExpression) {
       isIdentifier(node.property) &&
         ALL_RETURNING_NODES.includes(node.property.name) &&
-        helpers.getIsImportingTestingLibrary() &&
         context.report({
           node: node,
           loc: node.property.loc.start,
diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts
@@ -0,0 +1,122 @@
+import { createRuleTester } from './lib/test-utils';
+import rule, { RULE_NAME } from './fake-rule';
+
+const ruleTester = createRuleTester({
+  ecmaFeatures: {
+    jsx: true,
+  },
+});
+
+ruleTester.run(RULE_NAME, rule, {
+  valid: [
+    {
+      code: `
+      // case: nothing related to Testing Library at all
+      import { shallow } from 'enzyme';
+      
+      const wrapper = shallow(<MyComponent />);
+      `,
+    },
+    {
+      code: `
+      // case: render imported from other than custom module
+      import { render } from '@somewhere/else'
+      
+      const utils = render();
+      `,
+      settings: {
+        'testing-library/module': 'test-utils',
+      },
+    },
+    {
+      code: `
+      // case: prevent import which should trigger an error since it's imported
+      // from other than custom module
+      import { foo } from 'report-me'
+      `,
+      settings: {
+        'testing-library/module': 'test-utils',
+      },
+    },
+  ],
+  invalid: [
+    {
+      code: `
+      // case: import module forced to be reported
+      import { foo } from 'report-me'
+    `,
+      errors: [{ line: 3, column: 7, messageId: 'fakeError' }],
+    },
+    {
+      code: `
+      // case: render imported from any module by default (aggressive reporting)
+      import { render } from '@somewhere/else'
+      import { somethingElse } from 'another-module'
+      
+      const utils = render();
+      `,
+      errors: [
+        {
+          line: 6,
+          column: 21,
+          messageId: 'fakeError',
+        },
+      ],
+    },
+    {
+      code: `
+      // case: render imported from Testing Library module
+      import { render } from '@testing-library/react'
+      import { somethingElse } from 'another-module'
+      
+      const utils = render();
+      `,
+      errors: [
+        {
+          line: 6,
+          column: 21,
+          messageId: 'fakeError',
+        },
+      ],
+    },
+    {
+      code: `
+      // case: render imported from config custom module
+      import { render } from 'test-utils'
+      import { somethingElse } from 'another-module'
+      
+      const utils = render();
+      `,
+      settings: {
+        'testing-library/module': 'test-utils',
+      },
+      errors: [
+        {
+          line: 6,
+          column: 21,
+          messageId: 'fakeError',
+        },
+      ],
+    },
+    {
+      code: `
+      // case: render imported from Testing Library module if
+      // custom module setup
+      import { render } from '@testing-library/react'
+      import { somethingElse } from 'another-module'
+      
+      const utils = render();
+      `,
+      settings: {
+        'testing-library/module': 'test-utils',
+      },
+      errors: [
+        {
+          line: 7,
+          column: 21,
+          messageId: 'fakeError',
+        },
+      ],
+    },
+  ],
+});
diff --git a/tests/fake-rule.ts b/tests/fake-rule.ts
@@ -0,0 +1,55 @@
+/**
+ * @file Fake rule to be able to test createTestingLibraryRule and
+ * detectTestingLibraryUtils properly
+ */
+import { TSESTree } from '@typescript-eslint/experimental-utils';
+import { createTestingLibraryRule } from '../lib/create-testing-library-rule';
+
+export const RULE_NAME = 'fake-rule';
+type Options = [];
+type MessageIds = 'fakeError';
+
+export default createTestingLibraryRule<Options, MessageIds>({
+  name: RULE_NAME,
+  meta: {
+    type: 'problem',
+    docs: {
+      description: 'Fake rule to test rule maker and detection helpers',
+      category: 'Possible Errors',
+      recommended: false,
+    },
+    messages: {
+      fakeError: 'fake error reported',
+    },
+    fixable: null,
+    schema: [],
+  },
+  defaultOptions: [],
+  create(context) {
+    const reportRenderIdentifier = (node: TSESTree.Identifier) => {
+      if (node.name === 'render') {
+        context.report({
+          node,
+          messageId: 'fakeError',
+        });
+      }
+    };
+
+    const checkImportDeclaration = (node: TSESTree.ImportDeclaration) => {
+      // This is just to check that defining an `ImportDeclaration` doesn't
+      // override `ImportDeclaration` from `detectTestingLibraryUtils`
+
+      if (node.source.value === 'report-me') {
+        context.report({
+          node,
+          messageId: 'fakeError',
+        });
+      }
+    };
+
+    return {
+      'CallExpression Identifier': reportRenderIdentifier,
+      ImportDeclaration: checkImportDeclaration,
+    };
+  },
+});
diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts
