From 62ed216384b12298581a2adbe04196e9c9361534 Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 09:13:40 +0200 Subject: [PATCH 1/8] feat(rule): prefer expect queryBy --- README.md | 15 ++-- docs/rules/prefer-expect-query-by.md | 33 ++++++++ lib/index.js | 2 + lib/rules/prefer-expect-query-by.js | 99 +++++++++++++++++++++++ package-lock.json | 43 +++------- tests/__snapshots__/index.test.js.snap | 4 + tests/lib/rules/prefer-expect-query-by.js | 37 +++++++++ 7 files changed, 195 insertions(+), 38 deletions(-) create mode 100644 docs/rules/prefer-expect-query-by.md create mode 100644 lib/rules/prefer-expect-query-by.js create mode 100644 tests/lib/rules/prefer-expect-query-by.js diff --git a/README.md b/README.md index 7692a4fb..6456a55c 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*)` | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-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..8a56c539 --- /dev/null +++ b/docs/rules/prefer-expect-query-by.md @@ -0,0 +1,33 @@ +# Disallow the use of `expect(getBy*)` (prefer-expect-query-by) + +The (DOM) Testing Library support two 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 `expect(queryByText("Foo")).not.toBeInTheDocument()`. + +## Rule details + +This rule gives a notification whenever `expect(getBy*)` is used. + +This rule is enabled by default. + +### Default configuration + +The following patterns is considered erroneous: + +```js +test('some test', () => { + expect(getByText('Foo')).not.toBeInTheDocument(); +}); +``` + +The following pattern is considered non erroneous: + +```js +test('some test', async () => { + expect(queryByText('Foo')).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..44932c2f --- /dev/null +++ b/lib/rules/prefer-expect-query-by.js @@ -0,0 +1,99 @@ +'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: 'code', + }, + + create: context => ({ + CallExpression(node) { + if (hasExpectWithWrongGetByQuery(node)) { + const nodesGetBy = mapNodesForWrongGetByQuery(node); + context.report({ + node: node.callee, + messageId: 'expectQueryBy', + 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..04f17cfc 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": { @@ -3932,8 +3932,7 @@ "ansi-regex": { "version": "2.1.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "aproba": { "version": "1.2.0", @@ -3954,14 +3953,12 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -3976,20 +3973,17 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "core-util-is": { "version": "1.0.2", @@ -4106,8 +4100,7 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "ini": { "version": "1.3.5", @@ -4119,7 +4112,6 @@ "version": "1.0.0", "bundled": true, "dev": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -4134,7 +4126,6 @@ "version": "3.0.4", "bundled": true, "dev": true, - "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -4142,14 +4133,12 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "minipass": { "version": "2.3.5", "bundled": true, "dev": true, - "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -4168,7 +4157,6 @@ "version": "0.5.1", "bundled": true, "dev": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -4249,8 +4237,7 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "object-assign": { "version": "4.1.1", @@ -4262,7 +4249,6 @@ "version": "1.4.0", "bundled": true, "dev": true, - "optional": true, "requires": { "wrappy": "1" } @@ -4348,8 +4334,7 @@ "safe-buffer": { "version": "5.1.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "safer-buffer": { "version": "2.1.2", @@ -4385,7 +4370,6 @@ "version": "1.0.2", "bundled": true, "dev": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -4405,7 +4389,6 @@ "version": "3.0.1", "bundled": true, "dev": true, - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -4449,14 +4432,12 @@ "wrappy": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "yallist": { "version": "3.0.3", "bundled": true, - "dev": true, - "optional": true + "dev": true } } }, 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..bdf58f5a --- /dev/null +++ b/tests/lib/rules/prefer-expect-query-by.js @@ -0,0 +1,37 @@ +'use strict'; + +const RuleTester = require('eslint').RuleTester; +const rule = require('../../../lib/rules/prefer-expect-query-by'); + +const ruleTester = new RuleTester({ + parserOptions: { ecmaVersion: 2015, sourceType: 'module' }, +}); + +ruleTester.run('prefer-expect-query-by', rule, { + valid: [ + { code: "expect(queryByText('Hello')).not.toBeInTheDocument()" }, + { code: "expect(rendered.queryByText('Hello')).not.toBeInTheDocument()" }, + { code: "expect(queryAllByText('Hello')).not.toBeInTheDocument()" }, + { + code: "expect(rendered.queryAllByText('Hello')).not.toBeInTheDocument()", + }, + ], + invalid: [ + { + code: "expect(getByText('Hello')).not.toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + }, + { + code: "expect(rendered.getByText('Hello')).not.toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + }, + { + code: "expect(getAllByText('Hello')).not.toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + }, + { + code: "expect(rendered.getAllByText('Hello')).not.toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + }, + ], +}); From 353cc77250db0d25e540e1c4eed9f1a54daa355e Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 10:57:51 +0200 Subject: [PATCH 2/8] chore: regenerate lockfile --- package-lock.json | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 04f17cfc..f754867f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3932,7 +3932,8 @@ "ansi-regex": { "version": "2.1.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "aproba": { "version": "1.2.0", @@ -3953,12 +3954,14 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, + "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -3973,17 +3976,20 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "core-util-is": { "version": "1.0.2", @@ -4100,7 +4106,8 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "ini": { "version": "1.3.5", @@ -4112,6 +4119,7 @@ "version": "1.0.0", "bundled": true, "dev": true, + "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -4126,6 +4134,7 @@ "version": "3.0.4", "bundled": true, "dev": true, + "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -4133,12 +4142,14 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "minipass": { "version": "2.3.5", "bundled": true, "dev": true, + "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -4157,6 +4168,7 @@ "version": "0.5.1", "bundled": true, "dev": true, + "optional": true, "requires": { "minimist": "0.0.8" } @@ -4237,7 +4249,8 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "object-assign": { "version": "4.1.1", @@ -4249,6 +4262,7 @@ "version": "1.4.0", "bundled": true, "dev": true, + "optional": true, "requires": { "wrappy": "1" } @@ -4334,7 +4348,8 @@ "safe-buffer": { "version": "5.1.2", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "safer-buffer": { "version": "2.1.2", @@ -4370,6 +4385,7 @@ "version": "1.0.2", "bundled": true, "dev": true, + "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -4389,6 +4405,7 @@ "version": "3.0.1", "bundled": true, "dev": true, + "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -4432,12 +4449,14 @@ "wrappy": { "version": "1.0.2", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "yallist": { "version": "3.0.3", "bundled": true, - "dev": true + "dev": true, + "optional": true } } }, From 093b44b7ad28e1b548d5e4ef7e080ece3ca73d3b Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 11:22:15 +0200 Subject: [PATCH 3/8] refactor: improve docs and tests, include also findBy --- README.md | 2 +- docs/rules/prefer-expect-query-by.md | 44 +++++++++++++++---- lib/rules/prefer-expect-query-by.js | 12 +++++- tests/lib/rules/prefer-expect-query-by.js | 51 +++++++++++++++++++++++ 4 files changed, 98 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 6456a55c..f3a78cd9 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,7 @@ To enable this configuration use the `extends` property in your | [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*)` | ![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][] | ![fixable-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 index 8a56c539..042c8692 100644 --- a/docs/rules/prefer-expect-query-by.md +++ b/docs/rules/prefer-expect-query-by.md @@ -1,29 +1,57 @@ # Disallow the use of `expect(getBy*)` (prefer-expect-query-by) -The (DOM) Testing Library support two 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 `expect(queryByText("Foo")).not.toBeInTheDocument()`. +The (DOM) Testing Library support three types of queries: `getBy*`, `findBy*` and `queryBy*`. Using `getBy*` or `findBy*` 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*` or `findBy*` 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*`, `findAll*` and `queryAll*` queries. ## Rule details -This rule gives a notification whenever `expect(getBy*)` is used. +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. -### Default configuration - -The following patterns is considered erroneous: +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(); }); ``` -The following pattern is considered non erroneous: +Examples of **correct** code for this rule: ```js -test('some test', async () => { +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(); }); ``` diff --git a/lib/rules/prefer-expect-query-by.js b/lib/rules/prefer-expect-query-by.js index 44932c2f..3e455851 100644 --- a/lib/rules/prefer-expect-query-by.js +++ b/lib/rules/prefer-expect-query-by.js @@ -16,7 +16,12 @@ function isMemberExpression(node) { } function isUsingWrongQueries(node) { - return node.name.startsWith('getBy') || node.name.startsWith('getAllBy'); + return ( + node.name.startsWith('getBy') || + node.name.startsWith('getAllBy') || + node.name.startsWith('findBy') || + node.name.startsWith('findAllBy') + ); } function isNotNullOrUndefined(input) { @@ -89,7 +94,10 @@ module.exports = { fix(fixer) { return fixer.replaceText( nodesGetBy[0], - nodesGetBy[0].name.replace(/^(get(All)?(.*))$/, 'query$2$3') + nodesGetBy[0].name.replace( + /^((get|find)(All)?(.*))$/, + 'query$3$4' + ) ); }, }); diff --git a/tests/lib/rules/prefer-expect-query-by.js b/tests/lib/rules/prefer-expect-query-by.js index bdf58f5a..b9742285 100644 --- a/tests/lib/rules/prefer-expect-query-by.js +++ b/tests/lib/rules/prefer-expect-query-by.js @@ -15,23 +15,74 @@ ruleTester.run('prefer-expect-query-by', rule, { { code: "expect(rendered.queryAllByText('Hello')).not.toBeInTheDocument()", }, + { code: "expect(queryByText('Hello')).toBeInTheDocument()" }, + { code: "expect(rendered.queryByText('Hello')).toBeInTheDocument()" }, + { code: "expect(queryAllByText('Hello')).toBeInTheDocument()" }, + { + code: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", + }, ], invalid: [ { code: "expect(getByText('Hello')).not.toBeInTheDocument()", errors: [{ messageId: 'expectQueryBy' }], + output: "expect(queryByText('Hello')).not.toBeInTheDocument()", }, { code: "expect(rendered.getByText('Hello')).not.toBeInTheDocument()", errors: [{ messageId: 'expectQueryBy' }], + output: "expect(rendered.queryByText('Hello')).not.toBeInTheDocument()", }, { code: "expect(getAllByText('Hello')).not.toBeInTheDocument()", errors: [{ messageId: 'expectQueryBy' }], + output: "expect(queryAllByText('Hello')).not.toBeInTheDocument()", }, { code: "expect(rendered.getAllByText('Hello')).not.toBeInTheDocument()", errors: [{ messageId: 'expectQueryBy' }], + output: + "expect(rendered.queryAllByText('Hello')).not.toBeInTheDocument()", + }, + { + code: "expect(getByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(queryByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(rendered.getByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(rendered.queryByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(getAllByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(queryAllByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(rendered.getAllByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(findByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(queryByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(rendered.findByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(rendered.queryByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(findAllByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(queryAllByText('Hello')).toBeInTheDocument()", + }, + { + code: "expect(rendered.findAllByText('Hello')).toBeInTheDocument()", + errors: [{ messageId: 'expectQueryBy' }], + output: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", }, ], }); From e3e4231e2f5fc16b21e003dff56862fcee8cca9c Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 12:46:09 +0200 Subject: [PATCH 4/8] refactor: remove occurrences of findBy --- docs/rules/prefer-expect-query-by.md | 6 +++--- lib/rules/prefer-expect-query-by.js | 12 ++---------- tests/lib/rules/prefer-expect-query-by.js | 20 -------------------- 3 files changed, 5 insertions(+), 33 deletions(-) diff --git a/docs/rules/prefer-expect-query-by.md b/docs/rules/prefer-expect-query-by.md index 042c8692..cf4b6649 100644 --- a/docs/rules/prefer-expect-query-by.md +++ b/docs/rules/prefer-expect-query-by.md @@ -1,9 +1,9 @@ # Disallow the use of `expect(getBy*)` (prefer-expect-query-by) -The (DOM) Testing Library support three types of queries: `getBy*`, `findBy*` and `queryBy*`. Using `getBy*` or `findBy*` 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*` or `findBy*` 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 (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*`, `findAll*` and `queryAll*` queries. +> The same applies for the `getAll*` and `queryAll*` queries. ## Rule details diff --git a/lib/rules/prefer-expect-query-by.js b/lib/rules/prefer-expect-query-by.js index 3e455851..44932c2f 100644 --- a/lib/rules/prefer-expect-query-by.js +++ b/lib/rules/prefer-expect-query-by.js @@ -16,12 +16,7 @@ function isMemberExpression(node) { } function isUsingWrongQueries(node) { - return ( - node.name.startsWith('getBy') || - node.name.startsWith('getAllBy') || - node.name.startsWith('findBy') || - node.name.startsWith('findAllBy') - ); + return node.name.startsWith('getBy') || node.name.startsWith('getAllBy'); } function isNotNullOrUndefined(input) { @@ -94,10 +89,7 @@ module.exports = { fix(fixer) { return fixer.replaceText( nodesGetBy[0], - nodesGetBy[0].name.replace( - /^((get|find)(All)?(.*))$/, - 'query$3$4' - ) + nodesGetBy[0].name.replace(/^(get(All)?(.*))$/, 'query$2$3') ); }, }); diff --git a/tests/lib/rules/prefer-expect-query-by.js b/tests/lib/rules/prefer-expect-query-by.js index b9742285..c0f5f566 100644 --- a/tests/lib/rules/prefer-expect-query-by.js +++ b/tests/lib/rules/prefer-expect-query-by.js @@ -64,25 +64,5 @@ ruleTester.run('prefer-expect-query-by', rule, { errors: [{ messageId: 'expectQueryBy' }], output: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", }, - { - code: "expect(findByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(queryByText('Hello')).toBeInTheDocument()", - }, - { - code: "expect(rendered.findByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(rendered.queryByText('Hello')).toBeInTheDocument()", - }, - { - code: "expect(findAllByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(queryAllByText('Hello')).toBeInTheDocument()", - }, - { - code: "expect(rendered.findAllByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", - }, ], }); From 53a950c8716b6c63a37646d6cedb7c2a7929586c Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 13:16:20 +0200 Subject: [PATCH 5/8] refactor: generate test cases for all query methods --- tests/lib/rules/prefer-expect-query-by.js | 108 ++++++++++------------ 1 file changed, 51 insertions(+), 57 deletions(-) diff --git a/tests/lib/rules/prefer-expect-query-by.js b/tests/lib/rules/prefer-expect-query-by.js index c0f5f566..6cf1c11f 100644 --- a/tests/lib/rules/prefer-expect-query-by.js +++ b/tests/lib/rules/prefer-expect-query-by.js @@ -2,67 +2,61 @@ 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' }, }); -ruleTester.run('prefer-expect-query-by', rule, { - valid: [ - { code: "expect(queryByText('Hello')).not.toBeInTheDocument()" }, - { code: "expect(rendered.queryByText('Hello')).not.toBeInTheDocument()" }, - { code: "expect(queryAllByText('Hello')).not.toBeInTheDocument()" }, - { - code: "expect(rendered.queryAllByText('Hello')).not.toBeInTheDocument()", - }, - { code: "expect(queryByText('Hello')).toBeInTheDocument()" }, - { code: "expect(rendered.queryByText('Hello')).toBeInTheDocument()" }, - { code: "expect(queryAllByText('Hello')).toBeInTheDocument()" }, - { - code: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", - }, - ], - invalid: [ - { - code: "expect(getByText('Hello')).not.toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(queryByText('Hello')).not.toBeInTheDocument()", - }, - { - code: "expect(rendered.getByText('Hello')).not.toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(rendered.queryByText('Hello')).not.toBeInTheDocument()", - }, - { - code: "expect(getAllByText('Hello')).not.toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(queryAllByText('Hello')).not.toBeInTheDocument()", - }, - { - code: "expect(rendered.getAllByText('Hello')).not.toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: - "expect(rendered.queryAllByText('Hello')).not.toBeInTheDocument()", - }, - { - code: "expect(getByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(queryByText('Hello')).toBeInTheDocument()", - }, - { - code: "expect(rendered.getByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(rendered.queryByText('Hello')).toBeInTheDocument()", - }, - { - code: "expect(getAllByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(queryAllByText('Hello')).toBeInTheDocument()", - }, - { - code: "expect(rendered.getAllByText('Hello')).toBeInTheDocument()", - errors: [{ messageId: 'expectQueryBy' }], - output: "expect(rendered.queryAllByText('Hello')).toBeInTheDocument()", - }, +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) => { + const fixedQueryName = queryName.replace('get', 'query'); + return [ + ...invalidRules, + { + code: `expect(${queryName}('Hello')).toBeInTheDocument()`, + errors: [{ messageId: 'expectQueryBy' }], + output: `expect(${fixedQueryName}('Hello')).toBeInTheDocument()`, + }, + { + code: `expect(rendered.${queryName}('Hello')).toBeInTheDocument()`, + errors: [{ messageId: 'expectQueryBy' }], + output: `expect(rendered.${fixedQueryName}('Hello')).toBeInTheDocument()`, + }, + { + code: `expect(${queryName}('Hello')).not.toBeInTheDocument()`, + errors: [{ messageId: 'expectQueryBy' }], + output: `expect(${fixedQueryName}('Hello')).not.toBeInTheDocument()`, + }, + { + code: `expect(rendered.${queryName}('Hello')).not.toBeInTheDocument()`, + errors: [{ messageId: 'expectQueryBy' }], + output: `expect(rendered.${fixedQueryName}('Hello')).not.toBeInTheDocument()`, + }, + ]; + }, []), }); From 0fd4a6c3e6c968893028c4ba8eee938f45008ffb Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 13:23:25 +0200 Subject: [PATCH 6/8] refactor: disable autofix implementation --- lib/rules/prefer-expect-query-by.js | 17 ++++++++++------- tests/lib/rules/prefer-expect-query-by.js | 14 +++++--------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/rules/prefer-expect-query-by.js b/lib/rules/prefer-expect-query-by.js index 44932c2f..03a57bdc 100644 --- a/lib/rules/prefer-expect-query-by.js +++ b/lib/rules/prefer-expect-query-by.js @@ -82,16 +82,19 @@ module.exports = { create: context => ({ CallExpression(node) { if (hasExpectWithWrongGetByQuery(node)) { - const nodesGetBy = mapNodesForWrongGetByQuery(node); + // const nodesGetBy = mapNodesForWrongGetByQuery(node); context.report({ node: node.callee, messageId: 'expectQueryBy', - fix(fixer) { - return fixer.replaceText( - nodesGetBy[0], - nodesGetBy[0].name.replace(/^(get(All)?(.*))$/, 'query$2$3') - ); - }, + // 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/tests/lib/rules/prefer-expect-query-by.js b/tests/lib/rules/prefer-expect-query-by.js index 6cf1c11f..3972bd10 100644 --- a/tests/lib/rules/prefer-expect-query-by.js +++ b/tests/lib/rules/prefer-expect-query-by.js @@ -33,30 +33,26 @@ ruleTester.run('prefer-expect-query-by', rule, { ], [] ), - invalid: getByVariants.reduce((invalidRules, queryName) => { - const fixedQueryName = queryName.replace('get', 'query'); - return [ + invalid: getByVariants.reduce( + (invalidRules, queryName) => [ ...invalidRules, { code: `expect(${queryName}('Hello')).toBeInTheDocument()`, errors: [{ messageId: 'expectQueryBy' }], - output: `expect(${fixedQueryName}('Hello')).toBeInTheDocument()`, }, { code: `expect(rendered.${queryName}('Hello')).toBeInTheDocument()`, errors: [{ messageId: 'expectQueryBy' }], - output: `expect(rendered.${fixedQueryName}('Hello')).toBeInTheDocument()`, }, { code: `expect(${queryName}('Hello')).not.toBeInTheDocument()`, errors: [{ messageId: 'expectQueryBy' }], - output: `expect(${fixedQueryName}('Hello')).not.toBeInTheDocument()`, }, { code: `expect(rendered.${queryName}('Hello')).not.toBeInTheDocument()`, errors: [{ messageId: 'expectQueryBy' }], - output: `expect(rendered.${fixedQueryName}('Hello')).not.toBeInTheDocument()`, }, - ]; - }, []), + ], + [] + ), }); From 6a3f264e69a2a9c71893ba5c2eff2fc6af7b8c4b Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 13:24:21 +0200 Subject: [PATCH 7/8] refactor: remove fixable badge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index f3a78cd9..c992ff8b 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,7 @@ To enable this configuration use the `extends` property in your | [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][] | ![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 From 67d305c275655bc3efe911df9361c34cd8f71ede Mon Sep 17 00:00:00 2001 From: Nicola Molinari Date: Wed, 16 Oct 2019 14:39:37 +0200 Subject: [PATCH 8/8] refactor: fixable option to null --- lib/rules/prefer-expect-query-by.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/prefer-expect-query-by.js b/lib/rules/prefer-expect-query-by.js index 03a57bdc..9f7acb5a 100644 --- a/lib/rules/prefer-expect-query-by.js +++ b/lib/rules/prefer-expect-query-by.js @@ -76,7 +76,7 @@ module.exports = { }, schema: [], type: 'suggestion', - fixable: 'code', + fixable: null, }, create: context => ({