fix(prefer-screen-queries): avoid reporting custom queries (#342)

* fix(prefer-screen-queries): avoid reporting custom queries * docs(prefer-screen-queries): update examples, exceptions and reading Closes #340
testing-library · Apr 16, 2021 · 1c4391c · 1c4391c
1 parent 445adc8
commit 1c4391c
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 66 deletions.
diff --git a/docs/rules/prefer-screen-queries.md b/docs/rules/prefer-screen-queries.md
@@ -1,9 +1,16 @@
-# Suggest using `screen` while using queries (`testing-library/prefer-screen-queries`)
+# Suggest using `screen` while querying (`testing-library/prefer-screen-queries`)
 
 ## Rule Details
 
-DOM Testing Library (and other Testing Library frameworks built on top of it) exports a `screen` object which has every query (and a `debug` method). This works better with autocomplete and makes each test a little simpler to write and maintain.  
-This rule aims to force writing tests using queries directly from `screen` object rather than destructuring them from `render` result. Given the screen component does not expose utility methods such as `rerender()` or the `container` property, it is correct to use the `render` response in those scenarios.
+DOM Testing Library (and other Testing Library frameworks built on top of it) exports a `screen` object which has every query (and a `debug` method). This works better with autocomplete and makes each test a little simpler to write and maintain.
+
+This rule aims to force writing tests using built-in queries directly from `screen` object rather than destructuring them from `render` result. Given the screen component does not expose utility methods such as `rerender()` or the `container` property, it is correct to use the `render` returned value in those scenarios.
+
+However, there are 3 exceptions when this rule won't suggest using `screen` for querying:
+
+1. You are using a query chained to `within`
+2. You are using custom queries, so you can't access them through `screen`
+3. You are setting the `container` or `baseElement`, so you need to use the queries returned from `render`
 
 Examples of **incorrect** code for this rule:
 
@@ -65,8 +72,19 @@ unmount();
 const { getByText } = render(<Foo />, { baseElement: treeA });
 // using container
 const { getAllByText } = render(<Foo />, { container: treeA });
+
+// querying with a custom query imported from its own module
+import { getByIcon } from 'custom-queries';
+const element = getByIcon('search');
+
+// querying with a custom query returned from `render`
+const { getByIcon } = render(<Foo />);
+const element = getByIcon('search');
 ```
 
 ## Further Reading
 
-- [`screen` documentation](https://testing-library.com/docs/dom-testing-library/api-queries#screen)
+- [Common mistakes with React Testing Library - Not using `screen`](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-screen)
+- [`screen` documentation](https://testing-library.com/docs/queries/about#screen)
+- [Advanced - Custom Queries](https://testing-library.com/docs/dom-testing-library/api-custom-queries/)
+- [React Testing Library - Add custom queries](https://testing-library.com/docs/react-testing-library/setup/#add-custom-queries)
diff --git a/lib/detect-testing-library-utils.ts b/lib/detect-testing-library-utils.ts
@@ -64,6 +64,7 @@ type IsSyncQueryFn = (node: TSESTree.Identifier) => boolean;
 type IsAsyncQueryFn = (node: TSESTree.Identifier) => boolean;
 type IsQueryFn = (node: TSESTree.Identifier) => boolean;
 type IsCustomQueryFn = (node: TSESTree.Identifier) => boolean;
+type IsBuiltInQueryFn = (node: TSESTree.Identifier) => boolean;
 type IsAsyncUtilFn = (
   node: TSESTree.Identifier,
   validNames?: readonly typeof ASYNC_UTILS[number][]
@@ -98,6 +99,7 @@ export interface DetectionHelpers {
   isAsyncQuery: IsAsyncQueryFn;
   isQuery: IsQueryFn;
   isCustomQuery: IsCustomQueryFn;
+  isBuiltInQuery: IsBuiltInQueryFn;
   isAsyncUtil: IsAsyncUtilFn;
   isFireEventUtil: (node: TSESTree.Identifier) => boolean;
   isUserEventUtil: (node: TSESTree.Identifier) => boolean;
@@ -301,6 +303,10 @@ export function detectTestingLibraryUtils<
       return isQuery(node) && !ALL_QUERIES_COMBINATIONS.includes(node.name);
     };
 
+    const isBuiltInQuery = (node: TSESTree.Identifier): boolean => {
+      return ALL_QUERIES_COMBINATIONS.includes(node.name);
+    };
+
     /**
      * Determines whether a given node is a valid async util or not.
      *
@@ -704,6 +710,7 @@ export function detectTestingLibraryUtils<
       isAsyncQuery,
       isQuery,
       isCustomQuery,
+      isBuiltInQuery,
       isAsyncUtil,
       isFireEventUtil,
       isUserEventUtil,

diff --git a/lib/rules/prefer-screen-queries.ts b/lib/rules/prefer-screen-queries.ts
@@ -64,7 +64,7 @@ export default createTestingLibraryRule<Options, MessageIds>({
           if (
             isProperty(property) &&
             ASTUtils.isIdentifier(property.key) &&
-            helpers.isQuery(property.key)
+            helpers.isBuiltInQuery(property.key)
           ) {
             safeDestructuredQueries.push(property.key.name);
           }
@@ -115,7 +115,7 @@ export default createTestingLibraryRule<Options, MessageIds>({
         }
       },
       'CallExpression > Identifier'(node: TSESTree.Identifier) {
-        if (!helpers.isQuery(node)) {
+        if (!helpers.isBuiltInQuery(node)) {
           return;
         }
 
@@ -130,7 +130,7 @@ export default createTestingLibraryRule<Options, MessageIds>({
           return ['screen', ...withinDeclaredVariables].includes(name);
         }
 
-        if (!helpers.isQuery(node)) {
+        if (!helpers.isBuiltInQuery(node)) {
           return;
         }
 

diff --git a/tests/lib/rules/prefer-screen-queries.test.ts b/tests/lib/rules/prefer-screen-queries.test.ts
@@ -11,17 +11,13 @@ const ruleTester = createRuleTester();
 const CUSTOM_QUERY_COMBINATIONS = combineQueries(ALL_QUERIES_VARIANTS, [
   'ByIcon',
 ]);
-const ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS = [
-  ...ALL_QUERIES_COMBINATIONS,
-  ...CUSTOM_QUERY_COMBINATIONS,
-];
 
 ruleTester.run(RULE_NAME, rule, {
   valid: [
     {
       code: `const baz = () => 'foo'`,
     },
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       code: `screen.${queryMethod}()`,
     })),
     {
@@ -30,24 +26,45 @@ ruleTester.run(RULE_NAME, rule, {
     {
       code: `component.otherFunctionShouldNotThrow()`,
     },
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       code: `within(component).${queryMethod}()`,
     })),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       code: `within(screen.${queryMethod}()).${queryMethod}()`,
     })),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       code: `
         const { ${queryMethod} } = within(screen.getByText('foo'))
         ${queryMethod}(baz)
       `,
     })),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       code: `
         const myWithinVariable = within(foo)
         myWithinVariable.${queryMethod}('baz')
       `,
     })),
+    ...CUSTOM_QUERY_COMBINATIONS.map(
+      (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')
+      })
+    `
+    ),
+    ...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')
+      })
+    `
+    ),
     {
       code: `
         const screen = render(baz);
@@ -96,70 +113,56 @@ ruleTester.run(RULE_NAME, rule, {
         utils.unmount();
       `,
     },
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         const { ${queryMethod} } = render(baz, { baseElement: treeA })
         expect(${queryMethod}(baz)).toBeDefined()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         const { ${queryMethod}: aliasMethod } = render(baz, { baseElement: treeA })
         expect(aliasMethod(baz)).toBeDefined()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         const { ${queryMethod} } = render(baz, { container: treeA })
         expect(${queryMethod}(baz)).toBeDefined()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         const { ${queryMethod}: aliasMethod } = render(baz, { container: treeA })
         expect(aliasMethod(baz)).toBeDefined()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         const { ${queryMethod} } = render(baz, { baseElement: treeB, container: treeA })
         expect(${queryMethod}(baz)).toBeDefined()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         const { ${queryMethod}: aliasMethod } = render(baz, { baseElement: treeB, container: treeA })
         expect(aliasMethod(baz)).toBeDefined()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
-      (queryMethod: string) => ({
-        code: `
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod: string) => ({
+      code: `
         render(foo, { baseElement: treeA }).${queryMethod}()
       `,
-      })
-    ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    })),
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       settings: { 'testing-library/utils-module': 'test-utils' },
       code: `
         import { render as testUtilRender } from 'test-utils'
         import { render } from 'somewhere-else'
         const { ${queryMethod} } = render(foo)
         ${queryMethod}()`,
     })),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map((queryMethod) => ({
+    ...ALL_QUERIES_COMBINATIONS.map((queryMethod) => ({
       settings: {
         'testing-library/custom-renders': ['customRender'],
       },
@@ -171,7 +174,7 @@ ruleTester.run(RULE_NAME, rule, {
   ],
 
   invalid: [
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `
@@ -187,7 +190,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           settings: { 'testing-library/utils-module': 'test-utils' },
@@ -208,7 +211,7 @@ ruleTester.run(RULE_NAME, rule, {
         } as const)
     ),
 
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           settings: {
@@ -230,7 +233,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           settings: { 'testing-library/utils-module': 'test-utils' },
@@ -250,7 +253,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           settings: { 'testing-library/utils-module': 'test-utils' },
@@ -270,7 +273,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `render().${queryMethod}()`,
@@ -284,7 +287,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `render(foo, { hydrate: true }).${queryMethod}()`,
@@ -298,7 +301,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `component.${queryMethod}()`,
@@ -312,7 +315,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `
@@ -329,7 +332,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `
@@ -346,7 +349,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `
@@ -363,7 +366,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `
@@ -380,7 +383,7 @@ ruleTester.run(RULE_NAME, rule, {
           ],
         } as const)
     ),
-    ...ALL_BUILTIN_AND_CUSTOM_QUERIES_COMBINATIONS.map(
+    ...ALL_QUERIES_COMBINATIONS.map(
       (queryMethod) =>
         ({
           code: `