fix: include findBy variable declaration to prefer-find-by when auto fixing (#197)

gndelia · web-flow · commit 993fd8cdaeb9 · 2020-07-20T13:05:24.000+02:00
diff --git 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)<Options, MessageIds>({
   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<TSESTree.Expression>} 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)<Options, MessageIds>({
         // 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)<Options, MessageIds>({
   }
 })
 
-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)
 }
diff --git a/tests/lib/rules/prefer-find-by.test.ts b/tests/lib/rules/prefer-find-by.test.ts
@@ -1,18 +1,34 @@
-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: {
     jsx: true,
   },
 });
 
+function buildFindByMethod(queryMethod: string) {
+  return `${getFindByQueryVariant(queryMethod)}${queryMethod.split('By')[1]}`
+}
+
+function createScenario<T extends ValidTestCase<[]> | InvalidTestCase<MessageIds, []>>(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' })
+      `
+    }
   ],
 })
