-
Notifications
You must be signed in to change notification settings - Fork 154
feat: add prefer-explicit-assert rule #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
dc1a105
feat: add no-get-by-assert rule
Belco90 0b68fd5
refactor: rename no-get-by-assert rule to prefer-explicit-assert
Belco90 4bd438f
docs(prefer-explicit-assert): add references to prefer-expect-query-by
Belco90 939a1f7
feat(prefer-explicit-assert): add option for custom queries
Belco90 1366600
feat(prefer-explicit-assert): improve error message
Belco90 bfcfe4d
refactor(prefer-explicit-assert): rename error message id
Belco90 ec5843b
fix(prefer-explicit-assert): rename option argument
Belco90 d519aa5
fix(prefer-explicit-assert): check queries when not destructured
Belco90 d2a81bb
docs(prefer-explicit-assert): add non-destructured queries examples
Belco90 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 hidden or 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,83 @@ | ||
| # Suggest using explicit assertions rather than just `getBy*` queries (prefer-explicit-assert) | ||
|
|
||
| Testing Library `getBy*` queries throw an error if the element is not | ||
| found. Some users like this behavior to use the query itself as an | ||
| assert for the element existence in their tests, but other users don't | ||
| and prefer to explicitly assert the element existence, so this rule is | ||
| for users from the latter. | ||
|
|
||
| ## Rule Details | ||
|
|
||
| This rule aims to encourage users to explicitly assert existence of | ||
| elements in their tests rather than just use `getBy*` queries and expect | ||
| it doesn't throw an error so it's easier to understand what's the | ||
| expected behavior within the test. | ||
|
|
||
| > ⚠️ Please note that this rule is recommended to be used together with | ||
| > [prefer-expect-query-by](docs/rules/prefer-expect-query-by.md), so the | ||
| > combination of these two rules will force users to do 2 actual | ||
| > changes: wrap `getBy*` with `expect` assertion and then use `queryBy*` | ||
| > instead of `getBy*` for asserting. | ||
|
|
||
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```js | ||
| // just calling `getBy*` query expecting not to throw an error as an | ||
| // assert-like method, without actually either using the returned element | ||
| // or explicitly asserting | ||
| getByText('foo'); | ||
|
|
||
| const utils = render(<Component />); | ||
| utils.getByText('foo'); | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```js | ||
| // wrapping the get query within a `expect` and use some matcher for | ||
| // making the assertion more explicit | ||
| expect(getByText('foo')).toBeDefined(); | ||
|
|
||
| const utils = render(<Component />); | ||
| expect(utils.getByText('foo')).toBeDefined(); | ||
|
|
||
| // ⚠️ `getBy*` should be replaced by `queryBy*` when combined with `prefer-expect-query-by` rule | ||
| expect(queryByText('foo')).toBeDefined(); | ||
|
|
||
| // even more explicit if you use `@testing-library/jest-dom` matcher | ||
| // for checking the element is present in the document | ||
| expect(queryByText('foo')).toBeInTheDocument(); | ||
|
|
||
| // Doing something with the element returned without asserting is absolutely fine | ||
| await waitForElement(() => getByText('foo')); | ||
| fireEvent.click(getByText('bar')); | ||
| const quxElement = getByText('qux'); | ||
|
|
||
| // call directly something different than Testing Library query | ||
| getByNonTestingLibraryVariant('foo'); | ||
| ``` | ||
|
|
||
| ## Options | ||
|
|
||
| This rule accepts a single options argument: | ||
|
|
||
| - `customQueryNames`: this array option allows to extend default Testing | ||
| Library queries with custom ones for including them into rule | ||
| inspection. | ||
|
|
||
| ```js | ||
| "testing-library/prefer-expect-query-by": ["error", {"customQueryNames": ["getByIcon", "getBySomethingElse"]}], | ||
| ``` | ||
|
|
||
| ## When Not To Use It | ||
|
|
||
| If you prefer to use `getBy*` queries implicitly as an assert-like | ||
| method itself, then this rule is not recommended. | ||
|
|
||
| ## Related Rules | ||
|
|
||
| - [prefer-expect-query-by](docs/rules/prefer-expect-query-by.md) | ||
|
|
||
| ## Further Reading | ||
|
|
||
| - [getBy query](https://testing-library.com/docs/dom-testing-library/api-queries#getby) |
This file contains hidden or 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 hidden or 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,79 @@ | ||
| 'use strict'; | ||
|
|
||
| const { getDocsUrl, ALL_QUERIES_METHODS } = require('../utils'); | ||
|
|
||
| const ALL_GET_BY_QUERIES = ALL_QUERIES_METHODS.map( | ||
| queryMethod => `get${queryMethod}` | ||
| ); | ||
|
|
||
| const findCallExpressionParent = node => | ||
| node.type === 'CallExpression' ? node : findCallExpressionParent(node.parent); | ||
|
|
||
| const isValidQuery = (node, customQueryNames = []) => | ||
| ALL_GET_BY_QUERIES.includes(node.name) || | ||
| customQueryNames.includes(node.name); | ||
|
|
||
| const isDirectlyCalledByFunction = node => | ||
| node.parent.type === 'CallExpression'; | ||
|
|
||
| const isReturnedByArrowFunctionExpression = node => | ||
| node.parent.type === 'ArrowFunctionExpression'; | ||
|
|
||
| const isDeclared = node => node.parent.type === 'VariableDeclarator'; | ||
|
|
||
| const isReturnedByReturnStatement = node => | ||
| node.parent.type === 'ReturnStatement'; | ||
|
|
||
| module.exports = { | ||
| meta: { | ||
| type: 'suggestion', | ||
| docs: { | ||
| description: | ||
| 'Suggest using explicit assertions rather than just `getBy*` queries', | ||
| category: 'Best Practices', | ||
| recommended: false, | ||
| url: getDocsUrl('prefer-explicit-assert'), | ||
| }, | ||
| messages: { | ||
| preferExplicitAssert: | ||
| 'Wrap stand-alone `getBy*` query with `expect` function for better explicit assertion', | ||
| }, | ||
| fixable: null, | ||
| schema: [ | ||
| { | ||
| type: 'object', | ||
| properties: { | ||
| customQueryNames: { | ||
| type: 'array', | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
|
|
||
| create: function(context) { | ||
| return { | ||
| 'CallExpression Identifier'(node) { | ||
| const callExpressionNode = findCallExpressionParent(node); | ||
|
|
||
| let customQueryNames; | ||
| if (context.options && context.options.length > 0) { | ||
| [{ customQueryNames }] = context.options; | ||
| } | ||
|
|
||
| if ( | ||
| isValidQuery(node, customQueryNames) && | ||
| !isDirectlyCalledByFunction(callExpressionNode) && | ||
| !isReturnedByArrowFunctionExpression(callExpressionNode) && | ||
| !isDeclared(callExpressionNode) && | ||
| !isReturnedByReturnStatement(callExpressionNode) | ||
| ) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'preferExplicitAssert', | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; |
This file contains hidden or 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,110 @@ | ||
| 'use strict'; | ||
|
|
||
| const rule = require('../../../lib/rules/prefer-explicit-assert'); | ||
| const { ALL_QUERIES_METHODS } = require('../../../lib/utils'); | ||
| const RuleTester = require('eslint').RuleTester; | ||
|
|
||
| // ------------------------------------------------------------------------------ | ||
| // Tests | ||
| // ------------------------------------------------------------------------------ | ||
|
|
||
| const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2018 } }); | ||
| ruleTester.run('prefer-explicit-assert', rule, { | ||
| valid: [ | ||
| { | ||
| code: `getByText`, | ||
| }, | ||
| { | ||
| code: `const utils = render() | ||
|
|
||
| utils.getByText | ||
| `, | ||
| }, | ||
| { | ||
| code: `expect(getByText('foo')).toBeDefined()`, | ||
| }, | ||
| { | ||
| code: `const utils = render() | ||
|
|
||
| expect(utils.getByText('foo')).toBeDefined() | ||
| `, | ||
| }, | ||
| { | ||
| code: `expect(getByText('foo')).toBeInTheDocument();`, | ||
| }, | ||
| { | ||
| code: `async () => { await waitForElement(() => getByText('foo')) }`, | ||
| }, | ||
| { | ||
| code: `fireEvent.click(getByText('bar'));`, | ||
| }, | ||
| { | ||
| code: `const quxElement = getByText('qux')`, | ||
| }, | ||
| { | ||
| code: `() => { return getByText('foo') }`, | ||
| }, | ||
| { | ||
| code: `function bar() { return getByText('foo') }`, | ||
| }, | ||
| { | ||
| code: `getByIcon('foo')`, // custom `getBy` query not extended through options | ||
| }, | ||
| ], | ||
|
|
||
| invalid: [ | ||
| ...ALL_QUERIES_METHODS.map(queryMethod => ({ | ||
| code: `get${queryMethod}('foo')`, | ||
| errors: [ | ||
| { | ||
| messageId: 'preferExplicitAssert', | ||
| }, | ||
| ], | ||
| })), | ||
| ...ALL_QUERIES_METHODS.map(queryMethod => ({ | ||
| code: `const utils = render() | ||
|
|
||
| utils.get${queryMethod}('foo')`, | ||
| errors: [ | ||
| { | ||
| messageId: 'preferExplicitAssert', | ||
| line: 3, | ||
| column: 13, | ||
| }, | ||
| ], | ||
| })), | ||
| ...ALL_QUERIES_METHODS.map(queryMethod => ({ | ||
| code: `() => { | ||
| get${queryMethod}('foo') | ||
| doSomething() | ||
|
|
||
| get${queryMethod}('bar') | ||
| const quxElement = get${queryMethod}('qux') | ||
| } | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'preferExplicitAssert', | ||
| line: 2, | ||
| }, | ||
| { | ||
| messageId: 'preferExplicitAssert', | ||
| line: 5, | ||
| }, | ||
| ], | ||
| })), | ||
| { | ||
| code: `getByIcon('foo')`, // custom `getBy` query extended through options | ||
| options: [ | ||
| { | ||
| customQueryNames: ['getByIcon'], | ||
| }, | ||
| ], | ||
| errors: [ | ||
| { | ||
| messageId: 'preferExplicitAssert', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
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.
Can you also check for when queries are not destructured?
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.
Nice spot! This actually found some issues in the rule, so I'm adding those tests and fixing the rule implementation.
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.
Tests added, rule fixed and more examples added to the doc