diff --git a/README.md b/README.md index fff1c2ff..37a86b54 100644 --- a/README.md +++ b/README.md @@ -134,17 +134,17 @@ To enable this configuration use the `extends` property in your ## Supported Rules -| Rule | Description | Configurations | Fixable | -| -------------------------------------------------------------- | ------------------------------------------------------------------- | ------------------------------------------------------------------------- | ------------------ | -| [await-async-query](docs/rules/await-async-query.md) | Enforce async queries to have proper `await` | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [await-async-utils](docs/rules/await-async-utils.md) | Enforce async utils to be awaited properly | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [await-fire-event](docs/rules/await-fire-event.md) | Enforce async fire event methods to be awaited | ![vue-badge][] | | -| [no-await-sync-query](docs/rules/no-await-sync-query.md) | Disallow unnecessary `await` for sync queries | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [no-debug](docs/rules/no-debug.md) | Disallow the use of `debug` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [no-dom-import](docs/rules/no-dom-import.md) | Disallow importing from DOM Testing Library | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | -| [prefer-expect-query-by](docs/rules/prefer-expect-query-by.md) | Disallow the use of `expect(getBy*)` | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | -| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | -| [consistent-data-testid](docs/rules/consistent-data-testid.md) | Ensure `data-testid` values match a provided regex. | | | +| Rule | Description | Configurations | Fixable | +| --------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------- | ------------------ | +| [await-async-query](docs/rules/await-async-query.md) | Enforce async queries to have proper `await` | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [await-async-utils](docs/rules/await-async-utils.md) | Enforce async utils to be awaited properly | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [await-fire-event](docs/rules/await-fire-event.md) | Enforce async fire event methods to be awaited | ![vue-badge][] | | +| [consistent-data-testid](docs/rules/consistent-data-testid.md) | Ensure `data-testid` values match a provided regex. | | | +| [no-await-sync-query](docs/rules/no-await-sync-query.md) | Disallow unnecessary `await` for sync queries | ![recommended-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [no-debug](docs/rules/no-debug.md) | Disallow the use of `debug` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | +| [no-dom-import](docs/rules/no-dom-import.md) | Disallow importing from DOM Testing Library | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | +| [no-get-by-for-checking-element-not-present](docs/rules/no-get-by-for-checking-element-not-present) | Disallow the use of `getBy*` queries when checking elements are not present | | | +| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | [build-badge]: https://img.shields.io/travis/testing-library/eslint-plugin-testing-library?style=flat-square [build-url]: https://travis-ci.org/testing-library/eslint-plugin-testing-library diff --git a/docs/rules/no-get-by-for-checking-element-not-present.md b/docs/rules/no-get-by-for-checking-element-not-present.md new file mode 100644 index 00000000..77576d8a --- /dev/null +++ b/docs/rules/no-get-by-for-checking-element-not-present.md @@ -0,0 +1,61 @@ +# Disallow the use of `getBy*` queries when checking elements are not present (no-get-by-for-checking-element-not-present) + +The (DOM) Testing Library allows to query DOM elements using different types of queries such as `getBy*` and `queryBy*`. Using `getBy*` throws an error in case the element is not found. This is useful when: + +- using method like `waitForElement`, which are `async` functions that will wait for the element to be found until a certain timeout, after that the test will fail. +- using `getBy` queries as an assert itself, so if the element is not found the error thrown will work as the check itself within the test. + +However, when asserting if an element is not present or waiting for disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can: + +- assert element does not exist: `expect(queryByText("Foo")).not.toBeInTheDocument()` +- wait for disappearance: `await waitForElementToBeRemoved(() => queryByText('the mummy'))` + +## Rule details + +This rule fires whenever: + +- `expect` is used to assert element does not exist with `.not.toBeInTheDocument()` or `.toBeNull()` matchers +- `waitForElementToBeRemoved` async util is used to wait for element to be removed from DOM + +Examples of **incorrect** code for this rule: + +```js +test('some test', () => { + const { getByText } = render(); + expect(getByText('Foo')).not.toBeInTheDocument(); + expect(getByText('Foo')).toBeFalsy(); + expect(getByText('Foo')).toBeNull(); +}); +``` + +```js +test('some test', async () => { + const utils = render(); + await waitForElementToBeRemoved(() => utils.getByText('Foo')); +}); +``` + +Examples of **correct** code for this rule: + +```js +test('some test', () => { + const { getByText } = render(); + expect(getByText('Foo')).toBeInTheDocument(); + expect(queryByText('Foo')).not.toBeInTheDocument(); + expect(queryByText('Foo')).toBeFalsy(); +}); +``` + +```js +test('some test', async () => { + const utils = render(); + await waitForElementToBeRemoved(() => utils.queryByText('Foo')); +}); +``` + +## Further Reading + +- [Asserting elements are not present](https://testing-library.com/docs/guide-disappearance#asserting-elements-are-not-present) +- [Waiting for disappearance](https://testing-library.com/docs/guide-disappearance#waiting-for-disappearance) +- [jest-dom note about using `getBy` within assertions](https://testing-library.com/docs/ecosystem-jest-dom) +- [Testing Library queries cheatsheet](https://testing-library.com/docs/dom-testing-library/cheatsheet#queries) diff --git a/docs/rules/prefer-expect-query-by.md b/docs/rules/prefer-expect-query-by.md deleted file mode 100644 index 9f81e0b6..00000000 --- a/docs/rules/prefer-expect-query-by.md +++ /dev/null @@ -1,81 +0,0 @@ -# Disallow the use of `expect(getBy*)` (prefer-expect-query-by) - -The (DOM) Testing Library allows to query DOM elements using different types of queries such as `getBy*` and `queryBy*`. Using `getBy*` throws an error in case the element is not found. This is useful when: - -- using method like `waitForElement`, which are `async` functions that will wait for the element to be found until a certain timeout, after that the test will fail. -- using `getBy` queries as an assert itself, so if the element is not found the error thrown will work as the check itself within the test. - -However, when trying to assert if an element is not present or disappearance, we can't use `getBy*` as the test will fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`. - -> The same applies for the `getAll*` and `queryAll*` queries too. - -## Rule details - -This rule gives a notification whenever `expect` is used with one of the query functions that throw an error if the element is not found. - -Examples of **incorrect** code for this rule: - -```js -test('some test', () => { - const { getByText, getAllByText } = render(); - expect(getByText('Foo')).toBeInTheDocument(); - expect(getAllByText('Foo')[0]).toBeInTheDocument(); - expect(getByText('Foo')).not.toBeInTheDocument(); - expect(getAllByText('Foo')[0]).not.toBeInTheDocument(); -}); -``` - -```js -test('some test', async () => { - const utils = render(); - await wait(() => { - expect(utils.getByText('Foo')).toBeInTheDocument(); - }); - await wait(() => { - expect(utils.getAllByText('Foo')).toBeInTheDocument(); - }); - await wait(() => { - expect(utils.getByText('Foo')).not.toBeInTheDocument(); - }); - await wait(() => { - expect(utils.getAllByText('Foo')).not.toBeInTheDocument(); - }); -}); -``` - -Examples of **correct** code for this rule: - -```js -test('some test', () => { - const { queryByText, queryAllByText } = render(); - expect(queryByText('Foo')).toBeInTheDocument(); - expect(queryAllByText('Foo')[0]).toBeInTheDocument(); - expect(queryByText('Foo')).not.toBeInTheDocument(); - expect(queryAllByText('Foo')[0]).not.toBeInTheDocument(); -}); -``` - -```js -test('some test', async () => { - const utils = render(); - await wait(() => { - expect(utils.queryByText('Foo')).toBeInTheDocument(); - }); - await wait(() => { - expect(utils.queryAllByText('Foo')).toBeInTheDocument(); - }); - await wait(() => { - expect(utils.queryByText('Foo')).not.toBeInTheDocument(); - }); - await wait(() => { - expect(utils.queryAllByText('Foo')).not.toBeInTheDocument(); - }); -}); -``` - -## Further Reading - -- [Asserting elements are not present](https://testing-library.com/docs/guide-disappearance#asserting-elements-are-not-present) -- [Waiting for disappearance](https://testing-library.com/docs/guide-disappearance#waiting-for-disappearance) -- [jest-dom note about using `getBy` within assertions](https://testing-library.com/docs/ecosystem-jest-dom) -- [Testing Library queries cheatsheet](https://testing-library.com/docs/dom-testing-library/cheatsheet#queries) diff --git a/lib/index.js b/lib/index.js index c8f17612..58102e71 100644 --- a/lib/index.js +++ b/lib/index.js @@ -8,7 +8,7 @@ const rules = { 'no-await-sync-query': require('./rules/no-await-sync-query'), 'no-debug': require('./rules/no-debug'), 'no-dom-import': require('./rules/no-dom-import'), - 'prefer-expect-query-by': require('./rules/prefer-expect-query-by'), + 'no-get-by-for-checking-element-not-present': require('./rules/no-get-by-for-checking-element-not-present'), 'prefer-explicit-assert': require('./rules/prefer-explicit-assert'), }; diff --git a/lib/rules/no-get-by-for-checking-element-not-present.js b/lib/rules/no-get-by-for-checking-element-not-present.js new file mode 100644 index 00000000..96597d54 --- /dev/null +++ b/lib/rules/no-get-by-for-checking-element-not-present.js @@ -0,0 +1,78 @@ +'use strict'; + +const { getDocsUrl } = require('../utils'); + +const falsyMatchers = ['toBeNull', 'toBeFalsy']; + +module.exports = { + meta: { + docs: { + category: 'Best Practices', + description: + 'Disallow the use of `getBy*` queries when checking elements are not present', + recommended: 'error', + url: getDocsUrl('no-get-by-for-checking-element-not-present'), + }, + messages: { + expectQueryBy: + 'Use `getBy*` only when checking elements are present, otherwise use `queryBy*`', + }, + schema: [], + type: 'suggestion', + fixable: null, + }, + + create: context => ({ + [`Identifier[name=/getBy|getAllBy/]`](node) { + const expectCallNode = findClosestCallNode(node, 'expect'); + + // expect(getByText("foo"))... + if (expectCallNode) { + const expectStatement = expectCallNode.parent; + const matcher = expectStatement.property.name; + + if (matcher === 'not') { + const negatedMatcher = expectStatement.parent.property.name; + + if (!falsyMatchers.includes(negatedMatcher)) { + return context.report({ + node, + messageId: 'expectQueryBy', + }); + } + } + + if (falsyMatchers.includes(matcher)) { + return context.report({ + node, + messageId: 'expectQueryBy', + }); + } + } + + const waitCallNode = findClosestCallNode( + node, + 'waitForElementToBeRemoved' + ); + + if (waitCallNode) { + return context.report({ + node, + messageId: 'expectQueryBy', + }); + } + }, + }), +}; + +function findClosestCallNode(node, name) { + if (!node.parent) { + return false; + } + + if (node.type === 'CallExpression' && node.callee.name === name) { + return node; + } else { + return findClosestCallNode(node.parent, name); + } +} diff --git a/lib/rules/prefer-expect-query-by.js b/lib/rules/prefer-expect-query-by.js deleted file mode 100644 index c1b14387..00000000 --- a/lib/rules/prefer-expect-query-by.js +++ /dev/null @@ -1,102 +0,0 @@ -'use strict'; - -const { getDocsUrl } = require('../utils'); - -const AST_NODE_TYPES = { - Identifier: 'Identifier', - MemberExpression: 'MemberExpression', -}; - -function isIdentifier(node) { - return node.type === AST_NODE_TYPES.Identifier; -} - -function isMemberExpression(node) { - return node.type === AST_NODE_TYPES.MemberExpression; -} - -function isUsingWrongQueries(node) { - return node.name.startsWith('getBy') || node.name.startsWith('getAllBy'); -} - -function isNotNullOrUndefined(input) { - return input != null; -} - -function mapNodesForWrongGetByQuery(node) { - const nodeArguments = node.arguments; - return nodeArguments - .map(arg => { - if (!arg.callee) { - return null; - } - // Example: `expect(rendered.getBy*)` - if (isMemberExpression(arg.callee)) { - const node = arg.callee.property; - if (isIdentifier(node) && isUsingWrongQueries(node)) { - return node; - } - return null; - } - - // Example: `expect(getBy*)` - if (isIdentifier(arg.callee) && isUsingWrongQueries(arg.callee)) { - return arg.callee; - } - - return null; - }) - .filter(isNotNullOrUndefined); -} - -function hasExpectWithWrongGetByQuery(node) { - if ( - node.callee && - node.callee.type === AST_NODE_TYPES.Identifier && - node.callee.name === 'expect' && - node.arguments - ) { - const nodesGetBy = mapNodesForWrongGetByQuery(node); - return nodesGetBy.length > 0; - } - return false; -} - -module.exports = { - meta: { - docs: { - category: 'Best Practices', - description: 'Disallow using getBy* queries in expect calls', - recommended: 'error', - url: getDocsUrl('prefer-expect-query-by'), - }, - messages: { - expectQueryBy: - 'Using `expect(getBy*)` is not recommended, use `expect(queryBy*)` instead.', - }, - schema: [], - type: 'suggestion', - fixable: null, - }, - - create: context => ({ - CallExpression(node) { - if (hasExpectWithWrongGetByQuery(node)) { - // const nodesGetBy = mapNodesForWrongGetByQuery(node); - context.report({ - node: node.callee, - messageId: 'expectQueryBy', - // TODO: we keep the autofixing disabled for now, until we figure out - // a better way to amend for the edge cases. - // See also the related discussion: https://github.com/testing-library/eslint-plugin-testing-library/pull/22#discussion_r335394402 - // fix(fixer) { - // return fixer.replaceText( - // nodesGetBy[0], - // nodesGetBy[0].name.replace(/^(get(All)?(.*))$/, 'query$2$3') - // ); - // }, - }); - } - }, - }), -}; diff --git a/tests/lib/rules/no-get-by-for-checking-element-not-present.js b/tests/lib/rules/no-get-by-for-checking-element-not-present.js new file mode 100644 index 00000000..36d6f66f --- /dev/null +++ b/tests/lib/rules/no-get-by-for-checking-element-not-present.js @@ -0,0 +1,106 @@ +'use strict'; + +const RuleTester = require('eslint').RuleTester; +const rule = require('../../../lib/rules/no-get-by-for-checking-element-not-present'); +const { ALL_QUERIES_METHODS } = require('../../../lib/utils'); + +const ruleTester = new RuleTester({ + parserOptions: { ecmaVersion: 2017, sourceType: 'module' }, +}); + +const getByQueries = ALL_QUERIES_METHODS.map(method => `get${method}`); +const queryByQueries = ALL_QUERIES_METHODS.map(method => `query${method}`); + +const allQueryUseInAssertion = queryName => [ + queryName, + `rendered.${queryName}`, +]; + +const getValidAssertion = (query, matcher) => + allQueryUseInAssertion(query).map(query => ({ + code: `expect(${query}('Hello'))${matcher}`, + })); + +const getInvalidAssertion = (query, matcher) => + allQueryUseInAssertion(query).map(query => ({ + code: `expect(${query}('Hello'))${matcher}`, + errors: [{ messageId: 'expectQueryBy' }], + })); + +ruleTester.run('prefer-expect-query-by', rule, { + valid: [ + ...getByQueries.reduce( + (validRules, queryName) => [ + ...validRules, + ...getValidAssertion(queryName, '.toBeInTheDocument()'), + ...getValidAssertion(queryName, '.toBe("foo")'), + ...getValidAssertion(queryName, '.toBeTruthy()'), + ...getValidAssertion(queryName, '.toEqual("World")'), + ...getValidAssertion(queryName, '.not.toBeFalsy()'), + ...getValidAssertion(queryName, '.not.toBeNull()'), + ], + [] + ), + ...queryByQueries.reduce( + (validRules, queryName) => [ + ...validRules, + ...getValidAssertion(queryName, '.not.toBeInTheDocument()'), + ...getValidAssertion(queryName, '.toBeNull()'), + ...getValidAssertion(queryName, '.not.toBeTruthy()'), + ...getValidAssertion(queryName, '.toBeFalsy()'), + { + code: `(async () => { + await waitForElementToBeRemoved(() => { + return ${queryName}("hello") + }) + })()`, + }, + { + code: `(async () => { + await waitForElementToBeRemoved(() => ${queryName}("hello")) + })()`, + }, + { + code: `(async () => { + await waitForElementToBeRemoved(function() { + return ${queryName}("hello") + }) + })()`, + }, + ], + [] + ), + ], + invalid: getByQueries.reduce( + (invalidRules, queryName) => [ + ...invalidRules, + ...getInvalidAssertion(queryName, '.not.toBeInTheDocument()'), + ...getInvalidAssertion(queryName, '.toBeNull()'), + ...getInvalidAssertion(queryName, '.not.toBeTruthy()'), + ...getInvalidAssertion(queryName, '.toBeFalsy()'), + { + code: `(async () => { + await waitForElementToBeRemoved(() => { + return ${queryName}("hello") + }) + })()`, + errors: [{ messageId: 'expectQueryBy' }], + }, + { + code: `(async () => { + await waitForElementToBeRemoved(() => ${queryName}("hello")) + })()`, + errors: [{ messageId: 'expectQueryBy' }], + }, + { + code: `(async () => { + await waitForElementToBeRemoved(function() { + return ${queryName}("hello") + }) + })()`, + errors: [{ messageId: 'expectQueryBy' }], + }, + ], + [] + ), +}); diff --git a/tests/lib/rules/prefer-expect-query-by.js b/tests/lib/rules/prefer-expect-query-by.js deleted file mode 100644 index 7d99706e..00000000 --- a/tests/lib/rules/prefer-expect-query-by.js +++ /dev/null @@ -1,59 +0,0 @@ -'use strict'; - -const RuleTester = require('eslint').RuleTester; -const rule = require('../../../lib/rules/prefer-expect-query-by'); -const { ALL_QUERIES_METHODS } = require('../../../lib/utils'); - -const ruleTester = new RuleTester({ - parserOptions: { ecmaVersion: 2015, sourceType: 'module' }, -}); - -const queryByVariants = ALL_QUERIES_METHODS.reduce( - (variants, method) => [ - ...variants, - ...[`query${method}`, `queryAll${method}`], - ], - [] -); -const getByVariants = ALL_QUERIES_METHODS.reduce( - (variants, method) => [...variants, ...[`get${method}`, `getAll${method}`]], - [] -); - -ruleTester.run('prefer-expect-query-by', rule, { - valid: queryByVariants.reduce( - (validRules, queryName) => [ - ...validRules, - { code: `expect(${queryName}('Hello')).toBeInTheDocument()` }, - { code: `expect(${queryName}).toBeInTheDocument()` }, - { code: `expect(rendered.${queryName}('Hello')).toBeInTheDocument()` }, - { code: `expect(${queryName}('Hello')).not.toBeInTheDocument()` }, - { - code: `expect(rendered.${queryName}('Hello')).not.toBeInTheDocument()`, - }, - ], - [] - ), - invalid: getByVariants.reduce( - (invalidRules, queryName) => [ - ...invalidRules, - { - code: `expect(${queryName}('Hello')).toBeInTheDocument()`, - errors: [{ messageId: 'expectQueryBy' }], - }, - { - code: `expect(rendered.${queryName}('Hello')).toBeInTheDocument()`, - errors: [{ messageId: 'expectQueryBy' }], - }, - { - code: `expect(${queryName}('Hello')).not.toBeInTheDocument()`, - errors: [{ messageId: 'expectQueryBy' }], - }, - { - code: `expect(rendered.${queryName}('Hello')).not.toBeInTheDocument()`, - errors: [{ messageId: 'expectQueryBy' }], - }, - ], - [] - ), -});