diff --git a/README.md b/README.md index 7692a4fb..c992ff8b 100644 --- a/README.md +++ b/README.md @@ -128,13 +128,14 @@ 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-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][] | +| 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-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][] | | [build-badge]: https://img.shields.io/travis/Belco90/eslint-plugin-testing-library?style=flat-square [build-url]: https://travis-ci.org/belco90/eslint-plugin-testing-library diff --git a/docs/rules/prefer-expect-query-by.md b/docs/rules/prefer-expect-query-by.md new file mode 100644 index 00000000..cf4b6649 --- /dev/null +++ b/docs/rules/prefer-expect-query-by.md @@ -0,0 +1,61 @@ +# Disallow the use of `expect(getBy*)` (prefer-expect-query-by) + +The (DOM) Testing Library support three types of queries: `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. +However, when trying to assert if an element is not in the document, 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. + +## 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. + +This rule is enabled by default. + +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', () => { + const rendered = render(); + expect(rendered.getByText('Foo')).toBeInTheDocument(); + expect(rendered.getAllByText('Foo')[0]).toBeInTheDocument(); + expect(rendered.getByText('Foo')).not.toBeInTheDocument(); + expect(rendered.getAllByText('Foo')[0]).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', () => { + const rendered = render(); + expect(rendered.queryByText('Foo')).toBeInTheDocument(); + expect(rendered.queryAllByText('Foo')[0]).toBeInTheDocument(); + expect(rendered.queryByText('Foo')).not.toBeInTheDocument(); + expect(rendered.queryAllByText('Foo')[0]).not.toBeInTheDocument(); +}); +``` + +## Further Reading + +- [Appearance and Disappearance](https://testing-library.com/docs/guide-disappearance#asserting-elements-are-not-present) +- [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 28048165..faaf6432 100644 --- a/lib/index.js +++ b/lib/index.js @@ -6,11 +6,13 @@ 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'), }; const recommendedRules = { 'testing-library/await-async-query': 'error', 'testing-library/no-await-sync-query': 'error', + 'testing-library/prefer-expect-query-by': 'error', }; module.exports = { diff --git a/lib/rules/prefer-expect-query-by.js b/lib/rules/prefer-expect-query-by.js new file mode 100644 index 00000000..9f7acb5a --- /dev/null +++ b/lib/rules/prefer-expect-query-by.js @@ -0,0 +1,102 @@ +'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/Belco90/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/package-lock.json b/package-lock.json index 6b03a783..f754867f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "eslint-plugin-testing-library", - "version": "0.0.0-development", + "version": "0.0.0-semantically-released", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/tests/__snapshots__/index.test.js.snap b/tests/__snapshots__/index.test.js.snap index b83bb366..bf37617b 100644 --- a/tests/__snapshots__/index.test.js.snap +++ b/tests/__snapshots__/index.test.js.snap @@ -13,6 +13,7 @@ Object { "error", "angular", ], + "testing-library/prefer-expect-query-by": "error", }, } `; @@ -30,6 +31,7 @@ Object { "error", "react", ], + "testing-library/prefer-expect-query-by": "error", }, } `; @@ -42,6 +44,7 @@ Object { "rules": Object { "testing-library/await-async-query": "error", "testing-library/no-await-sync-query": "error", + "testing-library/prefer-expect-query-by": "error", }, } `; @@ -60,6 +63,7 @@ Object { "error", "vue", ], + "testing-library/prefer-expect-query-by": "error", }, } `; diff --git a/tests/lib/rules/prefer-expect-query-by.js b/tests/lib/rules/prefer-expect-query-by.js new file mode 100644 index 00000000..3972bd10 --- /dev/null +++ b/tests/lib/rules/prefer-expect-query-by.js @@ -0,0 +1,58 @@ +'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(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' }], + }, + ], + [] + ), +});