From 993fd8cdaeb9d1cccc6341aa54415944c2795088 Mon Sep 17 00:00:00 2001 From: Gonzalo D'Elia Date: Mon, 20 Jul 2020 08:05:24 -0300 Subject: [PATCH] fix: include findBy variable declaration to prefer-find-by when auto fixing (#197) --- lib/rules/prefer-find-by.ts | 85 ++++++++++--- tests/lib/rules/prefer-find-by.test.ts | 164 ++++++++++++++++++++----- 2 files changed, 198 insertions(+), 51 deletions(-) diff --git a/lib/rules/prefer-find-by.ts b/lib/rules/prefer-find-by.ts index a606c164..fed0eab2 100644 --- a/lib/rules/prefer-find-by.ts +++ b/lib/rules/prefer-find-by.ts @@ -1,9 +1,12 @@ import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; +import { ReportFixFunction, Scope, RuleFix } from '@typescript-eslint/experimental-utils/dist/ts-eslint' import { isIdentifier, isCallExpression, isMemberExpression, isArrowFunctionExpression, + isObjectPattern, + isProperty, } from '../node-utils'; import { getDocsUrl, SYNC_QUERIES_COMBINATIONS } from '../utils'; @@ -34,24 +37,22 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ create(context) { const sourceCode = context.getSourceCode(); + + /** * Reports the invalid usage of wait* plus getBy/QueryBy methods and automatically fixes the scenario * @param {TSESTree.CallExpression} node - The CallExpresion node that contains the wait* method * @param {'findBy' | 'findAllBy'} replacementParams.queryVariant - The variant method used to query: findBy/findByAll. * @param {string} replacementParams.queryMethod - Suffix string to build the query method (the query-part that comes after the "By"): LabelText, Placeholder, Text, Role, Title, etc. - * @param {Array} replacementParams.callArguments - Array of argument nodes which contain the parameters of the query inside the wait* method. - * @param {string=} replacementParams.caller - the variable name that targets screen or the value returned from `render` function. + * @param {ReportFixFunction} replacementParams.fix - Function that applies the fix to correct the code */ - function reportInvalidUsage(node: TSESTree.CallExpression, { queryVariant, queryMethod, callArguments, caller }: { queryVariant: 'findBy' | 'findAllBy', queryMethod: string, callArguments: TSESTree.Expression[], caller?: string }) { + function reportInvalidUsage(node: TSESTree.CallExpression, { queryVariant, queryMethod, fix }: { queryVariant: 'findBy' | 'findAllBy', queryMethod: string, fix: ReportFixFunction }) { context.report({ node, messageId: "preferFindBy", data: { queryVariant, queryMethod, fullQuery: sourceCode.getText(node) }, - fix(fixer) { - const newCode = `${caller ? `${caller}.` : ''}${queryVariant}${queryMethod}(${callArguments.map((node) => sourceCode.getText(node)).join(', ')})` - return fixer.replaceText(node, newCode) - } + fix, }); } @@ -72,24 +73,60 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ // ensure here it's one of the sync methods that we are calling if (isMemberExpression(argument.body.callee) && isIdentifier(argument.body.callee.property) && isIdentifier(argument.body.callee.object) && SYNC_QUERIES_COMBINATIONS.includes(argument.body.callee.property.name)) { // shape of () => screen.getByText - const queryMethod = argument.body.callee.property.name + const fullQueryMethod = argument.body.callee.property.name const caller = argument.body.callee.object.name - + const queryVariant = getFindByQueryVariant(fullQueryMethod) + const callArguments = argument.body.arguments + const queryMethod = fullQueryMethod.split('By')[1] + reportInvalidUsage(node, { - queryMethod: queryMethod.split('By')[1], - queryVariant: getFindByQueryVariant(queryMethod), - callArguments: argument.body.arguments, - caller, + queryMethod, + queryVariant, + fix(fixer) { + const newCode = `${caller}.${queryVariant}${queryMethod}(${callArguments.map((node) => sourceCode.getText(node)).join(', ')})` + return fixer.replaceText(node, newCode) + } }) return } if (isIdentifier(argument.body.callee) && SYNC_QUERIES_COMBINATIONS.includes(argument.body.callee.name)) { // shape of () => getByText - const queryMethod = argument.body.callee.name + const fullQueryMethod = argument.body.callee.name + const queryMethod = fullQueryMethod.split('By')[1] + const queryVariant = getFindByQueryVariant(fullQueryMethod) + const callArguments = argument.body.arguments + reportInvalidUsage(node, { - queryMethod: queryMethod.split('By')[1], - queryVariant: getFindByQueryVariant(queryMethod), - callArguments: argument.body.arguments, + 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)) + + // 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) && 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)) + } + + return allFixes + } }) return } @@ -98,6 +135,18 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ } }) -function getFindByQueryVariant(queryMethod: string) { +export function getFindByQueryVariant(queryMethod: string) { return queryMethod.includes('All') ? 'findAllBy' : 'findBy' +} + +function findRenderDefinitionDeclaration(scope: Scope.Scope | null, query: string): TSESTree.Identifier | null { + if (!scope) { + return null + } + let variable = scope.variables.find((v: Scope.Variable) => v.name === query) + if (variable) { + const def = variable.defs.find(({ name }) => name.name === query) + return def.name + } + return findRenderDefinitionDeclaration(scope.upper, query) } \ No newline at end of file diff --git a/tests/lib/rules/prefer-find-by.test.ts b/tests/lib/rules/prefer-find-by.test.ts index 6dac97b5..255d7187 100644 --- a/tests/lib/rules/prefer-find-by.test.ts +++ b/tests/lib/rules/prefer-find-by.test.ts @@ -1,7 +1,7 @@ -import { InvalidTestCase } from '@typescript-eslint/experimental-utils/dist/ts-eslint' +import { InvalidTestCase, ValidTestCase } from '@typescript-eslint/experimental-utils/dist/ts-eslint' import { createRuleTester } from '../test-utils'; import { ASYNC_QUERIES_COMBINATIONS, SYNC_QUERIES_COMBINATIONS } from '../../../lib/utils'; -import rule, { WAIT_METHODS, RULE_NAME } from '../../../lib/rules/prefer-find-by'; +import rule, { WAIT_METHODS, RULE_NAME, getFindByQueryVariant, MessageIds } from '../../../lib/rules/prefer-find-by'; const ruleTester = createRuleTester({ ecmaFeatures: { @@ -9,10 +9,26 @@ const ruleTester = createRuleTester({ }, }); +function buildFindByMethod(queryMethod: string) { + return `${getFindByQueryVariant(queryMethod)}${queryMethod.split('By')[1]}` +} + +function createScenario | InvalidTestCase>(callback: (waitMethod: string, queryMethod: string) => T) { + return WAIT_METHODS.reduce((acc: T[], waitMethod) => + acc.concat( + SYNC_QUERIES_COMBINATIONS + .map((queryMethod) => callback(waitMethod, queryMethod)) + ) + , []) +} + ruleTester.run(RULE_NAME, rule, { valid: [ ...ASYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ - code: `const submitButton = await ${queryMethod}('foo')` + code: ` + const { ${queryMethod} } = setup() + const submitButton = await ${queryMethod}('foo') + ` })), ...ASYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ code: `const submitButton = await screen.${queryMethod}('foo')` @@ -60,35 +76,117 @@ ruleTester.run(RULE_NAME, rule, { } ], invalid: [ - // using reduce + concat 'cause flatMap is not available in node10.x - ...WAIT_METHODS.reduce((acc: InvalidTestCase<'preferFindBy', []>[], waitMethod) => acc - .concat( - SYNC_QUERIES_COMBINATIONS.map((queryMethod: string) => ({ - code: `const submitButton = await ${waitMethod}(() => ${queryMethod}('foo', { name: 'baz' }))`, - errors: [{ - messageId: 'preferFindBy', - data: { - queryVariant: queryMethod.includes('All') ? 'findAllBy': 'findBy', - queryMethod: queryMethod.split('By')[1], - fullQuery: `${waitMethod}(() => ${queryMethod}('foo', { name: 'baz' }))`, - }, - }], - output: `const submitButton = await ${queryMethod.includes('All') ? 'findAllBy': 'findBy'}${queryMethod.split('By')[1]}('foo', { name: 'baz' })` - })) - ).concat( - SYNC_QUERIES_COMBINATIONS.map((queryMethod: string) => ({ - code: `const submitButton = await ${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' }))`, - errors: [{ - messageId: 'preferFindBy', - data: { - queryVariant: queryMethod.includes('All') ? 'findAllBy': 'findBy', - queryMethod: queryMethod.split('By')[1], - fullQuery: `${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' }))`, - } - }], - output: `const submitButton = await screen.${queryMethod.includes('All') ? 'findAllBy': 'findBy'}${queryMethod.split('By')[1]}('foo', { name: 'baz' })` - })) - ), - []) + ...createScenario((waitMethod: string, queryMethod: string) => ({ + code: ` + const { ${queryMethod} } = render() + const submitButton = await ${waitMethod}(() => ${queryMethod}('foo', { name: 'baz' })) + `, + errors: [{ + messageId: 'preferFindBy', + data: { + queryVariant: getFindByQueryVariant(queryMethod), + queryMethod: queryMethod.split('By')[1], + fullQuery: `${waitMethod}(() => ${queryMethod}('foo', { name: 'baz' }))`, + }, + }], + output: ` + 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' }))`, + errors: [{ + messageId: 'preferFindBy', + data: { + queryVariant: getFindByQueryVariant(queryMethod), + queryMethod: queryMethod.split('By')[1], + fullQuery: `${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' }))`, + } + }], + output: `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: ` + const { getByText, queryByLabelText, findAllByRole } = customRender() + it('foo', async () => { + const submitButton = await ${waitMethod}(() => getByText('baz', { name: 'button' })) + }) + `, + errors: [{ + messageId: 'preferFindBy', + data: { + queryVariant: 'findBy', + queryMethod: 'Text', + fullQuery: `${waitMethod}(() => getByText('baz', { name: 'button' }))`, + } + }], + output: ` + const { getByText, queryByLabelText, findAllByRole, findByText } = customRender() + it('foo', async () => { + const submitButton = await findByText('baz', { name: 'button' }) + }) + ` + })), + // // this scenario verifies when findBy* were already defined (because it was used elsewhere) + ...WAIT_METHODS.map((waitMethod: string) => ({ + code: ` + const { getAllByRole, findAllByRole } = customRender() + describe('some scenario', () => { + it('foo', async () => { + const submitButton = await ${waitMethod}(() => getAllByRole('baz', { name: 'button' })) + }) + }) + `, + errors: [{ + messageId: 'preferFindBy', + data: { + queryVariant: 'findAllBy', + queryMethod: 'Role', + fullQuery: `${waitMethod}(() => getAllByRole('baz', { name: 'button' }))`, + } + }], + output: ` + const { getAllByRole, findAllByRole } = customRender() + describe('some scenario', () => { + it('foo', async () => { + const submitButton = await findAllByRole('baz', { name: 'button' }) + }) + }) + ` + })), + // invalid code, as we need findBy* to be defined somewhere, but required for getting 100% coverage + { + code: `const submitButton = await waitFor(() => getByText('baz', { name: 'button' }))`, + errors: [{ + messageId: 'preferFindBy', + data: { + queryVariant: 'findBy', + queryMethod: 'Text', + fullQuery: `waitFor(() => getByText('baz', { name: 'button' }))` + } + }], + output: `const submitButton = await findByText('baz', { name: 'button' })` + }, + // this code would be invalid too, as findByRole is not defined anywhere. + { + code: ` + const getByRole = render().getByRole + const submitButton = await waitFor(() => getByRole('baz', { name: 'button' })) + `, + errors: [{ + messageId: 'preferFindBy', + data: { + queryVariant: 'findBy', + queryMethod: 'Role', + fullQuery: `waitFor(() => getByRole('baz', { name: 'button' }))` + } + }], + output: ` + const getByRole = render().getByRole + const submitButton = await findByRole('baz', { name: 'button' }) + ` + } ], }) \ No newline at end of file