From 28d69baef394b8bb4bc6710c4ad3991f64777441 Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sat, 20 Jun 2020 16:13:00 +0200 Subject: [PATCH 1/5] feat(no-promise-in-fire-event): add new no-promise-in-fire-event rule --- docs/rules/no-promise-in-fire-event.md | 31 +++++++ lib/index.ts | 2 + lib/node-utils.ts | 14 ++- lib/rules/no-promise-in-fire-event.ts | 83 +++++++++++++++++ .../rules/no-promise-in-fire-event.test.ts | 93 +++++++++++++++++++ 5 files changed, 220 insertions(+), 3 deletions(-) create mode 100644 docs/rules/no-promise-in-fire-event.md create mode 100644 lib/rules/no-promise-in-fire-event.ts create mode 100644 tests/lib/rules/no-promise-in-fire-event.test.ts diff --git a/docs/rules/no-promise-in-fire-event.md b/docs/rules/no-promise-in-fire-event.md new file mode 100644 index 00000000..aad643b5 --- /dev/null +++ b/docs/rules/no-promise-in-fire-event.md @@ -0,0 +1,31 @@ +# 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. + +Examples of **incorrect** code for this rule: + +```js +import { screen, fireEvent } from '@testing-library/react'; + +// usage of findBy queries +fireEvent.click(screen.findByRole('button')); + +// usage of promises +fireEvent.click(new Promise(jest.fn()) +``` + +Examples of **correct** code for this rule: + +```js +import { screen, fireEvent } from '@testing-library/react'; + +// use getBy queries +fireEvent.click(screen.getByRole('button')); + +// use awaited findBy queries +fireEvent.click(await screen.findByRole('button')); +``` + +## Further Reading + +- [A Github Issue explaining the problem](https://github.com/testing-library/dom-testing-library/issues/609) diff --git a/lib/index.ts b/lib/index.ts index 22996a13..6a1355cb 100644 --- a/lib/index.ts +++ b/lib/index.ts @@ -7,6 +7,7 @@ import noDebug from './rules/no-debug'; import noDomImport from './rules/no-dom-import'; import noManualCleanup from './rules/no-manual-cleanup'; import noWaitForEmptyCallback from './rules/no-wait-for-empty-callback'; +import noPromiseInFireEvent from './rules/no-promise-in-fire-event'; import preferExplicitAssert from './rules/prefer-explicit-assert'; import preferPresenceQueries from './rules/prefer-presence-queries'; import preferScreenQueries from './rules/prefer-screen-queries'; @@ -22,6 +23,7 @@ const rules = { 'no-debug': noDebug, 'no-dom-import': noDomImport, 'no-manual-cleanup': noManualCleanup, + 'no-promise-in-fire-event': noPromiseInFireEvent, 'no-wait-for-empty-callback': noWaitForEmptyCallback, 'prefer-explicit-assert': preferExplicitAssert, 'prefer-find-by': preferFindBy, diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 53e788b8..544df8fc 100644 --- a/lib/node-utils.ts +++ b/lib/node-utils.ts @@ -12,6 +12,12 @@ export function isAwaitExpression( return node && node.type === 'AwaitExpression'; } +export function isNewExpression( + node: TSESTree.Node +): node is TSESTree.NewExpression { + return node && node.type === 'NewExpression'; +} + export function isIdentifier(node: TSESTree.Node): node is TSESTree.Identifier { return node && node.type === 'Identifier'; } @@ -103,6 +109,8 @@ export function hasThenProperty(node: TSESTree.Node) { ); } -export function isArrowFunctionExpression(node: TSESTree.Node): node is TSESTree.ArrowFunctionExpression { - return node && node.type === 'ArrowFunctionExpression' -} \ No newline at end of file +export function isArrowFunctionExpression( + node: TSESTree.Node +): node is TSESTree.ArrowFunctionExpression { + return node && node.type === 'ArrowFunctionExpression'; +} diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts new file mode 100644 index 00000000..967c798c --- /dev/null +++ b/lib/rules/no-promise-in-fire-event.ts @@ -0,0 +1,83 @@ +import { TSESTree, ESLintUtils } from '@typescript-eslint/experimental-utils'; +import { getDocsUrl, ASYNC_QUERIES_VARIANTS } from '../utils'; +import { + isNewExpression, + isIdentifier, + isMemberExpression, + isImportSpecifier, + isCallExpression, +} from '../node-utils'; + +export const RULE_NAME = 'no-promise-in-fire-event'; +export type MessageIds = 'noPromiseInFireEvent'; +type Options = []; + +export default ESLintUtils.RuleCreator(getDocsUrl)({ + name: RULE_NAME, + meta: { + type: 'problem', + docs: { + description: + 'Disallow the use of promises passed to a `fireEvent` method', + category: 'Best Practices', + recommended: false, + }, + messages: { + noPromiseInFireEvent: + "A promise shouldn't be passed to a `fireEvent` method, instead pass the DOM element", + }, + fixable: 'code', + 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; + + const { references } = context.getDeclaredVariables( + fireEventImportNode + )[0]; + + for (const reference of references) { + const referenceNode = reference.identifier; + if ( + isMemberExpression(referenceNode.parent) && + isCallExpression(referenceNode.parent.parent) + ) { + const [element] = referenceNode.parent.parent + .arguments as TSESTree.Node[]; + if (element) { + if (isCallExpression(element) || isNewExpression(element)) { + const methodName = isIdentifier(element.callee) + ? element.callee.name + : isMemberExpression(element.callee) && + isIdentifier(element.callee.property) + ? element.callee.property.name + : ''; + + if ( + ASYNC_QUERIES_VARIANTS.some(q => methodName.startsWith(q)) || + methodName === 'Promise' + ) { + context.report({ + node: element, + 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 new file mode 100644 index 00000000..db40224b --- /dev/null +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -0,0 +1,93 @@ +import { createRuleTester } from '../test-utils'; +import rule, { RULE_NAME } from '../../../lib/rules/no-promise-in-fire-event'; + +const ruleTester = createRuleTester(); + +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(screen.getByRole('button')) + `, + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(queryByRole('button'))`, + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(someRef)`, + }, + { + code: `fireEvent.click(findByText('submit'))`, + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(promise)`, + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(await screen.findByRole('button')) + `, + }, + { + code: `fireEvent.click(Promise())`, + }, + ], + invalid: [ + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(screen.findByRole('button'))`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + }, + ], + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(findByText('submit'))`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + }, + ], + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(Promise('foo'))`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + }, + ], + }, + { + code: ` + import {fireEvent} from '@testing-library/foo'; + + fireEvent.click(new Promise('foo'))`, + errors: [ + { + messageId: 'noPromiseInFireEvent', + }, + ], + }, + ], +}); From 7315edadf39df45caad89405748485b80c1b9944 Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sat, 20 Jun 2020 16:53:01 +0200 Subject: [PATCH 2/5] test: 100% code coverage --- lib/rules/no-promise-in-fire-event.ts | 40 +++++++++++---------------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts index 967c798c..4779a1c2 100644 --- a/lib/rules/no-promise-in-fire-event.ts +++ b/lib/rules/no-promise-in-fire-event.ts @@ -49,31 +49,23 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ for (const reference of references) { const referenceNode = reference.identifier; - if ( - isMemberExpression(referenceNode.parent) && - isCallExpression(referenceNode.parent.parent) - ) { - const [element] = referenceNode.parent.parent - .arguments as TSESTree.Node[]; - if (element) { - if (isCallExpression(element) || isNewExpression(element)) { - const methodName = isIdentifier(element.callee) - ? element.callee.name - : isMemberExpression(element.callee) && - isIdentifier(element.callee.property) - ? element.callee.property.name - : ''; + const callExpression = referenceNode.parent + .parent as TSESTree.CallExpression; + const [element] = callExpression.arguments as TSESTree.Node[]; + if (isCallExpression(element) || isNewExpression(element)) { + const methodName = isIdentifier(element.callee) + ? element.callee.name + : ((element.callee as TSESTree.MemberExpression) + .property as TSESTree.Identifier).name; - if ( - ASYNC_QUERIES_VARIANTS.some(q => methodName.startsWith(q)) || - methodName === 'Promise' - ) { - context.report({ - node: element, - messageId: 'noPromiseInFireEvent', - }); - } - } + if ( + ASYNC_QUERIES_VARIANTS.some(q => methodName.startsWith(q)) || + methodName === 'Promise' + ) { + context.report({ + node: element, + messageId: 'noPromiseInFireEvent', + }); } } } From a00d01a49b4c119d226740aeba5b66a068a9ad46 Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sat, 20 Jun 2020 17:01:00 +0200 Subject: [PATCH 3/5] docs: add rule to readme --- README.md | 1 + lib/rules/no-promise-in-fire-event.ts | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 43895ff4..612210d9 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,7 @@ To enable this configuration use the `extends` property in your | [no-debug](docs/rules/no-debug.md) | Disallow the use of `debug` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [no-dom-import](docs/rules/no-dom-import.md) | Disallow importing from DOM Testing Library | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | | [no-manual-cleanup](docs/rules/no-manual-cleanup.md) | Disallow the use of `cleanup` | | | +| [no-promise-in-fire-event](docs/rules/no-promise-in-fire-event.md) | Disallow the use of promises passed to a `fireEvent` method | | | | [no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | | [prefer-find-by](docs/rules/prefer-find-by.md) | Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | diff --git a/lib/rules/no-promise-in-fire-event.ts b/lib/rules/no-promise-in-fire-event.ts index 4779a1c2..3221a3f1 100644 --- a/lib/rules/no-promise-in-fire-event.ts +++ b/lib/rules/no-promise-in-fire-event.ts @@ -3,7 +3,6 @@ import { getDocsUrl, ASYNC_QUERIES_VARIANTS } from '../utils'; import { isNewExpression, isIdentifier, - isMemberExpression, isImportSpecifier, isCallExpression, } from '../node-utils'; From 9e4d3119e648ed1e1df1c70a350a18b99d6d6c46 Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sun, 21 Jun 2020 10:01:14 +0200 Subject: [PATCH 4/5] chore: review changes --- docs/rules/no-promise-in-fire-event.md | 4 ++++ tests/lib/rules/no-promise-in-fire-event.test.ts | 1 + 2 files changed, 5 insertions(+) diff --git a/docs/rules/no-promise-in-fire-event.md b/docs/rules/no-promise-in-fire-event.md index aad643b5..3501e3a6 100644 --- a/docs/rules/no-promise-in-fire-event.md +++ b/docs/rules/no-promise-in-fire-event.md @@ -24,6 +24,10 @@ fireEvent.click(screen.getByRole('button')); // use awaited findBy queries 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)`, ``` ## Further Reading 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 db40224b..0ec11de3 100644 --- a/tests/lib/rules/no-promise-in-fire-event.test.ts +++ b/tests/lib/rules/no-promise-in-fire-event.test.ts @@ -31,6 +31,7 @@ ruleTester.run(RULE_NAME, rule, { code: ` import {fireEvent} from '@testing-library/foo'; + const promise = new Promise(); fireEvent.click(promise)`, }, { From fc084fab48ebdf70a2ad47a6ed54e9afbf101eba Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sun, 21 Jun 2020 10:21:15 +0200 Subject: [PATCH 5/5] chore: add rule to recommended config --- lib/index.ts | 1 + tests/__snapshots__/index.test.ts.snap | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/lib/index.ts b/lib/index.ts index 6a1355cb..886c2cd6 100644 --- a/lib/index.ts +++ b/lib/index.ts @@ -36,6 +36,7 @@ const recommendedRules = { 'testing-library/await-async-query': 'error', 'testing-library/await-async-utils': 'error', 'testing-library/no-await-sync-query': 'error', + 'testing-library/no-promise-in-fire-event': 'error', 'testing-library/no-wait-for-empty-callback': 'error', 'testing-library/prefer-find-by': 'error', 'testing-library/prefer-screen-queries': 'error', diff --git a/tests/__snapshots__/index.test.ts.snap b/tests/__snapshots__/index.test.ts.snap index 17d3f1f9..0113c3ea 100644 --- a/tests/__snapshots__/index.test.ts.snap +++ b/tests/__snapshots__/index.test.ts.snap @@ -14,6 +14,7 @@ Object { "error", "angular", ], + "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error", @@ -35,6 +36,7 @@ Object { "error", "react", ], + "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error", @@ -51,6 +53,7 @@ Object { "testing-library/await-async-query": "error", "testing-library/await-async-utils": "error", "testing-library/no-await-sync-query": "error", + "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error", @@ -73,6 +76,7 @@ Object { "error", "vue", ], + "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", "testing-library/prefer-screen-queries": "error",