From 256cd21b49da540147ea7b232cbcedc9eb981395 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sat, 1 May 2021 14:48:15 +0300 Subject: [PATCH 01/12] feat(no-wait-for-side-effects): report render in waitFor --- .../detect-testing-library-utils.ts | 43 ++- lib/node-utils/is-node-of-type.ts | 9 + lib/rules/no-wait-for-side-effects.ts | 65 +++- .../rules/no-wait-for-side-effects.test.ts | 323 ++++++++++++++++++ 4 files changed, 411 insertions(+), 29 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..a77fa3e4 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -75,7 +75,7 @@ type IsAsyncUtilFn = ( ) => boolean; type IsFireEventMethodFn = (node: TSESTree.Identifier) => boolean; type IsUserEventMethodFn = (node: TSESTree.Identifier) => boolean; -type IsRenderUtilFn = (node: TSESTree.Identifier) => boolean; +type IsRenderUtilFn = (node: TSESTree.Identifier | null) => boolean; type IsRenderVariableDeclaratorFn = ( node: TSESTree.VariableDeclarator ) => boolean; @@ -105,8 +105,8 @@ export interface DetectionHelpers { isCustomQuery: IsCustomQueryFn; isBuiltInQuery: IsBuiltInQueryFn; isAsyncUtil: IsAsyncUtilFn; - isFireEventUtil: (node: TSESTree.Identifier) => boolean; - isUserEventUtil: (node: TSESTree.Identifier) => boolean; + isFireEventUtil: (node: TSESTree.Identifier | null) => boolean; + isUserEventUtil: (node: TSESTree.Identifier | null) => boolean; isFireEventMethod: IsFireEventMethodFn; isUserEventMethod: IsUserEventMethodFn; isRenderUtil: IsRenderUtilFn; @@ -429,7 +429,9 @@ export function detectTestingLibraryUtils< * * Not to be confused with {@link isFireEventMethod} */ - const isFireEventUtil = (node: TSESTree.Identifier): boolean => { + const isFireEventUtil = (node: TSESTree.Identifier | null): boolean => { + if (!node) return false; + return isTestingLibraryUtil( node, (identifierNodeName, originalNodeName) => { @@ -443,7 +445,9 @@ export function detectTestingLibraryUtils< * * Not to be confused with {@link isUserEventMethod} */ - const isUserEventUtil = (node: TSESTree.Identifier): boolean => { + const isUserEventUtil = (node: TSESTree.Identifier | null): boolean => { + if (!node) return false; + const userEvent = findImportedUserEventSpecifier(); let userEventName: string | undefined; @@ -569,18 +573,25 @@ export function detectTestingLibraryUtils< * Testing Library. Otherwise, it means `custom-module` has been set up, so * 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); - } + const isRenderUtil: IsRenderUtilFn = (node) => { + if (!node) return false; - return [RENDER_NAME, ...getCustomRenders()].some( - (validRenderName) => - validRenderName === identifierNodeName || - (Boolean(originalNodeName) && validRenderName === originalNodeName) - ); - }); + return isTestingLibraryUtil( + node, + (identifierNodeName, originalNodeName) => { + if (isAggressiveRenderReportingEnabled()) { + return identifierNodeName.toLowerCase().includes(RENDER_NAME); + } + + return [RENDER_NAME, ...getCustomRenders()].some( + (validRenderName) => + validRenderName === identifierNodeName || + (Boolean(originalNodeName) && + validRenderName === originalNodeName) + ); + } + ); + }; const isRenderVariableDeclarator: IsRenderVariableDeclaratorFn = (node) => { if (!node.init) { diff --git a/lib/node-utils/is-node-of-type.ts b/lib/node-utils/is-node-of-type.ts index 89d8880b..ac8dbb5f 100644 --- a/lib/node-utils/is-node-of-type.ts +++ b/lib/node-utils/is-node-of-type.ts @@ -16,6 +16,15 @@ export const isCallExpression = isNodeOfType(AST_NODE_TYPES.CallExpression); export const isExpressionStatement = isNodeOfType( AST_NODE_TYPES.ExpressionStatement ); +export const isVariableDeclaration = isNodeOfType( + AST_NODE_TYPES.VariableDeclaration +); +export const isAssignmentExpression = isNodeOfType( + AST_NODE_TYPES.AssignmentExpression +); +export const isSequenceExpression = isNodeOfType( + AST_NODE_TYPES.SequenceExpression +); export const isImportDeclaration = isNodeOfType( AST_NODE_TYPES.ImportDeclaration ); diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index c48735ed..766d1ddc 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -2,6 +2,10 @@ import { TSESTree } from '@typescript-eslint/experimental-utils'; import { getPropertyIdentifierNode, isExpressionStatement, + isVariableDeclaration, + isAssignmentExpression, + isCallExpression, + isSequenceExpression, } from '../node-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; @@ -32,7 +36,11 @@ export default createTestingLibraryRule({ defaultOptions: [], create: function (context, _, helpers) { function isCallerWaitFor( - node: TSESTree.BlockStatement | TSESTree.CallExpression + node: + | TSESTree.BlockStatement + | TSESTree.CallExpression + | TSESTree.AssignmentExpression + | TSESTree.SequenceExpression ): boolean { if (!node.parent) { return false; @@ -52,18 +60,31 @@ export default createTestingLibraryRule({ body: TSESTree.Node[] ): TSESTree.ExpressionStatement[] { return body.filter((node) => { - if (!isExpressionStatement(node)) { + if (!isExpressionStatement(node) && !isVariableDeclaration(node)) { return false; } const expressionIdentifier = getPropertyIdentifierNode(node); - if (!expressionIdentifier) { - return false; - } + + const isRenderInVariableDeclaration = + isVariableDeclaration(node) && + node.declarations.some((declaration) => + helpers.isRenderVariableDeclarator(declaration) + ); + + const isRenderInAssignment = + isExpressionStatement(node) && + isAssignmentExpression(node.expression) && + helpers.isRenderUtil( + getPropertyIdentifierNode(node.expression.right) + ); return ( helpers.isFireEventUtil(expressionIdentifier) || - helpers.isUserEventUtil(expressionIdentifier) + helpers.isUserEventUtil(expressionIdentifier) || + helpers.isRenderUtil(expressionIdentifier) || + isRenderInVariableDeclaration || + isRenderInAssignment ); }) as TSESTree.ExpressionStatement[]; } @@ -86,19 +107,35 @@ export default createTestingLibraryRule({ } } - function reportImplicitReturnSideEffect(node: TSESTree.CallExpression) { + function reportImplicitReturnSideEffect( + node: + | TSESTree.CallExpression + | TSESTree.AssignmentExpression + | TSESTree.SequenceExpression + ) { if (!isCallerWaitFor(node)) { return; } - const expressionIdentifier = getPropertyIdentifierNode(node.callee); - if (!expressionIdentifier) { - return; - } + const expressionIdentifier = + isCallExpression(node) && getPropertyIdentifierNode(node.callee); + const isRenderInAssignment = + isAssignmentExpression(node) && + helpers.isRenderUtil(getPropertyIdentifierNode(node.right)); + const isRenderInSequenceAssignment = + isSequenceExpression(node) && + node.expressions.some( + (expression) => + isAssignmentExpression(expression) && + helpers.isRenderUtil(getPropertyIdentifierNode(expression.right)) + ); if ( - !helpers.isFireEventUtil(expressionIdentifier) && - !helpers.isUserEventUtil(expressionIdentifier) + !helpers.isFireEventUtil(expressionIdentifier || null) && + !helpers.isUserEventUtil(expressionIdentifier || null) && + !helpers.isRenderUtil(expressionIdentifier || null) && + !isRenderInAssignment && + !isRenderInSequenceAssignment ) { return; } @@ -112,6 +149,8 @@ export default createTestingLibraryRule({ return { 'CallExpression > ArrowFunctionExpression > BlockStatement': reportSideEffects, 'CallExpression > ArrowFunctionExpression > CallExpression': reportImplicitReturnSideEffect, + 'CallExpression > ArrowFunctionExpression > AssignmentExpression': reportImplicitReturnSideEffect, + 'CallExpression > ArrowFunctionExpression > SequenceExpression': reportImplicitReturnSideEffect, 'CallExpression > FunctionExpression > BlockStatement': reportSideEffects, }; }, diff --git a/tests/lib/rules/no-wait-for-side-effects.test.ts b/tests/lib/rules/no-wait-for-side-effects.test.ts index 8d1b552d..46d522a4 100644 --- a/tests/lib/rules/no-wait-for-side-effects.test.ts +++ b/tests/lib/rules/no-wait-for-side-effects.test.ts @@ -180,8 +180,331 @@ ruleTester.run(RULE_NAME, rule, { await waitFor(() => userEvent.click(button)) `, }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'somewhere-else'; + await waitFor(() => render()) + `, + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'somewhere-else'; + await waitFor(() => { + const { container } = render() + }) + `, + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'somewhere-else'; + const { rerender } = render() + await waitFor(() => { + rerender() + }) + `, + }, + { + settings: { 'testing-library/utils-module': '~/test-utils' }, + code: ` + import { waitFor } from '~/test-utils'; + import { render } from 'somewhere-else'; + await waitFor(() => render()) + `, + }, + { + settings: { 'testing-library/utils-module': '~/test-utils' }, + code: ` + import { waitFor } from '@testing-library/react'; + import { render } from 'somewhere-else'; + await waitFor(() => render()) + `, + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderWrapper } from 'somewhere-else'; + await waitFor(() => renderWrapper()) + `, + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderWrapper } from 'somewhere-else'; + await waitFor(() => { + renderWrapper() + }) + `, + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderWrapper } from 'somewhere-else'; + await waitFor(() => { + const { container } = renderWrapper() + }) + `, + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'somewhere-else'; + await waitFor(() => { + render() + }) + `, + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'test-utils'; + import { render } from 'somewhere-else'; + await waitFor(() => { + render() + }) + `, + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderWrapper } from 'somewhere-else'; + await waitFor(() => { + renderWrapper() + }) + `, + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => result = renderWrapper()) + `, + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'test-utils'; + import { render } from 'somewhere-else'; + await waitFor(() => result = render()) + `, + }, + { + settings: { 'testing-library/utils-module': 'test-utils' }, + code: ` + import { waitFor } from 'somewhere-else'; + await waitFor(() => result = render()) + `, + }, ], invalid: [ + // render + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => render()) + `, + errors: [{ line: 3, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(function() { + render() + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(function() { + const { container } = renderHelper() + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderHelper } from 'somewhere-else'; + await waitFor(() => renderHelper()) + `, + errors: [{ line: 4, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderHelper } from 'somewhere-else'; + await waitFor(() => { + renderHelper() + }) + `, + errors: [{ line: 5, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderHelper } from 'somewhere-else'; + await waitFor(() => { + const { container } = renderHelper() + }) + `, + errors: [{ line: 5, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + settings: { 'testing-library/custom-renders': ['renderHelper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderHelper } from 'somewhere-else'; + let container; + await waitFor(() => { + ({ container } = renderHelper()) + }) + `, + errors: [{ line: 6, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => result = render()) + `, + errors: [{ line: 3, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => (a = 5, result = render())) + `, + errors: [{ line: 3, column: 30, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + const { rerender } = render() + await waitFor(() => rerender()) + `, + errors: [{ line: 4, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor, render } from '@testing-library/react'; + await waitFor(() => render()) + `, + errors: [{ line: 3, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + const { rerender } = render() + await waitFor(() => rerender()) + `, + errors: [{ line: 4, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => renderHelper()) + `, + errors: [{ line: 3, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + import { render } from 'somewhere-else'; + await waitFor(() => render()) + `, + errors: [{ line: 4, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + settings: { 'testing-library/utils-module': '~/test-utils' }, + code: ` + import { waitFor, render } from '~/test-utils'; + await waitFor(() => render()) + `, + errors: [{ line: 3, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + settings: { 'testing-library/custom-renders': ['renderWrapper'] }, + code: ` + import { waitFor } from '@testing-library/react'; + import { renderWrapper } from 'somewhere-else'; + await waitFor(() => renderWrapper()) + `, + errors: [{ line: 4, column: 29, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => { + render() + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => { + const { container } = render() + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => { + const a = 5, + { container } = render() + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + const { rerender } = render() + await waitFor(() => { + rerender() + }) + `, + errors: [{ line: 5, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => { + render() + fireEvent.keyDown(input, {key: 'ArrowDown'}) + }) + `, + errors: [ + { line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }, + { line: 5, column: 11, messageId: 'noSideEffectsWaitFor' }, + ], + }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => { + render() + userEvent.click(button) + }) + `, + errors: [ + { line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }, + { line: 5, column: 11, messageId: 'noSideEffectsWaitFor' }, + ], + }, // fireEvent { code: ` From c39015e2340ed71bde78f2538bb5e6248392418e Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sat, 1 May 2021 18:28:20 +0300 Subject: [PATCH 02/12] refactor: move null check into isTestingLibraryUtil --- .../detect-testing-library-utils.ts | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 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 a77fa3e4..6930c212 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -173,7 +173,7 @@ export function detectTestingLibraryUtils< * reporting) */ function isTestingLibraryUtil( - node: TSESTree.Identifier, + node: TSESTree.Identifier | null, isUtilCallback: ( identifierNodeName: string, originalNodeName?: string @@ -430,8 +430,6 @@ export function detectTestingLibraryUtils< * Not to be confused with {@link isFireEventMethod} */ const isFireEventUtil = (node: TSESTree.Identifier | null): boolean => { - if (!node) return false; - return isTestingLibraryUtil( node, (identifierNodeName, originalNodeName) => { @@ -573,25 +571,18 @@ export function detectTestingLibraryUtils< * Testing Library. Otherwise, it means `custom-module` has been set up, so * only those nodes coming from Testing Library will be considered as valid. */ - const isRenderUtil: IsRenderUtilFn = (node) => { - if (!node) return false; - - return isTestingLibraryUtil( - node, - (identifierNodeName, originalNodeName) => { - if (isAggressiveRenderReportingEnabled()) { - return identifierNodeName.toLowerCase().includes(RENDER_NAME); - } - - return [RENDER_NAME, ...getCustomRenders()].some( - (validRenderName) => - validRenderName === identifierNodeName || - (Boolean(originalNodeName) && - validRenderName === originalNodeName) - ); + const isRenderUtil: IsRenderUtilFn = (node) => + isTestingLibraryUtil(node, (identifierNodeName, originalNodeName) => { + if (isAggressiveRenderReportingEnabled()) { + return identifierNodeName.toLowerCase().includes(RENDER_NAME); } - ); - }; + + return [RENDER_NAME, ...getCustomRenders()].some( + (validRenderName) => + validRenderName === identifierNodeName || + (Boolean(originalNodeName) && validRenderName === originalNodeName) + ); + }); const isRenderVariableDeclarator: IsRenderVariableDeclaratorFn = (node) => { if (!node.init) { From e8eb47164eb93af14b906ca403dd3cbc9435dc54 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sat, 1 May 2021 18:30:41 +0300 Subject: [PATCH 03/12] refactor: move null check into assignment --- lib/rules/no-wait-for-side-effects.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 766d1ddc..3ca32a22 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -117,8 +117,9 @@ export default createTestingLibraryRule({ return; } - const expressionIdentifier = - isCallExpression(node) && getPropertyIdentifierNode(node.callee); + const expressionIdentifier = isCallExpression(node) + ? getPropertyIdentifierNode(node.callee) + : null; const isRenderInAssignment = isAssignmentExpression(node) && helpers.isRenderUtil(getPropertyIdentifierNode(node.right)); @@ -131,9 +132,9 @@ export default createTestingLibraryRule({ ); if ( - !helpers.isFireEventUtil(expressionIdentifier || null) && - !helpers.isUserEventUtil(expressionIdentifier || null) && - !helpers.isRenderUtil(expressionIdentifier || null) && + !helpers.isFireEventUtil(expressionIdentifier) && + !helpers.isUserEventUtil(expressionIdentifier) && + !helpers.isRenderUtil(expressionIdentifier) && !isRenderInAssignment && !isRenderInSequenceAssignment ) { From 3986cddc86b447aa64980c3fa0bcc8898f64c846 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sat, 1 May 2021 18:36:09 +0300 Subject: [PATCH 04/12] refactor: add brackets around null check --- .../detect-testing-library-utils.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 6930c212..3f342b3d 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -444,7 +444,9 @@ export function detectTestingLibraryUtils< * Not to be confused with {@link isUserEventMethod} */ const isUserEventUtil = (node: TSESTree.Identifier | null): boolean => { - if (!node) return false; + if (!node) { + return false; + } const userEvent = findImportedUserEventSpecifier(); let userEventName: string | undefined; From f1e523793b369e2a65d52aff12104a54fecc8354 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sun, 2 May 2021 12:03:05 +0300 Subject: [PATCH 05/12] refactor: introduce early returns in getSideEffectNodes() --- lib/rules/no-wait-for-side-effects.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 3ca32a22..ec4e0b5a 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -64,14 +64,16 @@ export default createTestingLibraryRule({ return false; } - const expressionIdentifier = getPropertyIdentifierNode(node); - const isRenderInVariableDeclaration = isVariableDeclaration(node) && node.declarations.some((declaration) => helpers.isRenderVariableDeclarator(declaration) ); + if (isRenderInVariableDeclaration) { + return true; + } + const isRenderInAssignment = isExpressionStatement(node) && isAssignmentExpression(node.expression) && @@ -79,12 +81,16 @@ export default createTestingLibraryRule({ getPropertyIdentifierNode(node.expression.right) ); + if (isRenderInAssignment) { + return true; + } + + const expressionIdentifier = getPropertyIdentifierNode(node); + return ( helpers.isFireEventUtil(expressionIdentifier) || helpers.isUserEventUtil(expressionIdentifier) || - helpers.isRenderUtil(expressionIdentifier) || - isRenderInVariableDeclaration || - isRenderInAssignment + helpers.isRenderUtil(expressionIdentifier) ); }) as TSESTree.ExpressionStatement[]; } From 706ee2aff6fa15637f73abfc965b2d649455a87c Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sun, 2 May 2021 12:07:25 +0300 Subject: [PATCH 06/12] refactor: remove newlines --- lib/rules/no-wait-for-side-effects.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index ec4e0b5a..268cef8e 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -69,7 +69,6 @@ export default createTestingLibraryRule({ node.declarations.some((declaration) => helpers.isRenderVariableDeclarator(declaration) ); - if (isRenderInVariableDeclaration) { return true; } @@ -80,7 +79,6 @@ export default createTestingLibraryRule({ helpers.isRenderUtil( getPropertyIdentifierNode(node.expression.right) ); - if (isRenderInAssignment) { return true; } From ce7522054789dec65902657fc6ec62da69a833d3 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sun, 2 May 2021 12:25:20 +0300 Subject: [PATCH 07/12] refactor: move checks to separate functions to improve readability --- lib/rules/no-wait-for-side-effects.ts | 64 ++++++++++++++++----------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 268cef8e..84ffda2e 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -56,6 +56,41 @@ export default createTestingLibraryRule({ ); } + function isRenderInVariableDeclaration(node: TSESTree.Node) { + return ( + isVariableDeclaration(node) && + node.declarations.some((declaration) => + helpers.isRenderVariableDeclarator(declaration) + ) + ); + } + + function isRenderInAssignment(node: TSESTree.Node) { + return ( + isExpressionStatement(node) && + isAssignmentExpression(node.expression) && + helpers.isRenderUtil(getPropertyIdentifierNode(node.expression.right)) + ); + } + + function isRenderInImplicitReturnAssignment(node: TSESTree.Node) { + return ( + isAssignmentExpression(node) && + helpers.isRenderUtil(getPropertyIdentifierNode(node.right)) + ); + } + + function isRenderInSequenceAssignment(node: TSESTree.Node) { + return ( + isSequenceExpression(node) && + node.expressions.some( + (expression) => + isAssignmentExpression(expression) && + helpers.isRenderUtil(getPropertyIdentifierNode(expression.right)) + ) + ); + } + function getSideEffectNodes( body: TSESTree.Node[] ): TSESTree.ExpressionStatement[] { @@ -64,22 +99,11 @@ export default createTestingLibraryRule({ return false; } - const isRenderInVariableDeclaration = - isVariableDeclaration(node) && - node.declarations.some((declaration) => - helpers.isRenderVariableDeclarator(declaration) - ); - if (isRenderInVariableDeclaration) { + if (isRenderInVariableDeclaration(node)) { return true; } - const isRenderInAssignment = - isExpressionStatement(node) && - isAssignmentExpression(node.expression) && - helpers.isRenderUtil( - getPropertyIdentifierNode(node.expression.right) - ); - if (isRenderInAssignment) { + if (isRenderInAssignment(node)) { return true; } @@ -124,23 +148,13 @@ export default createTestingLibraryRule({ const expressionIdentifier = isCallExpression(node) ? getPropertyIdentifierNode(node.callee) : null; - const isRenderInAssignment = - isAssignmentExpression(node) && - helpers.isRenderUtil(getPropertyIdentifierNode(node.right)); - const isRenderInSequenceAssignment = - isSequenceExpression(node) && - node.expressions.some( - (expression) => - isAssignmentExpression(expression) && - helpers.isRenderUtil(getPropertyIdentifierNode(expression.right)) - ); if ( !helpers.isFireEventUtil(expressionIdentifier) && !helpers.isUserEventUtil(expressionIdentifier) && !helpers.isRenderUtil(expressionIdentifier) && - !isRenderInAssignment && - !isRenderInSequenceAssignment + !isRenderInImplicitReturnAssignment(node) && + !isRenderInSequenceAssignment(node) ) { return; } From d4c3bb513d724d3525e6df769982b37d5b33bf0f Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Sun, 2 May 2021 18:07:24 +0300 Subject: [PATCH 08/12] refactor: merge multiple if statements into one --- lib/rules/no-wait-for-side-effects.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 84ffda2e..079bf81e 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -99,11 +99,7 @@ export default createTestingLibraryRule({ return false; } - if (isRenderInVariableDeclaration(node)) { - return true; - } - - if (isRenderInAssignment(node)) { + if (isRenderInVariableDeclaration(node) || isRenderInAssignment(node)) { return true; } From 551875195b48e57108f00a96945aeb3239f3909c Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Mon, 3 May 2021 00:35:13 +0300 Subject: [PATCH 09/12] refactor: remove null types in utils --- .../detect-testing-library-utils.ts | 16 ++-- lib/rules/no-wait-for-side-effects.ts | 78 ++++++++++++++----- 2 files changed, 65 insertions(+), 29 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 3f342b3d..3d6662be 100644 --- a/lib/create-testing-library-rule/detect-testing-library-utils.ts +++ b/lib/create-testing-library-rule/detect-testing-library-utils.ts @@ -75,7 +75,7 @@ type IsAsyncUtilFn = ( ) => boolean; type IsFireEventMethodFn = (node: TSESTree.Identifier) => boolean; type IsUserEventMethodFn = (node: TSESTree.Identifier) => boolean; -type IsRenderUtilFn = (node: TSESTree.Identifier | null) => boolean; +type IsRenderUtilFn = (node: TSESTree.Identifier) => boolean; type IsRenderVariableDeclaratorFn = ( node: TSESTree.VariableDeclarator ) => boolean; @@ -105,8 +105,8 @@ export interface DetectionHelpers { isCustomQuery: IsCustomQueryFn; isBuiltInQuery: IsBuiltInQueryFn; isAsyncUtil: IsAsyncUtilFn; - isFireEventUtil: (node: TSESTree.Identifier | null) => boolean; - isUserEventUtil: (node: TSESTree.Identifier | null) => boolean; + isFireEventUtil: (node: TSESTree.Identifier) => boolean; + isUserEventUtil: (node: TSESTree.Identifier) => boolean; isFireEventMethod: IsFireEventMethodFn; isUserEventMethod: IsUserEventMethodFn; isRenderUtil: IsRenderUtilFn; @@ -173,7 +173,7 @@ export function detectTestingLibraryUtils< * reporting) */ function isTestingLibraryUtil( - node: TSESTree.Identifier | null, + node: TSESTree.Identifier, isUtilCallback: ( identifierNodeName: string, originalNodeName?: string @@ -429,7 +429,7 @@ export function detectTestingLibraryUtils< * * Not to be confused with {@link isFireEventMethod} */ - const isFireEventUtil = (node: TSESTree.Identifier | null): boolean => { + const isFireEventUtil = (node: TSESTree.Identifier): boolean => { return isTestingLibraryUtil( node, (identifierNodeName, originalNodeName) => { @@ -443,11 +443,7 @@ export function detectTestingLibraryUtils< * * Not to be confused with {@link isUserEventMethod} */ - const isUserEventUtil = (node: TSESTree.Identifier | null): boolean => { - if (!node) { - return false; - } - + const isUserEventUtil = (node: TSESTree.Identifier): boolean => { const userEvent = findImportedUserEventSpecifier(); let userEventName: string | undefined; diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 079bf81e..205b4169 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -66,29 +66,58 @@ export default createTestingLibraryRule({ } function isRenderInAssignment(node: TSESTree.Node) { - return ( - isExpressionStatement(node) && - isAssignmentExpression(node.expression) && - helpers.isRenderUtil(getPropertyIdentifierNode(node.expression.right)) + if ( + !isExpressionStatement(node) || + !isAssignmentExpression(node.expression) + ) { + return false; + } + + const expressionIdentifier = getPropertyIdentifierNode( + node.expression.right ); + + if (!expressionIdentifier) { + return false; + } + + return helpers.isRenderUtil(expressionIdentifier); } function isRenderInImplicitReturnAssignment(node: TSESTree.Node) { - return ( - isAssignmentExpression(node) && - helpers.isRenderUtil(getPropertyIdentifierNode(node.right)) - ); + if (!isAssignmentExpression(node)) { + return false; + } + + const expressionIdentifier = getPropertyIdentifierNode(node.right); + + if (!expressionIdentifier) { + return false; + } + + return helpers.isRenderUtil(expressionIdentifier); + } + + function isExpressionRenderUtil(expression: TSESTree.Expression) { + if (!isAssignmentExpression(expression)) { + return false; + } + + const expressionIdentifier = getPropertyIdentifierNode(expression.right); + + if (!expressionIdentifier) { + return false; + } + + return helpers.isRenderUtil(expressionIdentifier); } function isRenderInSequenceAssignment(node: TSESTree.Node) { - return ( - isSequenceExpression(node) && - node.expressions.some( - (expression) => - isAssignmentExpression(expression) && - helpers.isRenderUtil(getPropertyIdentifierNode(expression.right)) - ) - ); + if (!isSequenceExpression(node)) { + return false; + } + + return node.expressions.some(isExpressionRenderUtil); } function getSideEffectNodes( @@ -105,6 +134,10 @@ export default createTestingLibraryRule({ const expressionIdentifier = getPropertyIdentifierNode(node); + if (!expressionIdentifier) { + return false; + } + return ( helpers.isFireEventUtil(expressionIdentifier) || helpers.isUserEventUtil(expressionIdentifier) || @@ -146,15 +179,22 @@ export default createTestingLibraryRule({ : null; if ( - !helpers.isFireEventUtil(expressionIdentifier) && - !helpers.isUserEventUtil(expressionIdentifier) && - !helpers.isRenderUtil(expressionIdentifier) && + !expressionIdentifier && !isRenderInImplicitReturnAssignment(node) && !isRenderInSequenceAssignment(node) ) { return; } + if ( + expressionIdentifier && + !helpers.isFireEventUtil(expressionIdentifier) && + !helpers.isUserEventUtil(expressionIdentifier) && + !helpers.isRenderUtil(expressionIdentifier) + ) { + return; + } + context.report({ node, messageId: 'noSideEffectsWaitFor', From e149eedf87c7a3966b124a2d73f864647d81bc55 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Mon, 3 May 2021 13:09:58 +0300 Subject: [PATCH 10/12] refactor: merge two functions into one, add missing test case --- lib/rules/no-wait-for-side-effects.ts | 28 ++++++------------- .../rules/no-wait-for-side-effects.test.ts | 9 ++++++ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 205b4169..71b0c58b 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -65,7 +65,7 @@ export default createTestingLibraryRule({ ); } - function isRenderInAssignment(node: TSESTree.Node) { + function isRenderInExpressionStatement(node: TSESTree.Node) { if ( !isExpressionStatement(node) || !isAssignmentExpression(node.expression) @@ -84,27 +84,12 @@ export default createTestingLibraryRule({ return helpers.isRenderUtil(expressionIdentifier); } - function isRenderInImplicitReturnAssignment(node: TSESTree.Node) { + function isRenderInAssignmentExpression(node: TSESTree.Node) { if (!isAssignmentExpression(node)) { return false; } const expressionIdentifier = getPropertyIdentifierNode(node.right); - - if (!expressionIdentifier) { - return false; - } - - return helpers.isRenderUtil(expressionIdentifier); - } - - function isExpressionRenderUtil(expression: TSESTree.Expression) { - if (!isAssignmentExpression(expression)) { - return false; - } - - const expressionIdentifier = getPropertyIdentifierNode(expression.right); - if (!expressionIdentifier) { return false; } @@ -117,7 +102,7 @@ export default createTestingLibraryRule({ return false; } - return node.expressions.some(isExpressionRenderUtil); + return node.expressions.some(isRenderInAssignmentExpression); } function getSideEffectNodes( @@ -128,7 +113,10 @@ export default createTestingLibraryRule({ return false; } - if (isRenderInVariableDeclaration(node) || isRenderInAssignment(node)) { + if ( + isRenderInVariableDeclaration(node) || + isRenderInExpressionStatement(node) + ) { return true; } @@ -180,7 +168,7 @@ export default createTestingLibraryRule({ if ( !expressionIdentifier && - !isRenderInImplicitReturnAssignment(node) && + !isRenderInAssignmentExpression(node) && !isRenderInSequenceAssignment(node) ) { return; diff --git a/tests/lib/rules/no-wait-for-side-effects.test.ts b/tests/lib/rules/no-wait-for-side-effects.test.ts index 46d522a4..f802ffe5 100644 --- a/tests/lib/rules/no-wait-for-side-effects.test.ts +++ b/tests/lib/rules/no-wait-for-side-effects.test.ts @@ -459,6 +459,15 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], }, + { + code: ` + import { waitFor } from '@testing-library/react'; + await waitFor(() => { + result = render() + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + }, { code: ` import { waitFor } from '@testing-library/react'; From 8061a6ccfd1c74567277526579b99202511b6ff8 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Mon, 3 May 2021 13:41:38 +0300 Subject: [PATCH 11/12] feat: add docs entry --- docs/rules/no-wait-for-side-effects.md | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/rules/no-wait-for-side-effects.md b/docs/rules/no-wait-for-side-effects.md index 6f81179d..b545fae5 100644 --- a/docs/rules/no-wait-for-side-effects.md +++ b/docs/rules/no-wait-for-side-effects.md @@ -2,7 +2,7 @@ ## Rule Details -This rule aims to avoid the usage of side effects actions (`fireEvent` or `userEvent`) inside `waitFor`. +This rule aims to avoid the usage of side effects actions (`fireEvent`, `userEvent` or `render`) inside `waitFor`. Since `waitFor` is intended for things that have a non-deterministic amount of time between the action you performed and the assertion passing, the callback can be called (or checked for errors) a non-deterministic number of times and frequency. This will make your side-effect run multiple times. @@ -32,6 +32,18 @@ Example of **incorrect** code for this rule: userEvent.click(button); expect(b).toEqual('b'); }); + + // or + await waitFor(() => { + render() + expect(b).toEqual('b'); + }); + + // or + await waitFor(function() { + render() + expect(b).toEqual('b'); + }); }; ``` @@ -60,6 +72,18 @@ Examples of **correct** code for this rule: await waitFor(function() { expect(b).toEqual('b'); }); + + // or + render() + await waitFor(() => { + expect(b).toEqual('b'); + }); + + // or + render() + await waitFor(function() { + expect(b).toEqual('b'); + }); }; ``` From 8fb2a07fdacaa60e75d9f2190d4ed3ae28738d01 Mon Sep 17 00:00:00 2001 From: tozaicevas Date: Mon, 3 May 2021 14:45:25 +0300 Subject: [PATCH 12/12] refactor: pass function arg without wrapping it --- lib/rules/no-wait-for-side-effects.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 71b0c58b..b9a9cdd5 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -59,9 +59,7 @@ export default createTestingLibraryRule({ function isRenderInVariableDeclaration(node: TSESTree.Node) { return ( isVariableDeclaration(node) && - node.declarations.some((declaration) => - helpers.isRenderVariableDeclarator(declaration) - ) + node.declarations.some(helpers.isRenderVariableDeclarator) ); }