From 676458570621e4958ca693ba6584137d44b986c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 2 Dec 2020 20:00:44 +0100 Subject: [PATCH 01/15] refactor: extract utils for checking promise all methods --- lib/node-utils.ts | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 80a650cb..3c8b2e27 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; From 13e201fcb8a9df3de4e2b59b3ff22594e8bb2870 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 2 Dec 2020 20:01:54 +0100 Subject: [PATCH 02/15] test(await-async-query): add cases for promise all and allSettled --- tests/lib/rules/await-async-query.test.ts | 48 +++++++++++++++++++++++ 1 file changed, 48 insertions(+) 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')` From 1a494307ed76a58991fae3805ea440ed86247625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 2 Dec 2020 20:02:17 +0100 Subject: [PATCH 03/15] docs(await-async-query): add cases for promise all and allSettled --- docs/rules/await-async-query.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/rules/await-async-query.md b/docs/rules/await-async-query.md index fc635c48..934cc736 100644 --- a/docs/rules/await-async-query.md +++ b/docs/rules/await-async-query.md @@ -70,6 +70,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'); From 1dfd3896e142f63fc09077f4c0084211faf99295 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 04/15] refactor(await-async-utils): create rule with custom creator --- lib/rules/await-async-query.ts | 1 - lib/rules/await-async-utils.ts | 25 ++++++++++------------- tests/lib/rules/await-async-utils.test.ts | 2 ++ 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/rules/await-async-query.ts b/lib/rules/await-async-query.ts index a436efe1..cb1a3e70 100644 --- a/lib/rules/await-async-query.ts +++ b/lib/rules/await-async-query.ts @@ -40,7 +40,6 @@ export default createTestingLibraryRule({ const functionScope = getInnermostFunctionScope(context, node); if (functionScope) { - // save function wrapper calls rather than async calls to be reported later const returnStatementNode = getFunctionReturnStatementNode( functionScope.block ); diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index 2c2cda9e..5cfdd660 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,20 +1,17 @@ -import { - ESLintUtils, - TSESTree, - ASTUtils, -} from '@typescript-eslint/experimental-utils'; +import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; -import { getDocsUrl, ASYNC_UTILS, LIBRARY_MODULES } from '../utils'; +import { ASYNC_UTILS, LIBRARY_MODULES } from '../utils'; import { - isAwaited, - hasChainedThen, getVariableReferences, - isMemberExpression, - isImportSpecifier, - isImportNamespaceSpecifier, - isCallExpression, + hasChainedThen, isArrayExpression, + isAwaited, + isCallExpression, + isImportNamespaceSpecifier, + isImportSpecifier, + isMemberExpression, } from '../node-utils'; +import { createTestingLibraryRule } from '../create-testing-library-rule'; export const RULE_NAME = 'await-async-utils'; export type MessageIds = 'awaitAsyncUtil'; @@ -44,12 +41,12 @@ function isInPromiseAll(node: TSESTree.Node) { ); } -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', }, diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index e14ad2c0..05ae151e 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 273ac92343ee4779c8659ac728e00b13b6c26b97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 2 Dec 2020 20:20:16 +0100 Subject: [PATCH 05/15] refactor(await-async-utils): replace async utils regexp by method --- lib/detect-testing-library-utils.ts | 11 ++++++++++- lib/rules/await-async-utils.ts | 20 ++++++++++++-------- 2 files changed, 22 insertions(+), 9 deletions(-) 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/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index 5cfdd660..fefe36ea 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,6 +1,6 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; -import { ASYNC_UTILS, LIBRARY_MODULES } from '../utils'; +import { LIBRARY_MODULES } from '../utils'; import { getVariableReferences, hasChainedThen, @@ -17,8 +17,6 @@ export const RULE_NAME = 'await-async-utils'; export type MessageIds = 'awaitAsyncUtil'; type Options = []; -const ASYNC_UTILS_REGEXP = new RegExp(`^(${ASYNC_UTILS.join('|')})$`); - // verifies the CallExpression is Promise.all() function isPromiseAll(node: TSESTree.CallExpression) { return ( @@ -58,7 +56,7 @@ export default createTestingLibraryRule({ }, defaultOptions: [], - create(context) { + create(context, _, helpers) { const asyncUtilsUsage: Array<{ node: TSESTree.Identifier | TSESTree.MemberExpression; name: string; @@ -83,14 +81,20 @@ export default createTestingLibraryRule({ importedAsyncUtils.push(node.local.name); } }, - [`CallExpression > Identifier[name=${ASYNC_UTILS_REGEXP}]`]( - node: TSESTree.Identifier - ) { + [`CallExpression > Identifier`](node: TSESTree.Identifier) { + if (!helpers.isAsyncUtil(node)) { + return; + } + asyncUtilsUsage.push({ node, name: node.name }); }, - [`CallExpression > MemberExpression > Identifier[name=${ASYNC_UTILS_REGEXP}]`]( + [`CallExpression > MemberExpression > Identifier`]( node: TSESTree.Identifier ) { + if (!helpers.isAsyncUtil(node)) { + return; + } + const memberExpression = node.parent as TSESTree.MemberExpression; const identifier = memberExpression.object as TSESTree.Identifier; const memberExpressionName = identifier.name; From b2fdb907d79824c61497d4cc939e23f70ab29157 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Wed, 2 Dec 2020 20:50:36 +0100 Subject: [PATCH 06/15] refactor(await-async-utils): replace manual import checks by helper --- lib/rules/await-async-utils.ts | 32 ++--------------------- tests/lib/rules/await-async-utils.test.ts | 12 +++++---- 2 files changed, 9 insertions(+), 35 deletions(-) diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index fefe36ea..6cc335b3 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,14 +1,10 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; - -import { LIBRARY_MODULES } from '../utils'; import { getVariableReferences, hasChainedThen, isArrayExpression, isAwaited, isCallExpression, - isImportNamespaceSpecifier, - isImportSpecifier, isMemberExpression, } from '../node-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; @@ -61,26 +57,8 @@ export default createTestingLibraryRule({ node: TSESTree.Identifier | TSESTree.MemberExpression; name: string; }> = []; - const importedAsyncUtils: string[] = []; return { - 'ImportDeclaration > ImportSpecifier,ImportNamespaceSpecifier'( - node: TSESTree.Node - ) { - const parent = node.parent as TSESTree.ImportDeclaration; - - if (!LIBRARY_MODULES.includes(parent.source.value.toString())) { - return; - } - - if (isImportSpecifier(node)) { - importedAsyncUtils.push(node.imported.name); - } - - if (isImportNamespaceSpecifier(node)) { - importedAsyncUtils.push(node.local.name); - } - }, [`CallExpression > Identifier`](node: TSESTree.Identifier) { if (!helpers.isAsyncUtil(node)) { return; @@ -105,14 +83,8 @@ export default createTestingLibraryRule({ }); }, 'Program:exit'() { - const testingLibraryUtilUsage = asyncUtilsUsage.filter((usage) => { - if (isMemberExpression(usage.node)) { - const object = usage.node.object as TSESTree.Identifier; - - return importedAsyncUtils.includes(object.name); - } - - return importedAsyncUtils.includes(usage.name); + const testingLibraryUtilUsage = asyncUtilsUsage.filter(({ node }) => { + return helpers.isNodeComingFromTestingLibrary(node); }); testingLibraryUtilUsage.forEach(({ node, name }) => { diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index 05ae151e..09ca70eb 100644 --- a/tests/lib/rules/await-async-utils.test.ts +++ b/tests/lib/rules/await-async-utils.test.ts @@ -5,6 +5,7 @@ import { ASYNC_UTILS } from '../../../lib/utils'; const ruleTester = createRuleTester(); // FIXME: add cases for Promise.allSettled +// FIXME: check column on invalid cases ruleTester.run(RULE_NAME, rule, { valid: [ @@ -176,19 +177,20 @@ 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') }); `, - }, + })), { code: ` test('using unrelated promises with Promise.all do not throw an error', async () => { await Promise.all([ - someMethod(), + waitFor('not related to testing library'), promise1, await foo().then(() => baz()) ]) From 4d4e35a92cc63931d834f387298712cb8f62065d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 18:35:42 +0100 Subject: [PATCH 07/15] refactor(await-async-utils): replace manual promise checks by util --- lib/rules/await-async-utils.ts | 80 ++++++++++++---------------------- 1 file changed, 29 insertions(+), 51 deletions(-) diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index 6cc335b3..aa71cdc9 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,11 +1,8 @@ -import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; +import { TSESTree } from '@typescript-eslint/experimental-utils'; import { + findClosestCallExpressionNode, getVariableReferences, - hasChainedThen, - isArrayExpression, - isAwaited, - isCallExpression, - isMemberExpression, + isPromiseHandled, } from '../node-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; @@ -13,28 +10,6 @@ export const RULE_NAME = 'await-async-utils'; export type MessageIds = 'awaitAsyncUtil'; type Options = []; -// 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 createTestingLibraryRule({ name: RULE_NAME, meta: { @@ -88,38 +63,41 @@ export default createTestingLibraryRule({ }); testingLibraryUtilUsage.forEach(({ node, name }) => { - const references = getVariableReferences(context, node.parent.parent); + const closestCallExpression = findClosestCallExpressionNode( + node, + true + ); + + if (!closestCallExpression) { + return; + } - 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, + }, + }); + } } 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, }, }); - - break; } } } From ff4507d468cf59be67121d5770ae74d67693d7c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 18:48:34 +0100 Subject: [PATCH 08/15] refactor(await-async-utils): merge identifier and member expression nodes checks --- lib/rules/await-async-utils.ts | 86 +++++++++++++--------------------- 1 file changed, 32 insertions(+), 54 deletions(-) diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index aa71cdc9..64ba6b19 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -2,6 +2,7 @@ import { TSESTree } from '@typescript-eslint/experimental-utils'; import { findClosestCallExpressionNode, getVariableReferences, + isMemberExpression, isPromiseHandled, } from '../node-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; @@ -28,80 +29,57 @@ export default createTestingLibraryRule({ defaultOptions: [], create(context, _, helpers) { - const asyncUtilsUsage: Array<{ - node: TSESTree.Identifier | TSESTree.MemberExpression; - name: string; - }> = []; - return { - [`CallExpression > Identifier`](node: TSESTree.Identifier) { + 'CallExpression Identifier'(node: TSESTree.Identifier) { if (!helpers.isAsyncUtil(node)) { return; } - asyncUtilsUsage.push({ node, name: node.name }); - }, - [`CallExpression > MemberExpression > Identifier`]( - node: TSESTree.Identifier - ) { - if (!helpers.isAsyncUtil(node)) { + if ( + !helpers.isNodeComingFromTestingLibrary(node) && + !( + isMemberExpression(node.parent) && + helpers.isNodeComingFromTestingLibrary(node.parent) + ) + ) { return; } - const memberExpression = node.parent as TSESTree.MemberExpression; - const identifier = memberExpression.object as TSESTree.Identifier; - const memberExpressionName = identifier.name; + const closestCallExpression = findClosestCallExpressionNode(node, true); - asyncUtilsUsage.push({ - node: memberExpression, - name: memberExpressionName, - }); - }, - 'Program:exit'() { - const testingLibraryUtilUsage = asyncUtilsUsage.filter(({ node }) => { - return helpers.isNodeComingFromTestingLibrary(node); - }); + if (!closestCallExpression) { + return; + } - testingLibraryUtilUsage.forEach(({ node, name }) => { - const closestCallExpression = findClosestCallExpressionNode( - node, - true - ); + const references = getVariableReferences( + context, + closestCallExpression.parent + ); - if (!closestCallExpression) { - return; + if (references && references.length === 0) { + if (!isPromiseHandled(node as TSESTree.Identifier)) { + return context.report({ + node, + messageId: 'awaitAsyncUtil', + data: { + name: node.name, + }, + }); } - - const references = getVariableReferences( - context, - closestCallExpression.parent - ); - - if (references && references.length === 0) { - if (!isPromiseHandled(node as TSESTree.Identifier)) { + } else { + for (const reference of references) { + const referenceNode = reference.identifier as TSESTree.Identifier; + if (!isPromiseHandled(referenceNode)) { return context.report({ node, messageId: 'awaitAsyncUtil', data: { - name, + name: referenceNode.name, }, }); } - } else { - for (const reference of references) { - const referenceNode = reference.identifier as TSESTree.Identifier; - if (!isPromiseHandled(referenceNode)) { - return context.report({ - node, - messageId: 'awaitAsyncUtil', - data: { - name, - }, - }); - } - } } - }); + } }, }; }, From 77c41e9ce9bdacba79152c8d0bac7ca1a54e0884 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 19:07:18 +0100 Subject: [PATCH 09/15] test(await-async-query): check column on invalid cases --- tests/lib/rules/await-async-utils.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index 09ca70eb..13e4323c 100644 --- a/tests/lib/rules/await-async-utils.test.ts +++ b/tests/lib/rules/await-async-utils.test.ts @@ -5,7 +5,6 @@ import { ASYNC_UTILS } from '../../../lib/utils'; const ruleTester = createRuleTester(); // FIXME: add cases for Promise.allSettled -// FIXME: check column on invalid cases ruleTester.run(RULE_NAME, rule, { valid: [ @@ -207,7 +206,7 @@ ruleTester.run(RULE_NAME, rule, { ${asyncUtil}(() => getByLabelText('email')); }); `, - errors: [{ line: 5, messageId: 'awaitAsyncUtil' }], + errors: [{ line: 5, column: 11, messageId: 'awaitAsyncUtil' }], })), ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` @@ -217,7 +216,7 @@ ruleTester.run(RULE_NAME, rule, { asyncUtil.${asyncUtil}(() => getByLabelText('email')); }); `, - errors: [{ line: 5, messageId: 'awaitAsyncUtil' }], + errors: [{ line: 5, column: 21, messageId: 'awaitAsyncUtil' }], })), ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` From 20ce9034555f4e1388208849ce6da03dd5067ad2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 19:54:17 +0100 Subject: [PATCH 10/15] test(await-async-query): promise.allSettled cases --- tests/lib/rules/await-async-utils.test.ts | 29 +++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index 13e4323c..4b5cb22e 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) => ({ @@ -182,14 +180,37 @@ ruleTester.run(RULE_NAME, rule, { test('util unhandled but not related to testing library is valid', async () => { doSomethingElse(); ${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 does not trigger an error', 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 does not trigger an error', async () => { + Promise.allSettled([ + ${asyncUtil}(callback1), + ${asyncUtil}(callback2), + ]).then(() => {}) }); `, })), { code: ` test('using unrelated promises with Promise.all do not throw an error', async () => { - await Promise.all([ - waitFor('not related to testing library'), + Promise.all([ + waitForNotRelatedToTestingLibrary(), promise1, await foo().then(() => baz()) ]) From 90335bc8839f52755aac990b0cf325cefa068f4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 20:24:49 +0100 Subject: [PATCH 11/15] refactor(await-async-query): extract util to get innermost returning function name --- lib/node-utils.ts | 30 ++++++++++++++++++++++++++++++ lib/rules/await-async-query.ts | 26 +++----------------------- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 3c8b2e27..875e43a5 100644 --- a/lib/node-utils.ts +++ b/lib/node-utils.ts @@ -514,3 +514,33 @@ export function hasClosestExpectResolvesRejects(node: TSESTree.Node): boolean { return hasClosestExpectResolvesRejects(node.parent); } + +/** + * Gets the name of the function which returns the given Identifier. + */ +export function getInnermostReturningFunctionName( + context: RuleContext, + node: TSESTree.Identifier +): string | 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 getFunctionName(functionScope.block); +} diff --git a/lib/rules/await-async-query.ts b/lib/rules/await-async-query.ts index cb1a3e70..c1474fb8 100644 --- a/lib/rules/await-async-query.ts +++ b/lib/rules/await-async-query.ts @@ -1,10 +1,7 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { findClosestCallExpressionNode, - getFunctionName, - getFunctionReturnStatementNode, - getIdentifierNode, - getInnermostFunctionScope, + getInnermostReturningFunctionName, getVariableReferences, isPromiseHandled, } from '../node-utils'; @@ -37,25 +34,8 @@ export default createTestingLibraryRule({ const functionWrappersNames: string[] = []; function detectAsyncQueryWrapper(node: TSESTree.Identifier) { - const functionScope = getInnermostFunctionScope(context, node); - - if (functionScope) { - const returnStatementNode = getFunctionReturnStatementNode( - functionScope.block - ); - - if (!returnStatementNode) { - return; - } - - const returnStatementIdentifier = getIdentifierNode( - returnStatementNode - ); - - if (returnStatementIdentifier?.name === node.name) { - functionWrappersNames.push(getFunctionName(functionScope.block)); - } - } + const functionName = getInnermostReturningFunctionName(context, node); + functionName && functionWrappersNames.push(functionName); } return { From 674b2ce6f2707ce35039c7f63cc7840021ccac9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 20:37:07 +0100 Subject: [PATCH 12/15] feat(await-async-utils): report unhandled functions wrapping async utils --- lib/rules/await-async-utils.ts | 96 ++++++++++++++--------- tests/lib/rules/await-async-utils.test.ts | 48 +++++++++--- 2 files changed, 98 insertions(+), 46 deletions(-) diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index 64ba6b19..4594113a 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,6 +1,7 @@ import { TSESTree } from '@typescript-eslint/experimental-utils'; import { findClosestCallExpressionNode, + getInnermostReturningFunctionName, getVariableReferences, isMemberExpression, isPromiseHandled, @@ -8,7 +9,7 @@ import { 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 = []; export default createTestingLibraryRule({ @@ -22,6 +23,8 @@ export default createTestingLibraryRule({ }, messages: { awaitAsyncUtil: 'Promise returned from `{{ name }}` must be handled', + asyncUtilWrapper: + 'Promise returned from {{ name }} wrapper over async util must be handled', }, fixable: null, schema: [], @@ -29,55 +32,76 @@ export default createTestingLibraryRule({ defaultOptions: [], create(context, _, helpers) { + const functionWrappersNames: string[] = []; + + function detectAsyncUtilWrapper(node: TSESTree.Identifier) { + const functionName = getInnermostReturningFunctionName(context, node); + + functionName && functionWrappersNames.push(functionName); + } + return { 'CallExpression Identifier'(node: TSESTree.Identifier) { - if (!helpers.isAsyncUtil(node)) { - return; - } + if (helpers.isAsyncUtil(node)) { + if ( + !helpers.isNodeComingFromTestingLibrary(node) && + !( + isMemberExpression(node.parent) && + helpers.isNodeComingFromTestingLibrary(node.parent) + ) + ) { + return; + } - if ( - !helpers.isNodeComingFromTestingLibrary(node) && - !( - isMemberExpression(node.parent) && - helpers.isNodeComingFromTestingLibrary(node.parent) - ) - ) { - return; - } + // detect async query used within wrapper function for later analysis + detectAsyncUtilWrapper(node); - const closestCallExpression = findClosestCallExpressionNode(node, true); + const closestCallExpression = findClosestCallExpressionNode( + node, + true + ); - if (!closestCallExpression) { - return; - } + if (!closestCallExpression) { + return; + } - const references = getVariableReferences( - context, - closestCallExpression.parent - ); + 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 as TSESTree.Identifier; - if (!isPromiseHandled(referenceNode)) { + if (references && references.length === 0) { + if (!isPromiseHandled(node as TSESTree.Identifier)) { return context.report({ node, messageId: 'awaitAsyncUtil', data: { - name: referenceNode.name, + name: node.name, }, }); } + } else { + for (const reference of references) { + const referenceNode = reference.identifier as TSESTree.Identifier; + if (!isPromiseHandled(referenceNode)) { + return context.report({ + node, + messageId: 'awaitAsyncUtil', + data: { + name: referenceNode.name, + }, + }); + } + } + } + } 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-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index 4b5cb22e..83c218c6 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), @@ -187,7 +187,7 @@ ruleTester.run(RULE_NAME, rule, { ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util used in Promise.allSettled + await expression does not trigger an error', async () => { + test('${asyncUtil} util used in Promise.allSettled + await expression is valid', async () => { await Promise.allSettled([ ${asyncUtil}(callback1), ${asyncUtil}(callback2), @@ -198,7 +198,7 @@ ruleTester.run(RULE_NAME, rule, { ...ASYNC_UTILS.map((asyncUtil) => ({ code: ` import { ${asyncUtil} } from '@testing-library/dom'; - test('${asyncUtil} util used in Promise.allSettled + then method does not trigger an error', async () => { + test('${asyncUtil} util used in Promise.allSettled + then method is valid', async () => { Promise.allSettled([ ${asyncUtil}(callback1), ${asyncUtil}(callback2), @@ -206,9 +206,22 @@ ruleTester.run(RULE_NAME, rule, { }); `, })), + ...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 () => { + test('using unrelated promises with Promise.all is valid', async () => { Promise.all([ waitForNotRelatedToTestingLibrary(), promise1, @@ -222,7 +235,7 @@ ruleTester.run(RULE_NAME, rule, { ...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')); }); @@ -232,7 +245,7 @@ ruleTester.run(RULE_NAME, rule, { ...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')); }); @@ -242,7 +255,7 @@ ruleTester.run(RULE_NAME, rule, { ...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')); }); @@ -252,7 +265,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')); @@ -263,5 +276,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 }], + })), ], }); From 79f9fc43e5f10d4381bc370101bb98802d0a8deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 20:46:32 +0100 Subject: [PATCH 13/15] docs: minor improvements --- docs/rules/await-async-query.md | 5 +++-- docs/rules/await-async-utils.md | 38 +++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/docs/rules/await-async-query.md b/docs/rules/await-async-query.md index 934cc736..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) 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([ From 3ba17adddf1d66c3c27bd04f2680b87d59121412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Thu, 3 Dec 2020 21:21:45 +0100 Subject: [PATCH 14/15] test(await-async-utils): increase coverage --- tests/lib/rules/await-async-utils.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/lib/rules/await-async-utils.test.ts b/tests/lib/rules/await-async-utils.test.ts index 83c218c6..506f800f 100644 --- a/tests/lib/rules/await-async-utils.test.ts +++ b/tests/lib/rules/await-async-utils.test.ts @@ -230,6 +230,16 @@ ruleTester.run(RULE_NAME, rule, { }) `, }, + + // 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) => ({ From 03cb21fbda0fbe026c026ea3840554838eac9995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltra=CC=81n=20Alarco=CC=81n?= Date: Fri, 4 Dec 2020 12:39:15 +0100 Subject: [PATCH 15/15] refactor: repurpose getInnermostReturningFunctionName to getInnermostReturningFunction --- lib/node-utils.ts | 12 ++++++++---- lib/rules/await-async-query.ts | 9 ++++++--- lib/rules/await-async-utils.ts | 9 ++++++--- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/node-utils.ts b/lib/node-utils.ts index 875e43a5..9e0168cb 100644 --- a/lib/node-utils.ts +++ b/lib/node-utils.ts @@ -516,12 +516,16 @@ export function hasClosestExpectResolvesRejects(node: TSESTree.Node): boolean { } /** - * Gets the name of the function which returns the given Identifier. + * Gets the Function node which returns the given Identifier. */ -export function getInnermostReturningFunctionName( +export function getInnermostReturningFunction( context: RuleContext, node: TSESTree.Identifier -): string | undefined { +): + | TSESTree.FunctionDeclaration + | TSESTree.FunctionExpression + | TSESTree.ArrowFunctionExpression + | undefined { const functionScope = getInnermostFunctionScope(context, node); if (!functionScope) { @@ -542,5 +546,5 @@ export function getInnermostReturningFunctionName( return; } - return getFunctionName(functionScope.block); + return functionScope.block; } diff --git a/lib/rules/await-async-query.ts b/lib/rules/await-async-query.ts index c1474fb8..f49ff174 100644 --- a/lib/rules/await-async-query.ts +++ b/lib/rules/await-async-query.ts @@ -1,7 +1,8 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { findClosestCallExpressionNode, - getInnermostReturningFunctionName, + getFunctionName, + getInnermostReturningFunction, getVariableReferences, isPromiseHandled, } from '../node-utils'; @@ -34,8 +35,10 @@ export default createTestingLibraryRule({ const functionWrappersNames: string[] = []; function detectAsyncQueryWrapper(node: TSESTree.Identifier) { - const functionName = getInnermostReturningFunctionName(context, node); - functionName && functionWrappersNames.push(functionName); + const innerFunction = getInnermostReturningFunction(context, node); + if (innerFunction) { + functionWrappersNames.push(getFunctionName(innerFunction)); + } } return { diff --git a/lib/rules/await-async-utils.ts b/lib/rules/await-async-utils.ts index 4594113a..75ff5384 100644 --- a/lib/rules/await-async-utils.ts +++ b/lib/rules/await-async-utils.ts @@ -1,7 +1,8 @@ import { TSESTree } from '@typescript-eslint/experimental-utils'; import { findClosestCallExpressionNode, - getInnermostReturningFunctionName, + getFunctionName, + getInnermostReturningFunction, getVariableReferences, isMemberExpression, isPromiseHandled, @@ -35,9 +36,11 @@ export default createTestingLibraryRule({ const functionWrappersNames: string[] = []; function detectAsyncUtilWrapper(node: TSESTree.Identifier) { - const functionName = getInnermostReturningFunctionName(context, node); + const innerFunction = getInnermostReturningFunction(context, node); - functionName && functionWrappersNames.push(functionName); + if (innerFunction) { + functionWrappersNames.push(getFunctionName(innerFunction)); + } } return {