diff --git a/docs/rules/no-promise-in-fire-event.md b/docs/rules/no-promise-in-fire-event.md index 3501e3a6..2c8c238c 100644 --- a/docs/rules/no-promise-in-fire-event.md +++ b/docs/rules/no-promise-in-fire-event.md @@ -1,17 +1,24 @@ # Disallow the use of promises passed to a `fireEvent` method (no-promise-in-fire-event) -The `fireEvent` method expects that a DOM element is passed. +Methods from `fireEvent` expect to receive a DOM element. Passing a promise will end up in an error, so it must be prevented. Examples of **incorrect** code for this rule: ```js import { screen, fireEvent } from '@testing-library/react'; -// usage of findBy queries +// usage of unhandled findBy queries fireEvent.click(screen.findByRole('button')); -// usage of promises -fireEvent.click(new Promise(jest.fn()) +// usage of unhandled promises +fireEvent.click(new Promise(jest.fn())); + +// usage of references to unhandled promises +const promise = new Promise(); +fireEvent.click(promise); + +const anotherPromise = screen.findByRole('button'); +fireEvent.click(anotherPromise); ``` Examples of **correct** code for this rule: @@ -19,15 +26,20 @@ Examples of **correct** code for this rule: ```js import { screen, fireEvent } from '@testing-library/react'; -// use getBy queries +// usage of getBy queries fireEvent.click(screen.getByRole('button')); -// use awaited findBy queries +// usage of awaited findBy queries fireEvent.click(await screen.findByRole('button')); -// this won't give a linting error, but it will throw a runtime error +// usage of references to handled promises const promise = new Promise(); -fireEvent.click(promise)`, +const element = await promise; +fireEvent.click(element); + +const anotherPromise = screen.findByRole('button'); +const button = await anotherPromise; +fireEvent.click(button); ``` ## Further Reading diff --git a/lib/node-utils.ts b/lib/node-utils.ts index fcbc2f14..605330ac 100644 --- a/lib/node-utils.ts +++ b/lib/node-utils.ts @@ -83,12 +83,6 @@ export function isBlockStatement( return node?.type === AST_NODE_TYPES.BlockStatement; } -export function isVariableDeclarator( - node: TSESTree.Node -): node is TSESTree.VariableDeclarator { - return node?.type === AST_NODE_TYPES.VariableDeclarator; -} - export function isObjectPattern( node: TSESTree.Node ): node is TSESTree.ObjectPattern { @@ -191,14 +185,6 @@ export function isImportDeclaration( return node?.type === AST_NODE_TYPES.ImportDeclaration; } -export function isAwaited(node: TSESTree.Node): boolean { - return ( - ASTUtils.isAwaitExpression(node) || - isArrowFunctionExpression(node) || - isReturnStatement(node) - ); -} - export function hasChainedThen(node: TSESTree.Node): boolean { const parent = node.parent; @@ -211,11 +197,16 @@ export function hasChainedThen(node: TSESTree.Node): boolean { return hasThenProperty(parent); } +export function isPromiseIdentifier( + node: TSESTree.Node +): node is TSESTree.Identifier & { name: 'Promise' } { + return ASTUtils.isIdentifier(node) && node.name === 'Promise'; +} + export function isPromiseAll(node: TSESTree.CallExpression): boolean { return ( isMemberExpression(node.callee) && - ASTUtils.isIdentifier(node.callee.object) && - node.callee.object.name === 'Promise' && + isPromiseIdentifier(node.callee.object) && ASTUtils.isIdentifier(node.callee.property) && node.callee.property.name === 'all' ); @@ -224,8 +215,7 @@ export function isPromiseAll(node: TSESTree.CallExpression): boolean { export function isPromiseAllSettled(node: TSESTree.CallExpression): boolean { return ( isMemberExpression(node.callee) && - ASTUtils.isIdentifier(node.callee.object) && - node.callee.object.name === 'Promise' && + isPromiseIdentifier(node.callee.object) && ASTUtils.isIdentifier(node.callee.property) && node.callee.property.name === 'allSettled' ); @@ -304,7 +294,7 @@ export function getVariableReferences( node: TSESTree.Node ): TSESLint.Scope.Reference[] { return ( - (isVariableDeclarator(node) && + (ASTUtils.isVariableDeclarator(node) && context.getDeclaredVariables(node)[0]?.references?.slice(1)) || [] ); diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts index c51390a0..e83500fe 100644 --- a/lib/rules/no-promise-in-fire-event.ts +++ b/lib/rules/no-promise-in-fire-event.ts @@ -1,20 +1,18 @@ +import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; +import { createTestingLibraryRule } from '../create-testing-library-rule'; import { - TSESTree, - ESLintUtils, - ASTUtils, -} from '@typescript-eslint/experimental-utils'; -import { getDocsUrl, ASYNC_QUERIES_VARIANTS } from '../utils'; -import { - isNewExpression, - isImportSpecifier, + findClosestCallExpressionNode, + getIdentifierNode, isCallExpression, + isNewExpression, + isPromiseIdentifier, } from '../node-utils'; export const RULE_NAME = 'no-promise-in-fire-event'; export type MessageIds = 'noPromiseInFireEvent'; type Options = []; -export default ESLintUtils.RuleCreator(getDocsUrl)({ +export default createTestingLibraryRule({ name: RULE_NAME, meta: { type: 'problem', @@ -28,49 +26,74 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ noPromiseInFireEvent: "A promise shouldn't be passed to a `fireEvent` method, instead pass the DOM element", }, - fixable: 'code', + fixable: null, schema: [], }, defaultOptions: [], - create(context) { - return { - 'ImportDeclaration[source.value=/testing-library/]'( - node: TSESTree.ImportDeclaration - ) { - const fireEventImportNode = node.specifiers.find( - (specifier) => - isImportSpecifier(specifier) && - specifier.imported && - 'fireEvent' === specifier.imported.name - ) as TSESTree.ImportSpecifier; + create(context, _, helpers) { + function checkSuspiciousNode( + node: TSESTree.Node, + originalNode?: TSESTree.Node + ): void { + if (ASTUtils.isAwaitExpression(node)) { + return; + } - const { references } = context.getDeclaredVariables( - fireEventImportNode - )[0]; + if (isNewExpression(node)) { + if (isPromiseIdentifier(node.callee)) { + return context.report({ + node: originalNode ?? node, + messageId: 'noPromiseInFireEvent', + }); + } + } - for (const reference of references) { - const referenceNode = reference.identifier; - const callExpression = referenceNode.parent - .parent as TSESTree.CallExpression; - const [element] = callExpression.arguments as TSESTree.Node[]; - if (isCallExpression(element) || isNewExpression(element)) { - const methodName = ASTUtils.isIdentifier(element.callee) - ? element.callee.name - : ((element.callee as TSESTree.MemberExpression) - .property as TSESTree.Identifier).name; + if (isCallExpression(node)) { + const domElementIdentifier = getIdentifierNode(node); + + if ( + helpers.isAsyncQuery(domElementIdentifier) || + isPromiseIdentifier(domElementIdentifier) + ) { + return context.report({ + node: originalNode ?? node, + messageId: 'noPromiseInFireEvent', + }); + } + } + + if (ASTUtils.isIdentifier(node)) { + const nodeVariable = ASTUtils.findVariable( + context.getScope(), + node.name + ); + if (!nodeVariable || !nodeVariable.defs) { + return; + } - if ( - ASYNC_QUERIES_VARIANTS.some((q) => methodName.startsWith(q)) || - methodName === 'Promise' - ) { - context.report({ - node: element, - messageId: 'noPromiseInFireEvent', - }); - } - } + for (const definition of nodeVariable.defs) { + const variableDeclarator = definition.node as TSESTree.VariableDeclarator; + checkSuspiciousNode(variableDeclarator.init, node); } + } + } + + return { + 'CallExpression Identifier'(node: TSESTree.Identifier) { + if (!helpers.isFireEventMethod(node)) { + return; + } + + const closestCallExpression = findClosestCallExpressionNode(node, true); + + if (!closestCallExpression) { + return; + } + + const domElementArgument = closestCallExpression.arguments[0]; + + checkSuspiciousNode(domElementArgument); }, }; }, diff --git a/tests/lib/rules/no-promise-in-fire-event.test.ts b/tests/lib/rules/no-promise-in-fire-event.test.ts index 0ec11de3..99eff5cc 100644 --- a/tests/lib/rules/no-promise-in-fire-event.test.ts +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -25,7 +25,65 @@ ruleTester.run(RULE_NAME, rule, { fireEvent.click(someRef)`, }, { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(await screen.findByRole('button')) + `, + }, + { + code: ` + import {fireEvent} from '@testing-library/foo' + + const elementPromise = screen.findByRole('button') + const button = await elementPromise + fireEvent.click(button)`, + }, + { + settings: { + 'testing-library/module': 'test-utils', + }, + code: `// invalid usage but aggressive reporting opted-out + import { fireEvent } from 'somewhere-else' + fireEvent.click(findByText('submit')) + `, + }, + `// edge case for coverage: + // valid use case without call expression + // so there is no innermost function scope found + test('edge case for no innermost function scope', () => { + const click = fireEvent.click + }) + `, + `// edge case for coverage: + // new expression of something else than Promise + fireEvent.click(new SomeElement()) + `, + ], + invalid: [ + { + // aggressive reporting opted-in code: `fireEvent.click(findByText('submit'))`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + line: 1, + column: 17, + endColumn: 37, + }, + ], + }, + { + // aggressive reporting opted-in + code: `fireEvent.click(Promise())`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + line: 1, + column: 17, + endColumn: 26, + }, + ], }, { code: ` @@ -33,19 +91,30 @@ ruleTester.run(RULE_NAME, rule, { const promise = new Promise(); fireEvent.click(promise)`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + line: 5, + column: 25, + endColumn: 32, + }, + ], }, { code: ` - import {fireEvent} from '@testing-library/foo'; - - fireEvent.click(await screen.findByRole('button')) - `, - }, - { - code: `fireEvent.click(Promise())`, + import {fireEvent} from '@testing-library/foo' + + const elementPromise = screen.findByRole('button') + fireEvent.click(elementPromise)`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + line: 5, + column: 25, + endColumn: 39, + }, + ], }, - ], - invalid: [ { code: ` import {fireEvent} from '@testing-library/foo'; @@ -54,6 +123,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 52, }, ], }, @@ -65,6 +137,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 45, }, ], }, @@ -76,6 +151,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 39, }, ], }, @@ -87,6 +165,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 43, }, ], },