From 314405ce10ef46e2d281c8c67432990143f98938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Fri, 23 Apr 2021 20:28:52 +0200 Subject: [PATCH 01/22] refactor: rename function to detect potential Testing Library func --- .../detect-testing-library-utils.ts | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index 3d6662be..473aeda3 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -172,9 +172,9 @@ export function detectTestingLibraryUtils< * - it's imported from valid Testing Library module (depends on aggressive * reporting) */ - function isTestingLibraryUtil( + function isPotentialTestingLibraryFunction( node: TSESTree.Identifier, - isUtilCallback: ( + isPotentialFunctionCallback: ( identifierNodeName: string, originalNodeName?: string ) => boolean @@ -200,7 +200,7 @@ export function detectTestingLibraryUtils< ? importedUtilSpecifier.imported.name : undefined; - if (!isUtilCallback(node.name, originalNodeName)) { + if (!isPotentialFunctionCallback(node.name, originalNodeName)) { return false; } @@ -412,7 +412,7 @@ export function detectTestingLibraryUtils< * coming from Testing Library will be considered as valid. */ const isAsyncUtil: IsAsyncUtilFn = (node, validNames = ASYNC_UTILS) => { - return isTestingLibraryUtil( + return isPotentialTestingLibraryFunction( node, (identifierNodeName, originalNodeName) => { return ( @@ -430,7 +430,7 @@ export function detectTestingLibraryUtils< * Not to be confused with {@link isFireEventMethod} */ const isFireEventUtil = (node: TSESTree.Identifier): boolean => { - return isTestingLibraryUtil( + return isPotentialTestingLibraryFunction( node, (identifierNodeName, originalNodeName) => { return [identifierNodeName, originalNodeName].includes('fireEvent'); @@ -570,17 +570,21 @@ export function detectTestingLibraryUtils< * only those nodes coming from Testing Library will be considered as valid. */ const isRenderUtil: IsRenderUtilFn = (node) => - isTestingLibraryUtil(node, (identifierNodeName, originalNodeName) => { - if (isAggressiveRenderReportingEnabled()) { - return identifierNodeName.toLowerCase().includes(RENDER_NAME); - } + isPotentialTestingLibraryFunction( + node, + (identifierNodeName, originalNodeName) => { + if (isAggressiveRenderReportingEnabled()) { + return identifierNodeName.toLowerCase().includes(RENDER_NAME); + } - return [RENDER_NAME, ...getCustomRenders()].some( - (validRenderName) => - validRenderName === identifierNodeName || - (Boolean(originalNodeName) && validRenderName === originalNodeName) - ); - }); + return [RENDER_NAME, ...getCustomRenders()].some( + (validRenderName) => + validRenderName === identifierNodeName || + (Boolean(originalNodeName) && + validRenderName === originalNodeName) + ); + } + ); const isRenderVariableDeclarator: IsRenderVariableDeclaratorFn = (node) => { if (!node.init) { @@ -603,7 +607,7 @@ export function detectTestingLibraryUtils< return ( !isBuiltInConsole && - isTestingLibraryUtil( + isPotentialTestingLibraryFunction( identifierNode, (identifierNodeName, originalNodeName) => { return [identifierNodeName, originalNodeName] From 6fc1e2d46d36abff280148e94eca027c5b4a7c82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 24 Apr 2021 12:06:01 +0200 Subject: [PATCH 02/22] feat(no-unnecessary-act): first approach for the rule --- docs/rules/no-unnecessary-act.md | 0 .../detect-testing-library-utils.ts | 28 ++++++ lib/rules/no-unnecessary-act.ts | 99 +++++++++++++++++++ tests/lib/rules/no-unnecessary-act.test.ts | 0 4 files changed, 127 insertions(+) create mode 100644 docs/rules/no-unnecessary-act.md create mode 100644 lib/rules/no-unnecessary-act.ts create mode 100644 tests/lib/rules/no-unnecessary-act.test.ts diff --git a/docs/rules/no-unnecessary-act.md b/docs/rules/no-unnecessary-act.md new file mode 100644 index 00000000..e69de29b diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index 473aeda3..3fd7eb24 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -96,6 +96,7 @@ export interface DetectionHelpers { getTestingLibraryImportName: GetTestingLibraryImportNameFn; getCustomModuleImportName: GetCustomModuleImportNameFn; isTestingLibraryImported: IsTestingLibraryImportedFn; + isTestingLibraryUtil: (node: TSESTree.Identifier) => boolean; isGetQueryVariant: IsGetQueryVariantFn; isQueryQueryVariant: IsQueryQueryVariantFn; isFindQueryVariant: IsFindQueryVariantFn; @@ -112,6 +113,7 @@ export interface DetectionHelpers { isRenderUtil: IsRenderUtilFn; isRenderVariableDeclarator: IsRenderVariableDeclaratorFn; isDebugUtil: IsDebugUtilFn; + isActUtil: (node: TSESTree.Identifier) => boolean; isPresenceAssert: IsPresenceAssertFn; isAbsenceAssert: IsAbsenceAssertFn; canReportErrors: CanReportErrorsFn; @@ -618,6 +620,30 @@ export function detectTestingLibraryUtils< ); }; + const isActUtil = (node: TSESTree.Identifier): boolean => { + return isPotentialTestingLibraryFunction( + node, + (identifierNodeName, originalNodeName) => { + return [identifierNodeName, originalNodeName] + .filter(Boolean) + .includes('act'); + } + ); + + // TODO: check if `act` coming from 'react-dom/test-utils' + }; + + const isTestingLibraryUtil = (node: TSESTree.Identifier): boolean => { + return ( + isAsyncUtil(node) || + isQuery(node) || + isRenderUtil(node) || + isFireEventMethod(node) || + isUserEventMethod(node) || + isActUtil(node) + ); + }; + /** * Determines whether a given MemberExpression node is a presence assert * @@ -795,6 +821,7 @@ export function detectTestingLibraryUtils< getTestingLibraryImportName, getCustomModuleImportName, isTestingLibraryImported, + isTestingLibraryUtil, isGetQueryVariant, isQueryQueryVariant, isFindQueryVariant, @@ -811,6 +838,7 @@ export function detectTestingLibraryUtils< isRenderUtil, isRenderVariableDeclarator, isDebugUtil, + isActUtil, isPresenceAssert, isAbsenceAssert, canReportErrors, diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts new file mode 100644 index 00000000..1971f532 --- /dev/null +++ b/lib/rules/no-unnecessary-act.ts @@ -0,0 +1,99 @@ +import { TSESTree } from '@typescript-eslint/experimental-utils'; +import { createTestingLibraryRule } from '../create-testing-library-rule'; +import { + getDeepestIdentifierNode, + getPropertyIdentifierNode, + isCallExpression, + isExpressionStatement, +} from '../node-utils'; + +export const RULE_NAME = 'no-unnecessary-act'; +export type MessageIds = + | 'noUnnecessaryActTestingLibraryUtil' + | 'noUnnecessaryActEmptyFunction'; + +export default createTestingLibraryRule<[], MessageIds>({ + name: RULE_NAME, + meta: { + type: 'problem', + docs: { + description: + 'Disallow the use of `act` when wrapping Testing Library utils or empty functions', + category: 'Possible Errors', + recommended: false, + }, + messages: { + noUnnecessaryActTestingLibraryUtil: + 'Avoid wrapping Testing Library util calls in `act`', + noUnnecessaryActEmptyFunction: 'Avoid wrapping empty function in `act`', + }, + schema: [], + }, + defaultOptions: [], + + create(context, _, helpers) { + function hasNonTestingLibraryCall( + statements: TSESTree.Statement[] + ): boolean { + for (const statement of statements) { + if (!isExpressionStatement(statement)) { + continue; + } + + if (!isCallExpression(statement.expression)) { + continue; + } + + const identifier = getDeepestIdentifierNode(statement.expression); + + if (!identifier) { + continue; + } + + if (helpers.isTestingLibraryUtil(identifier)) { + continue; + } + + // at this point the statement is a non testing library call + return true; + } + return false; + } + + function checkNoUnnecessaryAct( + blockStatementNode: TSESTree.BlockStatement + ) { + const callExpressionNode = blockStatementNode?.parent?.parent as + | TSESTree.CallExpression + | undefined; + + if (!callExpressionNode) { + return; + } + + const callExpressionIdentifier = getPropertyIdentifierNode( + callExpressionNode + ); + + if (!callExpressionIdentifier) { + return; + } + + if (!helpers.isActUtil(callExpressionIdentifier)) { + return; + } + + // TODO: check if empty function body + + if (hasNonTestingLibraryCall(blockStatementNode.body)) { + return; + } + } + + return { + 'CallExpression > ArrowFunctionExpression > BlockStatement': checkNoUnnecessaryAct, + 'CallExpression > FunctionExpression > BlockStatement': checkNoUnnecessaryAct, + // TODO: add selector for call expression > arrow function > implicit return + }; + }, +}); diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts new file mode 100644 index 00000000..e69de29b From 7fd39ef3c142b435bf88c363e309b0e6ad16bea9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 24 Apr 2021 19:55:11 +0200 Subject: [PATCH 03/22] feat(no-unnecessary-act): first bunch of tests --- lib/rules/no-unnecessary-act.ts | 6 + tests/lib/rules/no-unnecessary-act.test.ts | 211 +++++++++++++++++++++ 2 files changed, 217 insertions(+) diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index 1971f532..08b235ab 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -35,6 +35,7 @@ export default createTestingLibraryRule<[], MessageIds>({ function hasNonTestingLibraryCall( statements: TSESTree.Statement[] ): boolean { + // TODO: refactor to use Array.every for (const statement of statements) { if (!isExpressionStatement(statement)) { continue; @@ -88,6 +89,11 @@ export default createTestingLibraryRule<[], MessageIds>({ if (hasNonTestingLibraryCall(blockStatementNode.body)) { return; } + + context.report({ + node: callExpressionIdentifier, + messageId: 'noUnnecessaryActTestingLibraryUtil', + }); } return { diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index e69de29b..9df8b328 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -0,0 +1,211 @@ +import { createRuleTester } from '../test-utils'; +import rule, { RULE_NAME } from '../../../lib/rules/no-unnecessary-act'; + +const ruleTester = createRuleTester(); + +/** + * - AGR stands for Aggressive Reporting + * - RTL stands for react-testing-library (@testing-library/react) + * - RTU stands for react-test-utils (react-dom/test-utils) + */ +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + code: `// case: RTL act wrapping non-RTL calls + import { act } from '@testing-library/react' + + test('valid case', async () => { + act(() => { + stuffThatDoesNotUseRTL(); + }); + + await act(async () => { + stuffThatDoesNotUseRTL(); + }); + + act(function() { + stuffThatDoesNotUseRTL(); + }); + + /* TODO get this one back + act(function() { + return stuffThatDoesNotUseRTL(); + }); + */ + + act(() => stuffThatDoesNotUseRTL()); + }); + `, + }, + { + code: `// case: RTU act wrapping non-RTL + import { act } from 'react-dom/test-utils' + + test('valid case', async () => { + act(() => { + stuffThatDoesNotUseRTL(); + }); + + await act(async () => { + stuffThatDoesNotUseRTL(); + }); + + act(function() { + stuffThatDoesNotUseRTL(); + }); + + /* TODO get this one back + act(function() { + return stuffThatDoesNotUseRTL(); + }); + */ + + act(() => stuffThatDoesNotUseRTL()); + }); + `, + }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTL act wrapping non-RTL - AGR disabled + import { act } from '@testing-library/react' + import { waitFor } from 'somewhere-else' + + test('valid case', async () => { + act(() => { + waitFor(); + }); + + await act(async () => { + waitFor(); + }); + + act(function() { + waitFor(); + }); + + /* TODO get this one back + act(function() { + return waitFor(); + }); + */ + + act(() => waitFor()); + }); + `, + }, + { + code: `// case: RTL act wrapping both RTL and non-RTL calls + import { act, render, waitFor } from '@testing-library/react' + + test('valid case', async () => { + act(() => { + render(); + stuffThatDoesNotUseRTL(); + }); + + await act(async () => { + waitFor(); + stuffThatDoesNotUseRTL(); + }); + + act(function() { + waitFor(); + stuffThatDoesNotUseRTL(); + }); + }); + `, + }, + ], + invalid: [ + { + code: `// case: RTL act wrapping RTL calls - callbacks with body (BlockStatement) + import { act, fireEvent, screen, render, waitFor, waitForElementToBeRemoved } from '@testing-library/react' + import userEvent from '@testing-library/user-event' + + test('invalid case', async () => { + act(() => { + fireEvent.click(el); + }); + + await act(async () => { + waitFor(() => {}); + }); + + await act(async () => { + waitForElementToBeRemoved(el); + }); + + act(function() { + const blah = screen.getByText('blah'); + }); + + act(function() { + render(something); + }); + + await act(() => { + const button = findByRole('button') + }); + + act(() => { + userEvent.click(el) + }); + + act(() => { + waitFor(); + const element = screen.getByText('blah'); + userEvent.click(element) + }); + }); + `, + errors: [ + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 6, column: 9 }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 10, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 14, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 18, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 22, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 26, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 30, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 34, + column: 9, + }, + ], + }, + + // TODO case: RTL act wrapping RTL calls - callbacks with return + // TODO case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) + // TODO case: RTU act wrapping RTL calls - callbacks with return + // TODO case: RTL act wrapping empty callback + // TODO case: RTU act wrapping empty callback + + // TODO cases like previous ones but with AGR disabled + ], +}); From 54ef6b3258507a6693ae5351a0a4d497787eb34e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 24 Apr 2021 19:57:52 +0200 Subject: [PATCH 04/22] test: update number of expected rules --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 2f9808cb..2ba1adc7 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -6,7 +6,7 @@ import plugin from '../lib'; const generateConfigs = () => exec(`npm run generate:configs`); -const numberOfRules = 24; +const numberOfRules = 25; const ruleNames = Object.keys(plugin.rules); // eslint-disable-next-line jest/expect-expect From 4b1d628cb366238dd64c79c5408c67351c725c5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 24 Apr 2021 21:23:17 +0200 Subject: [PATCH 05/22] feat(no-unnecessary-act): detect error from returns --- lib/node-utils/index.ts | 25 ++++ lib/node-utils/is-node-of-type.ts | 3 + lib/rules/no-unnecessary-act.ts | 61 ++++++++-- tests/lib/rules/no-unnecessary-act.test.ts | 131 +++++++++++++++++++-- 4 files changed, 202 insertions(+), 18 deletions(-) diff --git a/lib/node-utils/index.ts b/lib/node-utils/index.ts index caa94108..3d6fd80f 100644 --- a/lib/node-utils/index.ts +++ b/lib/node-utils/index.ts @@ -17,6 +17,7 @@ import { isLiteral, isMemberExpression, isReturnStatement, + isVariableDeclaration, } from './is-node-of-type'; export * from './is-node-of-type'; @@ -510,3 +511,27 @@ export function hasImportMatch( return importNode.local.name === identifierName; } + +export function getStatementCallExpression( + statement: TSESTree.Statement +): TSESTree.CallExpression | undefined { + if ( + isExpressionStatement(statement) && + isCallExpression(statement.expression) + ) { + return statement.expression; + } + + if (isReturnStatement(statement) && isCallExpression(statement.argument)) { + return statement.argument; + } + + if (isVariableDeclaration(statement)) { + for (const declaration of statement.declarations) { + if (isCallExpression(declaration.init)) { + return declaration.init; + } + } + } + return undefined; +} diff --git a/lib/node-utils/is-node-of-type.ts b/lib/node-utils/is-node-of-type.ts index ac8dbb5f..96b6fbc2 100644 --- a/lib/node-utils/is-node-of-type.ts +++ b/lib/node-utils/is-node-of-type.ts @@ -43,3 +43,6 @@ export const isObjectExpression = isNodeOfType(AST_NODE_TYPES.ObjectExpression); export const isObjectPattern = isNodeOfType(AST_NODE_TYPES.ObjectPattern); export const isProperty = isNodeOfType(AST_NODE_TYPES.Property); export const isReturnStatement = isNodeOfType(AST_NODE_TYPES.ReturnStatement); +export const isVariableDeclaration = isNodeOfType( + AST_NODE_TYPES.VariableDeclaration +); diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index 08b235ab..d67f7bbb 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -3,8 +3,7 @@ import { createTestingLibraryRule } from '../create-testing-library-rule'; import { getDeepestIdentifierNode, getPropertyIdentifierNode, - isCallExpression, - isExpressionStatement, + getStatementCallExpression, } from '../node-utils'; export const RULE_NAME = 'no-unnecessary-act'; @@ -32,20 +31,21 @@ export default createTestingLibraryRule<[], MessageIds>({ defaultOptions: [], create(context, _, helpers) { + /** + * Determines whether a given list of statements has some call non-related to Testing Library utils. + */ function hasNonTestingLibraryCall( statements: TSESTree.Statement[] ): boolean { // TODO: refactor to use Array.every for (const statement of statements) { - if (!isExpressionStatement(statement)) { - continue; - } + const callExpression = getStatementCallExpression(statement); - if (!isCallExpression(statement.expression)) { + if (!callExpression) { continue; } - const identifier = getDeepestIdentifierNode(statement.expression); + const identifier = getDeepestIdentifierNode(callExpression); if (!identifier) { continue; @@ -61,7 +61,7 @@ export default createTestingLibraryRule<[], MessageIds>({ return false; } - function checkNoUnnecessaryAct( + function checkNoUnnecessaryActFromBlockStatement( blockStatementNode: TSESTree.BlockStatement ) { const callExpressionNode = blockStatementNode?.parent?.parent as @@ -96,10 +96,49 @@ export default createTestingLibraryRule<[], MessageIds>({ }); } + function checkNoUnnecessaryActFromImplicitReturn( + node: TSESTree.CallExpression + ) { + const nodeIdentifier = getDeepestIdentifierNode(node); + + if (!nodeIdentifier) { + return; + } + + const parentCallExpression = node?.parent?.parent as + | TSESTree.CallExpression + | undefined; + + if (!parentCallExpression) { + return; + } + + const parentCallExpressionIdentifier = getPropertyIdentifierNode( + parentCallExpression + ); + + if (!parentCallExpressionIdentifier) { + return; + } + + if (!helpers.isActUtil(parentCallExpressionIdentifier)) { + return; + } + + if (!helpers.isTestingLibraryUtil(nodeIdentifier)) { + return; + } + + context.report({ + node: parentCallExpressionIdentifier, + messageId: 'noUnnecessaryActTestingLibraryUtil', + }); + } + return { - 'CallExpression > ArrowFunctionExpression > BlockStatement': checkNoUnnecessaryAct, - 'CallExpression > FunctionExpression > BlockStatement': checkNoUnnecessaryAct, - // TODO: add selector for call expression > arrow function > implicit return + 'CallExpression > ArrowFunctionExpression > BlockStatement': checkNoUnnecessaryActFromBlockStatement, + 'CallExpression > FunctionExpression > BlockStatement': checkNoUnnecessaryActFromBlockStatement, + 'CallExpression > ArrowFunctionExpression > CallExpression': checkNoUnnecessaryActFromImplicitReturn, }; }, }); diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index 9df8b328..ded84593 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -25,13 +25,12 @@ ruleTester.run(RULE_NAME, rule, { act(function() { stuffThatDoesNotUseRTL(); + const a = foo(); }); - /* TODO get this one back act(function() { return stuffThatDoesNotUseRTL(); }); - */ act(() => stuffThatDoesNotUseRTL()); }); @@ -54,11 +53,9 @@ ruleTester.run(RULE_NAME, rule, { stuffThatDoesNotUseRTL(); }); - /* TODO get this one back act(function() { return stuffThatDoesNotUseRTL(); }); - */ act(() => stuffThatDoesNotUseRTL()); }); @@ -85,11 +82,9 @@ ruleTester.run(RULE_NAME, rule, { waitFor(); }); - /* TODO get this one back act(function() { return waitFor(); }); - */ act(() => waitFor()); }); @@ -200,7 +195,129 @@ ruleTester.run(RULE_NAME, rule, { ], }, - // TODO case: RTL act wrapping RTL calls - callbacks with return + { + code: `// case: RTL act wrapping RTL calls - callbacks with return + import { act, fireEvent, screen, render, waitFor } from '@testing-library/react' + import userEvent from '@testing-library/user-event' + + test('invalid case', async () => { + act(() => fireEvent.click(el)) + act(() => screen.getByText('blah')) + act(() => findByRole('button')) + act(() => userEvent.click(el)) + await act(async () => userEvent.type('hi', el)) + act(() => render(foo)) + await act(async () => render(fo)) + act(() => waitFor(() => {})) + await act(async () => waitFor(() => {})) + + act(function () { + return fireEvent.click(el); + }); + act(function () { + return screen.getByText('blah'); + }); + act(function () { + return findByRole('button'); + }); + act(function () { + return userEvent.click(el); + }); + await act(async function () { + return userEvent.type('hi', el); + }); + act(function () { + return render(foo); + }); + await act(async function () { + return render(fo); + }); + act(function () { + return waitFor(() => {}); + }); + await act(async function () { + return waitFor(() => {}); + }); + }); + `, + errors: [ + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 6, column: 9 }, + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 7, column: 9 }, + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 8, column: 9 }, + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 9, column: 9 }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 10, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 11, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 12, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 13, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 14, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 16, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 19, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 22, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 25, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 28, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 31, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 34, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 37, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 40, + column: 15, + }, + ], + }, + // TODO case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) // TODO case: RTU act wrapping RTL calls - callbacks with return // TODO case: RTL act wrapping empty callback From 573c689586054012bb02cc1199a11a7ec0f228d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sun, 25 Apr 2021 21:49:40 +0200 Subject: [PATCH 06/22] refactor: extract util to determine if a function is empty --- lib/node-utils/index.ts | 17 +++++++++++++++++ lib/rules/no-wait-for-empty-callback.ts | 5 ++--- tests/lib/rules/no-unnecessary-act.test.ts | 2 +- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/node-utils/index.ts b/lib/node-utils/index.ts index 3d6fd80f..36a501de 100644 --- a/lib/node-utils/index.ts +++ b/lib/node-utils/index.ts @@ -535,3 +535,20 @@ export function getStatementCallExpression( } return undefined; } + +/** + * Determines whether a given function node is considered as empty function or not. + * + * A function is considered empty if its body is empty. + * + * Note that comments don't affect the check. + * + * If node given is not a function, `false` will be returned. + */ +export function isEmptyFunction(node: TSESTree.Node): boolean | undefined { + if (ASTUtils.isFunction(node) && isBlockStatement(node.body)) { + return node.body.body.length === 0; + } + + return false; +} diff --git a/lib/rules/no-wait-for-empty-callback.ts b/lib/rules/no-wait-for-empty-callback.ts index 97e2ac64..0ca71193 100644 --- a/lib/rules/no-wait-for-empty-callback.ts +++ b/lib/rules/no-wait-for-empty-callback.ts @@ -1,8 +1,8 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { getPropertyIdentifierNode, - isBlockStatement, isCallExpression, + isEmptyFunction, } from '../node-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; @@ -57,8 +57,7 @@ export default createTestingLibraryRule({ } if ( - isBlockStatement(node.body) && - node.body.body.length === 0 && + isEmptyFunction(node) && isCallExpression(node.parent) && ASTUtils.isIdentifier(node.parent.callee) ) { diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index ded84593..5481c784 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -318,9 +318,9 @@ ruleTester.run(RULE_NAME, rule, { ], }, + // TODO case: RTL act wrapping empty callback // TODO case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) // TODO case: RTU act wrapping RTL calls - callbacks with return - // TODO case: RTL act wrapping empty callback // TODO case: RTU act wrapping empty callback // TODO cases like previous ones but with AGR disabled From 9e5442cc2b4d39e2c7ba6dd02eaecc56ea082f1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Wed, 28 Apr 2021 16:33:44 +0200 Subject: [PATCH 07/22] feat(no-unnecessary-act): detect error for empty callbacks --- lib/rules/no-unnecessary-act.ts | 18 ++++++-- tests/lib/rules/no-unnecessary-act.test.ts | 54 +++++++++++++++++++++- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index d67f7bbb..537c3dca 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -4,6 +4,7 @@ import { getDeepestIdentifierNode, getPropertyIdentifierNode, getStatementCallExpression, + isEmptyFunction, } from '../node-utils'; export const RULE_NAME = 'no-unnecessary-act'; @@ -64,11 +65,15 @@ export default createTestingLibraryRule<[], MessageIds>({ function checkNoUnnecessaryActFromBlockStatement( blockStatementNode: TSESTree.BlockStatement ) { - const callExpressionNode = blockStatementNode?.parent?.parent as + const functionNode = blockStatementNode?.parent as + | TSESTree.FunctionExpression + | TSESTree.ArrowFunctionExpression + | undefined; + const callExpressionNode = functionNode?.parent as | TSESTree.CallExpression | undefined; - if (!callExpressionNode) { + if (!callExpressionNode || !functionNode) { return; } @@ -84,7 +89,14 @@ export default createTestingLibraryRule<[], MessageIds>({ return; } - // TODO: check if empty function body + if (isEmptyFunction(functionNode)) { + context.report({ + node: callExpressionIdentifier, + messageId: 'noUnnecessaryActEmptyFunction', + }); + + return; + } if (hasNonTestingLibraryCall(blockStatementNode.body)) { return; diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index 5481c784..ceadf6a6 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -96,7 +96,7 @@ ruleTester.run(RULE_NAME, rule, { test('valid case', async () => { act(() => { - render(); + render(element); stuffThatDoesNotUseRTL(); }); @@ -112,6 +112,39 @@ ruleTester.run(RULE_NAME, rule, { }); `, }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: non-RTL act wrapping RTL - AGR disabled + import { act } from 'somewhere-else' + import { waitFor } from '@testing-library/react' + + test('valid case', async () => { + act(() => { + waitFor(); + }); + + await act(async () => { + waitFor(); + }); + + act(function() { + waitFor(); + }); + + act(function() { + return waitFor(); + }); + + act(() => waitFor()); + + act(() => {}) + await act(async () => {}) + act(function() {}) + }); + `, + }, ], invalid: [ { @@ -317,8 +350,25 @@ ruleTester.run(RULE_NAME, rule, { }, ], }, + { + code: `// case: RTL act wrapping empty callback + import { act } from '@testing-library/react' + + test('invalid case', async () => { + await act(async () => {}) + act(() => {}) + await act(async function () {}) + act(function () {}) + }) + `, + errors: [ + { messageId: 'noUnnecessaryActEmptyFunction', line: 5, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + ], + }, - // TODO case: RTL act wrapping empty callback // TODO case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) // TODO case: RTU act wrapping RTL calls - callbacks with return // TODO case: RTU act wrapping empty callback From 964209c01dbc953a1a307cdf0b81682587a877ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Wed, 28 Apr 2021 16:42:39 +0200 Subject: [PATCH 08/22] refactor(no-unnecessary-act): function to find non-testing-library calls --- lib/rules/no-unnecessary-act.ts | 37 +++++++--------------- tests/lib/rules/no-unnecessary-act.test.ts | 2 +- 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index 537c3dca..82607cc8 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -33,33 +33,26 @@ export default createTestingLibraryRule<[], MessageIds>({ create(context, _, helpers) { /** - * Determines whether a given list of statements has some call non-related to Testing Library utils. + * Determines whether some call is non Testing Library related for a given list of statements. */ - function hasNonTestingLibraryCall( + function hasSomeNonTestingLibraryCall( statements: TSESTree.Statement[] ): boolean { - // TODO: refactor to use Array.every - for (const statement of statements) { + return statements.some((statement) => { const callExpression = getStatementCallExpression(statement); if (!callExpression) { - continue; + return false; } const identifier = getDeepestIdentifierNode(callExpression); if (!identifier) { - continue; + return false; } - if (helpers.isTestingLibraryUtil(identifier)) { - continue; - } - - // at this point the statement is a non testing library call - return true; - } - return false; + return !helpers.isTestingLibraryUtil(identifier); + }); } function checkNoUnnecessaryActFromBlockStatement( @@ -94,18 +87,12 @@ export default createTestingLibraryRule<[], MessageIds>({ node: callExpressionIdentifier, messageId: 'noUnnecessaryActEmptyFunction', }); - - return; - } - - if (hasNonTestingLibraryCall(blockStatementNode.body)) { - return; + } else if (!hasSomeNonTestingLibraryCall(blockStatementNode.body)) { + context.report({ + node: callExpressionIdentifier, + messageId: 'noUnnecessaryActTestingLibraryUtil', + }); } - - context.report({ - node: callExpressionIdentifier, - messageId: 'noUnnecessaryActTestingLibraryUtil', - }); } function checkNoUnnecessaryActFromImplicitReturn( diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index ceadf6a6..7e3de758 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -373,6 +373,6 @@ ruleTester.run(RULE_NAME, rule, { // TODO case: RTU act wrapping RTL calls - callbacks with return // TODO case: RTU act wrapping empty callback - // TODO cases like previous ones but with AGR disabled + // TODO case: mixed scenarios - AGR disabled ], }); From 6a3a3b3aaf884177d6d6a2510ef04205919b3ba1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sun, 2 May 2021 20:02:27 +0200 Subject: [PATCH 09/22] refactor(no-unnecessary-act): update docs recommended config --- lib/rules/no-unnecessary-act.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index 82607cc8..28f94218 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -20,7 +20,12 @@ export default createTestingLibraryRule<[], MessageIds>({ description: 'Disallow the use of `act` when wrapping Testing Library utils or empty functions', category: 'Possible Errors', - recommended: false, + recommendedConfig: { + dom: false, + angular: false, + react: false, // this should be enabled on v5 of the plugin + vue: false, + }, }, messages: { noUnnecessaryActTestingLibraryUtil: From 9ebe5ae920df99b48c63d8d8c3d6bc1952efca41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sun, 2 May 2021 21:35:56 +0200 Subject: [PATCH 10/22] fix: check if node is coming from Testing Library correctly isNodeComingFromTestingLibrary wasn't checking if the imported declaration name matches some Testing Library valid module --- docs/rules/no-unnecessary-act.md | 1 + .../detect-testing-library-utils.ts | 35 ++- lib/node-utils/index.ts | 17 ++ tests/lib/rules/no-unnecessary-act.test.ts | 287 +++++++++++++++++- 4 files changed, 333 insertions(+), 7 deletions(-) diff --git a/docs/rules/no-unnecessary-act.md b/docs/rules/no-unnecessary-act.md index e69de29b..70b786d1 100644 --- a/docs/rules/no-unnecessary-act.md +++ b/docs/rules/no-unnecessary-act.md @@ -0,0 +1 @@ +// TODO diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index 3fd7eb24..42b2fa10 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -5,6 +5,7 @@ import { } from '@typescript-eslint/experimental-utils'; import { + findClosestVariableDeclaratorNode, getAssertNodeInfo, getDeepestIdentifierNode, getImportModuleName, @@ -12,6 +13,7 @@ import { getReferenceNode, hasImportMatch, ImportModuleNode, + isCallExpression, isImportDeclaration, isImportDefaultSpecifier, isImportNamespaceSpecifier, @@ -805,6 +807,31 @@ export function detectTestingLibraryUtils< return false; } + const importDeclaration = (() => { + if (isImportDeclaration(importNode.parent)) { + return importNode.parent; + } + + const variableDeclarator = findClosestVariableDeclaratorNode( + importNode + ); + + if (isCallExpression(variableDeclarator?.init)) { + return variableDeclarator?.init; + } + + return undefined; + })(); + + if (!importDeclaration) { + return false; + } + + const importDeclarationName = getImportModuleName(importDeclaration); + if (!importDeclarationName) { + return false; + } + const identifierName: string | undefined = getPropertyIdentifierNode(node) ?.name; @@ -812,7 +839,13 @@ export function detectTestingLibraryUtils< return false; } - return hasImportMatch(importNode, identifierName); + const hasImportElementMatch = hasImportMatch(importNode, identifierName); + const hasImportModuleMatch = + /testing-library/g.test(importDeclarationName) || + (typeof customModuleSetting === 'string' && + importDeclarationName.endsWith(customModuleSetting)); + + return hasImportElementMatch && hasImportModuleMatch; }; const helpers: DetectionHelpers = { diff --git a/lib/node-utils/index.ts b/lib/node-utils/index.ts index 36a501de..3e84f78b 100644 --- a/lib/node-utils/index.ts +++ b/lib/node-utils/index.ts @@ -77,6 +77,23 @@ export function findClosestCallExpressionNode( return findClosestCallExpressionNode(node.parent, shouldRestrictInnerScope); } +export function findClosestVariableDeclaratorNode( + node: TSESTree.Node | undefined +): TSESTree.VariableDeclarator | null { + if (!node) { + return null; + } + + if (ASTUtils.isVariableDeclarator(node)) { + return node; + } + + return findClosestVariableDeclaratorNode(node.parent); +} + +/** + * TODO: remove this one in favor of {@link findClosestCallExpressionNode} + */ export function findClosestCallNode( node: TSESTree.Node, name: string diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index 7e3de758..6e65544e 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -5,8 +5,8 @@ const ruleTester = createRuleTester(); /** * - AGR stands for Aggressive Reporting - * - RTL stands for react-testing-library (@testing-library/react) - * - RTU stands for react-test-utils (react-dom/test-utils) + * - RTL stands for React Testing Library (@testing-library/react) + * - RTU stands for React Test Utils (react-dom/test-utils) */ ruleTester.run(RULE_NAME, rule, { valid: [ @@ -147,6 +147,7 @@ ruleTester.run(RULE_NAME, rule, { }, ], invalid: [ + // cases for act related to React Testing Library { code: `// case: RTL act wrapping RTL calls - callbacks with body (BlockStatement) import { act, fireEvent, screen, render, waitFor, waitForElementToBeRemoved } from '@testing-library/react' @@ -227,7 +228,6 @@ ruleTester.run(RULE_NAME, rule, { }, ], }, - { code: `// case: RTL act wrapping RTL calls - callbacks with return import { act, fireEvent, screen, render, waitFor } from '@testing-library/react' @@ -368,10 +368,285 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, ], }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTL act wrapping empty callback - require version + const { act } = require('@testing-library/react'); + + test('invalid case', async () => { + await act(async () => {}) + act(() => {}) + await act(async function () {}) + act(function () {}) + }) + `, + errors: [ + { messageId: 'noUnnecessaryActEmptyFunction', line: 5, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + ], + }, + + // cases for act related to React Test Utils + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) + import { act } from 'react-dom/test-utils'; + import { fireEvent, screen, render, waitFor, waitForElementToBeRemoved } from 'test-utils' + import userEvent from '@testing-library/user-event' + + test('invalid case', async () => { + act(() => { + fireEvent.click(el); + }); + + await act(async () => { + waitFor(() => {}); + }); + + await act(async () => { + waitForElementToBeRemoved(el); + }); + + act(function() { + const blah = screen.getByText('blah'); + }); + + act(function() { + render(something); + }); + + await act(() => { + const button = findByRole('button') + }); - // TODO case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) - // TODO case: RTU act wrapping RTL calls - callbacks with return - // TODO case: RTU act wrapping empty callback + act(() => { + userEvent.click(el) + }); + + act(() => { + waitFor(); + const element = screen.getByText('blah'); + userEvent.click(element) + }); + }); + `, + errors: [ + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 7, column: 9 }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 11, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 15, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 19, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 23, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 27, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 31, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 35, + column: 9, + }, + ], + }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTU act wrapping RTL calls - callbacks with return + import { act } from 'react-dom/test-utils'; + import { fireEvent, screen, render, waitFor } from 'test-utils' + import userEvent from '@testing-library/user-event' + + test('invalid case', async () => { + act(() => fireEvent.click(el)) + act(() => screen.getByText('blah')) + act(() => findByRole('button')) + act(() => userEvent.click(el)) + await act(async () => userEvent.type('hi', el)) + act(() => render(foo)) + await act(async () => render(fo)) + act(() => waitFor(() => {})) + await act(async () => waitFor(() => {})) + + act(function () { + return fireEvent.click(el); + }); + act(function () { + return screen.getByText('blah'); + }); + act(function () { + return findByRole('button'); + }); + act(function () { + return userEvent.click(el); + }); + await act(async function () { + return userEvent.type('hi', el); + }); + act(function () { + return render(foo); + }); + await act(async function () { + return render(fo); + }); + act(function () { + return waitFor(() => {}); + }); + await act(async function () { + return waitFor(() => {}); + }); + }); + `, + errors: [ + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 7, column: 9 }, + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 8, column: 9 }, + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 9, column: 9 }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 10, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 11, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 12, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 13, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 14, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 15, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 17, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 20, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 23, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 26, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 29, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 32, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 35, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 38, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 41, + column: 15, + }, + ], + }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTU act wrapping empty callback + import { act } from 'react-dom/test-utils'; + + test('invalid case', async () => { + await act(async () => {}) + act(() => {}) + await act(async function () {}) + act(function () {}) + }) + `, + errors: [ + { messageId: 'noUnnecessaryActEmptyFunction', line: 5, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + ], + }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTU act wrapping empty callback - require version + const { act } = require('react-dom/test-utils'); + + test('invalid case', async () => { + await act(async () => {}) + act(() => {}) + await act(async function () {}) + act(function () {}) + }) + `, + errors: [ + { messageId: 'noUnnecessaryActEmptyFunction', line: 5, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + ], + }, // TODO case: mixed scenarios - AGR disabled ], From 88822034a3485185846d6ffeba97b63d804d4446 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Mon, 3 May 2021 10:28:41 +0200 Subject: [PATCH 11/22] feat: save react-dom/test-utils import in detection mechanism --- .../detect-testing-library-utils.ts | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index 42b2fa10..c8c2e511 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -124,6 +124,7 @@ export interface DetectionHelpers { } const USER_EVENT_PACKAGE = '@testing-library/user-event'; +const REACT_DOM_TEST_UTILS_PACKAGE = 'react-dom/test-utils'; const FIRE_EVENT_NAME = 'fireEvent'; const USER_EVENT_NAME = 'userEvent'; const RENDER_NAME = 'render'; @@ -157,6 +158,7 @@ export function detectTestingLibraryUtils< let importedTestingLibraryNode: ImportModuleNode | null = null; let importedCustomModuleNode: ImportModuleNode | null = null; let importedUserEventLibraryNode: ImportModuleNode | null = null; + let importedReactDomTestUtilsNode: ImportModuleNode | null = null; // Init options based on shared ESLint settings const customModuleSetting = @@ -889,11 +891,14 @@ export function detectTestingLibraryUtils< * parts of the file. */ ImportDeclaration(node: TSESTree.ImportDeclaration) { + if (typeof node.source.value !== 'string') { + return; + } // check only if testing library import not found yet so we avoid // to override importedTestingLibraryNode after it's found if ( !importedTestingLibraryNode && - /testing-library/g.test(node.source.value as string) + /testing-library/g.test(node.source.value) ) { importedTestingLibraryNode = node; } @@ -904,7 +909,7 @@ export function detectTestingLibraryUtils< if ( customModule && !importedCustomModuleNode && - String(node.source.value).endsWith(customModule) + node.source.value.endsWith(customModule) ) { importedCustomModuleNode = node; } @@ -913,10 +918,19 @@ export function detectTestingLibraryUtils< // to override importedUserEventLibraryNode after it's found if ( !importedUserEventLibraryNode && - String(node.source.value) === USER_EVENT_PACKAGE + node.source.value === USER_EVENT_PACKAGE ) { importedUserEventLibraryNode = node; } + + // check only if react-dom/test-utils import not found yet so we avoid + // to override importedReactDomTestUtilsNode after it's found + if ( + !importedUserEventLibraryNode && + node.source.value === REACT_DOM_TEST_UTILS_PACKAGE + ) { + importedReactDomTestUtilsNode = node; + } }, // Check if Testing Library related modules are loaded with required. From d44285c8e1aa729f653de5808f5fb110ed1a8755 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Mon, 3 May 2021 11:24:36 +0200 Subject: [PATCH 12/22] refactor: extract function for finding import specifier --- .../detect-testing-library-utils.ts | 65 ++++--------------- lib/node-utils/index.ts | 55 ++++++++++++++++ 2 files changed, 68 insertions(+), 52 deletions(-) diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index c8c2e511..3e2d0f20 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -6,6 +6,7 @@ import { import { findClosestVariableDeclaratorNode, + findImportSpecifier, getAssertNodeInfo, getDeepestIdentifierNode, getImportModuleName, @@ -196,7 +197,7 @@ export function detectTestingLibraryUtils< return false; } - const importedUtilSpecifier = getImportedUtilSpecifier( + const importedUtilSpecifier = getTestingLibraryImportedUtilSpecifier( referenceNodeIdentifier ); @@ -470,7 +471,9 @@ export function detectTestingLibraryUtils< * Determines whether a given node is fireEvent method or not */ const isFireEventMethod: IsFireEventMethodFn = (node) => { - const fireEventUtil = findImportedUtilSpecifier(FIRE_EVENT_NAME); + const fireEventUtil = findImportedTestingLibraryUtilSpecifier( + FIRE_EVENT_NAME + ); let fireEventUtilName: string | undefined; if (fireEventUtil) { @@ -687,60 +690,18 @@ export function detectTestingLibraryUtils< }; /** - * Gets a string and verifies if it was imported/required by Testing Library - * related module. + * Finds the import util specifier related to Testing Library for a given name. */ - const findImportedUtilSpecifier: FindImportedUtilSpecifierFn = ( + const findImportedTestingLibraryUtilSpecifier: FindImportedUtilSpecifierFn = ( specifierName - ) => { + ): TSESTree.ImportClause | TSESTree.Identifier | undefined => { const node = getCustomModuleImportNode() ?? getTestingLibraryImportNode(); if (!node) { return undefined; } - if (isImportDeclaration(node)) { - const namedExport = node.specifiers.find((n) => { - return ( - isImportSpecifier(n) && - [n.imported.name, n.local.name].includes(specifierName) - ); - }); - - // it is "import { foo [as alias] } from 'baz'"" - if (namedExport) { - return namedExport; - } - - // it could be "import * as rtl from 'baz'" - return node.specifiers.find((n) => isImportNamespaceSpecifier(n)); - } else { - if (!ASTUtils.isVariableDeclarator(node.parent)) { - return undefined; - } - const requireNode = node.parent; - - if (ASTUtils.isIdentifier(requireNode.id)) { - // this is const rtl = require('foo') - return requireNode.id; - } - - // this should be const { something } = require('foo') - if (!isObjectPattern(requireNode.id)) { - return undefined; - } - - const property = requireNode.id.properties.find( - (n) => - isProperty(n) && - ASTUtils.isIdentifier(n.key) && - n.key.name === specifierName - ); - if (!property) { - return undefined; - } - return (property as TSESTree.Property).key as TSESTree.Identifier; - } + return findImportSpecifier(specifierName, node); }; const findImportedUserEventSpecifier: () => TSESTree.Identifier | null = () => { @@ -774,7 +735,7 @@ export function detectTestingLibraryUtils< return null; }; - const getImportedUtilSpecifier = ( + const getTestingLibraryImportedUtilSpecifier = ( node: TSESTree.MemberExpression | TSESTree.Identifier ): TSESTree.ImportClause | TSESTree.Identifier | undefined => { const identifierName: string | undefined = getPropertyIdentifierNode(node) @@ -784,7 +745,7 @@ export function detectTestingLibraryUtils< return undefined; } - return findImportedUtilSpecifier(identifierName); + return findImportedTestingLibraryUtilSpecifier(identifierName); }; /** @@ -803,7 +764,7 @@ export function detectTestingLibraryUtils< const isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn = ( node ) => { - const importNode = getImportedUtilSpecifier(node); + const importNode = getTestingLibraryImportedUtilSpecifier(node); if (!importNode) { return false; @@ -877,7 +838,7 @@ export function detectTestingLibraryUtils< isPresenceAssert, isAbsenceAssert, canReportErrors, - findImportedUtilSpecifier, + findImportedUtilSpecifier: findImportedTestingLibraryUtilSpecifier, isNodeComingFromTestingLibrary, }; diff --git a/lib/node-utils/index.ts b/lib/node-utils/index.ts index 3e84f78b..54f5b6fd 100644 --- a/lib/node-utils/index.ts +++ b/lib/node-utils/index.ts @@ -14,8 +14,12 @@ import { isCallExpression, isExpressionStatement, isImportDeclaration, + isImportNamespaceSpecifier, + isImportSpecifier, isLiteral, isMemberExpression, + isObjectPattern, + isProperty, isReturnStatement, isVariableDeclaration, } from './is-node-of-type'; @@ -569,3 +573,54 @@ export function isEmptyFunction(node: TSESTree.Node): boolean | undefined { return false; } + +/** + * Finds the import specifier matching a given name for a given import module node. + */ +export function findImportSpecifier( + specifierName: string, + node: ImportModuleNode +): TSESTree.ImportClause | TSESTree.Identifier | undefined { + if (isImportDeclaration(node)) { + const namedExport = node.specifiers.find((n) => { + return ( + isImportSpecifier(n) && + [n.imported.name, n.local.name].includes(specifierName) + ); + }); + + // it is "import { foo [as alias] } from 'baz'"" + if (namedExport) { + return namedExport; + } + + // it could be "import * as rtl from 'baz'" + return node.specifiers.find((n) => isImportNamespaceSpecifier(n)); + } else { + if (!ASTUtils.isVariableDeclarator(node.parent)) { + return undefined; + } + const requireNode = node.parent; + + if (ASTUtils.isIdentifier(requireNode.id)) { + // this is const rtl = require('foo') + return requireNode.id; + } + + // this should be const { something } = require('foo') + if (!isObjectPattern(requireNode.id)) { + return undefined; + } + + const property = requireNode.id.properties.find( + (n) => + isProperty(n) && + ASTUtils.isIdentifier(n.key) && + n.key.name === specifierName + ); + if (!property) { + return undefined; + } + return (property as TSESTree.Property).key as TSESTree.Identifier; + } +} From e464ecee371ea726225b3ee4ca9460a81a17d3f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Mon, 3 May 2021 15:48:21 +0200 Subject: [PATCH 13/22] refactor(no-unnecessary-act): detect act from React DOM test utils --- .../detect-testing-library-utils.ts | 84 +++++++- lib/node-utils/index.ts | 2 +- tests/lib/rules/no-unnecessary-act.test.ts | 185 +++++++++++++----- 3 files changed, 216 insertions(+), 55 deletions(-) diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index 3e2d0f20..55a45749 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -17,12 +17,9 @@ import { isCallExpression, isImportDeclaration, isImportDefaultSpecifier, - isImportNamespaceSpecifier, isImportSpecifier, isLiteral, isMemberExpression, - isObjectPattern, - isProperty, } from '../node-utils'; import { ABSENCE_MATCHERS, @@ -627,8 +624,15 @@ export function detectTestingLibraryUtils< ); }; + /** + * Determines whether a given node is some reportable `act` util. + * + * An `act` is reportable if some of these conditions is met: + * - it's related to Testing Library module (this depends on Aggressive Reporting) + * - it's related to React DOM Test Utils + */ const isActUtil = (node: TSESTree.Identifier): boolean => { - return isPotentialTestingLibraryFunction( + const isTestingLibraryAct = isPotentialTestingLibraryFunction( node, (identifierNodeName, originalNodeName) => { return [identifierNodeName, originalNodeName] @@ -637,7 +641,65 @@ export function detectTestingLibraryUtils< } ); - // TODO: check if `act` coming from 'react-dom/test-utils' + const isReactDomTestUtilsAct = (() => { + if (!importedReactDomTestUtilsNode) { + return false; + } + const referenceNode = getReferenceNode(node); + const referenceNodeIdentifier = getPropertyIdentifierNode( + referenceNode + ); + if (!referenceNodeIdentifier) { + return false; + } + + const importedUtilSpecifier = findImportSpecifier( + node.name, + importedReactDomTestUtilsNode + ); + if (!importedUtilSpecifier) { + return false; + } + + const importDeclaration = (() => { + if (isImportDeclaration(importedUtilSpecifier.parent)) { + return importedUtilSpecifier.parent; + } + + const variableDeclarator = findClosestVariableDeclaratorNode( + importedUtilSpecifier + ); + + if (isCallExpression(variableDeclarator?.init)) { + return variableDeclarator?.init; + } + + return undefined; + })(); + if (!importDeclaration) { + return false; + } + + const importDeclarationName = getImportModuleName(importDeclaration); + if (!importDeclarationName) { + return false; + } + + if (importDeclarationName !== REACT_DOM_TEST_UTILS_PACKAGE) { + return false; + } + + const originalNodeName = + isImportSpecifier(importedUtilSpecifier) && + importedUtilSpecifier.local.name !== + importedUtilSpecifier.imported.name + ? importedUtilSpecifier.imported.name + : undefined; + + return [node.name, originalNodeName].filter(Boolean).includes('act'); + })(); + + return isTestingLibraryAct || isReactDomTestUtilsAct; }; const isTestingLibraryUtil = (node: TSESTree.Identifier): boolean => { @@ -938,6 +1000,18 @@ export function detectTestingLibraryUtils< ) { importedUserEventLibraryNode = callExpression; } + + if ( + !importedReactDomTestUtilsNode && + args.some( + (arg) => + isLiteral(arg) && + typeof arg.value === 'string' && + arg.value === REACT_DOM_TEST_UTILS_PACKAGE + ) + ) { + importedReactDomTestUtilsNode = callExpression; + } }, }; diff --git a/lib/node-utils/index.ts b/lib/node-utils/index.ts index 54f5b6fd..b1a9fe83 100644 --- a/lib/node-utils/index.ts +++ b/lib/node-utils/index.ts @@ -589,7 +589,7 @@ export function findImportSpecifier( ); }); - // it is "import { foo [as alias] } from 'baz'"" + // it is "import { foo [as alias] } from 'baz'" if (namedExport) { return namedExport; } diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index 6e65544e..ee0eed1d 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -18,20 +18,20 @@ ruleTester.run(RULE_NAME, rule, { act(() => { stuffThatDoesNotUseRTL(); }); - + await act(async () => { stuffThatDoesNotUseRTL(); }); - + act(function() { stuffThatDoesNotUseRTL(); const a = foo(); }); - + act(function() { return stuffThatDoesNotUseRTL(); }); - + act(() => stuffThatDoesNotUseRTL()); }); `, @@ -44,19 +44,19 @@ ruleTester.run(RULE_NAME, rule, { act(() => { stuffThatDoesNotUseRTL(); }); - + await act(async () => { stuffThatDoesNotUseRTL(); }); - + act(function() { stuffThatDoesNotUseRTL(); }); - + act(function() { return stuffThatDoesNotUseRTL(); }); - + act(() => stuffThatDoesNotUseRTL()); }); `, @@ -73,19 +73,19 @@ ruleTester.run(RULE_NAME, rule, { act(() => { waitFor(); }); - + await act(async () => { waitFor(); }); - + act(function() { waitFor(); }); - + act(function() { return waitFor(); }); - + act(() => waitFor()); }); `, @@ -99,12 +99,12 @@ ruleTester.run(RULE_NAME, rule, { render(element); stuffThatDoesNotUseRTL(); }); - + await act(async () => { waitFor(); stuffThatDoesNotUseRTL(); }); - + act(function() { waitFor(); stuffThatDoesNotUseRTL(); @@ -124,19 +124,19 @@ ruleTester.run(RULE_NAME, rule, { act(() => { waitFor(); }); - + await act(async () => { waitFor(); }); - + act(function() { waitFor(); }); - + act(function() { return waitFor(); }); - + act(() => waitFor()); act(() => {}) @@ -157,23 +157,23 @@ ruleTester.run(RULE_NAME, rule, { act(() => { fireEvent.click(el); }); - + await act(async () => { waitFor(() => {}); }); - + await act(async () => { waitForElementToBeRemoved(el); }); - + act(function() { const blah = screen.getByText('blah'); }); - + act(function() { render(something); }); - + await act(() => { const button = findByRole('button') }); @@ -181,7 +181,90 @@ ruleTester.run(RULE_NAME, rule, { act(() => { userEvent.click(el) }); - + + act(() => { + waitFor(); + const element = screen.getByText('blah'); + userEvent.click(element) + }); + }); + `, + errors: [ + { messageId: 'noUnnecessaryActTestingLibraryUtil', line: 6, column: 9 }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 10, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 14, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 18, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 22, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 26, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 30, + column: 9, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 34, + column: 9, + }, + ], + }, + { + settings: { + 'testing-library/utils-module': 'test-utils', + }, + code: `// case: RTL act wrapping RTL calls - callbacks with body (BlockStatement) - AGR disabled + import { act, fireEvent, screen, render, waitFor, waitForElementToBeRemoved } from 'test-utils' + import userEvent from '@testing-library/user-event' + + test('invalid case', async () => { + act(() => { + fireEvent.click(el); + }); + + await act(async () => { + waitFor(() => {}); + }); + + await act(async () => { + waitForElementToBeRemoved(el); + }); + + act(function() { + const blah = screen.getByText('blah'); + }); + + act(function() { + render(something); + }); + + await act(() => { + const button = findByRole('button') + }); + + act(() => { + userEvent.click(el) + }); + act(() => { waitFor(); const element = screen.getByText('blah'); @@ -393,34 +476,34 @@ ruleTester.run(RULE_NAME, rule, { // cases for act related to React Test Utils { settings: { - 'testing-library/utils-module': 'test-utils', + 'testing-library/utils-module': 'custom-testing-module', }, code: `// case: RTU act wrapping RTL calls - callbacks with body (BlockStatement) import { act } from 'react-dom/test-utils'; - import { fireEvent, screen, render, waitFor, waitForElementToBeRemoved } from 'test-utils' + import { fireEvent, screen, render, waitFor, waitForElementToBeRemoved } from 'custom-testing-module' import userEvent from '@testing-library/user-event' test('invalid case', async () => { act(() => { fireEvent.click(el); }); - + await act(async () => { waitFor(() => {}); }); - + await act(async () => { waitForElementToBeRemoved(el); }); - + act(function() { const blah = screen.getByText('blah'); }); - + act(function() { render(something); }); - + await act(() => { const button = findByRole('button') }); @@ -428,7 +511,7 @@ ruleTester.run(RULE_NAME, rule, { act(() => { userEvent.click(el) }); - + act(() => { waitFor(); const element = screen.getByText('blah'); @@ -477,11 +560,11 @@ ruleTester.run(RULE_NAME, rule, { }, { settings: { - 'testing-library/utils-module': 'test-utils', + 'testing-library/utils-module': 'custom-testing-module', }, code: `// case: RTU act wrapping RTL calls - callbacks with return import { act } from 'react-dom/test-utils'; - import { fireEvent, screen, render, waitFor } from 'test-utils' + import { fireEvent, screen, render, waitFor } from 'custom-testing-module' import userEvent from '@testing-library/user-event' test('invalid case', async () => { @@ -607,44 +690,48 @@ ruleTester.run(RULE_NAME, rule, { }, { settings: { - 'testing-library/utils-module': 'test-utils', + 'testing-library/utils-module': 'off', }, code: `// case: RTU act wrapping empty callback import { act } from 'react-dom/test-utils'; + import { render } from '@testing-library/react' test('invalid case', async () => { - await act(async () => {}) - act(() => {}) - await act(async function () {}) - act(function () {}) - }) + render(element); + await act(async () => {}); + act(() => {}); + await act(async function () {}); + act(function () {}); + }); `, errors: [ - { messageId: 'noUnnecessaryActEmptyFunction', line: 5, column: 15 }, - { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 9, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 10, column: 9 }, ], }, { settings: { - 'testing-library/utils-module': 'test-utils', + 'testing-library/utils-module': 'off', }, code: `// case: RTU act wrapping empty callback - require version const { act } = require('react-dom/test-utils'); + const { render } = require('@testing-library/react'); test('invalid case', async () => { - await act(async () => {}) - act(() => {}) - await act(async function () {}) - act(function () {}) + render(element); + await act(async () => {}); + act(() => {}); + await act(async function () {}); + act(function () {}); }) `, errors: [ - { messageId: 'noUnnecessaryActEmptyFunction', line: 5, column: 15 }, - { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 9, column: 15 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 10, column: 9 }, ], }, From 50d5d78360ce0355e2dd604258544c678c30f5f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Mon, 3 May 2021 18:29:50 +0200 Subject: [PATCH 14/22] fix: remove duplicated isVariableDeclaration declaration --- lib/node-utils/is-node-of-type.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/node-utils/is-node-of-type.ts b/lib/node-utils/is-node-of-type.ts index 96b6fbc2..ac8dbb5f 100644 --- a/lib/node-utils/is-node-of-type.ts +++ b/lib/node-utils/is-node-of-type.ts @@ -43,6 +43,3 @@ export const isObjectExpression = isNodeOfType(AST_NODE_TYPES.ObjectExpression); export const isObjectPattern = isNodeOfType(AST_NODE_TYPES.ObjectPattern); export const isProperty = isNodeOfType(AST_NODE_TYPES.Property); export const isReturnStatement = isNodeOfType(AST_NODE_TYPES.ReturnStatement); -export const isVariableDeclaration = isNodeOfType( - AST_NODE_TYPES.VariableDeclaration -); From 489be0447a6f7e01a6b0982d5d8d339aba531944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Tue, 4 May 2021 21:58:21 +0200 Subject: [PATCH 15/22] feat(no-unnecessary-act): detect edge and mixed cases --- .../detect-testing-library-utils.ts | 18 ++++---- lib/rules/no-unnecessary-act.ts | 25 ++++------- tests/lib/rules/no-unnecessary-act.test.ts | 44 ++++++++++++++++++- 3 files changed, 62 insertions(+), 25 deletions(-) diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index 55a45749..cf23b8f3 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -689,14 +689,10 @@ export function detectTestingLibraryUtils< return false; } - const originalNodeName = - isImportSpecifier(importedUtilSpecifier) && - importedUtilSpecifier.local.name !== - importedUtilSpecifier.imported.name - ? importedUtilSpecifier.imported.name - : undefined; - - return [node.name, originalNodeName].filter(Boolean).includes('act'); + return hasImportMatch( + importedUtilSpecifier, + referenceNodeIdentifier.name + ); })(); return isTestingLibraryAct || isReactDomTestUtilsAct; @@ -832,6 +828,12 @@ export function detectTestingLibraryUtils< return false; } + const referenceNode = getReferenceNode(node); + const referenceNodeIdentifier = getPropertyIdentifierNode(referenceNode); + if (!referenceNodeIdentifier) { + return false; + } + const importDeclaration = (() => { if (isImportDeclaration(importNode.parent)) { return importNode.parent; diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index 28f94218..c928f00e 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -2,7 +2,6 @@ import { TSESTree } from '@typescript-eslint/experimental-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; import { getDeepestIdentifierNode, - getPropertyIdentifierNode, getStatementCallExpression, isEmptyFunction, } from '../node-utils'; @@ -75,26 +74,23 @@ export default createTestingLibraryRule<[], MessageIds>({ return; } - const callExpressionIdentifier = getPropertyIdentifierNode( - callExpressionNode - ); - - if (!callExpressionIdentifier) { + const identifierNode = getDeepestIdentifierNode(callExpressionNode); + if (!identifierNode) { return; } - if (!helpers.isActUtil(callExpressionIdentifier)) { + if (!helpers.isActUtil(identifierNode)) { return; } if (isEmptyFunction(functionNode)) { context.report({ - node: callExpressionIdentifier, + node: identifierNode, messageId: 'noUnnecessaryActEmptyFunction', }); } else if (!hasSomeNonTestingLibraryCall(blockStatementNode.body)) { context.report({ - node: callExpressionIdentifier, + node: identifierNode, messageId: 'noUnnecessaryActTestingLibraryUtil', }); } @@ -117,15 +113,12 @@ export default createTestingLibraryRule<[], MessageIds>({ return; } - const parentCallExpressionIdentifier = getPropertyIdentifierNode( - parentCallExpression - ); - - if (!parentCallExpressionIdentifier) { + const identifierNode = getDeepestIdentifierNode(parentCallExpression); + if (!identifierNode) { return; } - if (!helpers.isActUtil(parentCallExpressionIdentifier)) { + if (!helpers.isActUtil(identifierNode)) { return; } @@ -134,7 +127,7 @@ export default createTestingLibraryRule<[], MessageIds>({ } context.report({ - node: parentCallExpressionIdentifier, + node: identifierNode, messageId: 'noUnnecessaryActTestingLibraryUtil', }); } diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index ee0eed1d..79cbc1f4 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -735,6 +735,48 @@ ruleTester.run(RULE_NAME, rule, { ], }, - // TODO case: mixed scenarios - AGR disabled + { + settings: { + 'testing-library/utils-module': 'custom-testing-module', + 'testing-library/custom-renders': 'off', + }, + code: `// case: mixed scenarios - AGR disabled + import * as ReactTestUtils from 'react-dom/test-utils'; + import { act as renamedAct, fireEvent, screen as renamedScreen, render, waitFor } from 'custom-testing-module' + import userEvent from '@testing-library/user-event' + import { act, waitForElementToBeRemoved } from 'somewhere-else' + + test('invalid case', async () => { + ReactTestUtils.act(() => {}) + await ReactTestUtils.act(() => render()) + await renamedAct(async () => waitFor()) + renamedAct(function() { renamedScreen.findByRole('button') }) + + // these are valid + await renamedAct(() => waitForElementToBeRemoved(element)) + act(() => {}) + await act(async () => { userEvent.click(element) }) + act(function() { return renamedScreen.getByText('foo') }) + }); + `, + errors: [ + { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 24 }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 9, + column: 30, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 10, + column: 15, + }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 11, + column: 9, + }, + ], + }, ], }); From a7de170926afa5021ab5ed006d50e06e580ca653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Tue, 4 May 2021 22:55:33 +0200 Subject: [PATCH 16/22] docs(no-unnecessary-act): add rule details --- docs/rules/no-unnecessary-act.md | 118 ++++++++++++++++++++++++++++++- lib/rules/no-unnecessary-act.ts | 2 +- 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/docs/rules/no-unnecessary-act.md b/docs/rules/no-unnecessary-act.md index 70b786d1..f6896a4f 100644 --- a/docs/rules/no-unnecessary-act.md +++ b/docs/rules/no-unnecessary-act.md @@ -1 +1,117 @@ -// TODO +# Disallow wrapping Testing Library utils or empty callbacks in `act` (`testing-library/no-unnecessary-act`) + +> ⚠️ The `act` method is only available on the following Testing Library packages: +> +> - `@testing-library/react` (supported by this plugin) +> - `@testing-library/preact` (not supported yet by this plugin) +> - `@testing-library/svelte` (not supported yet by this plugin) + +## Rule Details + +This rule aims to avoid the usage of `act` to wrap Testing Library utils just to silence "not wrapped in act(...)" warnings. + +All Testing Library utils are already wrapped in `act`. Most of the time, if you're seeing an `act` warning, it's not just something to be silenced, but it's actually telling you that something unexpected is happening in your test. + +Additionally, wrapping empty callbacks in `act` is also an incorrect way of silencing "not wrapped in act(...)" warnings. + +Code violations reported by this rule will pinpoint those unnecessary `act`, helping to understand when `act` actually is necessary. + +Example of **incorrect** code for this rule: + +```js +// ❌ wrapping things related to Testing Library in `act` is incorrect +import { + act, + render, + screen, + waitFor, + fireEvent, +} from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +// ... + +act(() => { + render(); +}); + +await act(async () => waitFor(() => {})); + +act(() => screen.getByRole('button')); + +act(() => { + fireEvent.click(element); +}); + +act(() => { + userEvent.click(element); +}); +``` + +```js +// ❌ wrapping empty callbacks in `act` is incorrect +import { act } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +// ... + +act(() => {}); + +await act(async () => {}); +``` + +```js +// ❌ wrapping things related to Testing Library in React DOM Test Utils `act` is also incorrect +import { act } from 'react-dom/test-utils'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +// ... + +act(() => { + render(); +}); + +await act(async () => waitFor(() => {})); + +act(() => screen.getByRole('button')); + +act(() => { + fireEvent.click(element); +}); + +act(() => { + userEvent.click(element); +}); +``` + +Examples of **correct** code for this rule: + +```js +// ✅ wrapping things not related to Testing Library in `act` is correct +import { act } from '@testing-library/react'; +import { stuffThatDoesNotUseRTL } from 'somwhere-else'; + +// ... + +act(() => { + stuffThatDoesNotUseRTL(); +}); +``` + +```js +// ✅ wrapping both things related and not related to Testing Library in `act` is correct +import { act, screen } from '@testing-library/react'; +import { stuffThatDoesNotUseRTL } from 'somwhere-else'; + +await act(async () => { + await screen.findByRole('button'); + stuffThatDoesNotUseRTL(); +}); +``` + +## Further Reading + +- [Inspiration for this rule](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#wrapping-things-in-act-unnecessarily) +- [Fix the "not wrapped in act(...)" warning](https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning) +- [About React Testing Library `act`](https://testing-library.com/docs/react-testing-library/api/#act) diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index c928f00e..e3222527 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -17,7 +17,7 @@ export default createTestingLibraryRule<[], MessageIds>({ type: 'problem', docs: { description: - 'Disallow the use of `act` when wrapping Testing Library utils or empty functions', + 'Disallow wrapping Testing Library utils or empty callbacks in `act`', category: 'Possible Errors', recommendedConfig: { dom: false, From d6b4247d4d031b7f67ee80261fe06661551a772b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Tue, 4 May 2021 23:01:05 +0200 Subject: [PATCH 17/22] docs(no-unnecessary-act): include rule in README --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f438182c..1bd131a8 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,7 @@ To enable this configuration use the `extends` property in your | [testing-library/no-node-access](docs/rules/no-node-access.md) | Disallow direct Node access | ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [testing-library/no-promise-in-fire-event](docs/rules/no-promise-in-fire-event.md) | Disallow the use of promises passed to a `fireEvent` method | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [testing-library/no-render-in-setup](docs/rules/no-render-in-setup.md) | Disallow the use of `render` in setup functions | | | +| [testing-library/no-unnecessary-act](docs/rules/no-unnecessary-act.md) | Disallow wrapping Testing Library utils or empty callbacks in `act` | | | | [testing-library/no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [testing-library/no-wait-for-multiple-assertions](docs/rules/no-wait-for-multiple-assertions.md) | Disallow the use of multiple expect inside `waitFor` | | | | [testing-library/no-wait-for-side-effects](docs/rules/no-wait-for-side-effects.md) | Disallow the use of side effects inside `waitFor` | | | @@ -200,8 +201,8 @@ To enable this configuration use the `extends` property in your | [testing-library/prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | | [testing-library/prefer-find-by](docs/rules/prefer-find-by.md) | Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | | [testing-library/prefer-presence-queries](docs/rules/prefer-presence-queries.md) | Enforce specific queries when checking element is present or not | | | -| [testing-library/prefer-user-event](docs/rules/prefer-user-event.md) | Suggest using `userEvent` library instead of `fireEvent` for simulating user interaction | | | | [testing-library/prefer-screen-queries](docs/rules/prefer-screen-queries.md) | Suggest using screen while using queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [testing-library/prefer-user-event](docs/rules/prefer-user-event.md) | Suggest using `userEvent` library instead of `fireEvent` for simulating user interaction | | | | [testing-library/prefer-wait-for](docs/rules/prefer-wait-for.md) | Use `waitFor` instead of deprecated wait methods | | ![fixable-badge][] | | [testing-library/render-result-naming-convention](docs/rules/render-result-naming-convention.md) | Enforce a valid naming for return value from `render` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | From 82e7ad0e3e4ec5c3c5bd446dc356d87b524836b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Tue, 4 May 2021 23:20:48 +0200 Subject: [PATCH 18/22] refactor: rename findImportedTestingLibraryUtilSpecifier in helpers --- .../detect-testing-library-utils.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts index cf23b8f3..6a85322f 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -83,7 +83,7 @@ type IsDebugUtilFn = (identifierNode: TSESTree.Identifier) => boolean; type IsPresenceAssertFn = (node: TSESTree.MemberExpression) => boolean; type IsAbsenceAssertFn = (node: TSESTree.MemberExpression) => boolean; type CanReportErrorsFn = () => boolean; -type FindImportedUtilSpecifierFn = ( +type FindImportedTestingLibraryUtilSpecifierFn = ( specifierName: string ) => TSESTree.ImportClause | TSESTree.Identifier | undefined; type IsNodeComingFromTestingLibraryFn = ( @@ -117,7 +117,7 @@ export interface DetectionHelpers { isPresenceAssert: IsPresenceAssertFn; isAbsenceAssert: IsAbsenceAssertFn; canReportErrors: CanReportErrorsFn; - findImportedUtilSpecifier: FindImportedUtilSpecifierFn; + findImportedTestingLibraryUtilSpecifier: FindImportedTestingLibraryUtilSpecifierFn; isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn; } @@ -750,7 +750,7 @@ export function detectTestingLibraryUtils< /** * Finds the import util specifier related to Testing Library for a given name. */ - const findImportedTestingLibraryUtilSpecifier: FindImportedUtilSpecifierFn = ( + const findImportedTestingLibraryUtilSpecifier: FindImportedTestingLibraryUtilSpecifierFn = ( specifierName ): TSESTree.ImportClause | TSESTree.Identifier | undefined => { const node = getCustomModuleImportNode() ?? getTestingLibraryImportNode(); @@ -902,7 +902,7 @@ export function detectTestingLibraryUtils< isPresenceAssert, isAbsenceAssert, canReportErrors, - findImportedUtilSpecifier: findImportedTestingLibraryUtilSpecifier, + findImportedTestingLibraryUtilSpecifier, isNodeComingFromTestingLibrary, }; From 963e70ed09a61a177b28964d0d616e569aea7381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Thu, 6 May 2021 21:47:17 +0200 Subject: [PATCH 19/22] docs(no-unnecessary-act): simplify examples --- docs/rules/no-unnecessary-act.md | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/docs/rules/no-unnecessary-act.md b/docs/rules/no-unnecessary-act.md index f6896a4f..065b6c10 100644 --- a/docs/rules/no-unnecessary-act.md +++ b/docs/rules/no-unnecessary-act.md @@ -27,6 +27,7 @@ import { waitFor, fireEvent, } from '@testing-library/react'; +// ^ act imported from 'react-dom/test-utils' will be reported too import userEvent from '@testing-library/user-event'; // ... @@ -51,6 +52,7 @@ act(() => { ```js // ❌ wrapping empty callbacks in `act` is incorrect import { act } from '@testing-library/react'; +// ^ act imported from 'react-dom/test-utils' will be reported too import userEvent from '@testing-library/user-event'; // ... @@ -60,31 +62,6 @@ act(() => {}); await act(async () => {}); ``` -```js -// ❌ wrapping things related to Testing Library in React DOM Test Utils `act` is also incorrect -import { act } from 'react-dom/test-utils'; -import { render, screen, waitFor, fireEvent } from '@testing-library/react'; -import userEvent from '@testing-library/user-event'; - -// ... - -act(() => { - render(); -}); - -await act(async () => waitFor(() => {})); - -act(() => screen.getByRole('button')); - -act(() => { - fireEvent.click(element); -}); - -act(() => { - userEvent.click(element); -}); -``` - Examples of **correct** code for this rule: ```js From 50a72c16f39b44e3bfad5b672589c639885c5b73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Thu, 6 May 2021 22:19:01 +0200 Subject: [PATCH 20/22] refactor: remove unnecessary comment --- lib/rules/no-unnecessary-act.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-unnecessary-act.ts b/lib/rules/no-unnecessary-act.ts index e3222527..152dd226 100644 --- a/lib/rules/no-unnecessary-act.ts +++ b/lib/rules/no-unnecessary-act.ts @@ -22,7 +22,7 @@ export default createTestingLibraryRule<[], MessageIds>({ recommendedConfig: { dom: false, angular: false, - react: false, // this should be enabled on v5 of the plugin + react: false, vue: false, }, }, From dc64f8ba7a433f1ffb52b748717c8cbd42b381da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Fri, 7 May 2021 00:18:58 +0200 Subject: [PATCH 21/22] fix(no-unnecessary-act): cover more trees searching for statements --- lib/node-utils/index.ts | 31 ++++++++++++++++++---- tests/lib/rules/no-unnecessary-act.test.ts | 10 ++++++- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/lib/node-utils/index.ts b/lib/node-utils/index.ts index b1a9fe83..62e7dc3d 100644 --- a/lib/node-utils/index.ts +++ b/lib/node-utils/index.ts @@ -10,6 +10,7 @@ import { RuleContext } from '@typescript-eslint/experimental-utils/dist/ts-eslin import { isArrayExpression, isArrowFunctionExpression, + isAssignmentExpression, isBlockStatement, isCallExpression, isExpressionStatement, @@ -536,11 +537,31 @@ export function hasImportMatch( export function getStatementCallExpression( statement: TSESTree.Statement ): TSESTree.CallExpression | undefined { - if ( - isExpressionStatement(statement) && - isCallExpression(statement.expression) - ) { - return statement.expression; + if (isExpressionStatement(statement)) { + const { expression } = statement; + if (isCallExpression(expression)) { + return expression; + } + + if ( + ASTUtils.isAwaitExpression(expression) && + isCallExpression(expression.argument) + ) { + return expression.argument; + } + + if (isAssignmentExpression(expression)) { + if (isCallExpression(expression.right)) { + return expression.right; + } + + if ( + ASTUtils.isAwaitExpression(expression.right) && + isCallExpression(expression.right.argument) + ) { + return expression.right.argument; + } + } } if (isReturnStatement(statement) && isCallExpression(statement.argument)) { diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index 79cbc1f4..d84a7c51 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -18,9 +18,17 @@ ruleTester.run(RULE_NAME, rule, { act(() => { stuffThatDoesNotUseRTL(); }); + + act(function() { + a = stuffThatDoesNotUseRTL(); + }); + + act(function() { + a = await stuffThatDoesNotUseRTL(); + }); await act(async () => { - stuffThatDoesNotUseRTL(); + await stuffThatDoesNotUseRTL(); }); act(function() { From d07669f70cc0216569c7f5e836d3a50c98840a7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Fri, 7 May 2021 16:28:20 +0200 Subject: [PATCH 22/22] test(no-unnecessary-act): extra cases for thenable promises --- tests/lib/rules/no-unnecessary-act.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/lib/rules/no-unnecessary-act.test.ts b/tests/lib/rules/no-unnecessary-act.test.ts index d84a7c51..b5bb7992 100644 --- a/tests/lib/rules/no-unnecessary-act.test.ts +++ b/tests/lib/rules/no-unnecessary-act.test.ts @@ -41,6 +41,9 @@ ruleTester.run(RULE_NAME, rule, { }); act(() => stuffThatDoesNotUseRTL()); + + act(() => stuffThatDoesNotUseRTL()).then(() => {}) + act(stuffThatDoesNotUseRTL().then(() => {})) }); `, }, @@ -362,6 +365,9 @@ ruleTester.run(RULE_NAME, rule, { await act(async function () { return waitFor(() => {}); }); + act(async function () { + return waitFor(() => {}); + }).then(() => {}) }); `, errors: [ @@ -439,6 +445,11 @@ ruleTester.run(RULE_NAME, rule, { line: 40, column: 15, }, + { + messageId: 'noUnnecessaryActTestingLibraryUtil', + line: 43, + column: 9, + }, ], }, { @@ -471,6 +482,7 @@ ruleTester.run(RULE_NAME, rule, { act(() => {}) await act(async function () {}) act(function () {}) + act(function () {}).then(() => {}) }) `, errors: [ @@ -478,6 +490,7 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'noUnnecessaryActEmptyFunction', line: 6, column: 9 }, { messageId: 'noUnnecessaryActEmptyFunction', line: 7, column: 15 }, { messageId: 'noUnnecessaryActEmptyFunction', line: 8, column: 9 }, + { messageId: 'noUnnecessaryActEmptyFunction', line: 9, column: 9 }, ], },