From 2acf2267093757d09bb25454f572278be281777e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Mon, 17 May 2021 00:58:18 +0200 Subject: [PATCH 1/6] refactor: simplify rule implementation --- lib/rules/prefer-screen-queries.ts | 70 +++++++++---------- tests/lib/rules/prefer-screen-queries.test.ts | 38 ++++++++++ 2 files changed, 72 insertions(+), 36 deletions(-) diff --git a/lib/rules/prefer-screen-queries.ts b/lib/rules/prefer-screen-queries.ts index eb649f72..6c60cccc 100644 --- a/lib/rules/prefer-screen-queries.ts +++ b/lib/rules/prefer-screen-queries.ts @@ -2,6 +2,7 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; import { + getDeepestIdentifierNode, isCallExpression, isMemberExpression, isObjectExpression, @@ -78,6 +79,10 @@ export default createTestingLibraryRule({ } } + function isIdentifierAllowed(name: string) { + return ['screen', ...withinDeclaredVariables].includes(name); + } + // keep here those queries which are safe and shouldn't be reported // (from within, from render + container/base element, not related to TL, etc) const safeDestructuredQueries: string[] = []; @@ -113,52 +118,45 @@ export default createTestingLibraryRule({ // save the destructured query methods as safe since they are coming // from within or render + base/container options saveSafeDestructuredQueries(node); - return; - } - - if (ASTUtils.isIdentifier(node.id)) { + } else if (ASTUtils.isIdentifier(node.id)) { withinDeclaredVariables.push(node.id.name); } }, - 'CallExpression > Identifier'(node: TSESTree.Identifier) { - if (!helpers.isBuiltInQuery(node)) { - return; - } + CallExpression(node) { + const identifierNode = getDeepestIdentifierNode(node); - if ( - !safeDestructuredQueries.some((queryName) => queryName === node.name) - ) { - reportInvalidUsage(node); - } - }, - 'MemberExpression > Identifier'(node: TSESTree.Identifier) { - function isIdentifierAllowed(name: string) { - return ['screen', ...withinDeclaredVariables].includes(name); - } - - if (!helpers.isBuiltInQuery(node)) { + if (!identifierNode) { return; } - if ( - ASTUtils.isIdentifier(node) && - isMemberExpression(node.parent) && - isCallExpression(node.parent.object) && - ASTUtils.isIdentifier(node.parent.object.callee) && - node.parent.object.callee.name !== 'within' && - helpers.isRenderUtil(node.parent.object.callee) && - !usesContainerOrBaseElement(node.parent.object) - ) { - reportInvalidUsage(node); + if (!helpers.isBuiltInQuery(identifierNode)) { return; } - if ( - isMemberExpression(node.parent) && - ASTUtils.isIdentifier(node.parent.object) && - !isIdentifierAllowed(node.parent.object.name) - ) { - reportInvalidUsage(node); + if (isMemberExpression(identifierNode.parent)) { + const memberExpressionNode = identifierNode.parent; + if ( + isCallExpression(memberExpressionNode.object) && + ASTUtils.isIdentifier(memberExpressionNode.object.callee) && + memberExpressionNode.object.callee.name !== 'within' && + helpers.isRenderUtil(memberExpressionNode.object.callee) && + !usesContainerOrBaseElement(memberExpressionNode.object) + ) { + reportInvalidUsage(identifierNode); + } else if ( + ASTUtils.isIdentifier(memberExpressionNode.object) && + !isIdentifierAllowed(memberExpressionNode.object.name) + ) { + reportInvalidUsage(identifierNode); + } + } else { + const isSafeDestructuredQuery = safeDestructuredQueries.some( + (queryName) => queryName === identifierNode.name + ); + + if (!isSafeDestructuredQuery) { + reportInvalidUsage(identifierNode); + } } }, }; diff --git a/tests/lib/rules/prefer-screen-queries.test.ts b/tests/lib/rules/prefer-screen-queries.test.ts index 052433b9..364e2615 100644 --- a/tests/lib/rules/prefer-screen-queries.test.ts +++ b/tests/lib/rules/prefer-screen-queries.test.ts @@ -413,5 +413,43 @@ ruleTester.run(RULE_NAME, rule, { ], } as const) ), + { + code: ` // issue #367 + import { render } from '@testing-library/react'; + + function setup() { + return render(
); + } + + it('undetected 1', () => { + const { getByText } = setup(); + getByText('foo')).toBeInTheDocument(); + }); + + it('undetected 2', () => { + const results = setup(); + const { getByText } = results; + expect(getByText('foo')).toBe('foo'); + }); + `, + errors: [ + { + messageId: 'preferScreenQueries', + line: 10, + column: 16, + data: { + name: 'getByText', + }, + }, + { + messageId: 'preferScreenQueries', + line: 16, + column: 16, + data: { + name: 'getByText', + }, + }, + ], + }, ], }); From 0be38be9af80ffc4b79ed244aaa1eeab72dba001 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Wed, 19 May 2021 00:19:22 +0200 Subject: [PATCH 2/6] fix(prefer-screen-queries): detect render wrappers properly --- lib/rules/prefer-screen-queries.ts | 27 +++++++++++++++++-- tests/lib/rules/prefer-screen-queries.test.ts | 8 +++--- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/lib/rules/prefer-screen-queries.ts b/lib/rules/prefer-screen-queries.ts index 6c60cccc..791c70e6 100644 --- a/lib/rules/prefer-screen-queries.ts +++ b/lib/rules/prefer-screen-queries.ts @@ -3,6 +3,8 @@ import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; import { getDeepestIdentifierNode, + getFunctionName, + getInnermostReturningFunction, isCallExpression, isMemberExpression, isObjectExpression, @@ -55,6 +57,22 @@ export default createTestingLibraryRule({ defaultOptions: [], create(context, _, helpers) { + const renderWrapperNames: string[] = []; + + function detectRenderWrapper(node: TSESTree.Identifier): void { + const innerFunction = getInnermostReturningFunction(context, node); + + if (innerFunction) { + renderWrapperNames.push(getFunctionName(innerFunction)); + } + } + + function isReportableRender(node: TSESTree.Identifier): boolean { + return ( + helpers.isRenderUtil(node) || renderWrapperNames.includes(node.name) + ); + } + function reportInvalidUsage(node: TSESTree.Identifier) { context.report({ node, @@ -98,8 +116,9 @@ export default createTestingLibraryRule({ return; } - const isComingFromValidRender = helpers.isRenderUtil(node.init.callee); + const isComingFromValidRender = isReportableRender(node.init.callee); + // TODO: why is saving `setup` here? that's the problem if (!isComingFromValidRender) { // save the destructured query methods as safe since they are coming // from render not related to TL @@ -129,6 +148,10 @@ export default createTestingLibraryRule({ return; } + if (helpers.isRenderUtil(identifierNode)) { + detectRenderWrapper(identifierNode); + } + if (!helpers.isBuiltInQuery(identifierNode)) { return; } @@ -139,7 +162,7 @@ export default createTestingLibraryRule({ isCallExpression(memberExpressionNode.object) && ASTUtils.isIdentifier(memberExpressionNode.object.callee) && memberExpressionNode.object.callee.name !== 'within' && - helpers.isRenderUtil(memberExpressionNode.object.callee) && + isReportableRender(memberExpressionNode.object.callee) && !usesContainerOrBaseElement(memberExpressionNode.object) ) { reportInvalidUsage(identifierNode); diff --git a/tests/lib/rules/prefer-screen-queries.test.ts b/tests/lib/rules/prefer-screen-queries.test.ts index 364e2615..147b16a3 100644 --- a/tests/lib/rules/prefer-screen-queries.test.ts +++ b/tests/lib/rules/prefer-screen-queries.test.ts @@ -48,7 +48,7 @@ ruleTester.run(RULE_NAME, rule, { (query) => ` import { render } from '@testing-library/react' import { ${query} } from 'custom-queries' - + test("imported custom queries, since they can't be used through screen", () => { render(foo) ${query}('bar') @@ -58,7 +58,7 @@ ruleTester.run(RULE_NAME, rule, { ...CUSTOM_QUERY_COMBINATIONS.map( (query) => ` import { render } from '@testing-library/react' - + test("render-returned custom queries, since they can't be used through screen", () => { const { ${query} } = render(foo) ${query}('bar') @@ -71,7 +71,7 @@ ruleTester.run(RULE_NAME, rule, { }, code: ` import { render } from '@testing-library/react' - + test("custom queries + custom-queries setting, since they can't be used through screen", () => { const { ${query} } = render(foo) ${query}('bar') @@ -423,7 +423,7 @@ ruleTester.run(RULE_NAME, rule, { it('undetected 1', () => { const { getByText } = setup(); - getByText('foo')).toBeInTheDocument(); + expect(getByText('foo')).toBeInTheDocument(); }); it('undetected 2', () => { From 59dfaf15fe2998461f8aa6b6137ef3c89bcbb946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Wed, 19 May 2021 00:33:34 +0200 Subject: [PATCH 3/6] test(prefer-screen-queries): add another test case --- tests/lib/rules/prefer-screen-queries.test.ts | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/tests/lib/rules/prefer-screen-queries.test.ts b/tests/lib/rules/prefer-screen-queries.test.ts index 147b16a3..c94a1474 100644 --- a/tests/lib/rules/prefer-screen-queries.test.ts +++ b/tests/lib/rules/prefer-screen-queries.test.ts @@ -414,19 +414,56 @@ ruleTester.run(RULE_NAME, rule, { } as const) ), { - code: ` // issue #367 + code: ` // issue #367 - example A import { render } from '@testing-library/react'; function setup() { return render(
); } - it('undetected 1', () => { + it('foo', async () => { + const { getByText } = await setup(); + expect(getByText('foo')).toBeInTheDocument(); + }); + + it('bar', () => { + const { getByText } = setup(); + expect(getByText('foo')).toBeInTheDocument(); + }); + `, + errors: [ + { + messageId: 'preferScreenQueries', + line: 10, + column: 16, + data: { + name: 'getByText', + }, + }, + { + messageId: 'preferScreenQueries', + line: 15, + column: 16, + data: { + name: 'getByText', + }, + }, + ], + }, + { + code: ` // issue #367 - example B + import { render } from '@testing-library/react'; + + function setup() { + return render(
); + } + + it('foo', () => { const { getByText } = setup(); expect(getByText('foo')).toBeInTheDocument(); }); - it('undetected 2', () => { + it('bar', () => { const results = setup(); const { getByText } = results; expect(getByText('foo')).toBe('foo'); From c0af9189977b3ad0ecfc7a86bb4a5e5146600c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Fri, 21 May 2021 11:11:41 +0200 Subject: [PATCH 4/6] refactor: remove todo --- lib/rules/prefer-screen-queries.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/rules/prefer-screen-queries.ts b/lib/rules/prefer-screen-queries.ts index 791c70e6..7ecb6afa 100644 --- a/lib/rules/prefer-screen-queries.ts +++ b/lib/rules/prefer-screen-queries.ts @@ -118,7 +118,6 @@ export default createTestingLibraryRule({ const isComingFromValidRender = isReportableRender(node.init.callee); - // TODO: why is saving `setup` here? that's the problem if (!isComingFromValidRender) { // save the destructured query methods as safe since they are coming // from render not related to TL From 904a3450df48abbd96f2b57eca04d60ff5ee6f11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 22 May 2021 19:33:07 +0200 Subject: [PATCH 5/6] refactor: simplify nested if statements --- lib/rules/prefer-screen-queries.ts | 71 +++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 20 deletions(-) diff --git a/lib/rules/prefer-screen-queries.ts b/lib/rules/prefer-screen-queries.ts index 7ecb6afa..9963e228 100644 --- a/lib/rules/prefer-screen-queries.ts +++ b/lib/rules/prefer-screen-queries.ts @@ -155,31 +155,62 @@ export default createTestingLibraryRule({ return; } - if (isMemberExpression(identifierNode.parent)) { - const memberExpressionNode = identifierNode.parent; - if ( - isCallExpression(memberExpressionNode.object) && - ASTUtils.isIdentifier(memberExpressionNode.object.callee) && - memberExpressionNode.object.callee.name !== 'within' && - isReportableRender(memberExpressionNode.object.callee) && - !usesContainerOrBaseElement(memberExpressionNode.object) - ) { - reportInvalidUsage(identifierNode); - } else if ( - ASTUtils.isIdentifier(memberExpressionNode.object) && - !isIdentifierAllowed(memberExpressionNode.object.name) - ) { - reportInvalidUsage(identifierNode); - } - } else { + if (!isMemberExpression(identifierNode.parent)) { const isSafeDestructuredQuery = safeDestructuredQueries.some( (queryName) => queryName === identifierNode.name ); - - if (!isSafeDestructuredQuery) { - reportInvalidUsage(identifierNode); + if (isSafeDestructuredQuery) { + return; } + + reportInvalidUsage(identifierNode); + return; } + + const memberExpressionNode = identifierNode.parent; + if ( + isCallExpression(memberExpressionNode.object) && + ASTUtils.isIdentifier(memberExpressionNode.object.callee) && + memberExpressionNode.object.callee.name !== 'within' && + isReportableRender(memberExpressionNode.object.callee) && + !usesContainerOrBaseElement(memberExpressionNode.object) + ) { + reportInvalidUsage(identifierNode); + return; + } + + if ( + ASTUtils.isIdentifier(memberExpressionNode.object) && + !isIdentifierAllowed(memberExpressionNode.object.name) + ) { + reportInvalidUsage(identifierNode); + } + + // if (isMemberExpression(identifierNode.parent)) { + // const memberExpressionNode = identifierNode.parent; + // if ( + // isCallExpression(memberExpressionNode.object) && + // ASTUtils.isIdentifier(memberExpressionNode.object.callee) && + // memberExpressionNode.object.callee.name !== 'within' && + // isReportableRender(memberExpressionNode.object.callee) && + // !usesContainerOrBaseElement(memberExpressionNode.object) + // ) { + // reportInvalidUsage(identifierNode); + // } else if ( + // ASTUtils.isIdentifier(memberExpressionNode.object) && + // !isIdentifierAllowed(memberExpressionNode.object.name) + // ) { + // reportInvalidUsage(identifierNode); + // } + // } else { + // const isSafeDestructuredQuery = safeDestructuredQueries.some( + // (queryName) => queryName === identifierNode.name + // ); + // + // if (!isSafeDestructuredQuery) { + // reportInvalidUsage(identifierNode); + // } + // } }, }; }, From 24788392ceb3498d91b583f523a21779ed3b85c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Beltr=C3=A1n=20Alarc=C3=B3n?= Date: Sat, 22 May 2021 19:38:24 +0200 Subject: [PATCH 6/6] refactor: remove commented out code --- lib/rules/prefer-screen-queries.ts | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/lib/rules/prefer-screen-queries.ts b/lib/rules/prefer-screen-queries.ts index 9963e228..bfdd2773 100644 --- a/lib/rules/prefer-screen-queries.ts +++ b/lib/rules/prefer-screen-queries.ts @@ -185,32 +185,6 @@ export default createTestingLibraryRule({ ) { reportInvalidUsage(identifierNode); } - - // if (isMemberExpression(identifierNode.parent)) { - // const memberExpressionNode = identifierNode.parent; - // if ( - // isCallExpression(memberExpressionNode.object) && - // ASTUtils.isIdentifier(memberExpressionNode.object.callee) && - // memberExpressionNode.object.callee.name !== 'within' && - // isReportableRender(memberExpressionNode.object.callee) && - // !usesContainerOrBaseElement(memberExpressionNode.object) - // ) { - // reportInvalidUsage(identifierNode); - // } else if ( - // ASTUtils.isIdentifier(memberExpressionNode.object) && - // !isIdentifierAllowed(memberExpressionNode.object.name) - // ) { - // reportInvalidUsage(identifierNode); - // } - // } else { - // const isSafeDestructuredQuery = safeDestructuredQueries.some( - // (queryName) => queryName === identifierNode.name - // ); - // - // if (!isSafeDestructuredQuery) { - // reportInvalidUsage(identifierNode); - // } - // } }, }; },