From b93dffe47901794501e94c16fbb986b9e2cd3ec5 Mon Sep 17 00:00:00 2001 From: Senja Jarva Date: Mon, 12 Sep 2022 12:06:17 +0300 Subject: [PATCH 1/5] fix: no-wait-for-side-effects false positive inside .then() Added tests and a mention of edge case to rule documentation Refs: #500 --- docs/rules/no-wait-for-side-effects.md | 9 ++++ lib/rules/no-wait-for-side-effects.ts | 28 ++++++++++- .../rules/no-wait-for-side-effects.test.ts | 48 +++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/docs/rules/no-wait-for-side-effects.md b/docs/rules/no-wait-for-side-effects.md index b545fae5..9e477c79 100644 --- a/docs/rules/no-wait-for-side-effects.md +++ b/docs/rules/no-wait-for-side-effects.md @@ -73,6 +73,15 @@ Examples of **correct** code for this rule: expect(b).toEqual('b'); }); + // or + userEvent.click(button); + await waitFor(function() { + expect(b).toEqual('b'); + }).then(() => { + // Outside of waitFor, e.g. inside a .then() side effects are allowed + fireEvent.click(button); + }); + // or render() await waitFor(() => { diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index a0fd85fe..6033c722 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -1,4 +1,4 @@ -import { TSESTree } from '@typescript-eslint/utils'; +import { ASTUtils, TSESTree } from '@typescript-eslint/utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; import { @@ -8,6 +8,7 @@ import { isAssignmentExpression, isCallExpression, isSequenceExpression, + isMemberExpression, } from '../node-utils'; export const RULE_NAME = 'no-wait-for-side-effects'; @@ -56,6 +57,27 @@ export default createTestingLibraryRule({ ); } + function isCallerThen( + node: + | TSESTree.AssignmentExpression + | TSESTree.BlockStatement + | TSESTree.CallExpression + | TSESTree.SequenceExpression + ): boolean { + if (!node.parent) { + return false; + } + + const callExpressionNode = node.parent.parent as TSESTree.CallExpression; + + const test = + isMemberExpression(callExpressionNode.callee) && + ASTUtils.isIdentifier(callExpressionNode.callee.property) && + callExpressionNode.callee.property.name === 'then'; + + return test; + } + function isRenderInVariableDeclaration(node: TSESTree.Node) { return ( isVariableDeclaration(node) && @@ -137,6 +159,10 @@ export default createTestingLibraryRule({ return; } + if (isCallerThen(node)) { + return; + } + getSideEffectNodes(node.body).forEach((sideEffectNode) => context.report({ node: sideEffectNode, 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 2d536d51..bbf9e5a5 100644 --- a/tests/lib/rules/no-wait-for-side-effects.test.ts +++ b/tests/lib/rules/no-wait-for-side-effects.test.ts @@ -99,6 +99,19 @@ ruleTester.run(RULE_NAME, rule, { await waitFor(function() { expect(b).toEqual('b') }) + `, + }, + { + // Issue #500, https://github.com/testing-library/eslint-plugin-testing-library/issues/500 + code: ` + import { waitFor } from '${testingFramework}'; + userEvent.click(button) + await waitFor(function() { + expect(b).toEqual('b') + }).then(() => { + // Side effects are allowed inside .then() + userEvent.click(button); + }) `, }, ]), @@ -722,6 +735,41 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], } as const, + { + // Issue #500, https://github.com/testing-library/eslint-plugin-testing-library/issues/500 + code: ` + import { waitFor } from '${testingFramework}'; + await waitFor(function() { + userEvent.click(button) + expect(b).toEqual('b') + }).then(() => { + userEvent.click(button) // Side effects are allowed inside .then() + expect(b).toEqual('b') + }) + `, + errors: [{ line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }], + } as const, + { + // Issue #500, https://github.com/testing-library/eslint-plugin-testing-library/issues/500 + code: ` + import { waitFor } from '${testingFramework}'; + await waitFor(function() { + userEvent.click(button) + expect(b).toEqual('b') + }).then(() => { + userEvent.click(button) // Side effects are allowed inside .then() + expect(b).toEqual('b') + await waitFor(() => { + fireEvent.keyDown(input, {key: 'ArrowDown'}) // But not if there is a another waitFor with side effects inside the .then() + expect(b).toEqual('b') + }) + }) + `, + errors: [ + { line: 4, column: 11, messageId: 'noSideEffectsWaitFor' }, + { line: 10, column: 13, messageId: 'noSideEffectsWaitFor' }, + ], + } as const, ]), { From 88676ac85e926218faec7d90d2b2e3dad5758784 Mon Sep 17 00:00:00 2001 From: Senja Jarva Date: Tue, 13 Sep 2022 11:06:38 +0300 Subject: [PATCH 2/5] fix: review feedback Remove awaits as the promise is waited by .then() --- tests/lib/rules/no-wait-for-side-effects.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 bbf9e5a5..08dc384d 100644 --- a/tests/lib/rules/no-wait-for-side-effects.test.ts +++ b/tests/lib/rules/no-wait-for-side-effects.test.ts @@ -106,7 +106,7 @@ ruleTester.run(RULE_NAME, rule, { code: ` import { waitFor } from '${testingFramework}'; userEvent.click(button) - await waitFor(function() { + waitFor(function() { expect(b).toEqual('b') }).then(() => { // Side effects are allowed inside .then() @@ -753,7 +753,7 @@ ruleTester.run(RULE_NAME, rule, { // Issue #500, https://github.com/testing-library/eslint-plugin-testing-library/issues/500 code: ` import { waitFor } from '${testingFramework}'; - await waitFor(function() { + waitFor(function() { userEvent.click(button) expect(b).toEqual('b') }).then(() => { From 41cab9eff9c22343610a759f11b58d0f07b0205f Mon Sep 17 00:00:00 2001 From: Senja Jarva Date: Tue, 13 Sep 2022 11:07:56 +0300 Subject: [PATCH 3/5] fix: review feedback Reuse existing helper function to make solution simpler --- lib/rules/no-wait-for-side-effects.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/rules/no-wait-for-side-effects.ts b/lib/rules/no-wait-for-side-effects.ts index 6033c722..24ace87f 100644 --- a/lib/rules/no-wait-for-side-effects.ts +++ b/lib/rules/no-wait-for-side-effects.ts @@ -1,4 +1,4 @@ -import { ASTUtils, TSESTree } from '@typescript-eslint/utils'; +import { TSESTree } from '@typescript-eslint/utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; import { @@ -8,7 +8,7 @@ import { isAssignmentExpression, isCallExpression, isSequenceExpression, - isMemberExpression, + hasThenProperty, } from '../node-utils'; export const RULE_NAME = 'no-wait-for-side-effects'; @@ -70,12 +70,7 @@ export default createTestingLibraryRule({ const callExpressionNode = node.parent.parent as TSESTree.CallExpression; - const test = - isMemberExpression(callExpressionNode.callee) && - ASTUtils.isIdentifier(callExpressionNode.callee.property) && - callExpressionNode.callee.property.name === 'then'; - - return test; + return hasThenProperty(callExpressionNode.callee); } function isRenderInVariableDeclaration(node: TSESTree.Node) { From 60578ffc508e9840f5c9afa69000a86614bdfb61 Mon Sep 17 00:00:00 2001 From: Senja Jarva Date: Tue, 13 Sep 2022 11:10:44 +0300 Subject: [PATCH 4/5] fix: review feedback Remove await as the promise is waited by .then() --- docs/rules/no-wait-for-side-effects.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-wait-for-side-effects.md b/docs/rules/no-wait-for-side-effects.md index 9e477c79..741a91a3 100644 --- a/docs/rules/no-wait-for-side-effects.md +++ b/docs/rules/no-wait-for-side-effects.md @@ -75,7 +75,7 @@ Examples of **correct** code for this rule: // or userEvent.click(button); - await waitFor(function() { + waitFor(function() { expect(b).toEqual('b'); }).then(() => { // Outside of waitFor, e.g. inside a .then() side effects are allowed From 56ff8378fee6b358518094afeef473f601e2e3a0 Mon Sep 17 00:00:00 2001 From: Senja Jarva Date: Tue, 13 Sep 2022 11:12:46 +0300 Subject: [PATCH 5/5] fix: review feedback Remove await as the promise is waited by .then() --- tests/lib/rules/no-wait-for-side-effects.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 08dc384d..38d05047 100644 --- a/tests/lib/rules/no-wait-for-side-effects.test.ts +++ b/tests/lib/rules/no-wait-for-side-effects.test.ts @@ -739,7 +739,7 @@ ruleTester.run(RULE_NAME, rule, { // Issue #500, https://github.com/testing-library/eslint-plugin-testing-library/issues/500 code: ` import { waitFor } from '${testingFramework}'; - await waitFor(function() { + waitFor(function() { userEvent.click(button) expect(b).toEqual('b') }).then(() => {