From d2844a1a6727b83b4097f8118b0247e5a28d0e8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Tue, 20 Oct 2020 18:54:09 +0200 Subject: [PATCH 1/7] refactor: check testing library imports automatically --- lib/create-testing-library-rule.ts | 2 +- lib/detect-testing-library-utils.ts | 34 ++++++++++++------ lib/rules/no-node-access.ts | 3 +- tests/create-testing-library-rule.test.ts | 34 ++++++++++++++++++ tests/fake-rule.ts | 42 +++++++++++++++++++++++ 5 files changed, 101 insertions(+), 14 deletions(-) create mode 100644 tests/create-testing-library-rule.test.ts create mode 100644 tests/fake-rule.ts diff --git a/lib/create-testing-library-rule.ts b/lib/create-testing-library-rule.ts index a34d9fef..25ee809a 100644 --- a/lib/create-testing-library-rule.ts +++ b/lib/create-testing-library-rule.ts @@ -26,7 +26,7 @@ export function createTestingLibraryRule< detectionHelpers: Readonly ) => TRuleListener; }> -): TSESLint.RuleModule { +): TSESLint.RuleModule { const { create, ...remainingConfig } = config; return ESLintUtils.RuleCreator(getDocsUrl)({ diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 25a7991f..721cfa71 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -1,7 +1,8 @@ import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; export type DetectionHelpers = { - getIsImportingTestingLibrary: () => boolean; + getIsTestingLibraryImported: () => boolean; + canReportErrors: () => boolean; }; /** @@ -21,19 +22,24 @@ export function detectTestingLibraryUtils< return ( context: Readonly>, optionsWithDefault: Readonly - ): TRuleListener => { + ): TSESLint.RuleListener => { let isImportingTestingLibrary = false; // TODO: init here options based on shared ESLint config - // helpers for Testing Library detection + // Helpers for Testing Library detection. const helpers: DetectionHelpers = { - getIsImportingTestingLibrary() { + getIsTestingLibraryImported() { return isImportingTestingLibrary; }, + canReportErrors() { + return this.getIsTestingLibraryImported(); + }, }; - // instructions for Testing Library detection + // Instructions for Testing Library detection. + // `ImportDeclaration` must be first in order to know if Testing Library + // is imported ASAP. const detectionInstructions: TSESLint.RuleListener = { ImportDeclaration(node: TSESTree.ImportDeclaration) { isImportingTestingLibrary = /testing-library/g.test( @@ -44,17 +50,23 @@ export function detectTestingLibraryUtils< // update given rule to inject Testing Library detection const ruleInstructions = ruleCreate(context, optionsWithDefault, helpers); - const enhancedRuleInstructions = Object.assign({}, ruleInstructions); + const enhancedRuleInstructions: TSESLint.RuleListener = {}; + + // The order here is important too: detection instructions must come before + // than rule instructions to: + // - detect Testing Library things before the rule is applied + // - be able to prevent the rule about to be applied if necessary + const allKeys = new Set( + Object.keys(detectionInstructions).concat(Object.keys(ruleInstructions)) + ); - Object.keys(detectionInstructions).forEach((instruction) => { - (enhancedRuleInstructions as TSESLint.RuleListener)[instruction] = ( - node - ) => { + 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 index 098787ef..2c6f97e2 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -25,11 +25,10 @@ export default createTestingLibraryRule({ }, 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 new file mode 100644 index 00000000..6b2f5943 --- /dev/null +++ b/tests/create-testing-library-rule.test.ts @@ -0,0 +1,34 @@ +import { createRuleTester } from './lib/test-utils'; +import rule, { RULE_NAME } from './fake-rule'; + +const ruleTester = createRuleTester(); + +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + // should NOT report `render` imported for non-related Testing Library module + code: ` + import { render } from '@somewhere/else' + + const utils = render(); + `, + }, + ], + invalid: [ + { + // should report `render` imported from Testing Library module + code: ` + import { render } from '@testing-library/react' + + const utils = render(); + `, + errors: [ + { + line: 4, + column: 21, + messageId: 'fakeError', + }, + ], + }, + ], +}); diff --git a/tests/fake-rule.ts b/tests/fake-rule.ts new file mode 100644 index 00000000..8e746319 --- /dev/null +++ b/tests/fake-rule.ts @@ -0,0 +1,42 @@ +/** + * @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({ + 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', + }); + } + }; + + return { + 'CallExpression Identifier': reportRenderIdentifier, + }; + }, +}); From 824150df227ded82335fee07ef4477d1732dd52d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 21 Oct 2020 21:49:16 +0200 Subject: [PATCH 2/7] feat: add new shared setting for testing library module --- lib/detect-testing-library-utils.ts | 45 +++++++++++++--- tests/create-testing-library-rule.test.ts | 65 +++++++++++++++++++++-- tests/lib/rules/no-node-access.test.ts | 26 +++++++-- 3 files changed, 120 insertions(+), 16 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 721cfa71..aa87ab3b 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -5,6 +5,10 @@ export type DetectionHelpers = { canReportErrors: () => boolean; }; +export type TestingLibrarySettings = { + 'testing-library/module'?: string; +}; + /** * Enhances a given rule `create` with helpers to detect Testing Library utils. */ @@ -14,23 +18,37 @@ export function detectTestingLibraryUtils< TRuleListener extends TSESLint.RuleListener = TSESLint.RuleListener >( ruleCreate: ( - context: Readonly>, + context: Readonly< + TSESLint.RuleContext & { + settings: TestingLibrarySettings; + } + >, optionsWithDefault: Readonly, detectionHelpers: Readonly ) => TRuleListener ) { return ( - context: Readonly>, + context: Readonly< + TSESLint.RuleContext & { + settings: TestingLibrarySettings; + } + >, optionsWithDefault: Readonly ): TSESLint.RuleListener => { - let isImportingTestingLibrary = false; + 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. const helpers: DetectionHelpers = { getIsTestingLibraryImported() { - return isImportingTestingLibrary; + if (!customModule) { + return true; + } + + return isImportingTestingLibraryModule || isImportingCustomModule; }, canReportErrors() { return this.getIsTestingLibraryImported(); @@ -42,9 +60,20 @@ export function detectTestingLibraryUtils< // is imported ASAP. const detectionInstructions: TSESLint.RuleListener = { 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); + } }, }; diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index 6b2f5943..e6bd97c1 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/tests/create-testing-library-rule.test.ts @@ -6,25 +6,84 @@ const ruleTester = createRuleTester(); ruleTester.run(RULE_NAME, rule, { valid: [ { - // should NOT report `render` imported for non-related Testing Library module code: ` + // case: render imported for different custom module import { render } from '@somewhere/else' const utils = render(); `, + settings: { + 'testing-library/module': 'test-utils', + }, }, ], invalid: [ { - // should report `render` imported from Testing Library module 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: 4, + line: 7, column: 21, messageId: 'fakeError', }, diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index ee432767..9c7cbcc4 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -51,22 +51,38 @@ ruleTester.run(RULE_NAME, rule, { within(signinModal).getByPlaceholderText('Username'); `, }, - { + /*{ + // TODO: this one should be valid indeed. Rule implementation must be improved + // to track where the nodes are coming from. This one wasn't reported before + // just because this code is not importing TL module, but that's a really + // brittle check. Instead, this one shouldn't be reported since `children` + // it's just a property not related to a node code: ` const Component = props => { return
{props.children}
} `, - }, + },*/ { - // Not importing a testing-library package code: ` - const closestButton = document.getElementById('submit-btn').closest('button'); - expect(closestButton).toBeInTheDocument(); + // case: importing custom module + const closestButton = document.getElementById('submit-btn').closest('button'); + expect(closestButton).toBeInTheDocument(); `, + settings: { + 'testing-library/module': 'test-utils', + }, }, ], invalid: [ + { + code: ` + // case: without importing TL (aggressive reporting) + const closestButton = document.getElementById('submit-btn') + expect(closestButton).toBeInTheDocument(); + `, + errors: [{ messageId: 'noNodeAccess', line: 3 }], + }, { code: ` import { screen } from '@testing-library/react'; From 4b16c7bb9b55fca6743536f114ea746ad7221f02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 22 Oct 2020 16:48:08 +0200 Subject: [PATCH 3/7] test: increase coverage for create testing library rule --- jest.config.js | 6 ------ tests/create-testing-library-rule.test.ts | 14 +++++++++++++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/jest.config.js b/jest.config.js index 963a9b50..967d2ee9 100644 --- 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/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index e6bd97c1..e92eb0e3 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/tests/create-testing-library-rule.test.ts @@ -1,10 +1,22 @@ import { createRuleTester } from './lib/test-utils'; import rule, { RULE_NAME } from './fake-rule'; -const ruleTester = createRuleTester(); +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(); + `, + }, { code: ` // case: render imported for different custom module From 4f8e43f551a4c58cbc8c289052053bd2d8b0f29e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 25 Oct 2020 12:11:23 +0100 Subject: [PATCH 4/7] refactor: extract common enhanced rule create types --- lib/create-testing-library-rule.ts | 8 ++--- lib/detect-testing-library-utils.ts | 45 ++++++++++++++++------------- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/lib/create-testing-library-rule.ts b/lib/create-testing-library-rule.ts index 25ee809a..96f47d66 100644 --- 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,11 +20,7 @@ export function createTestingLibraryRule< name: string; meta: CreateRuleMeta; defaultOptions: Readonly; - create: ( - context: Readonly>, - optionsWithDefault: Readonly, - detectionHelpers: Readonly - ) => TRuleListener; + create: EnhancedRuleCreate; }> ): TSESLint.RuleModule { const { create, ...remainingConfig } = config; diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index aa87ab3b..db97b518 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -1,14 +1,33 @@ 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 & { + settings: TestingLibrarySettings; + } +>; + +export type EnhancedRuleCreate< + TOptions extends readonly unknown[], + TMessageIds extends string, + TRuleListener extends TSESLint.RuleListener = TSESLint.RuleListener +> = ( + context: TestingLibraryContext, + optionsWithDefault: Readonly, + detectionHelpers: Readonly +) => TRuleListener; + export type DetectionHelpers = { getIsTestingLibraryImported: () => boolean; canReportErrors: () => boolean; }; -export type TestingLibrarySettings = { - 'testing-library/module'?: string; -}; - /** * Enhances a given rule `create` with helpers to detect Testing Library utils. */ @@ -16,23 +35,9 @@ export function detectTestingLibraryUtils< TOptions extends readonly unknown[], TMessageIds extends string, TRuleListener extends TSESLint.RuleListener = TSESLint.RuleListener ->( - ruleCreate: ( - context: Readonly< - TSESLint.RuleContext & { - settings: TestingLibrarySettings; - } - >, - optionsWithDefault: Readonly, - detectionHelpers: Readonly - ) => TRuleListener -) { +>(ruleCreate: EnhancedRuleCreate) { return ( - context: Readonly< - TSESLint.RuleContext & { - settings: TestingLibrarySettings; - } - >, + context: TestingLibraryContext, optionsWithDefault: Readonly ): TSESLint.RuleListener => { let isImportingTestingLibraryModule = false; From e9d7bc3132f3294efffae6086d8864bff69511da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 25 Oct 2020 12:22:57 +0100 Subject: [PATCH 5/7] docs: add jsdoc to detection helpers --- lib/detect-testing-library-utils.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index db97b518..96a9720e 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -48,6 +48,18 @@ export function detectTestingLibraryUtils< // Helpers for Testing Library detection. const helpers: DetectionHelpers = { + /** + * 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; @@ -55,6 +67,10 @@ export function detectTestingLibraryUtils< return isImportingTestingLibraryModule || isImportingCustomModule; }, + + /** + * Wraps all conditions that must be met to report rules. + */ canReportErrors() { return this.getIsTestingLibraryImported(); }, From 476b7f637d7acaf8010477b72c886afd7a87b49e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 25 Oct 2020 12:30:39 +0100 Subject: [PATCH 6/7] docs: update old comments related to ImportDeclaration --- lib/detect-testing-library-utils.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 96a9720e..cf5ca51f 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -77,9 +77,14 @@ export function detectTestingLibraryUtils< }; // Instructions for Testing Library detection. - // `ImportDeclaration` must be first in order to know if Testing Library - // is imported ASAP. 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) { if (!isImportingTestingLibraryModule) { // check only if testing library import not found yet so we avoid @@ -102,14 +107,12 @@ export function detectTestingLibraryUtils< const ruleInstructions = ruleCreate(context, optionsWithDefault, helpers); const enhancedRuleInstructions: TSESLint.RuleListener = {}; - // The order here is important too: detection instructions must come before - // than rule instructions to: - // - detect Testing Library things before the rule is applied - // - be able to prevent the rule about to be applied if necessary const allKeys = new Set( Object.keys(detectionInstructions).concat(Object.keys(ruleInstructions)) ); + // 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) { From 400d343fe1c523e3228477df567d4ce198cb1135 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 25 Oct 2020 12:49:35 +0100 Subject: [PATCH 7/7] test: check rule listener are combined properly --- tests/create-testing-library-rule.test.ts | 19 ++++++++++++++++++- tests/fake-rule.ts | 13 +++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index e92eb0e3..14977c72 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/tests/create-testing-library-rule.test.ts @@ -19,7 +19,7 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` - // case: render imported for different custom module + // case: render imported from other than custom module import { render } from '@somewhere/else' const utils = render(); @@ -28,8 +28,25 @@ ruleTester.run(RULE_NAME, rule, { '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) diff --git a/tests/fake-rule.ts b/tests/fake-rule.ts index 8e746319..1bad0a53 100644 --- a/tests/fake-rule.ts +++ b/tests/fake-rule.ts @@ -35,8 +35,21 @@ export default createTestingLibraryRule({ } }; + 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, }; }, });