feat: add prefer-find-by rule #144
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| # Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries (prefer-find-by) | ||
|
|
||
| findBy* queries are a simple combination of getBy* queries and waitFor. The findBy\* queries accept the waitFor options as the last argument. (i.e. screen.findByText('text', queryOptions, waitForOptions)) | ||
|
|
||
| ## Rule details | ||
|
|
||
| This rule aims to use `findBy*` or `findAllBy*` queries to wait for elements, rather than using `waitFor`, or the deprecated methods `waitForElement` and `wait`. | ||
| This rules analyzes those cases where `waitFor` is used with just one query method, in the form of an arrow function with only one statement (that is, without a block of statements). Given the callback could be more complex, this rule does not consider function callbacks or arrow functions with blocks of code | ||
|
|
||
| Examples of **incorrect** code for this rule | ||
|
|
||
| ```js | ||
| // arrow functions with one statement, using screen and any sync query method | ||
| const submitButton = await waitFor(() => | ||
| screen.getByRole('button', { name: /submit/i }) | ||
| ); | ||
| const submitButton = await waitFor(() => | ||
| screen.getAllByTestId('button', { name: /submit/i }) | ||
| ); | ||
|
|
||
| // arrow functions with one statement, calling any sync query method | ||
| const submitButton = await waitFor(() => | ||
| queryByLabel('button', { name: /submit/i }) | ||
| ); | ||
|
|
||
| const submitButton = await waitFor(() => | ||
| queryAllByText('button', { name: /submit/i }) | ||
| ); | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```js | ||
| // using findBy* methods | ||
| const submitButton = await findByText('foo'); | ||
| const submitButton = await screen.findAllByRole('table'); | ||
|
|
||
| // using waitForElementToBeRemoved | ||
| await waitForElementToBeRemoved(() => screen.findAllByRole('button')); | ||
| await waitForElementToBeRemoved(() => queryAllByLabel('my label')); | ||
| await waitForElementToBeRemoved(document.querySelector('foo')); | ||
|
|
||
| // using waitFor with a function | ||
| await waitFor(function() { | ||
| foo(); | ||
| return getByText('name'); | ||
| }); | ||
|
|
||
| // passing a reference of a function | ||
| function myCustomFunction() { | ||
| foo(); | ||
| return getByText('name'); | ||
| } | ||
| await waitFor(myCustomFunction); | ||
|
|
||
| // using waitFor with an arrow function with a code block | ||
| await waitFor(() => { | ||
| baz(); | ||
| return queryAllByText('foo'); | ||
| }); | ||
|
|
||
| // using a custom arrow function | ||
| await waitFor(() => myCustomFunction()); | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // using expects inside waitFor | ||
| await waitFor(() => expect(screen.getByText('bar')).toBeDisabled()); | ||
| await waitFor(() => expect(getAllByText('bar')).toBeDisabled()); | ||
| ``` | ||
|
|
||
| ## When Not To Use It | ||
|
|
||
| - Not encouraging use of findBy shortcut from testing library best practices | ||
|
|
||
| ## Further Reading | ||
|
|
||
| - Documentation for [findBy\* queries](https://testing-library.com/docs/dom-testing-library/api-queries#findby) | ||
|
|
||
| - Common mistakes with RTL, by Kent C. Dodds: [Using waitFor to wait for elements that can be queried with find\*](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-waitfor-to-wait-for-elements-that-can-be-queried-with-find) | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
| import { | ||
| isIdentifier, | ||
| isCallExpression, | ||
| isMemberExpression, | ||
| isArrowFunctionExpression, | ||
| } from '../node-utils'; | ||
| import { getDocsUrl, SYNC_QUERIES_COMBINATIONS } from '../utils'; | ||
|
|
||
| export const RULE_NAME = 'prefer-find-by'; | ||
|
|
||
| type Options = []; | ||
| export type MessageIds = 'preferFindBy'; | ||
|
|
||
| export const WAIT_METHODS = ['waitFor', 'waitForElement', 'wait'] | ||
|
|
||
| export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | ||
| name: RULE_NAME, | ||
| meta: { | ||
| type: 'suggestion', | ||
| docs: { | ||
| description: 'Suggest using find* instead of waitFor to wait for elements', | ||
| category: 'Best Practices', | ||
| recommended: 'warn', | ||
| }, | ||
| messages: { | ||
| preferFindBy: 'Prefer {{queryVariant}}{{queryMethod}} method over using await {{fullQuery}}' | ||
| }, | ||
| fixable: null, | ||
|
There was a problem hiding this comment. Can we not make the simple ones fixable? There was a problem hiding this comment. This is still a draft Ben. Leave the guy work on this! We are discussing in the original issue if addressing it here or in a 2nd PR. There was a problem hiding this comment. Was just an idea. Not trying to clock this or anything... |
||
| schema: [] | ||
| }, | ||
| defaultOptions: [], | ||
|
|
||
| create(context) { | ||
|
|
||
| function reportInvalidUsage(node: TSESTree.CallExpression, { queryVariant, queryMethod, fullQuery }: { queryVariant: string, queryMethod: string, fullQuery: string}) { | ||
| context.report({ | ||
| node, | ||
| messageId: "preferFindBy", | ||
| data: { queryVariant, queryMethod, fullQuery }, | ||
| }); | ||
| } | ||
|
|
||
| const sourceCode = context.getSourceCode(); | ||
|
|
||
| return { | ||
| 'AwaitExpression > CallExpression'(node: TSESTree.CallExpression) { | ||
| if (!isIdentifier(node.callee) || !WAIT_METHODS.includes(node.callee.name)) { | ||
| return | ||
| } | ||
| // ensure the only argument is an arrow function expression - if the arrow function is a block | ||
| // we skip it | ||
| const argument = node.arguments[0] | ||
| if (!isArrowFunctionExpression(argument)) { | ||
| return | ||
| } | ||
| if (!isCallExpression(argument.body)) { | ||
| return | ||
| } | ||
| // 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 | ||
| reportInvalidUsage(node, { | ||
| queryMethod: queryMethod.split('By')[1], | ||
| queryVariant: getFindByQueryVariant(queryMethod), | ||
| fullQuery: sourceCode.getText(node) | ||
| }) | ||
| return | ||
| } | ||
| if (isIdentifier(argument.body.callee) && SYNC_QUERIES_COMBINATIONS.includes(argument.body.callee.name)) { | ||
| // shape of () => getByText | ||
| const queryMethod = argument.body.callee.name | ||
| reportInvalidUsage(node, { | ||
| queryMethod: queryMethod.split('By')[1], | ||
| queryVariant: getFindByQueryVariant(queryMethod), | ||
| fullQuery: sourceCode.getText(node) | ||
| }) | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| function getFindByQueryVariant(queryMethod: string) { | ||
| return queryMethod.includes('All') ? 'findAllBy' : 'findBy' | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import { InvalidTestCase } 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'; | ||
|
|
||
| const ruleTester = createRuleTester({ | ||
| ecmaFeatures: { | ||
| jsx: true, | ||
| }, | ||
| }); | ||
|
|
||
| ruleTester.run(RULE_NAME, rule, { | ||
| valid: [ | ||
| ...ASYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: `const submitButton = await ${queryMethod}('foo')` | ||
| })), | ||
| ...ASYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: `const submitButton = await screen.${queryMethod}('foo')` | ||
| })), | ||
| ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: `await waitForElementToBeRemoved(() => ${queryMethod}(baz))` | ||
| })), | ||
| ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: `await waitFor(function() { | ||
| return ${queryMethod}('baz', { name: 'foo' }) | ||
| })` | ||
| })), | ||
| { | ||
| code: `await waitFor(() => myCustomFunction())` | ||
| }, | ||
| { | ||
| code: `await waitFor(customFunctionReference)` | ||
| }, | ||
| { | ||
| code: `await waitForElementToBeRemoved(document.querySelector('foo'))` | ||
| }, | ||
| ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: ` | ||
| await waitFor(() => { | ||
| foo() | ||
| return ${queryMethod}() | ||
| }) | ||
| ` | ||
| })), | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: ` | ||
| await waitFor(() => expect(screen.${queryMethod}('baz')).toBeDisabled()); | ||
| ` | ||
| })), | ||
| ...SYNC_QUERIES_COMBINATIONS.map((queryMethod) => ({ | ||
| code: ` | ||
| await waitFor(() => expect(${queryMethod}('baz')).toBeInTheDocument()); | ||
| ` | ||
| })) | ||
| ], | ||
| 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' }))`, | ||
| } | ||
| }] | ||
| })) | ||
| ).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' }))`, | ||
| } | ||
| }] | ||
| })) | ||
| ), | ||
| []) | ||
| ], | ||
| }) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add some correct usages of waitFor here as well.