diff --git a/docs/rules/await-async-query.md b/docs/rules/await-async-query.md index fc635c48..162acd2e 100644 --- a/docs/rules/await-async-query.md +++ b/docs/rules/await-async-query.md @@ -12,10 +12,11 @@ found. Those queries variants are: - `findAllBy*` This rule aims to prevent users from forgetting to handle the returned -promise from those async queries to be fulfilled, which could lead to -errors in the tests. The promise will be considered as handled when: +promise from those async queries, which could lead to +problems in the tests. The promise will be considered as handled when: - using the `await` operator +- wrapped within `Promise.all` or `Promise.allSettled` methods - chaining the `then` method - chaining `resolves` or `rejects` from jest - it's returned from a function (in this case, that particular function will be analyzed by this rule too) @@ -70,6 +71,19 @@ const findMyButton = () => findByText('my button'); const someButton = await findMyButton(); ``` +```js +// several promises handled with `Promise.all` is correct +await Promise.all([findByText('my button'), findByText('something else')]); +``` + +```js +// several promises handled `Promise.allSettled` is correct +await Promise.allSettled([ + findByText('my button'), + findByText('something else'), +]); +``` + ```js // using a resolves/rejects matcher is also correct expect(findByTestId('alert')).resolves.toBe('Success'); diff --git a/docs/rules/await-async-utils.md b/docs/rules/await-async-utils.md index 10313f21..d1772aaf 100644 --- a/docs/rules/await-async-utils.md +++ b/docs/rules/await-async-utils.md @@ -1,4 +1,4 @@ -# Enforce async utils to be awaited properly (await-async-utils) +# Enforce promises from async utils to be handled (await-async-utils) Ensure that promises returned by async utils are handled properly. @@ -6,13 +6,21 @@ Ensure that promises returned by async utils are handled properly. Testing library provides several utilities for dealing with asynchronous code. These are useful to wait for an element until certain criteria or situation happens. The available async utils are: -- `waitFor` _(introduced in dom-testing-library v7)_ +- `waitFor` _(introduced since dom-testing-library v7)_ - `waitForElementToBeRemoved` -- `wait` _(**deprecated** in dom-testing-library v7)_ -- `waitForElement` _(**deprecated** in dom-testing-library v7)_ -- `waitForDomChange` _(**deprecated** in dom-testing-library v7)_ +- `wait` _(**deprecated** since dom-testing-library v7)_ +- `waitForElement` _(**deprecated** since dom-testing-library v7)_ +- `waitForDomChange` _(**deprecated** since dom-testing-library v7)_ -This rule aims to prevent users from forgetting to handle the returned promise from those async utils, which could lead to unexpected errors in the tests execution. The promises can be handled by using either `await` operator or `then` method. +This rule aims to prevent users from forgetting to handle the returned +promise from async utils, which could lead to +problems in the tests. The promise will be considered as handled when: + +- using the `await` operator +- wrapped within `Promise.all` or `Promise.allSettled` methods +- chaining the `then` method +- chaining `resolves` or `rejects` from jest +- it's returned from a function (in this case, that particular function will be analyzed by this rule too) Examples of **incorrect** code for this rule: @@ -32,6 +40,14 @@ test('something incorrectly', async () => { waitFor(() => {}, { timeout: 100 }); waitForElementToBeRemoved(() => document.querySelector('div.getOuttaHere')); + + // wrap an async util within a function... + const makeCustomWait = () => { + return waitForElementToBeRemoved(() => + document.querySelector('div.getOuttaHere') + ); + }; + makeCustomWait(); // ...but not handling promise from it is incorrect }); ``` @@ -56,9 +72,13 @@ test('something correctly', async () => { .then(() => console.log('DOM changed!')) .catch((err) => console.log(`Error you need to deal with: ${err}`)); - // return the promise within a function is correct too! - const makeCustomWait = () => - waitForElementToBeRemoved(() => document.querySelector('div.getOuttaHere')); + // wrap an async util within a function... + const makeCustomWait = () => { + return waitForElementToBeRemoved(() => + document.querySelector('div.getOuttaHere') + ); + }; + await makeCustomWait(); // ...and handling promise from it is correct // using Promise.all combining the methods await Promise.all([ diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index ba57a7cd..580a7cbb 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -15,7 +15,7 @@ import { isCallExpression, isObjectPattern, } from './node-utils'; -import { ABSENCE_MATCHERS, PRESENCE_MATCHERS } from './utils'; +import { ABSENCE_MATCHERS, ASYNC_UTILS, PRESENCE_MATCHERS } from './utils'; export type TestingLibrarySettings = { 'testing-library/module'?: string; @@ -53,6 +53,7 @@ export type DetectionHelpers = { isFindByQuery: (node: TSESTree.Identifier) => boolean; isSyncQuery: (node: TSESTree.Identifier) => boolean; isAsyncQuery: (node: TSESTree.Identifier) => boolean; + isAsyncUtil: (node: TSESTree.Identifier) => boolean; isPresenceAssert: (node: TSESTree.MemberExpression) => boolean; isAbsenceAssert: (node: TSESTree.MemberExpression) => boolean; canReportErrors: () => boolean; @@ -168,6 +169,13 @@ export function detectTestingLibraryUtils< return isFindByQuery(node); }; + /** + * Determines whether a given node is async util or not. + */ + const isAsyncUtil: DetectionHelpers['isAsyncUtil'] = (node) => { + return ASYNC_UTILS.includes(node.name); + }; + /** * Determines whether a given MemberExpression node is a presence assert * @@ -312,6 +320,7 @@ export function detectTestingLibraryUtils< isFindByQuery, isSyncQuery, isAsyncQuery, + isAsyncUtil, isPresenceAssert, isAbsenceAssert, canReportErrors, diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 80a650cb..9e0168cb 100644 --- a/lib/node-utils.ts +++ b/lib/node-utils.ts @@ -212,11 +212,45 @@ export function hasChainedThen(node: TSESTree.Node): boolean { return hasThenProperty(parent); } +export function isPromiseAll(node: TSESTree.CallExpression): boolean { + return ( + isMemberExpression(node.callee) && + ASTUtils.isIdentifier(node.callee.object) && + node.callee.object.name === 'Promise' && + ASTUtils.isIdentifier(node.callee.property) && + node.callee.property.name === 'all' + ); +} + +export function isPromiseAllSettled(node: TSESTree.CallExpression): boolean { + return ( + isMemberExpression(node.callee) && + ASTUtils.isIdentifier(node.callee.object) && + node.callee.object.name === 'Promise' && + ASTUtils.isIdentifier(node.callee.property) && + node.callee.property.name === 'allSettled' + ); +} + +export function isPromisesArrayResolved(node: TSESTree.Node): boolean { + const parent = node.parent; + + return ( + isCallExpression(parent) && + isArrayExpression(parent.parent) && + isCallExpression(parent.parent.parent) && + (isPromiseAll(parent.parent.parent) || + isPromiseAllSettled(parent.parent.parent)) + ); +} + /** * Determines whether an Identifier related to a promise is considered as handled. * * It will be considered as handled if: * - it belongs to the `await` expression + * - it belongs to the `Promise.all` method + * - it belongs to the `Promise.allSettled` method * - it's chained with the `then` method * - it's returned from a function * - has `resolves` or `rejects` @@ -250,6 +284,10 @@ export function isPromiseHandled(nodeIdentifier: TSESTree.Identifier): boolean { if (hasChainedThen(node)) { return true; } + + if (isPromisesArrayResolved(node)) { + return true; + } } return false; @@ -476,3 +514,37 @@ export function hasClosestExpectResolvesRejects(node: TSESTree.Node): boolean { return hasClosestExpectResolvesRejects(node.parent); } + +/** + * Gets the Function node which returns the given Identifier. + */ +export function getInnermostReturningFunction( + context: RuleContext, + node: TSESTree.Identifier +): + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression + | TSESTree.ArrowFunctionExpression + | undefined { + const functionScope = getInnermostFunctionScope(context, node); + + if (!functionScope) { + return; + } + + const returnStatementNode = getFunctionReturnStatementNode( + functionScope.block + ); + + if (!returnStatementNode) { + return; + } + + const returnStatementIdentifier = getIdentifierNode(returnStatementNode); + + if (returnStatementIdentifier?.name !== node.name) { + return; + } + + return functionScope.block; +} diff --git a/lib/rules/await-async-query.ts b/lib/rules/await-async-query.ts index a436efe1..f49ff174 100644 --- a/lib/rules/await-async-query.ts +++ b/lib/rules/await-async-query.ts @@ -2,9 +2,7 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { findClosestCallExpressionNode, getFunctionName, - getFunctionReturnStatementNode, - getIdentifierNode, - getInnermostFunctionScope, + getInnermostReturningFunction, getVariableReferences, isPromiseHandled, } from '../node-utils'; @@ -37,25 +35,9 @@ export default createTestingLibraryRule({ const functionWrappersNames: string[] = []; function detectAsyncQueryWrapper(node: TSESTree.Identifier) { - const functionScope = getInnermostFunctionScope(context, node); - - if (functionScope) { - // save function wrapper calls rather than async calls to be reported later - const returnStatementNode = getFunctionReturnStatementNode( - functionScope.block - ); - - if (!returnStatementNode) { - return; - } - - const returnStatementIdentifier = getIdentifierNode( - returnStatementNode - ); - - if (returnStatementIdentifier?.name === node.name) { - functionWrappersNames.push(getFunctionName(functionScope.block)); - } + const innerFunction = getInnermostReturningFunction(context, node); + if (innerFunction) { + functionWrappersNames.push(getFunctionName(innerFunction)); } } diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index 2c2cda9e..75ff5384 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,156 +1,112 @@ +import { TSESTree } from '@typescript-eslint/experimental-utils'; import { - ESLintUtils, - TSESTree, - ASTUtils, -} from '@typescript-eslint/experimental-utils'; - -import { getDocsUrl, ASYNC_UTILS, LIBRARY_MODULES } from '../utils'; -import { - isAwaited, - hasChainedThen, + findClosestCallExpressionNode, + getFunctionName, + getInnermostReturningFunction, getVariableReferences, isMemberExpression, - isImportSpecifier, - isImportNamespaceSpecifier, - isCallExpression, - isArrayExpression, + isPromiseHandled, } from '../node-utils'; +import { createTestingLibraryRule } from '../create-testing-library-rule'; export const RULE_NAME = 'await-async-utils'; -export type MessageIds = 'awaitAsyncUtil'; +export type MessageIds = 'awaitAsyncUtil' | 'asyncUtilWrapper'; type Options = []; -const ASYNC_UTILS_REGEXP = new RegExp(`^(${ASYNC_UTILS.join('|')})$`); - -// verifies the CallExpression is Promise.all() -function isPromiseAll(node: TSESTree.CallExpression) { - return ( - isMemberExpression(node.callee) && - ASTUtils.isIdentifier(node.callee.object) && - node.callee.object.name === 'Promise' && - ASTUtils.isIdentifier(node.callee.property) && - node.callee.property.name === 'all' - ); -} - -// verifies the node is part of an array used in a CallExpression -function isInPromiseAll(node: TSESTree.Node) { - const parent = node.parent; - return ( - isCallExpression(parent) && - isArrayExpression(parent.parent) && - isCallExpression(parent.parent.parent) && - isPromiseAll(parent.parent.parent) - ); -} - -export default ESLintUtils.RuleCreator(getDocsUrl)({ +export default createTestingLibraryRule({ name: RULE_NAME, meta: { type: 'problem', docs: { - description: 'Enforce async utils to be awaited properly', + description: 'Enforce promises from async utils to be handled', category: 'Best Practices', recommended: 'warn', }, messages: { awaitAsyncUtil: 'Promise returned from `{{ name }}` must be handled', + asyncUtilWrapper: + 'Promise returned from {{ name }} wrapper over async util must be handled', }, fixable: null, schema: [], }, defaultOptions: [], - create(context) { - const asyncUtilsUsage: Array<{ - node: TSESTree.Identifier | TSESTree.MemberExpression; - name: string; - }> = []; - const importedAsyncUtils: string[] = []; + create(context, _, helpers) { + const functionWrappersNames: string[] = []; - return { - 'ImportDeclaration > ImportSpecifier,ImportNamespaceSpecifier'( - node: TSESTree.Node - ) { - const parent = node.parent as TSESTree.ImportDeclaration; + function detectAsyncUtilWrapper(node: TSESTree.Identifier) { + const innerFunction = getInnermostReturningFunction(context, node); - if (!LIBRARY_MODULES.includes(parent.source.value.toString())) { - return; - } + if (innerFunction) { + functionWrappersNames.push(getFunctionName(innerFunction)); + } + } - if (isImportSpecifier(node)) { - importedAsyncUtils.push(node.imported.name); - } + return { + 'CallExpression Identifier'(node: TSESTree.Identifier) { + if (helpers.isAsyncUtil(node)) { + if ( + !helpers.isNodeComingFromTestingLibrary(node) && + !( + isMemberExpression(node.parent) && + helpers.isNodeComingFromTestingLibrary(node.parent) + ) + ) { + return; + } - if (isImportNamespaceSpecifier(node)) { - importedAsyncUtils.push(node.local.name); - } - }, - [`CallExpression > Identifier[name=${ASYNC_UTILS_REGEXP}]`]( - node: TSESTree.Identifier - ) { - asyncUtilsUsage.push({ node, name: node.name }); - }, - [`CallExpression > MemberExpression > Identifier[name=${ASYNC_UTILS_REGEXP}]`]( - node: TSESTree.Identifier - ) { - const memberExpression = node.parent as TSESTree.MemberExpression; - const identifier = memberExpression.object as TSESTree.Identifier; - const memberExpressionName = identifier.name; + // detect async query used within wrapper function for later analysis + detectAsyncUtilWrapper(node); - asyncUtilsUsage.push({ - node: memberExpression, - name: memberExpressionName, - }); - }, - 'Program:exit'() { - const testingLibraryUtilUsage = asyncUtilsUsage.filter((usage) => { - if (isMemberExpression(usage.node)) { - const object = usage.node.object as TSESTree.Identifier; + const closestCallExpression = findClosestCallExpressionNode( + node, + true + ); - return importedAsyncUtils.includes(object.name); + if (!closestCallExpression) { + return; } - return importedAsyncUtils.includes(usage.name); - }); - - testingLibraryUtilUsage.forEach(({ node, name }) => { - const references = getVariableReferences(context, node.parent.parent); - - if ( - references && - references.length === 0 && - !isAwaited(node.parent.parent) && - !hasChainedThen(node) && - !isInPromiseAll(node) - ) { - context.report({ - node, - messageId: 'awaitAsyncUtil', - data: { - name, - }, - }); + const references = getVariableReferences( + context, + closestCallExpression.parent + ); + + if (references && references.length === 0) { + if (!isPromiseHandled(node as TSESTree.Identifier)) { + return context.report({ + node, + messageId: 'awaitAsyncUtil', + data: { + name: node.name, + }, + }); + } } else { for (const reference of references) { - const referenceNode = reference.identifier; - if ( - !isAwaited(referenceNode.parent) && - !hasChainedThen(referenceNode) - ) { - context.report({ + const referenceNode = reference.identifier as TSESTree.Identifier; + if (!isPromiseHandled(referenceNode)) { + return context.report({ node, messageId: 'awaitAsyncUtil', data: { - name, + name: referenceNode.name, }, }); - - break; } } } - }); + } else if (functionWrappersNames.includes(node.name)) { + // check async queries used within a wrapper previously detected + if (!isPromiseHandled(node)) { + return context.report({ + node, + messageId: 'asyncUtilWrapper', + data: { name: node.name }, + }); + } + } }, }; }, diff --git a/tests/lib/rules/await-async-query.test.ts b/tests/lib/rules/await-async-query.test.ts index 75e74e8e..3c00a59a 100644 --- a/tests/lib/rules/await-async-query.test.ts +++ b/tests/lib/rules/await-async-query.test.ts @@ -114,6 +114,54 @@ ruleTester.run(RULE_NAME, rule, { ` ), + // async queries are valid when wrapped within Promise.all + await expression + ...createTestCase( + (query) => ` + doSomething() + + await Promise.all([ + ${query}('foo'), + ${query}('bar'), + ]); + ` + ), + + // async queries are valid when wrapped within Promise.all + then chained + ...createTestCase( + (query) => ` + doSomething() + + Promise.all([ + ${query}('foo'), + ${query}('bar'), + ]).then() + ` + ), + + // async queries are valid when wrapped within Promise.allSettled + await expression + ...createTestCase( + (query) => ` + doSomething() + + await Promise.allSettled([ + ${query}('foo'), + ${query}('bar'), + ]); + ` + ), + + // async queries are valid when wrapped within Promise.allSettled + then chained + ...createTestCase( + (query) => ` + doSomething() + + Promise.allSettled([ + ${query}('foo'), + ${query}('bar'), + ]).then() + ` + ), + // async queries are valid with promise returned in arrow function ...createTestCase( (query) => `const anArrowFunction = () => ${query}('foo')` diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index e14ad2c0..506f800f 100644 --- a/tests/lib/rules/await-async-utils.test.ts +++ b/tests/lib/rules/await-async-utils.test.ts @@ -123,7 +123,7 @@ ruleTester.run(RULE_NAME, rule, { ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util used in with Promise.all() does not trigger an error', async () => { + test('${asyncUtil} util used in with Promise.all() is valid', async () => { await Promise.all([ ${asyncUtil}(callback1), ${asyncUtil}(callback2), @@ -134,7 +134,7 @@ ruleTester.run(RULE_NAME, rule, { ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util used in with Promise.all() with an await does not trigger an error', async () => { + test('${asyncUtil} util used in with Promise.all() with an await is valid', async () => { await Promise.all([ await ${asyncUtil}(callback1), await ${asyncUtil}(callback2), @@ -145,7 +145,7 @@ ruleTester.run(RULE_NAME, rule, { ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util used in with Promise.all() with ".then" does not trigger an error', async () => { + test('${asyncUtil} util used in with Promise.all() with ".then" is valid', async () => { Promise.all([ ${asyncUtil}(callback1), ${asyncUtil}(callback2), @@ -174,51 +174,98 @@ ruleTester.run(RULE_NAME, rule, { }); `, }, - { + ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` - test('util not related to testing library is valid', async () => { + import { ${asyncUtil} } from '@somewhere/else'; + test('util unhandled but not related to testing library is valid', async () => { doSomethingElse(); - waitNotRelatedToTestingLibrary(); + ${asyncUtil}('not related to testing library') + waitForNotRelatedToTestingLibrary() }); `, - }, + })), + ...ASYNC_UTILS.map((asyncUtil) => ({ + code: ` + import { ${asyncUtil} } from '@testing-library/dom'; + test('${asyncUtil} util used in Promise.allSettled + await expression is valid', async () => { + await Promise.allSettled([ + ${asyncUtil}(callback1), + ${asyncUtil}(callback2), + ]); + }); + `, + })), + ...ASYNC_UTILS.map((asyncUtil) => ({ + code: ` + import { ${asyncUtil} } from '@testing-library/dom'; + test('${asyncUtil} util used in Promise.allSettled + then method is valid', async () => { + Promise.allSettled([ + ${asyncUtil}(callback1), + ${asyncUtil}(callback2), + ]).then(() => {}) + }); + `, + })), + ...ASYNC_UTILS.map((asyncUtil) => ({ + code: ` + import { ${asyncUtil} } from '@testing-library/dom'; + + function waitForSomethingAsync() { + return ${asyncUtil}(() => somethingAsync()) + } + + test('handled promise from function wrapping ${asyncUtil} util is valid', async () => { + await waitForSomethingAsync() + }); + `, + })), { code: ` - test('using unrelated promises with Promise.all do not throw an error', async () => { - await Promise.all([ - someMethod(), + test('using unrelated promises with Promise.all is valid', async () => { + Promise.all([ + waitForNotRelatedToTestingLibrary(), promise1, await foo().then(() => baz()) ]) }) `, }, + + // edge case for coverage + // valid async query usage without any function defined + // so there is no innermost function scope found + ` + import { waitFor } from '@testing-library/dom'; + test('edge case for no innermost function scope', () => { + const foo = waitFor + }) + `, ], invalid: [ ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util not waited', () => { + test('${asyncUtil} util not waited is invalid', () => { doSomethingElse(); ${asyncUtil}(() => getByLabelText('email')); }); `, - errors: [{ line: 5, messageId: 'awaitAsyncUtil' }], + errors: [{ line: 5, column: 11, messageId: 'awaitAsyncUtil' }], })), ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import * as asyncUtil from '@testing-library/dom'; - test('asyncUtil.${asyncUtil} util not waited', () => { + test('asyncUtil.${asyncUtil} util not handled is invalid', () => { doSomethingElse(); asyncUtil.${asyncUtil}(() => getByLabelText('email')); }); `, - errors: [{ line: 5, messageId: 'awaitAsyncUtil' }], + errors: [{ line: 5, column: 21, messageId: 'awaitAsyncUtil' }], })), ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util promise saved not waited', () => { + test('${asyncUtil} util promise saved not handled is invalid', () => { doSomethingElse(); const aPromise = ${asyncUtil}(() => getByLabelText('email')); }); @@ -228,7 +275,7 @@ ruleTester.run(RULE_NAME, rule, { ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('several ${asyncUtil} utils not waited', () => { + test('several ${asyncUtil} utils not handled are invalid', () => { const aPromise = ${asyncUtil}(() => getByLabelText('username')); doSomethingElse(aPromise); ${asyncUtil}(() => getByLabelText('email')); @@ -239,5 +286,20 @@ ruleTester.run(RULE_NAME, rule, { { line: 6, column: 11, messageId: 'awaitAsyncUtil' }, ], })), + ...ASYNC_UTILS.map((asyncUtil) => ({ + code: ` + import { ${asyncUtil}, render } from '@testing-library/dom'; + + function waitForSomethingAsync() { + return ${asyncUtil}(() => somethingAsync()) + } + + test('unhandled promise from function wrapping ${asyncUtil} util is invalid', async () => { + render() + waitForSomethingAsync() + }); + `, + errors: [{ messageId: 'asyncUtilWrapper', line: 10, column: 11 }], + })), ], });