feat: add prefer-query-matchers rule (#750)

* feat: add prefer-query-matchers rule Co-authored-by: Dale Karp <[email protected]> * test: cover prefer-query-matchers with test Co-authored-by: Dale Karp <[email protected]> * feat: set default options to no configured entries to check for Co-authored-by: Dale Karp <[email protected]> * docs: add query doc and supported rules table entry * style: prettify rule file * fix: do not include prefer-query-matchers by default * fix: failing test * fix: make generated tests more realistic AST * fix: comment out test names --------- Co-authored-by: Dale Karp <[email protected]>
testing-library · May 10, 2023 · 31516ad · 31516ad
1 parent 3f2f4f8
commit 31516ad
Show file tree

Hide file tree

Showing 7 changed files with 609 additions and 1 deletion.
diff --git a/README.md b/README.md
@@ -228,6 +228,7 @@ module.exports = {
 | [prefer-find-by](docs/rules/prefer-find-by.md)                                   | Suggest using `find(All)By*` query instead of `waitFor` + `get(All)By*` to wait for elements | ![badge-angular][] ![badge-dom][] ![badge-marko][] ![badge-react][] ![badge-vue][] | 🔧  |
 | [prefer-presence-queries](docs/rules/prefer-presence-queries.md)                 | Ensure appropriate `get*`/`query*` queries are used with their respective matchers           | ![badge-angular][] ![badge-dom][] ![badge-marko][] ![badge-react][] ![badge-vue][] |     |
 | [prefer-query-by-disappearance](docs/rules/prefer-query-by-disappearance.md)     | Suggest using `queryBy*` queries when waiting for disappearance                              | ![badge-angular][] ![badge-dom][] ![badge-marko][] ![badge-react][] ![badge-vue][] |     |
+| [prefer-query-matchers](docs/rules/prefer-query-matchers.md)                     | Ensure the configured `get*`/`query*` query is used with the corresponding matchers          |                                                                                    |     |
 | [prefer-screen-queries](docs/rules/prefer-screen-queries.md)                     | Suggest using `screen` while querying                                                        | ![badge-angular][] ![badge-dom][] ![badge-marko][] ![badge-react][] ![badge-vue][] |     |
 | [prefer-user-event](docs/rules/prefer-user-event.md)                             | Suggest using `userEvent` over `fireEvent` for simulating user interactions                  |                                                                                    |     |
 | [prefer-wait-for](docs/rules/prefer-wait-for.md)                                 | Use `waitFor` instead of deprecated wait methods                                             |                                                                                    | 🔧  |

diff --git a/docs/rules/prefer-presence-queries.md b/docs/rules/prefer-presence-queries.md
@@ -43,6 +43,7 @@ Examples of **correct** code for this rule:
 ```js
 test('some test', async () => {
 	render(<App />);
+
 	// check element is present with `getBy*`
 	expect(screen.getByText('button')).toBeInTheDocument();
 	expect(screen.getAllByText('button')[9]).toBeTruthy();

diff --git a/docs/rules/prefer-query-matchers.md b/docs/rules/prefer-query-matchers.md
@@ -0,0 +1,85 @@
+# Ensure the configured `get*`/`query*` query is used with the corresponding matchers (`testing-library/prefer-query-matchers`)
+
+<!-- end auto-generated rule header -->
+
+The (DOM) Testing Library allows to query DOM elements using different types of queries such as `get*` and `query*`. Using `get*` throws an error in case the element is not found, while `query*` returns null instead of throwing (or empty array for `queryAllBy*` ones).
+
+It may be helpful to ensure that either `get*` or `query*` are always used for a given matcher. For example, `.toBeVisible()` and the negation `.not.toBeVisible()` both assume that an element exists in the DOM and will error if not. Using `get*` with `.toBeVisible()` ensures that if the element is not found the error thrown will offer better info than with `query*`.
+
+## Rule details
+
+This rule must be configured with a list of `validEntries`: for a given matcher, is `get*` or `query*` required.
+
+Assuming the following configuration:
+
+```json
+{
+	"testing-library/prefer-query-matchers": [
+		2,
+		{
+			"validEntries": [{ "matcher": "toBeVisible", "query": "get" }]
+		}
+	]
+}
+```
+
+Examples of **incorrect** code for this rule with the above configuration:
+
+```js
+test('some test', () => {
+	render(<App />);
+
+	// use configured matcher with the disallowed `query*`
+	expect(screen.queryByText('button')).toBeVisible();
+	expect(screen.queryByText('button')).not.toBeVisible();
+	expect(screen.queryAllByText('button')[0]).toBeVisible();
+	expect(screen.queryAllByText('button')[0]).not.toBeVisible();
+});
+```
+
+Examples of **correct** code for this rule:
+
+```js
+test('some test', async () => {
+	render(<App />);
+	// use configured matcher with the allowed `get*`
+	expect(screen.getByText('button')).toBeVisible();
+	expect(screen.getByText('button')).not.toBeVisible();
+	expect(screen.getAllByText('button')[0]).toBeVisible();
+	expect(screen.getAllByText('button')[0]).not.toBeVisible();
+
+	// use an unconfigured matcher with either `get* or `query*
+	expect(screen.getByText('button')).toBeEnabled();
+	expect(screen.getAllByText('checkbox')[0]).not.toBeChecked();
+	expect(screen.queryByText('button')).toHaveFocus();
+	expect(screen.queryAllByText('button')[0]).not.toMatchMyCustomMatcher();
+
+	// `findBy*` queries are out of the scope for this rule
+	const button = await screen.findByText('submit');
+	expect(button).toBeVisible();
+});
+```
+
+## Options
+
+| Option         | Required | Default | Details                                                                                                                                                                                                            |
+| -------------- | -------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `validEntries` | No       | `[]`    | A list of objects with a `matcher` property (the name of any matcher, such as "toBeVisible") and a `query` property (either "get" or "query"). Indicates whether `get*` or `query*` are allowed with this matcher. |
+
+## Example
+
+```json
+{
+	"testing-library/prefer-query-matchers": [
+		2,
+		{
+			"validEntries": [{ "matcher": "toBeVisible", "query": "get" }]
+		}
+	]
+}
+```
+
+## Further Reading
+
+- [Testing Library queries cheatsheet](https://testing-library.com/docs/dom-testing-library/cheatsheet#queries)
+- [jest-dom note about using `getBy` within assertions](https://testing-library.com/docs/ecosystem-jest-dom)
diff --git a/lib/create-testing-library-rule/detect-testing-library-utils.ts b/lib/create-testing-library-rule/detect-testing-library-utils.ts
@@ -85,6 +85,10 @@ type IsDebugUtilFn = (
 	validNames?: ReadonlyArray<(typeof DEBUG_UTILS)[number]>
 ) => boolean;
 type IsPresenceAssertFn = (node: TSESTree.MemberExpression) => boolean;
+type IsMatchingAssertFn = (
+	node: TSESTree.MemberExpression,
+	matcherName: string
+) => boolean;
 type IsAbsenceAssertFn = (node: TSESTree.MemberExpression) => boolean;
 type CanReportErrorsFn = () => boolean;
 type FindImportedTestingLibraryUtilSpecifierFn = (
@@ -122,6 +126,7 @@ export interface DetectionHelpers {
 	isActUtil: (node: TSESTree.Identifier) => boolean;
 	isPresenceAssert: IsPresenceAssertFn;
 	isAbsenceAssert: IsAbsenceAssertFn;
+	isMatchingAssert: IsMatchingAssertFn;
 	canReportErrors: CanReportErrorsFn;
 	findImportedTestingLibraryUtilSpecifier: FindImportedTestingLibraryUtilSpecifierFn;
 	isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn;
@@ -819,6 +824,16 @@ export function detectTestingLibraryUtils<
 				: ABSENCE_MATCHERS.includes(matcher);
 		};
 
+		const isMatchingAssert: IsMatchingAssertFn = (node, matcherName) => {
+			const { matcher } = getAssertNodeInfo(node);
+
+			if (!matcher) {
+				return false;
+			}
+
+			return matcher === matcherName;
+		};
+
 		/**
 		 * Finds the import util specifier related to Testing Library for a given name.
 		 */
@@ -977,6 +992,7 @@ export function detectTestingLibraryUtils<
 			isDebugUtil,
 			isActUtil,
 			isPresenceAssert,
+			isMatchingAssert,
 			isAbsenceAssert,
 			canReportErrors,
 			findImportedTestingLibraryUtilSpecifier,

diff --git a/lib/rules/prefer-query-matchers.ts b/lib/rules/prefer-query-matchers.ts
@@ -0,0 +1,105 @@
+import { TSESTree } from '@typescript-eslint/utils';
+
+import { createTestingLibraryRule } from '../create-testing-library-rule';
+import { findClosestCallNode, isMemberExpression } from '../node-utils';
+
+export const RULE_NAME = 'prefer-query-matchers';
+export type MessageIds = 'wrongQueryForMatcher';
+export type Options = [
+	{
+		validEntries: {
+			query: 'get' | 'query';
+			matcher: string;
+		}[];
+	}
+];
+
+export default createTestingLibraryRule<Options, MessageIds>({
+	name: RULE_NAME,
+	meta: {
+		docs: {
+			description:
+				'Ensure the configured `get*`/`query*` query is used with the corresponding matchers',
+			recommendedConfig: {
+				dom: false,
+				angular: false,
+				react: false,
+				vue: false,
+				marko: false,
+			},
+		},
+		messages: {
+			wrongQueryForMatcher: 'Use `{{ query }}By*` queries for {{ matcher }}',
+		},
+		schema: [
+			{
+				type: 'object',
+				additionalProperties: false,
+				properties: {
+					validEntries: {
+						type: 'array',
+						items: {
+							type: 'object',
+							properties: {
+								query: {
+									type: 'string',
+									enum: ['get', 'query'],
+								},
+								matcher: {
+									type: 'string',
+								},
+							},
+						},
+					},
+				},
+			},
+		],
+		type: 'suggestion',
+	},
+	defaultOptions: [
+		{
+			validEntries: [],
+		},
+	],
+
+	create(context, [{ validEntries }], helpers) {
+		return {
+			'CallExpression Identifier'(node: TSESTree.Identifier) {
+				const expectCallNode = findClosestCallNode(node, 'expect');
+
+				if (!expectCallNode || !isMemberExpression(expectCallNode.parent)) {
+					return;
+				}
+
+				// Sync queries (getBy and queryBy) and corresponding ones
+				// are supported. If none found, stop the rule.
+				if (!helpers.isSyncQuery(node)) {
+					return;
+				}
+
+				const isGetBy = helpers.isGetQueryVariant(node);
+				const expectStatement = expectCallNode.parent;
+				for (const entry of validEntries) {
+					const { query, matcher } = entry;
+					const isMatchingAssertForThisEntry = helpers.isMatchingAssert(
+						expectStatement,
+						matcher
+					);
+
+					if (!isMatchingAssertForThisEntry) {
+						continue;
+					}
+
+					const actualQuery = isGetBy ? 'get' : 'query';
+					if (query !== actualQuery) {
+						context.report({
+							node,
+							messageId: 'wrongQueryForMatcher',
+							data: { query, matcher },
+						});
+					}
+				}
+			},
+		};
+	},
+});
diff --git a/tests/index.test.ts b/tests/index.test.ts
@@ -3,7 +3,7 @@ import { resolve } from 'path';
 
 import plugin from '../lib';
 
-const numberOfRules = 27;
+const numberOfRules = 28;
 const ruleNames = Object.keys(plugin.rules);
 
 // eslint-disable-next-line jest/expect-expect