From 5ed8788a9f5bc9060eedd0c553eb82bbb9827e7e Mon Sep 17 00:00:00 2001 From: Gonzalo D'Elia Date: Fri, 11 Dec 2020 18:04:42 -0300 Subject: [PATCH 1/2] refactor: migrate prefer-find-by to v4 --- lib/rules/prefer-find-by.ts | 11 +- tests/lib/rules/prefer-find-by.test.ts | 137 ++++++++++++++++++------- 2 files changed, 104 insertions(+), 44 deletions(-) diff --git a/lib/rules/prefer-find-by.ts b/lib/rules/prefer-find-by.ts index 1150b848..c97a2bc8 100644 --- a/lib/rules/prefer-find-by.ts +++ b/lib/rules/prefer-find-by.ts @@ -1,8 +1,4 @@ -import { - ESLintUtils, - TSESTree, - ASTUtils, -} from '@typescript-eslint/experimental-utils'; +import { TSESTree, ASTUtils } from '@typescript-eslint/experimental-utils'; import { ReportFixFunction, RuleFix, @@ -15,7 +11,8 @@ import { isObjectPattern, isProperty, } from '../node-utils'; -import { getDocsUrl, SYNC_QUERIES_COMBINATIONS } from '../utils'; +import { createTestingLibraryRule } from '../create-testing-library-rule'; +import { SYNC_QUERIES_COMBINATIONS } from '../utils'; export const RULE_NAME = 'prefer-find-by'; export type MessageIds = 'preferFindBy'; @@ -51,7 +48,7 @@ function findRenderDefinitionDeclaration( return findRenderDefinitionDeclaration(scope.upper, query); } -export default ESLintUtils.RuleCreator(getDocsUrl)({ +export default createTestingLibraryRule({ name: RULE_NAME, meta: { type: 'suggestion', diff --git a/tests/lib/rules/prefer-find-by.test.ts b/tests/lib/rules/prefer-find-by.test.ts index da743103..a6a37d9a 100644 --- a/tests/lib/rules/prefer-find-by.test.ts +++ b/tests/lib/rules/prefer-find-by.test.ts @@ -38,60 +38,110 @@ ruleTester.run(RULE_NAME, rule, { valid: [ ...ASYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ code: ` - const { ${queryMethod} } = setup() - const submitButton = await ${queryMethod}('foo') + it('tests', async () => { + const { ${queryMethod} } = setup() + const submitButton = await ${queryMethod}('foo') + }) `, })), ...ASYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ - code: `const submitButton = await screen.${queryMethod}('foo')`, + code: ` + import {screen} from '@testing-library/foo'; + it('tests', async () => { + const submitButton = await screen.${queryMethod}('foo') + }) + `, })), ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ - code: `await waitForElementToBeRemoved(() => ${queryMethod}(baz))`, + code: ` + import {waitForElementToBeRemoved} from '@testing-library/foo'; + it('tests', async () => { + await waitForElementToBeRemoved(() => ${queryMethod}(baz)) + }) + `, })), ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ - code: `await waitFor(function() { - return ${queryMethod}('baz', { name: 'foo' }) - })`, + code: ` + import {waitFor} from '@testing-library/foo'; + + it('tests', async () => { + await waitFor(function() { + return ${queryMethod}('baz', { name: 'foo' }) + }) + }) + `, })), { - code: `await waitFor(() => myCustomFunction())`, + code: ` + import {waitFor} from '@testing-library/foo'; + + it('tests', async () => { + await waitFor(() => myCustomFunction()) + }) + `, }, { - code: `await waitFor(customFunctionReference)`, + code: ` + import {waitFor} from '@testing-library/foo'; + it('tests', async () => { + await waitFor(customFunctionReference) + }) + `, }, { - code: `await waitForElementToBeRemoved(document.querySelector('foo'))`, + code: ` + import {waitForElementToBeRemoved} from '@testing-library/foo'; + it('tests', async () => { + const { container } = render() + await waitForElementToBeRemoved(container.querySelector('foo')) + }) + `, }, ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ code: ` - await waitFor(() => { - foo() - return ${queryMethod}() + import {waitFor} from '@testing-library/foo'; + it('tests', async () => { + await waitFor(() => { + foo() + return ${queryMethod}() + }) }) `, })), ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ code: ` - await waitFor(() => expect(screen.${queryMethod}('baz')).toBeDisabled()); + import {screen, waitFor} from '@testing-library/foo'; + it('tests', async () => { + await waitFor(() => expect(screen.${queryMethod}('baz')).toBeDisabled()); + }) `, })), ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ code: ` - await waitFor(() => expect(${queryMethod}('baz')).toBeInTheDocument()); + import {waitFor} from '@testing-library/foo'; + it('tests', async () => { + await waitFor(() => expect(${queryMethod}('baz')).toBeInTheDocument()); + }) `, })), { code: ` - await waitFor(); - await wait(); + import {waitFor} from '@testing-library/foo'; + it('tests', async () => { + await waitFor(); + await wait(); + }) `, }, ], invalid: [ ...createScenario((waitMethod: string, queryMethod: string) => ({ code: ` - const { ${queryMethod} } = render() - const submitButton = await ${waitMethod}(() => ${queryMethod}('foo', { name: 'baz' })) + import {${waitMethod}} from '@testing-library/foo'; + it('tests', async () => { + const { ${queryMethod} } = render() + const submitButton = await ${waitMethod}(() => ${queryMethod}('foo', { name: 'baz' })) + }) `, errors: [ { @@ -104,14 +154,22 @@ ruleTester.run(RULE_NAME, rule, { }, ], output: ` - const { ${queryMethod}, ${buildFindByMethod(queryMethod)} } = render() - const submitButton = await ${buildFindByMethod( - queryMethod - )}('foo', { name: 'baz' }) + import {${waitMethod}} from '@testing-library/foo'; + it('tests', async () => { + const { ${queryMethod}, ${buildFindByMethod(queryMethod)} } = render() + const submitButton = await ${buildFindByMethod( + queryMethod + )}('foo', { name: 'baz' }) + }) `, })), ...createScenario((waitMethod: string, queryMethod: string) => ({ - code: `const submitButton = await ${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' }))`, + code: ` + import {${waitMethod}, screen} from '@testing-library/foo'; + it('tests', async () => { + const submitButton = await ${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' })) + }) + `, errors: [ { messageId: 'preferFindBy', @@ -122,15 +180,21 @@ ruleTester.run(RULE_NAME, rule, { }, }, ], - output: `const submitButton = await screen.${buildFindByMethod( - queryMethod - )}('foo', { name: 'baz' })`, + output: ` + import {${waitMethod}, screen} from '@testing-library/foo'; + it('tests', async () => { + const submitButton = await screen.${buildFindByMethod( + queryMethod + )}('foo', { name: 'baz' }) + }) + `, })), // // this scenario verifies it works when the render function is defined in another scope ...WAIT_METHODS.map((waitMethod: string) => ({ code: ` + import {${waitMethod}} from '@testing-library/foo'; const { getByText, queryByLabelText, findAllByRole } = customRender() - it('foo', async () => { + it('tests', async () => { const submitButton = await ${waitMethod}(() => getByText('baz', { name: 'button' })) }) `, @@ -145,8 +209,9 @@ ruleTester.run(RULE_NAME, rule, { }, ], output: ` + import {${waitMethod}} from '@testing-library/foo'; const { getByText, queryByLabelText, findAllByRole, findByText } = customRender() - it('foo', async () => { + it('tests', async () => { const submitButton = await findByText('baz', { name: 'button' }) }) `, @@ -154,11 +219,10 @@ ruleTester.run(RULE_NAME, rule, { // // this scenario verifies when findBy* were already defined (because it was used elsewhere) ...WAIT_METHODS.map((waitMethod: string) => ({ code: ` + import {${waitMethod}} from '@testing-library/foo'; const { getAllByRole, findAllByRole } = customRender() - describe('some scenario', () => { - it('foo', async () => { - const submitButton = await ${waitMethod}(() => getAllByRole('baz', { name: 'button' })) - }) + it('tests', async () => { + const submitButton = await ${waitMethod}(() => getAllByRole('baz', { name: 'button' })) }) `, errors: [ @@ -172,11 +236,10 @@ ruleTester.run(RULE_NAME, rule, { }, ], output: ` + import {${waitMethod}} from '@testing-library/foo'; const { getAllByRole, findAllByRole } = customRender() - describe('some scenario', () => { - it('foo', async () => { - const submitButton = await findAllByRole('baz', { name: 'button' }) - }) + it('tests', async () => { + const submitButton = await findAllByRole('baz', { name: 'button' }) }) `, })), From 28e08b5174853d7f622731ee3f34eea9534b9a53 Mon Sep 17 00:00:00 2001 From: Gonzalo D'Elia Date: Sat, 12 Dec 2020 16:04:29 -0300 Subject: [PATCH 2/2] refactor: applied pr suggestions --- lib/detect-testing-library-utils.ts | 16 ++- lib/rules/prefer-find-by.ts | 125 ++++++++++++---------- tests/create-testing-library-rule.test.ts | 18 ++-- tests/fake-rule.ts | 6 ++ tests/lib/rules/prefer-find-by.test.ts | 54 ++++++++++ 5 files changed, 153 insertions(+), 66 deletions(-) diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts index f3066304..ea4e4d41 100644 --- a/lib/detect-testing-library-utils.ts +++ b/lib/detect-testing-library-utils.ts @@ -14,7 +14,12 @@ import { isMemberExpression, isProperty, } from './node-utils'; -import { ABSENCE_MATCHERS, ASYNC_UTILS, PRESENCE_MATCHERS } from './utils'; +import { + ABSENCE_MATCHERS, + ASYNC_UTILS, + PRESENCE_MATCHERS, + ALL_QUERIES_COMBINATIONS, +} from './utils'; export type TestingLibrarySettings = { 'testing-library/module'?: string; @@ -52,6 +57,7 @@ export type DetectionHelpers = { isFindByQuery: (node: TSESTree.Identifier) => boolean; isSyncQuery: (node: TSESTree.Identifier) => boolean; isAsyncQuery: (node: TSESTree.Identifier) => boolean; + isCustomQuery: (node: TSESTree.Identifier) => boolean; isAsyncUtil: (node: TSESTree.Identifier) => boolean; isFireEventMethod: (node: TSESTree.Identifier) => boolean; isPresenceAssert: (node: TSESTree.MemberExpression) => boolean; @@ -180,6 +186,13 @@ export function detectTestingLibraryUtils< return isFindByQuery(node); }; + const isCustomQuery: DetectionHelpers['isCustomQuery'] = (node) => { + return ( + (isSyncQuery(node) || isAsyncQuery(node)) && + !ALL_QUERIES_COMBINATIONS.includes(node.name) + ); + }; + /** * Determines whether a given node is async util or not. */ @@ -356,6 +369,7 @@ export function detectTestingLibraryUtils< isFindByQuery, isSyncQuery, isAsyncQuery, + isCustomQuery, isAsyncUtil, isFireEventMethod, isPresenceAssert, diff --git a/lib/rules/prefer-find-by.ts b/lib/rules/prefer-find-by.ts index c97a2bc8..0786cb67 100644 --- a/lib/rules/prefer-find-by.ts +++ b/lib/rules/prefer-find-by.ts @@ -12,7 +12,6 @@ import { isProperty, } from '../node-utils'; import { createTestingLibraryRule } from '../create-testing-library-rule'; -import { SYNC_QUERIES_COMBINATIONS } from '../utils'; export const RULE_NAME = 'prefer-find-by'; export type MessageIds = 'preferFindBy'; @@ -67,7 +66,7 @@ export default createTestingLibraryRule({ }, defaultOptions: [], - create(context) { + create(context, _, helpers) { const sourceCode = context.getSourceCode(); /** @@ -123,7 +122,7 @@ export default createTestingLibraryRule({ isMemberExpression(argument.body.callee) && ASTUtils.isIdentifier(argument.body.callee.property) && ASTUtils.isIdentifier(argument.body.callee.object) && - SYNC_QUERIES_COMBINATIONS.includes(argument.body.callee.property.name) + helpers.isSyncQuery(argument.body.callee.property) ) { // shape of () => screen.getByText const fullQueryMethod = argument.body.callee.property.name; @@ -136,6 +135,11 @@ export default createTestingLibraryRule({ queryMethod, queryVariant, fix(fixer) { + const property = ((argument.body as TSESTree.CallExpression) + .callee as TSESTree.MemberExpression).property; + if (helpers.isCustomQuery(property as TSESTree.Identifier)) { + return; + } const newCode = `${caller}.${queryVariant}${queryMethod}(${callArguments .map((node) => sourceCode.getText(node)) .join(', ')})`; @@ -145,65 +149,74 @@ export default createTestingLibraryRule({ return; } if ( - ASTUtils.isIdentifier(argument.body.callee) && - SYNC_QUERIES_COMBINATIONS.includes(argument.body.callee.name) + !ASTUtils.isIdentifier(argument.body.callee) || + !helpers.isSyncQuery(argument.body.callee) ) { - // shape of () => getByText - const fullQueryMethod = argument.body.callee.name; - const queryMethod = fullQueryMethod.split('By')[1]; - const queryVariant = getFindByQueryVariant(fullQueryMethod); - const callArguments = argument.body.arguments; + return; + } + // shape of () => getByText + const fullQueryMethod = argument.body.callee.name; + const queryMethod = fullQueryMethod.split('By')[1]; + const queryVariant = getFindByQueryVariant(fullQueryMethod); + const callArguments = argument.body.arguments; - reportInvalidUsage(node, { - queryMethod, - queryVariant, - fix(fixer) { - const findByMethod = `${queryVariant}${queryMethod}`; - const allFixes: RuleFix[] = []; - // this updates waitFor with findBy* - const newCode = `${findByMethod}(${callArguments - .map((node) => sourceCode.getText(node)) - .join(', ')})`; - allFixes.push(fixer.replaceText(node, newCode)); + reportInvalidUsage(node, { + queryMethod, + queryVariant, + fix(fixer) { + // we know from above callee is an Identifier + if ( + helpers.isCustomQuery( + (argument.body as TSESTree.CallExpression) + .callee as TSESTree.Identifier + ) + ) { + return; + } + const findByMethod = `${queryVariant}${queryMethod}`; + const allFixes: RuleFix[] = []; + // this updates waitFor with findBy* + const newCode = `${findByMethod}(${callArguments + .map((node) => sourceCode.getText(node)) + .join(', ')})`; + allFixes.push(fixer.replaceText(node, newCode)); - // this adds the findBy* declaration - adding it to the list of destructured variables { findBy* } = render() - const definition = findRenderDefinitionDeclaration( - context.getScope(), - fullQueryMethod - ); - // I think it should always find it, otherwise code should not be valid (it'd be using undeclared variables) - if (!definition) { + // this adds the findBy* declaration - adding it to the list of destructured variables { findBy* } = render() + const definition = findRenderDefinitionDeclaration( + context.getScope(), + fullQueryMethod + ); + // I think it should always find it, otherwise code should not be valid (it'd be using undeclared variables) + if (!definition) { + return allFixes; + } + // check the declaration is part of a destructuring + if (isObjectPattern(definition.parent.parent)) { + const allVariableDeclarations = definition.parent.parent; + // verify if the findBy* method was already declared + if ( + allVariableDeclarations.properties.some( + (p) => + isProperty(p) && + ASTUtils.isIdentifier(p.key) && + p.key.name === findByMethod + ) + ) { return allFixes; } - // check the declaration is part of a destructuring - if (isObjectPattern(definition.parent.parent)) { - const allVariableDeclarations = definition.parent.parent; - // verify if the findBy* method was already declared - if ( - allVariableDeclarations.properties.some( - (p) => - isProperty(p) && - ASTUtils.isIdentifier(p.key) && - p.key.name === findByMethod - ) - ) { - return allFixes; - } - // the last character of a destructuring is always a "}", so we should replace it with the findBy* declaration - const textDestructuring = sourceCode.getText( - allVariableDeclarations - ); - const text = - textDestructuring.substring(0, textDestructuring.length - 2) + - `, ${findByMethod} }`; - allFixes.push(fixer.replaceText(allVariableDeclarations, text)); - } + // the last character of a destructuring is always a "}", so we should replace it with the findBy* declaration + const textDestructuring = sourceCode.getText( + allVariableDeclarations + ); + const text = + textDestructuring.substring(0, textDestructuring.length - 2) + + `, ${findByMethod} }`; + allFixes.push(fixer.replaceText(allVariableDeclarations, text)); + } - return allFixes; - }, - }); - return; - } + return allFixes; + }, + }); }, }; }, diff --git a/tests/create-testing-library-rule.test.ts b/tests/create-testing-library-rule.test.ts index aa047282..b57fbede 100644 --- a/tests/create-testing-library-rule.test.ts +++ b/tests/create-testing-library-rule.test.ts @@ -468,21 +468,21 @@ ruleTester.run(RULE_NAME, rule, { // case: custom "getBy*" query reported without import (aggressive reporting) getByIcon('search') `, - errors: [{ line: 3, column: 7, messageId: 'getByError' }], + errors: [{ line: 3, column: 7, messageId: 'customQueryError' }], }, { code: ` // case: custom "queryBy*" query reported without import (aggressive reporting) queryByIcon('search') `, - errors: [{ line: 3, column: 7, messageId: 'queryByError' }], + errors: [{ line: 3, column: 7, messageId: 'customQueryError' }], }, { code: ` // case: custom "findBy*" query reported without import (aggressive reporting) findByIcon('search') `, - errors: [{ line: 3, column: 7, messageId: 'findByError' }], + errors: [{ line: 3, column: 7, messageId: 'customQueryError' }], }, { settings: { @@ -564,7 +564,7 @@ ruleTester.run(RULE_NAME, rule, { import { render } from '@testing-library/react' getByIcon('search') `, - errors: [{ line: 4, column: 7, messageId: 'getByError' }], + errors: [{ line: 4, column: 7, messageId: 'customQueryError' }], }, { filename: 'MyComponent.spec.js', @@ -576,7 +576,7 @@ ruleTester.run(RULE_NAME, rule, { import { render } from '@testing-library/framework' queryByIcon('search') `, - errors: [{ line: 4, column: 7, messageId: 'queryByError' }], + errors: [{ line: 4, column: 7, messageId: 'customQueryError' }], }, { filename: 'MyComponent.spec.js', @@ -588,7 +588,7 @@ ruleTester.run(RULE_NAME, rule, { import { render } from '@testing-library/framework' findByIcon('search') `, - errors: [{ line: 4, column: 7, messageId: 'findByError' }], + errors: [{ line: 4, column: 7, messageId: 'customQueryError' }], }, { settings: { @@ -599,7 +599,7 @@ ruleTester.run(RULE_NAME, rule, { import { render } from 'test-utils' getByIcon('search') `, - errors: [{ line: 4, column: 7, messageId: 'getByError' }], + errors: [{ line: 4, column: 7, messageId: 'customQueryError' }], }, { filename: 'MyComponent.spec.js', @@ -611,7 +611,7 @@ ruleTester.run(RULE_NAME, rule, { import { render } from 'test-utils' queryByIcon('search') `, - errors: [{ line: 4, column: 7, messageId: 'queryByError' }], + errors: [{ line: 4, column: 7, messageId: 'customQueryError' }], }, { filename: 'MyComponent.spec.js', @@ -623,7 +623,7 @@ ruleTester.run(RULE_NAME, rule, { import { render } from 'test-utils' findByIcon('search') `, - errors: [{ line: 4, column: 7, messageId: 'findByError' }], + errors: [{ line: 4, column: 7, messageId: 'customQueryError' }], }, { settings: { diff --git a/tests/fake-rule.ts b/tests/fake-rule.ts index 25f2237e..8a4808da 100644 --- a/tests/fake-rule.ts +++ b/tests/fake-rule.ts @@ -12,6 +12,7 @@ type MessageIds = | 'getByError' | 'queryByError' | 'findByError' + | 'customQueryError' | 'presenceAssertError' | 'absenceAssertError'; @@ -29,6 +30,7 @@ export default createTestingLibraryRule({ getByError: 'some error related to getBy reported', queryByError: 'some error related to queryBy reported', findByError: 'some error related to findBy reported', + customQueryError: 'some error related to a customQuery reported', presenceAssertError: 'some error related to presence assert reported', absenceAssertError: 'some error related to absence assert reported', }, @@ -43,6 +45,10 @@ export default createTestingLibraryRule({ return context.report({ node, messageId: 'fakeError' }); } + if (helpers.isCustomQuery(node)) { + return context.report({ node, messageId: 'customQueryError' }); + } + // force queries to be reported if (helpers.isGetByQuery(node)) { return context.report({ node, messageId: 'getByError' }); diff --git a/tests/lib/rules/prefer-find-by.test.ts b/tests/lib/rules/prefer-find-by.test.ts index a6a37d9a..0aec799a 100644 --- a/tests/lib/rules/prefer-find-by.test.ts +++ b/tests/lib/rules/prefer-find-by.test.ts @@ -279,5 +279,59 @@ ruleTester.run(RULE_NAME, rule, { const submitButton = await findByRole('baz', { name: 'button' }) `, }, + // custom query triggers the error but there is no fix - so output is the same + ...WAIT_METHODS.map((waitMethod: string) => ({ + code: ` + import {${waitMethod},render} from '@testing-library/foo'; + it('tests', async () => { + const { getByCustomQuery } = render() + const submitButton = await ${waitMethod}(() => getByCustomQuery('baz')) + }) + `, + errors: [ + { + messageId: 'preferFindBy', + data: { + queryVariant: 'findBy', + queryMethod: 'CustomQuery', + fullQuery: `${waitMethod}(() => getByCustomQuery('baz'))`, + }, + }, + ], + output: ` + import {${waitMethod},render} from '@testing-library/foo'; + it('tests', async () => { + const { getByCustomQuery } = render() + const submitButton = await ${waitMethod}(() => getByCustomQuery('baz')) + }) + `, + })), + // custom query triggers the error but there is no fix - so output is the same + ...WAIT_METHODS.map((waitMethod: string) => ({ + code: ` + import {${waitMethod},render,screen} from '@testing-library/foo'; + it('tests', async () => { + const { getByCustomQuery } = render() + const submitButton = await ${waitMethod}(() => screen.getByCustomQuery('baz')) + }) + `, + errors: [ + { + messageId: 'preferFindBy', + data: { + queryVariant: 'findBy', + queryMethod: 'CustomQuery', + fullQuery: `${waitMethod}(() => screen.getByCustomQuery('baz'))`, + }, + }, + ], + output: ` + import {${waitMethod},render,screen} from '@testing-library/foo'; + it('tests', async () => { + const { getByCustomQuery } = render() + const submitButton = await ${waitMethod}(() => screen.getByCustomQuery('baz')) + }) + `, + })), ], });