feat: add prefer-explicit-assert rule #29
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| # Disallow `getBy*` queries as assertion method itself (no-get-by-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. | ||
|
|
||
| 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 using the returned element | ||
| 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(); | ||
|
Collaborator
There was a problem hiding this comment. Shouldn't it be
Member
Author
There was a problem hiding this comment. That's actually from a different rule so I would avoid having this one encouraging two things: asserting explicitly and asserting with
Collaborator
There was a problem hiding this comment. Yes, but as
Member
Author
There was a problem hiding this comment. Well, in the example of this specific rule that's fine. It would be weird to see
Collaborator
There was a problem hiding this comment. Sure! What about putting a side note stating that though
Member
Author
There was a problem hiding this comment. That sounds absolutely reasonable. I'll address the feedback here later.
Contributor
There was a problem hiding this comment. My 2 cents: I understand both points. However, since this is a documentation to something that is to be considered "valid", I would expect that we write things that are "valid", which is not really the case with At least we should mention to wrap it into an |
||
|
|
||
| // even more explicit if you use `@testing-library/jest-dom` matcher | ||
| // for checking the element is present in the document | ||
| expect(getByText('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'); | ||
| ``` | ||
|
|
||
| ## 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. | ||
|
|
||
| ## Further Reading | ||
|
|
||
| - [getBy query](https://testing-library.com/docs/dom-testing-library/api-queries#getby) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| 'use strict'; | ||
|
|
||
| const { getDocsUrl } = require('../utils'); | ||
|
|
||
| const isDirectlyCalledByFunction = node => | ||
| node.parent.parent.type === 'CallExpression'; | ||
|
|
||
| const isReturnedByArrowFunctionExpression = node => | ||
| node.parent.parent.type === 'ArrowFunctionExpression'; | ||
|
|
||
| const isDeclared = node => node.parent.parent.type === 'VariableDeclarator'; | ||
|
|
||
| const isReturnedByReturnStatement = node => | ||
| node.parent.parent.type === 'ReturnStatement'; | ||
|
|
||
| module.exports = { | ||
| meta: { | ||
| type: 'suggestion', | ||
| docs: { | ||
| description: 'Disallow `getBy*` queries as assertion method itself', | ||
| category: 'Best Practices', | ||
| recommended: false, | ||
| url: getDocsUrl('no-get-by-assert'), | ||
| }, | ||
| messages: { | ||
| noGetByAssert: 'Disallowed use of `getBy*` query as implicit assert', | ||
|
||
| }, | ||
| fixable: null, | ||
| schema: [], | ||
| }, | ||
|
|
||
| create: function(context) { | ||
| return { | ||
| [`CallExpression > Identifier[name=${/^getBy(LabelText|PlaceholderText|Text|AltText|Title|DisplayValue|Role|TestId)$/}]`]( | ||
|
||
| node | ||
| ) { | ||
| if ( | ||
| !isDirectlyCalledByFunction(node) && | ||
| !isReturnedByArrowFunctionExpression(node) && | ||
| !isDeclared(node) && | ||
| !isReturnedByReturnStatement(node) | ||
|
||
| ) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'noGetByAssert', | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| 'use strict'; | ||
|
|
||
| const rule = require('../../../lib/rules/no-get-by-assert'); | ||
| const { ALL_QUERIES_METHODS } = require('../../../lib/utils'); | ||
| const RuleTester = require('eslint').RuleTester; | ||
|
|
||
| // ------------------------------------------------------------------------------ | ||
| // Tests | ||
| // ------------------------------------------------------------------------------ | ||
|
|
||
| const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2018 } }); | ||
| ruleTester.run('no-get-by-assert', rule, { | ||
| valid: [ | ||
| { | ||
| code: `getByText`, | ||
| }, | ||
| { | ||
| code: `expect(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: `getByNonTestingLibraryVariant('foo')`, | ||
| }, | ||
| ], | ||
|
|
||
| invalid: [ | ||
| ...ALL_QUERIES_METHODS.map(queryMethod => ({ | ||
| code: `get${queryMethod}('foo')`, | ||
| errors: [ | ||
| { | ||
| messageId: 'noGetByAssert', | ||
| }, | ||
| ], | ||
| })), | ||
| ...ALL_QUERIES_METHODS.map(queryMethod => ({ | ||
| code: `() => { | ||
| get${queryMethod}('foo') | ||
| doSomething() | ||
|
|
||
| get${queryMethod}('bar') | ||
| const quxElement = get${queryMethod}('qux') | ||
| } | ||
| `, | ||
| errors: [ | ||
| { | ||
| messageId: 'noGetByAssert', | ||
| line: 2, | ||
| }, | ||
| { | ||
| messageId: 'noGetByAssert', | ||
| line: 5, | ||
| }, | ||
| ], | ||
| })), | ||
| ], | ||
| }); |
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.
The rule name sounds a bit strange to me. When it comes to preferences, ESLint rules are usually named
prefer-..., I think it should be the case here.no-get-by-assertimplies here that doing assertions withgetByqueries is wrong. I think something likeprefer-query-by-assertis more clear to the user as discussed in the issueThere 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'm happy to rename this to
prefer-*rule but I would avoid using query-by for same reason I commented before, so I would go for something likeprefer-explicit-assert. What do you think?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.
prefer-explicit-assertsounds good to me!