From 2a7ab42a64335b4c33dc1b3ade56c223772d6018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Wed, 10 Mar 2021 21:18:20 +0100 Subject: [PATCH 1/5] feat(render-result-naming-convention): detect render calls from wrappers --- lib/rules/render-result-naming-convention.ts | 29 +++++++++-- .../render-result-naming-convention.test.ts | 49 +++++++++++++++++++ 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/lib/rules/render-result-naming-convention.ts b/lib/rules/render-result-naming-convention.ts index f68e5170..b322a89a 100644 --- a/lib/rules/render-result-naming-convention.ts +++ b/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,7 +35,22 @@ export default createTestingLibraryRule({ 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); @@ -38,7 +58,10 @@ export default createTestingLibraryRule({ return; } - if (!helpers.isRenderUtil(initIdentifierNode)) { + if ( + !helpers.isRenderUtil(initIdentifierNode) && + !renderWrapperNames.includes(initIdentifierNode.name) + ) { return; } diff --git a/tests/lib/rules/render-result-naming-convention.test.ts b/tests/lib/rules/render-result-naming-convention.test.ts index 8dea67cf..0fc631a0 100644 --- a/tests/lib/rules/render-result-naming-convention.test.ts +++ b/tests/lib/rules/render-result-naming-convention.test.ts @@ -260,6 +260,55 @@ ruleTester.run(RULE_NAME, rule, { }, ], }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { render } from '@testing-library/react'; + + const setup = () => render(); + + test('aggressive reporting disabled - should report nested render from TL package', () => { + const wrapper = setup(); + const button = wrapper.getByText('some button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 7, + column: 17, + }, + ], + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { render } from 'test-utils'; + + function setup() { + doSomethingElse(); + return render() + } + + test('aggressive reporting disabled - should report nested render from custom utils module', () => { + const wrapper = setup(); + const button = wrapper.getByText('some button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 10, + column: 17, + }, + ], + }, { code: ` import { customRender } from 'test-utils'; From 97e8146cca4b52b883de1298eeb1d62fd41661f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Wed, 10 Mar 2021 22:41:56 +0100 Subject: [PATCH 2/5] 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' --- lib/detect-testing-library-utils.ts | 21 ++++++++++++++++--- tests/create-testing-library-rule.test.ts | 20 ------------------ .../render-result-naming-convention.test.ts | 14 +++++++++++++ 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 8b4e0cc9..5c56d528 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -139,10 +139,25 @@ export function detectTestingLibraryUtils< const referenceNode = getReferenceNode(node); const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode); - return ( - isAggressiveModuleReportingEnabled() || - isNodeComingFromTestingLibrary(referenceNodeIdentifier) + if (isAggressiveModuleReportingEnabled()) { + return true; + } + + // TODO: extract this into function, combined with logic from isFireEventMethod + // TODO: include some tests create-testing-library-rule + const importNode = findImportedUtilSpecifier( + referenceNodeIdentifier.name ); + + if (!importNode) { + return false; + } + + if (ASTUtils.isIdentifier(importNode)) { + return importNode.name === referenceNodeIdentifier.name; + } + + return importNode.local.name === referenceNodeIdentifier.name; } /** diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index 68c119cb..3fb1d662 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/tests/create-testing-library-rule.test.ts @@ -642,26 +642,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', diff --git a/tests/lib/rules/render-result-naming-convention.test.ts b/tests/lib/rules/render-result-naming-convention.test.ts index 0fc631a0..45389746 100644 --- a/tests/lib/rules/render-result-naming-convention.test.ts +++ b/tests/lib/rules/render-result-naming-convention.test.ts @@ -134,6 +134,20 @@ ruleTester.run(RULE_NAME, rule, { }); `, }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { render as testingLibraryRender } from '@testing-library/react'; + import { render } from '@somewhere/else' + + const setup = () => render(); + + test('aggressive reporting disabled - should not report nested render not related to TL', () => { + const wrapper = setup(); + const button = wrapper.getByText('some button'); + }); + `, + }, ], invalid: [ { From 2617373bc6438c1c7ddf599d644c16b06d2cff07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Thu, 11 Mar 2021 19:10:34 +0100 Subject: [PATCH 3/5] refactor: split checks for import matching node name in different methods --- lib/detect-testing-library-utils.ts | 42 ++++++++++++----------- lib/node-utils.ts | 11 ++++++ tests/create-testing-library-rule.test.ts | 28 +++++++++++++++ tests/lib/rules/prefer-wait-for.test.ts | 15 ++++++++ 4 files changed, 76 insertions(+), 20 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 5c56d528..4c81951f 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -8,6 +8,7 @@ import { getImportModuleName, getPropertyIdentifierNode, getReferenceNode, + hasImportMatch, ImportModuleNode, isImportDeclaration, isImportNamespaceSpecifier, @@ -127,6 +128,12 @@ 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, @@ -136,28 +143,14 @@ export function detectTestingLibraryUtils< return false; } - const referenceNode = getReferenceNode(node); - const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode); - if (isAggressiveModuleReportingEnabled()) { return true; } - // TODO: extract this into function, combined with logic from isFireEventMethod - // TODO: include some tests create-testing-library-rule - const importNode = findImportedUtilSpecifier( - referenceNodeIdentifier.name - ); - - if (!importNode) { - return false; - } - - if (ASTUtils.isIdentifier(importNode)) { - return importNode.name === referenceNodeIdentifier.name; - } + const referenceNode = getReferenceNode(node); + const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode); - return importNode.local.name === referenceNodeIdentifier.name; + return isNodeComingFromTestingLibrary(referenceNodeIdentifier); } /** @@ -454,9 +447,12 @@ export function detectTestingLibraryUtils< 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 @@ -464,7 +460,13 @@ export function detectTestingLibraryUtils< const identifierName: string | undefined = getPropertyIdentifierNode(node) .name; - return !!findImportedUtilSpecifier(identifierName); + const importNode = findImportedUtilSpecifier(identifierName); + + if (!importNode) { + return false; + } + + return hasImportMatch(importNode, identifierName); }; const helpers: DetectionHelpers = { diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 2fcd4e75..4aba63cd 100644 --- a/lib/node-utils.ts +++ b/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; +} diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index 3fb1d662..4907e995 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/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 { diff --git a/tests/lib/rules/prefer-wait-for.test.ts b/tests/lib/rules/prefer-wait-for.test.ts index e905c328..f9a9ca8b 100644 --- a/tests/lib/rules/prefer-wait-for.test.ts +++ b/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'; From 5e8d1a449d943ed77869226d4da29d1a7f42647e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Thu, 11 Mar 2021 19:23:39 +0100 Subject: [PATCH 4/5] test(render-result-naming-convention): add extra invalid case for wrapped function --- .../render-result-naming-convention.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/lib/rules/render-result-naming-convention.test.ts b/tests/lib/rules/render-result-naming-convention.test.ts index 45389746..3178c862 100644 --- a/tests/lib/rules/render-result-naming-convention.test.ts +++ b/tests/lib/rules/render-result-naming-convention.test.ts @@ -406,5 +406,29 @@ ruleTester.run(RULE_NAME, rule, { }, ], }, + { + code: ` + import { render as testingLibraryRender } from '@testing-library/react'; + + const setup = () => { + return testingLibraryRender(); + }; + + test('should report render result called "wrapper" from renamed render wrapped in a function', async () => { + const wrapper = setup(); + await wrapper.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 9, + column: 17, + }, + ], + }, ], }); From 0a591b47727c1e2a6e8c2a26fa91a0ca7358cd2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 13 Mar 2021 20:57:42 +0100 Subject: [PATCH 5/5] fix(render-result-naming-convention): cover more renamed imports --- lib/detect-testing-library-utils.ts | 87 ++++++++++++++----- .../render-result-naming-convention.test.ts | 78 +++++++++++++++++ 2 files changed, 145 insertions(+), 20 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 4c81951f..312c3b63 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -137,9 +137,24 @@ export function detectTestingLibraryUtils< */ 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; } @@ -147,9 +162,6 @@ export function detectTestingLibraryUtils< return true; } - const referenceNode = getReferenceNode(node); - const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode); - return isNodeComingFromTestingLibrary(referenceNodeIdentifier); } @@ -280,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) ); }; @@ -355,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); - }); + ); }; /** @@ -410,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( @@ -437,10 +472,22 @@ 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. */ @@ -457,15 +504,15 @@ export function detectTestingLibraryUtils< const isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn = ( node ) => { - const identifierName: string | undefined = getPropertyIdentifierNode(node) - .name; - - const importNode = findImportedUtilSpecifier(identifierName); + const importNode = getImportedUtilSpecifier(node); if (!importNode) { return false; } + const identifierName: string | undefined = getPropertyIdentifierNode(node) + .name; + return hasImportMatch(importNode, identifierName); }; diff --git a/tests/lib/rules/render-result-naming-convention.test.ts b/tests/lib/rules/render-result-naming-convention.test.ts index 3178c862..9043ca5f 100644 --- a/tests/lib/rules/render-result-naming-convention.test.ts +++ b/tests/lib/rules/render-result-naming-convention.test.ts @@ -148,6 +148,27 @@ ruleTester.run(RULE_NAME, rule, { }); `, }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + 'testing-library/custom-renders': ['customRender'], + }, + code: ` + import { customRender as myRender } from 'test-utils'; + import { customRender } from 'non-related' + + const setup = () => { + return customRender(); + }; + + test( + 'both render and module aggressive reporting disabled - should not report render result called "wrapper" from nont-related renamed custom render wrapped in a function', + async () => { + const wrapper = setup(); + await wrapper.findByRole('button'); + }); + `, + }, ], invalid: [ { @@ -430,5 +451,62 @@ ruleTester.run(RULE_NAME, rule, { }, ], }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { render as testingLibraryRender } from '@testing-library/react'; + + const setup = () => { + return testingLibraryRender(); + }; + + test( + 'aggressive reporting disabled - should report render result called "wrapper" from renamed render wrapped in a function', + async () => { + const wrapper = setup(); + await wrapper.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 11, + column: 17, + }, + ], + }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + 'testing-library/custom-renders': ['customRender'], + }, + code: ` + import { customRender as myRender } from 'test-utils'; + + const setup = () => { + return myRender(); + }; + + test( + 'both render and module aggressive reporting disabled - should report render result called "wrapper" from renamed custom render wrapped in a function', + async () => { + const wrapper = setup(); + await wrapper.findByRole('button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 11, + column: 17, + }, + ], + }, ], });