diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index 3b242da1..7d04d424 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -5,6 +5,7 @@ import { } from '@typescript-eslint/experimental-utils'; import { getAssertNodeInfo, + getIdentifierNode, getImportModuleName, ImportModuleNode, isImportDeclaration, @@ -24,6 +25,7 @@ import { export type TestingLibrarySettings = { 'testing-library/module'?: string; 'testing-library/filename-pattern'?: string; + 'testing-library/custom-renders'?: string[]; }; export type TestingLibraryContext< @@ -60,6 +62,7 @@ export type DetectionHelpers = { isCustomQuery: (node: TSESTree.Identifier) => boolean; isAsyncUtil: (node: TSESTree.Identifier) => boolean; isFireEventMethod: (node: TSESTree.Identifier) => boolean; + isRenderUtil: (node: TSESTree.Node) => boolean; isPresenceAssert: (node: TSESTree.MemberExpression) => boolean; isAbsenceAssert: (node: TSESTree.MemberExpression) => boolean; canReportErrors: () => boolean; @@ -74,6 +77,7 @@ export type DetectionHelpers = { const DEFAULT_FILENAME_PATTERN = '^.*\\.(test|spec)\\.[jt]sx?$'; const FIRE_EVENT_NAME = 'fireEvent'; +const RENDER_NAME = 'render'; /** * Enhances a given rule `create` with helpers to detect Testing Library utils. @@ -95,15 +99,31 @@ export function detectTestingLibraryUtils< const filenamePattern = context.settings['testing-library/filename-pattern'] ?? DEFAULT_FILENAME_PATTERN; + const customRenders = context.settings['testing-library/custom-renders']; /** - * Determines whether aggressive reporting is enabled or not. + * Determines whether aggressive module reporting is enabled or not. * - * Aggressive reporting is considered as enabled when: - * - custom module is not set (so we need to assume everything - * matching TL utils is related to TL no matter where it was imported from) + * This aggressive reporting mechanism is considered as enabled when custom + * module is not set, so we need to assume everything matching Testing + * Library utils is related to Testing Library no matter from where module + * they are coming from. Otherwise, this aggressive reporting mechanism is + * opted-out in favour to report only those utils coming from Testing + * Library package or custom module set up on settings. */ - const isAggressiveReportingEnabled = () => !customModule; + const isAggressiveModuleReportingEnabled = () => !customModule; + + /** + * Determines whether aggressive render reporting is enabled or not. + * + * This aggressive reporting mechanism is considered as enabled when custom + * renders are not set, so we need to assume every method containing + * "render" is a valid Testing Library `render`. Otherwise, this aggressive + * reporting mechanism is opted-out in favour to report only `render` or + * names set up on custom renders setting. + */ + const isAggressiveRenderReportingEnabled = () => + !Array.isArray(customRenders) || customRenders.length === 0; // Helpers for Testing Library detection. const getTestingLibraryImportNode: DetectionHelpers['getTestingLibraryImportNode'] = () => { @@ -135,7 +155,7 @@ export function detectTestingLibraryUtils< * or custom module are imported. */ const isTestingLibraryImported: DetectionHelpers['isTestingLibraryImported'] = () => { - if (isAggressiveReportingEnabled()) { + if (isAggressiveModuleReportingEnabled()) { return true; } @@ -215,7 +235,7 @@ export function detectTestingLibraryUtils< fireEventUtilName = ASTUtils.isIdentifier(fireEventUtil) ? fireEventUtil.name : fireEventUtil.local.name; - } else if (isAggressiveReportingEnabled()) { + } else if (isAggressiveModuleReportingEnabled()) { fireEventUtilName = FIRE_EVENT_NAME; } @@ -256,6 +276,48 @@ export function detectTestingLibraryUtils< return regularCall || wildcardCall; }; + /** + * Determines whether a given node is a valid render util or not. + * + * A node will be interpreted as a valid render based on two conditions: + * the name matches with a valid "render" option, and the node is coming + * from Testing Library module. This depends on: + * + * - Aggressive render reporting: if enabled, then every node name + * containing "render" will be assumed as Testing Library render util. + * Otherwise, it means `custom-modules` has been set up, so only those nodes + * named as "render" or some of the `custom-modules` options will be + * considered as Testing Library render util. + * - Aggressive module reporting: if enabled, then it doesn't matter from + * where the given node was imported from as it will be considered part of + * Testing Library. Otherwise, it means `custom-module` has been set up, so + * only those nodes coming from Testing Library will be considered as valid. + */ + const isRenderUtil: DetectionHelpers['isRenderUtil'] = (node) => { + const identifier = getIdentifierNode(node); + + if (!identifier) { + return false; + } + + const isNameMatching = (function () { + if (isAggressiveRenderReportingEnabled()) { + return identifier.name.toLowerCase().includes(RENDER_NAME); + } + + return [RENDER_NAME, ...customRenders].includes(identifier.name); + })(); + + if (!isNameMatching) { + return false; + } + + return ( + isAggressiveModuleReportingEnabled() || + isNodeComingFromTestingLibrary(identifier) + ); + }; + /** * Determines whether a given MemberExpression node is a presence assert * @@ -376,6 +438,7 @@ export function detectTestingLibraryUtils< isCustomQuery, isAsyncUtil, isFireEventMethod, + isRenderUtil, isPresenceAssert, isAbsenceAssert, canReportErrors, diff --git a/lib/rules/render-result-naming-convention.ts b/lib/rules/render-result-naming-convention.ts index f6686b6f..3d4bb171 100644 --- a/lib/rules/render-result-naming-convention.ts +++ b/lib/rules/render-result-naming-convention.ts @@ -1,27 +1,18 @@ -import { - ESLintUtils, - TSESTree, - ASTUtils, -} from '@typescript-eslint/experimental-utils'; -import { getDocsUrl, hasTestingLibraryImportModule } from '../utils'; -import { - isCallExpression, - isImportSpecifier, - isMemberExpression, - isObjectPattern, - isRenderVariableDeclarator, -} from '../node-utils'; +import { createTestingLibraryRule } from '../create-testing-library-rule'; +import { isObjectPattern } from '../node-utils'; +import { ASTUtils } from '@typescript-eslint/experimental-utils'; export const RULE_NAME = 'render-result-naming-convention'; export type MessageIds = 'renderResultNamingConvention'; -type Options = [{ renderFunctions?: string[] }]; + +type Options = []; const ALLOWED_VAR_NAMES = ['view', 'utils']; const ALLOWED_VAR_NAMES_TEXT = ALLOWED_VAR_NAMES.map( (name) => `\`${name}\`` ).join(', '); -export default ESLintUtils.RuleCreator(getDocsUrl)({ +export default createTestingLibraryRule({ name: RULE_NAME, meta: { type: 'suggestion', @@ -31,103 +22,27 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ recommended: false, }, messages: { - renderResultNamingConvention: `\`{{ varName }}\` is not a recommended name for \`render\` returned value. Instead, you should destructure it, or call it using one of the valid choices: ${ALLOWED_VAR_NAMES_TEXT}`, + renderResultNamingConvention: `\`{{ renderResultName }}\` is not a recommended name for \`render\` returned value. Instead, you should destructure it, or name it using one of: ${ALLOWED_VAR_NAMES_TEXT}`, }, fixable: null, - schema: [ - { - type: 'object', - properties: { - renderFunctions: { - type: 'array', - }, - }, - }, - ], + schema: [], }, - defaultOptions: [ - { - renderFunctions: [], - }, - ], - - create(context, [options]) { - const { renderFunctions } = options; - let renderAlias: string | undefined; - let wildcardImportName: string | undefined; + defaultOptions: [], + create(context, _, helpers) { return { - // check named imports - ImportDeclaration(node: TSESTree.ImportDeclaration) { - if (!hasTestingLibraryImportModule(node)) { - return; - } - const renderImport = node.specifiers.find( - (node) => isImportSpecifier(node) && node.imported.name === 'render' - ); - - if (!renderImport) { - return; - } - - renderAlias = renderImport.local.name; - }, - // check wildcard imports - 'ImportDeclaration ImportNamespaceSpecifier'( - node: TSESTree.ImportNamespaceSpecifier - ) { - if ( - !hasTestingLibraryImportModule( - node.parent as TSESTree.ImportDeclaration - ) - ) { + VariableDeclarator(node) { + if (!helpers.isRenderUtil(node.init)) { return; } - wildcardImportName = node.local.name; - }, - VariableDeclarator(node: TSESTree.VariableDeclarator) { // check if destructuring return value from render if (isObjectPattern(node.id)) { return; } - const isValidRenderDeclarator = isRenderVariableDeclarator(node, [ - ...renderFunctions, - renderAlias, - ]); - const isValidWildcardImport = !!wildcardImportName; - - // check if is a Testing Library related import - if (!isValidRenderDeclarator && !isValidWildcardImport) { - return; - } - - const renderFunctionName = - isCallExpression(node.init) && - ASTUtils.isIdentifier(node.init.callee) && - node.init.callee.name; - - const renderFunctionObjectName = - isCallExpression(node.init) && - isMemberExpression(node.init.callee) && - ASTUtils.isIdentifier(node.init.callee.property) && - ASTUtils.isIdentifier(node.init.callee.object) && - node.init.callee.property.name === 'render' && - node.init.callee.object.name; - - const isRenderAlias = !!renderAlias; - const isCustomRender = renderFunctions.includes(renderFunctionName); - const isWildCardRender = - renderFunctionObjectName && - renderFunctionObjectName === wildcardImportName; - - // check if is a qualified render function - if (!isRenderAlias && !isCustomRender && !isWildCardRender) { - return; - } - const renderResultName = ASTUtils.isIdentifier(node.id) && node.id.name; + const isAllowedRenderResultName = ALLOWED_VAR_NAMES.includes( renderResultName ); @@ -141,7 +56,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ node, messageId: 'renderResultNamingConvention', data: { - varName: renderResultName, + renderResultName, }, }); }, diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index b57fbede..5d820df7 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/tests/create-testing-library-rule.test.ts @@ -90,6 +90,25 @@ ruleTester.run(RULE_NAME, rule, { `, }, + // Test Cases for renders + { + code: ` + // case: aggressive render enabled - method not containing "render" + import { somethingElse } from '@somewhere/else' + + const utils = somethingElse() + `, + }, + { + settings: { 'testing-library/custom-renders': ['renderWithRedux'] }, + code: ` + // case: aggressive render disabled - method not matching valid render + import { customRender } from '@somewhere/else' + + const utils = customRender() + `, + }, + // Test Cases for all settings mixed { settings: { @@ -220,6 +239,24 @@ ruleTester.run(RULE_NAME, rule, { obj.tl.waitFor(() => {}) `, }, + { + settings: { 'testing-library/module': 'test-utils' }, + code: ` + // case: aggressive render enabled, but module disabled - not coming from TL + import { render } from 'somewhere-else' + + const utils = render() + `, + }, + { + filename: 'file.not.matching.js', + code: ` + // case: aggressive render and module enabled, but file name not matching + import { render } from '@testing-library/react' + + const utils = render() + `, + }, ], invalid: [ // Test Cases for Imports & Filename @@ -261,7 +298,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 6, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -278,7 +315,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 7, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -295,7 +332,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 7, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -315,7 +352,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 7, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -335,7 +372,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 7, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -356,7 +393,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 8, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -377,7 +414,7 @@ ruleTester.run(RULE_NAME, rule, { { line: 8, column: 21, - messageId: 'fakeError', + messageId: 'renderError', }, ], }, @@ -407,7 +444,73 @@ ruleTester.run(RULE_NAME, rule, { const utils = render(); `, - errors: [{ line: 7, column: 21, messageId: 'fakeError' }], + errors: [{ line: 7, column: 21, messageId: 'renderError' }], + }, + + // Test Cases for renders + { + code: ` + // case: aggressive render enabled - Testing Library render + import { render } from '@testing-library/react' + + const utils = render() + `, + errors: [{ line: 5, column: 21, messageId: 'renderError' }], + }, + { + code: ` + // case: aggressive render enabled - Testing Library render wildcard imported + import * as rtl from '@testing-library/react' + + const utils = rtl.render() + `, + errors: [ + { line: 5, column: 21, messageId: 'fakeError' }, + { line: 5, column: 25, messageId: 'renderError' }, + ], + }, + { + code: ` + // case: aggressive render enabled - any method containing "render" + import { someRender } from '@somewhere/else' + + const utils = someRender() + `, + errors: [{ line: 5, column: 21, messageId: 'renderError' }], + }, + { + settings: { 'testing-library/custom-renders': ['customRender'] }, + code: ` + // case: aggressive render disabled - Testing Library render + import { render } from '@testing-library/react' + + const utils = render() + `, + errors: [{ line: 5, column: 21, messageId: 'renderError' }], + }, + { + settings: { + 'testing-library/custom-renders': ['customRender', 'renderWithRedux'], + }, + code: ` + // case: aggressive render disabled - valid custom render + import { customRender } from 'test-utils' + + const utils = customRender() + `, + errors: [{ line: 5, column: 21, messageId: 'renderError' }], + }, + { + settings: { + 'testing-library/custom-renders': ['customRender', 'renderWithRedux'], + }, + code: ` + // case: aggressive render disabled - default render from custom module + import { render } from 'test-utils' + + const utils = render() + `, + errors: [{ line: 5, column: 21, messageId: 'renderError' }], }, // Test Cases for presence/absence assertions @@ -635,5 +738,26 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [{ line: 3, column: 9, messageId: 'fakeError' }], }, + + // Test Cases for all settings mixed + { + filename: 'MyComponent.custom-suffix.js', + settings: { + 'testing-library/custom-renders': ['customRender', 'renderWithRedux'], + 'testing-library/module': 'test-utils', + 'testing-library/filename-pattern': 'custom-suffix\\.js', + }, + code: ` + // case: all aggressive reporting disabled and filename setup - matching all custom settings + import { renderWithRedux, waitFor, screen } from 'test-utils' + + const { getByRole } = renderWithRedux() + const el = getByRole('button') + `, + errors: [ + { line: 5, column: 29, messageId: 'renderError' }, + { line: 6, column: 18, messageId: 'getByError' }, + ], + }, ], }); diff --git a/tests/fake-rule.ts b/tests/fake-rule.ts index 5d9e71d8..0851d6b9 100644 --- a/tests/fake-rule.ts +++ b/tests/fake-rule.ts @@ -9,6 +9,7 @@ export const RULE_NAME = 'fake-rule'; type Options = []; type MessageIds = | 'fakeError' + | 'renderError' | 'getByError' | 'queryByError' | 'findByError' @@ -27,6 +28,7 @@ export default createTestingLibraryRule({ }, messages: { fakeError: 'fake error reported', + renderError: 'some error related to render util reported', getByError: 'some error related to getBy reported', queryByError: 'some error related to queryBy reported', findByError: 'some error related to findBy reported', @@ -41,8 +43,8 @@ export default createTestingLibraryRule({ create(context, _, helpers) { const reportCallExpressionIdentifier = (node: TSESTree.Identifier) => { // force "render" to be reported - if (node.name === 'render') { - return context.report({ node, messageId: 'fakeError' }); + if (helpers.isRenderUtil(node)) { + return context.report({ node, messageId: 'renderError' }); } if (helpers.isCustomQuery(node)) { diff --git a/tests/lib/rules/render-result-naming-convention.test.ts b/tests/lib/rules/render-result-naming-convention.test.ts index 7f51163c..8dea67cf 100644 --- a/tests/lib/rules/render-result-naming-convention.test.ts +++ b/tests/lib/rules/render-result-naming-convention.test.ts @@ -93,11 +93,7 @@ ruleTester.run(RULE_NAME, rule, { const button = screen.getByText('some button'); }); `, - options: [ - { - renderFunctions: ['customRender'], - }, - ], + settings: { 'testing-library/custom-renders': ['customRender'] }, }, { code: ` @@ -108,11 +104,7 @@ ruleTester.run(RULE_NAME, rule, { await view.findByRole('button'); }); `, - options: [ - { - renderFunctions: ['customRender'], - }, - ], + settings: { 'testing-library/custom-renders': ['customRender'] }, }, { code: ` @@ -123,53 +115,7 @@ ruleTester.run(RULE_NAME, rule, { await utils.findByRole('button'); }); `, - options: [ - { - renderFunctions: ['customRender'], - }, - ], - }, - { - code: ` - import { render } from '@foo/bar'; - - test('should not report from render not related to testing library', () => { - const wrapper = render(); - const button = wrapper.getByText('some button'); - }); - `, - }, - { - code: ` - import { render } from '@foo/bar'; - - test('should not report from render not imported from testing library', () => { - const wrapper = render(); - const button = wrapper.getByText('some button'); - }); - `, - }, - { - code: ` - import * as RTL from '@foo/bar'; - - test('should not report from wildcard render not imported from testing library', () => { - const wrapper = RTL.render(); - const button = wrapper.getByText('some button'); - }); - `, - }, - { - code: ` - function render() { - return 'whatever'; - } - - test('should not report from custom render not related to testing library', () => { - const wrapper = render(); - const button = wrapper.getByText('some button'); - }); - `, + settings: { 'testing-library/custom-renders': ['customRender'] }, }, { code: ` @@ -203,7 +149,7 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'renderResultNamingConvention', data: { - varName: 'wrapper', + renderResultName: 'wrapper', }, line: 5, column: 17, @@ -223,7 +169,7 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'renderResultNamingConvention', data: { - varName: 'wrapper', + renderResultName: 'wrapper', }, line: 5, column: 17, @@ -243,7 +189,7 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'renderResultNamingConvention', data: { - varName: 'component', + renderResultName: 'component', }, line: 5, column: 17, @@ -280,7 +226,7 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'renderResultNamingConvention', data: { - varName: 'wrapper', + renderResultName: 'wrapper', }, line: 5, column: 17, @@ -307,7 +253,7 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'renderResultNamingConvention', data: { - varName: 'wrapper', + renderResultName: 'wrapper', }, line: 6, column: 17, @@ -323,21 +269,79 @@ ruleTester.run(RULE_NAME, rule, { const button = wrapper.getByText('some button'); }); `, - options: [ + settings: { 'testing-library/custom-renders': ['customRender'] }, + errors: [ { - renderFunctions: ['customRender'], + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 5, + column: 17, }, ], + }, + { + code: ` + import { render } from '@foo/bar'; + + test('aggressive reporting - should report from render not related to testing library', () => { + const wrapper = render(); + const button = wrapper.getByText('some button'); + }); + `, errors: [ { messageId: 'renderResultNamingConvention', data: { - varName: 'wrapper', + renderResultName: 'wrapper', }, line: 5, column: 17, }, ], }, + { + code: ` + import * as RTL from '@foo/bar'; + + test('aggressive reporting - should report from wildcard render not imported from testing library', () => { + const wrapper = RTL.render(); + const button = wrapper.getByText('some button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 5, + column: 17, + }, + ], + }, + { + code: ` + function render() { + return 'whatever'; + } + + test('aggressive reporting - should report from custom render not related to testing library', () => { + const wrapper = render(); + const button = wrapper.getByText('some button'); + }); + `, + errors: [ + { + messageId: 'renderResultNamingConvention', + data: { + renderResultName: 'wrapper', + }, + line: 7, + column: 17, + }, + ], + }, ], });