From dee556f9f356fa60cd11ecd081ef9640903b729a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 2 Dec 2020 20:13:19 +0100 Subject: [PATCH 1/8] refactor(await-async-utils): create rule with custom creator --- tests/lib/rules/await-async-utils.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index 506f800f..c39bf486 100644 --- a/tests/lib/rules/await-async-utils.test.ts +++ b/tests/lib/rules/await-async-utils.test.ts @@ -4,6 +4,8 @@ import { ASYNC_UTILS } from '../../../lib/utils'; const ruleTester = createRuleTester(); +// FIXME: add cases for Promise.allSettled + ruleTester.run(RULE_NAME, rule, { valid: [ ...ASYNC_UTILS.map((asyncUtil) => ({ From b60b423746030872343f8ad36653d809825bc09a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 6 Dec 2020 14:17:44 +0100 Subject: [PATCH 2/8] test(await-async-utils): remove outdated comment --- tests/lib/rules/await-async-utils.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index c39bf486..506f800f 100644 --- a/tests/lib/rules/await-async-utils.test.ts +++ b/tests/lib/rules/await-async-utils.test.ts @@ -4,8 +4,6 @@ import { ASYNC_UTILS } from '../../../lib/utils'; const ruleTester = createRuleTester(); -// FIXME: add cases for Promise.allSettled - ruleTester.run(RULE_NAME, rule, { valid: [ ...ASYNC_UTILS.map((asyncUtil) => ({ From 5cf1d68c8084e7d3a491993a44d63bd7c7e85302 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 6 Dec 2020 18:37:18 +0100 Subject: [PATCH 3/8] docs(no-promise-in-fire-event): improve description and examples --- docs/rules/no-promise-in-fire-event.md | 6 +++--- lib/rules/no-promise-in-fire-event.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/rules/no-promise-in-fire-event.md b/docs/rules/no-promise-in-fire-event.md index 3501e3a6..9f0398df 100644 --- a/docs/rules/no-promise-in-fire-event.md +++ b/docs/rules/no-promise-in-fire-event.md @@ -1,6 +1,6 @@ # 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: @@ -11,7 +11,7 @@ import { screen, fireEvent } from '@testing-library/react'; fireEvent.click(screen.findByRole('button')); // usage of promises -fireEvent.click(new Promise(jest.fn()) +fireEvent.click(new Promise(jest.fn())); ``` Examples of **correct** code for this rule: @@ -27,7 +27,7 @@ fireEvent.click(await screen.findByRole('button')); // this won't give a linting error, but it will throw a runtime error const promise = new Promise(); -fireEvent.click(promise)`, +fireEvent.click(promise); ``` ## Further Reading diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts index c51390a0..ae9218dd 100644 --- a/lib/rules/no-promise-in-fire-event.ts +++ b/lib/rules/no-promise-in-fire-event.ts @@ -28,7 +28,7 @@ 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: [], From 45b8d67166a12016994c30a5ed22394612a434b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 6 Dec 2020 18:44:28 +0100 Subject: [PATCH 4/8] docs(no-promise-in-fire-event): improve invalid errors checks --- tests/lib/rules/no-promise-in-fire-event.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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..5956adae 100644 --- a/tests/lib/rules/no-promise-in-fire-event.test.ts +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -28,6 +28,7 @@ ruleTester.run(RULE_NAME, rule, { code: `fireEvent.click(findByText('submit'))`, }, { + // TODO: report this as invalid code: ` import {fireEvent} from '@testing-library/foo'; @@ -54,6 +55,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 52, }, ], }, @@ -65,6 +69,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 45, }, ], }, @@ -76,6 +83,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 39, }, ], }, @@ -87,6 +97,9 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { messageId: 'noPromiseInFireEvent', + line: 4, + column: 25, + endColumn: 43, }, ], }, From 84cddcb055657f6f7ec276eb1d1486146b893e22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Sun, 6 Dec 2020 20:53:51 +0100 Subject: [PATCH 5/8] refactor(no-promise-in-fire-event): use custom rule creator and helpers --- lib/node-utils.ts | 20 ++--- lib/rules/no-promise-in-fire-event.ts | 87 ++++++++++--------- .../rules/no-promise-in-fire-event.test.ts | 28 ++++-- 3 files changed, 77 insertions(+), 58 deletions(-) diff --git a/lib/node-utils.ts b/lib/node-utils.ts index fcbc2f14..3422b242 100644 --- a/lib/node-utils.ts +++ b/lib/node-utils.ts @@ -191,14 +191,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 +203,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 +221,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' ); diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts index ae9218dd..8090c666 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', @@ -33,42 +31,49 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ }, defaultOptions: [], - create(context) { + create(context, _, helpers) { 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; + 'CallExpression Identifier'(node: TSESTree.Identifier) { + if (!helpers.isFireEventMethod(node)) { + return; + } + + const closestCallExpression = findClosestCallExpressionNode(node, true); + + if (!closestCallExpression) { + return; + } - const { references } = context.getDeclaredVariables( - fireEventImportNode - )[0]; + const domElementArgument = closestCallExpression.arguments[0]; - 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 (ASTUtils.isAwaitExpression(domElementArgument)) { + return; + } + + if (isNewExpression(domElementArgument)) { + if (isPromiseIdentifier(domElementArgument.callee)) { + return context.report({ + node: domElementArgument, + messageId: 'noPromiseInFireEvent', + }); + } + } + + if (isCallExpression(domElementArgument)) { + const domElementIdentifier = getIdentifierNode(domElementArgument); + + if (!domElementIdentifier) { + return; + } - if ( - ASYNC_QUERIES_VARIANTS.some((q) => methodName.startsWith(q)) || - methodName === 'Promise' - ) { - context.report({ - node: element, - messageId: 'noPromiseInFireEvent', - }); - } + if ( + helpers.isAsyncQuery(domElementIdentifier) || + isPromiseIdentifier(domElementIdentifier) + ) { + return context.report({ + node: domElementArgument, + messageId: 'noPromiseInFireEvent', + }); } } }, 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 5956adae..46cb0567 100644 --- a/tests/lib/rules/no-promise-in-fire-event.test.ts +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -24,9 +24,6 @@ ruleTester.run(RULE_NAME, rule, { fireEvent.click(someRef)`, }, - { - code: `fireEvent.click(findByText('submit'))`, - }, { // TODO: report this as invalid code: ` @@ -42,11 +39,32 @@ ruleTester.run(RULE_NAME, rule, { fireEvent.click(await screen.findByRole('button')) `, }, + ], + 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, + }, + ], }, - ], - invalid: [ { code: ` import {fireEvent} from '@testing-library/foo'; From bfb822009c5eaa1e3ab5b97a2bc36c2480de2ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Mon, 7 Dec 2020 09:53:25 +0100 Subject: [PATCH 6/8] feat(no-promise-in-fire-event): detect promise in variable references --- lib/node-utils.ts | 8 +- lib/rules/no-promise-in-fire-event.ts | 80 ++++++++++++------- .../rules/no-promise-in-fire-event.test.ts | 55 +++++++++++-- 3 files changed, 100 insertions(+), 43 deletions(-) diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 3422b242..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 { @@ -300,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 8090c666..e6b5c3ed 100644 --- a/lib/rules/no-promise-in-fire-event.ts +++ b/lib/rules/no-promise-in-fire-event.ts @@ -32,50 +32,74 @@ export default createTestingLibraryRule({ defaultOptions: [], create(context, _, helpers) { - return { - 'CallExpression Identifier'(node: TSESTree.Identifier) { - if (!helpers.isFireEventMethod(node)) { - return; + function checkSuspiciousNode( + node: TSESTree.Node, + originalNode?: TSESTree.Node + ): void { + if (ASTUtils.isAwaitExpression(node)) { + return; + } + + if (isNewExpression(node)) { + if (isPromiseIdentifier(node.callee)) { + return context.report({ + node: originalNode ?? node, + messageId: 'noPromiseInFireEvent', + }); } + } - const closestCallExpression = findClosestCallExpressionNode(node, true); + if (isCallExpression(node)) { + const domElementIdentifier = getIdentifierNode(node); - if (!closestCallExpression) { + if (!domElementIdentifier) { return; } - const domElementArgument = closestCallExpression.arguments[0]; + if ( + helpers.isAsyncQuery(domElementIdentifier) || + isPromiseIdentifier(domElementIdentifier) + ) { + return context.report({ + node: originalNode ?? node, + messageId: 'noPromiseInFireEvent', + }); + } + } - if (ASTUtils.isAwaitExpression(domElementArgument)) { + if (ASTUtils.isIdentifier(node)) { + const nodeVariable = ASTUtils.findVariable( + context.getScope(), + node.name + ); + if (!nodeVariable || !nodeVariable.defs) { return; } - if (isNewExpression(domElementArgument)) { - if (isPromiseIdentifier(domElementArgument.callee)) { - return context.report({ - node: domElementArgument, - messageId: 'noPromiseInFireEvent', - }); + for (const definition of nodeVariable.defs) { + if (!ASTUtils.isVariableDeclarator(definition.node)) { + return; } + checkSuspiciousNode(definition.node.init, node); } + } + } - if (isCallExpression(domElementArgument)) { - const domElementIdentifier = getIdentifierNode(domElementArgument); + return { + 'CallExpression Identifier'(node: TSESTree.Identifier) { + if (!helpers.isFireEventMethod(node)) { + return; + } - if (!domElementIdentifier) { - return; - } + const closestCallExpression = findClosestCallExpressionNode(node, true); - if ( - helpers.isAsyncQuery(domElementIdentifier) || - isPromiseIdentifier(domElementIdentifier) - ) { - return context.report({ - node: domElementArgument, - messageId: 'noPromiseInFireEvent', - }); - } + 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 46cb0567..298aa47f 100644 --- a/tests/lib/rules/no-promise-in-fire-event.test.ts +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -24,14 +24,6 @@ ruleTester.run(RULE_NAME, rule, { fireEvent.click(someRef)`, }, - { - // TODO: report this as invalid - code: ` - import {fireEvent} from '@testing-library/foo'; - - const promise = new Promise(); - fireEvent.click(promise)`, - }, { code: ` import {fireEvent} from '@testing-library/foo'; @@ -39,6 +31,23 @@ ruleTester.run(RULE_NAME, rule, { 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')) + `, + }, ], invalid: [ { @@ -69,6 +78,36 @@ ruleTester.run(RULE_NAME, rule, { code: ` import {fireEvent} from '@testing-library/foo'; + const promise = new Promise(); + fireEvent.click(promise)`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + line: 5, + column: 25, + endColumn: 32, + }, + ], + }, + { + code: ` + import {fireEvent} from '@testing-library/foo' + + const elementPromise = screen.findByRole('button') + fireEvent.click(elementPromise)`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + line: 5, + column: 25, + endColumn: 39, + }, + ], + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + fireEvent.click(screen.findByRole('button'))`, errors: [ { From 74b87e37ef658b7d7aab0baeadf775388adbabd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Mon, 7 Dec 2020 10:03:54 +0100 Subject: [PATCH 7/8] docs(no-promise-in-fire-event): update examples --- docs/rules/no-promise-in-fire-event.md | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/docs/rules/no-promise-in-fire-event.md b/docs/rules/no-promise-in-fire-event.md index 9f0398df..2c8c238c 100644 --- a/docs/rules/no-promise-in-fire-event.md +++ b/docs/rules/no-promise-in-fire-event.md @@ -7,11 +7,18 @@ 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 +// 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 From 3f1b5e6fa3003f77a73790749bacb6f30e1e69ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Mon, 7 Dec 2020 11:38:23 +0100 Subject: [PATCH 8/8] test(no-promise-in-fire-event): increase coverage up to 100% --- lib/rules/no-promise-in-fire-event.ts | 10 ++-------- tests/lib/rules/no-promise-in-fire-event.test.ts | 11 +++++++++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts index e6b5c3ed..e83500fe 100644 --- a/lib/rules/no-promise-in-fire-event.ts +++ b/lib/rules/no-promise-in-fire-event.ts @@ -52,10 +52,6 @@ export default createTestingLibraryRule({ if (isCallExpression(node)) { const domElementIdentifier = getIdentifierNode(node); - if (!domElementIdentifier) { - return; - } - if ( helpers.isAsyncQuery(domElementIdentifier) || isPromiseIdentifier(domElementIdentifier) @@ -77,10 +73,8 @@ export default createTestingLibraryRule({ } for (const definition of nodeVariable.defs) { - if (!ASTUtils.isVariableDeclarator(definition.node)) { - return; - } - checkSuspiciousNode(definition.node.init, node); + const variableDeclarator = definition.node as TSESTree.VariableDeclarator; + checkSuspiciousNode(variableDeclarator.init, node); } } } 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 298aa47f..99eff5cc 100644 --- a/tests/lib/rules/no-promise-in-fire-event.test.ts +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -48,6 +48,17 @@ ruleTester.run(RULE_NAME, rule, { 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: [ {