From a7ee93c71c97310dc078470aa3bbd4fb60e9d514 Mon Sep 17 00:00:00 2001 From: Yuanting Jin Date: Sat, 5 Aug 2023 09:49:43 +0800 Subject: [PATCH] fix(prefer-in-document): false positive on .toHaveLength(1) matcher with *AllBy* query (#311) Co-authored-by: Doma Co-authored-by: Doma Co-authored-by: Gareth Jones --- README.md | 31 +-- docs/rules/prefer-in-document.md | 5 +- src/__tests__/lib/rules/prefer-in-document.js | 253 +++++++++--------- src/rules/prefer-in-document.js | 118 ++++++-- 4 files changed, 246 insertions(+), 161 deletions(-) diff --git a/README.md b/README.md index 92c953a..e52e6ef 100644 --- a/README.md +++ b/README.md @@ -96,21 +96,22 @@ module.exports = { πŸ’Ό Configurations enabled in.\ βœ… Set in the `recommended` configuration.\ -πŸ”§ Automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/user-guide/command-line-interface#--fix). - -| NameΒ Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β  | Description | πŸ’Ό | πŸ”§ | -| :----------------------------------------------------------------------- | :-------------------------------------------------------------------- | :- | :- | -| [prefer-checked](docs/rules/prefer-checked.md) | prefer toBeChecked over checking attributes | βœ… | πŸ”§ | -| [prefer-empty](docs/rules/prefer-empty.md) | Prefer toBeEmpty over checking innerHTML | βœ… | πŸ”§ | -| [prefer-enabled-disabled](docs/rules/prefer-enabled-disabled.md) | prefer toBeDisabled or toBeEnabled over checking attributes | βœ… | πŸ”§ | -| [prefer-focus](docs/rules/prefer-focus.md) | prefer toHaveFocus over checking document.activeElement | βœ… | πŸ”§ | -| [prefer-in-document](docs/rules/prefer-in-document.md) | Prefer .toBeInTheDocument() for asserting the existence of a DOM node | βœ… | πŸ”§ | -| [prefer-required](docs/rules/prefer-required.md) | prefer toBeRequired over checking properties | βœ… | πŸ”§ | -| [prefer-to-have-attribute](docs/rules/prefer-to-have-attribute.md) | prefer toHaveAttribute over checking getAttribute/hasAttribute | βœ… | πŸ”§ | -| [prefer-to-have-class](docs/rules/prefer-to-have-class.md) | prefer toHaveClass over checking element className | βœ… | πŸ”§ | -| [prefer-to-have-style](docs/rules/prefer-to-have-style.md) | prefer toHaveStyle over checking element style | βœ… | πŸ”§ | -| [prefer-to-have-text-content](docs/rules/prefer-to-have-text-content.md) | Prefer toHaveTextContent over checking element.textContent | βœ… | πŸ”§ | -| [prefer-to-have-value](docs/rules/prefer-to-have-value.md) | prefer toHaveValue over checking element.value | βœ… | πŸ”§ | +πŸ”§ Automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/user-guide/command-line-interface#--fix).\ +πŸ’‘ Manually fixable by [editor suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions). + +| NameΒ Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β Β  | Description | πŸ’Ό | πŸ”§ | πŸ’‘ | +| :----------------------------------------------------------------------- | :-------------------------------------------------------------------- | :- | :- | :- | +| [prefer-checked](docs/rules/prefer-checked.md) | prefer toBeChecked over checking attributes | βœ… | πŸ”§ | | +| [prefer-empty](docs/rules/prefer-empty.md) | Prefer toBeEmpty over checking innerHTML | βœ… | πŸ”§ | | +| [prefer-enabled-disabled](docs/rules/prefer-enabled-disabled.md) | prefer toBeDisabled or toBeEnabled over checking attributes | βœ… | πŸ”§ | | +| [prefer-focus](docs/rules/prefer-focus.md) | prefer toHaveFocus over checking document.activeElement | βœ… | πŸ”§ | | +| [prefer-in-document](docs/rules/prefer-in-document.md) | Prefer .toBeInTheDocument() for asserting the existence of a DOM node | βœ… | πŸ”§ | πŸ’‘ | +| [prefer-required](docs/rules/prefer-required.md) | prefer toBeRequired over checking properties | βœ… | πŸ”§ | | +| [prefer-to-have-attribute](docs/rules/prefer-to-have-attribute.md) | prefer toHaveAttribute over checking getAttribute/hasAttribute | βœ… | πŸ”§ | | +| [prefer-to-have-class](docs/rules/prefer-to-have-class.md) | prefer toHaveClass over checking element className | βœ… | πŸ”§ | | +| [prefer-to-have-style](docs/rules/prefer-to-have-style.md) | prefer toHaveStyle over checking element style | βœ… | πŸ”§ | | +| [prefer-to-have-text-content](docs/rules/prefer-to-have-text-content.md) | Prefer toHaveTextContent over checking element.textContent | βœ… | πŸ”§ | | +| [prefer-to-have-value](docs/rules/prefer-to-have-value.md) | prefer toHaveValue over checking element.value | βœ… | πŸ”§ | | diff --git a/docs/rules/prefer-in-document.md b/docs/rules/prefer-in-document.md index a66848d..3e22abc 100644 --- a/docs/rules/prefer-in-document.md +++ b/docs/rules/prefer-in-document.md @@ -2,7 +2,7 @@ πŸ’Ό This rule is enabled in the βœ… `recommended` config. -πŸ”§ This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix). +πŸ”§πŸ’‘ This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions). @@ -11,6 +11,7 @@ This rule enforces checking existance of DOM nodes using `.toBeInTheDocument()`. The rule prefers that matcher over various existance checks such as `.toHaveLength(1)`, `.not.toBeNull()` and similar. +However it's considered OK to use `.toHaveLength(value)` matcher with `*AllBy*` queries. Examples of **incorrect** code for this rule: @@ -46,7 +47,7 @@ expect(screen.getByText("foo").length).toBe(1); expect(screen.queryByText("foo")).toBeInTheDocument(); expect(await screen.findByText("foo")).toBeInTheDocument(); expect(queryByText("foo")).toBeInTheDocument(); -expect(wrapper.queryAllByTestId("foo")).toBeInTheDocument(); +expect(wrapper.queryAllByTestId("foo")).toHaveLength(1); expect(screen.getAllByLabel("foo-bar")).toHaveLength(2); expect(notAQuery("foo-bar")).toHaveLength(1); diff --git a/src/__tests__/lib/rules/prefer-in-document.js b/src/__tests__/lib/rules/prefer-in-document.js index adc3a8b..74b330e 100644 --- a/src/__tests__/lib/rules/prefer-in-document.js +++ b/src/__tests__/lib/rules/prefer-in-document.js @@ -26,6 +26,33 @@ function invalidCase(code, output) { }; } +function invalidCaseWithSuggestions( + code, + messageData, + replaceQueryOutput, + replaceMatcherOutput +) { + return { + code, + errors: [ + { + messageId: "invalid-combination-length-1", + data: messageData, + suggestions: [ + { + desc: `Replace ${messageData.query} with ${messageData.allQuery}`, + output: replaceQueryOutput, + }, + { + desc: "Replace .toHaveLength(1) with .toBeInTheDocument()", + output: replaceMatcherOutput, + }, + ], + }, + ], + }; +} + const valid = [ "expect().toBe(true)", ...["getByText", "getByRole"].map((q) => [ @@ -101,6 +128,48 @@ const valid = [ ` const element = getByText('value') expect(element).toBeInTheDocument`, + + // *AllBy* queries with `.toHaveLength(1)` is valid + // see conclusion at https://github.com/testing-library/eslint-plugin-jest-dom/issues/171#issuecomment-895074086 + `expect(screen.getAllByRole('foo')).toHaveLength(1)`, + `expect(await screen.findAllByRole('foo')).toHaveLength(1)`, + `expect(getAllByRole('foo')).toHaveLength(1)`, + `expect(wrapper.getAllByRole('foo')).toHaveLength(1)`, + `const foo = screen.getAllByRole('foo'); + expect(foo).toHaveLength(1);`, + `const foo = getAllByRole('foo'); + expect(foo).toHaveLength(1);`, + `let foo; + foo = getAllByRole('foo'); + expect(foo).toHaveLength(1);`, + `let foo; + foo = screen.getAllByRole('foo'); + expect(foo).toHaveLength(1);`, + + `expect(screen.getAllByRole('foo')).toHaveLength(1,//comment + )`, + `const foo = screen.getAllByRole('foo'); + expect(foo).toHaveLength(1,//comment + );`, + `let foo; + foo = screen.getAllByRole('foo'); + expect(foo).toHaveLength(1,//comment + );`, + + // *AllBy* queries with `.toHaveLength(0)` is valid + // see conclusion at https://github.com/testing-library/eslint-plugin-jest-dom/issues/171#issuecomment-895074086 + `expect(screen.queryAllByTestId("foo")).toHaveLength(0)`, + `expect(queryAllByText('foo')).toHaveLength(0)`, + `let foo; + foo = screen.queryAllByText('foo'); + expect(foo).toHaveLength(0);`, + + `expect(screen.getAllByRole('foo')).toHaveLength(0//comment + )`, + `let foo; + foo = screen.getAllByRole('foo'); + expect(foo).toHaveLength(0//comment + );`, ]; const invalid = [ invalidCase( @@ -135,18 +204,6 @@ const invalid = [ `expect(screen.getAllByRole('foo')).toHaveLength(1,2,3,)`, `expect(screen.getByRole('foo')).toBeInTheDocument()` ), - invalidCase( - `expect(screen.getAllByRole('foo')).toHaveLength(0//comment -)`, - `expect(screen.getByRole('foo')).not.toBeInTheDocument(//comment -)` - ), - invalidCase( - `expect(screen.getAllByRole('foo')).toHaveLength(1,//comment -)`, - `expect(screen.getByRole('foo')).toBeInTheDocument(//comment -)` - ), invalidCase( `expect(screen.getAllByRole('foo')).toHaveLength(0,2,3//comment )`, @@ -179,143 +236,105 @@ const invalid = [ `expect(screen.getAllByRole('foo')).toHaveLength(1,2,/*comment*/3,)`, `expect(screen.getByRole('foo')).toBeInTheDocument(/*comment*/)` ), - invalidCase( + // Report invalid combination of *By* query with .toHaveLength(1) assertion + // and suggest fixes by: + // - Replacing *By* with *AllBy* query + // - Replacing .toHaveLength(1) with .toBeInTheDocument() assertion + invalidCaseWithSuggestions( `expect(screen.getByText('foo')).toHaveLength(1)`, + { + query: "getByText", + allQuery: "getAllByText", + }, + `expect(screen.getAllByText('foo')).toHaveLength(1)`, `expect(screen.getByText('foo')).toBeInTheDocument()` ), - invalidCase( + invalidCaseWithSuggestions( `const NUM_BUTTONS=1; expect(screen.getByText('foo')).toHaveLength(NUM_BUTTONS)`, + { + query: "getByText", + allQuery: "getAllByText", + }, + `const NUM_BUTTONS=1; + expect(screen.getAllByText('foo')).toHaveLength(NUM_BUTTONS)`, `const NUM_BUTTONS=1; expect(screen.getByText('foo')).toBeInTheDocument()` ), - invalidCase( - `expect(screen.queryAllByTestId("foo")).toHaveLength(0)`, - `expect(screen.queryByTestId("foo")).not.toBeInTheDocument()` - ), - invalidCase( + invalidCaseWithSuggestions( `expect(getByText('foo')).toHaveLength(1)`, + { + query: "getByText", + allQuery: "getAllByText", + }, + `expect(getAllByText('foo')).toHaveLength(1)`, `expect(getByText('foo')).toBeInTheDocument()` ), - invalidCase( + invalidCaseWithSuggestions( `expect(wrapper.getByText('foo')).toHaveLength(1)`, + { + query: "getByText", + allQuery: "getAllByText", + }, + `expect(wrapper.getAllByText('foo')).toHaveLength(1)`, `expect(wrapper.getByText('foo')).toBeInTheDocument()` ), - invalidCase( + invalidCaseWithSuggestions( `const foo = screen.getByText('foo'); expect(foo).toHaveLength(1);`, + { + query: "getByText", + allQuery: "getAllByText", + }, + `const foo = screen.getAllByText('foo'); + expect(foo).toHaveLength(1);`, `const foo = screen.getByText('foo'); expect(foo).toBeInTheDocument();` ), - invalidCase( + invalidCaseWithSuggestions( `const foo = getByText('foo'); expect(foo).toHaveLength(1);`, + { + query: "getByText", + allQuery: "getAllByText", + }, + `const foo = getAllByText('foo'); + expect(foo).toHaveLength(1);`, `const foo = getByText('foo'); expect(foo).toBeInTheDocument();` ), - invalidCase( + invalidCaseWithSuggestions( `let foo; foo = getByText('foo'); expect(foo).toHaveLength(1);`, + { + query: "getByText", + allQuery: "getAllByText", + }, `let foo; - foo = getByText('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `let foo; - foo = screen.getByText('foo'); + foo = getAllByText('foo'); expect(foo).toHaveLength(1);`, `let foo; - foo = screen.getByText('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `expect(screen.getAllByRole('foo')).toHaveLength(1)`, - `expect(screen.getByRole('foo')).toBeInTheDocument()` - ), - invalidCase( - `expect(await screen.findAllByRole('foo')).toHaveLength(1)`, - `expect(await screen.findByRole('foo')).toBeInTheDocument()` - ), - invalidCase( - `expect(getAllByRole('foo')).toHaveLength(1)`, - `expect(getByRole('foo')).toBeInTheDocument()` - ), - invalidCase( - `expect(wrapper.getAllByRole('foo')).toHaveLength(1)`, - `expect(wrapper.getByRole('foo')).toBeInTheDocument()` - ), - invalidCase( - `const foo = screen.getAllByRole('foo'); - expect(foo).toHaveLength(1);`, - `const foo = screen.getByRole('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `const foo = getAllByRole('foo'); - expect(foo).toHaveLength(1);`, - `const foo = getByRole('foo'); + foo = getByText('foo'); expect(foo).toBeInTheDocument();` ), - invalidCase( + invalidCaseWithSuggestions( `let foo; - foo = getAllByRole('foo'); + foo = screen.getByText('foo'); expect(foo).toHaveLength(1);`, + { + query: "getByText", + allQuery: "getAllByText", + }, `let foo; - foo = getByRole('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `let foo; - foo = screen.getAllByRole('foo'); + foo = screen.getAllByText('foo'); expect(foo).toHaveLength(1);`, `let foo; - foo = screen.getByRole('foo'); + foo = screen.getByText('foo'); expect(foo).toBeInTheDocument();` ), - invalidCase( - `expect(screen.getByText('foo')).toHaveLength(1)`, - `expect(screen.getByText('foo')).toBeInTheDocument()` - ), - invalidCase( - `expect(getByText('foo')).toHaveLength(1)`, - `expect(getByText('foo')).toBeInTheDocument()` - ), - invalidCase( - `expect(wrapper.getByText('foo')).toHaveLength(1)`, - `expect(wrapper.getByText('foo')).toBeInTheDocument()` - ), - invalidCase( - `const foo = screen.getByText('foo'); - expect(foo).toHaveLength(1);`, - `const foo = screen.getByText('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `const foo = getByText('foo'); - expect(foo).toHaveLength(1);`, - `const foo = getByText('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `let foo; - foo = getByText('foo'); - expect(foo).toHaveLength(1);`, - `let foo; - foo = getByText('foo'); - expect(foo).toBeInTheDocument();` - ), - invalidCase( - `let foo; - foo = screen.getByText('foo'); - expect(foo).toHaveLength(1);`, - `let foo; - foo = screen.getByText('foo'); - expect(foo).toBeInTheDocument();` - ), - // Invalid cases that applies to queryBy* and queryAllBy* invalidCase( @@ -397,10 +416,6 @@ const invalid = [ expect(foo).toBeInTheDocument();` ), - invalidCase( - `expect(queryAllByText('foo')).toHaveLength(0)`, - `expect(queryByText('foo')).not.toBeInTheDocument()` - ), invalidCase( `expect(queryAllByText('foo')).toBeNull()`, `expect(queryByText('foo')).not.toBeInTheDocument()` @@ -421,14 +436,6 @@ const invalid = [ `expect(queryAllByText('foo')) .not .toBeDefined()`, `expect(queryByText('foo')) .not .toBeInTheDocument()` ), - invalidCase( - `let foo; - foo = screen.queryAllByText('foo'); - expect(foo).toHaveLength(0);`, - `let foo; - foo = screen.queryByText('foo'); - expect(foo).not.toBeInTheDocument();` - ), invalidCase( `let foo; foo = screen.queryAllByText('foo'); @@ -541,12 +548,6 @@ const invalid = [ span = getByText('foo') as HTMLSpanElement expect(span).toBeInTheDocument()` ), - invalidCase( - `const things = screen.getAllByText("foo"); - expect(things).toHaveLength(1);`, - `const things = screen.getByText("foo"); - expect(things).toBeInTheDocument();` - ), ]; const ruleTester = new RuleTester({ diff --git a/src/rules/prefer-in-document.js b/src/rules/prefer-in-document.js index d9a5618..d9b255b 100644 --- a/src/rules/prefer-in-document.js +++ b/src/rules/prefer-in-document.js @@ -20,7 +20,10 @@ export const meta = { fixable: "code", messages: { "use-document": `Prefer .toBeInTheDocument() for asserting DOM node existence`, + "invalid-combination-length-1": `Invalid combination of {{ query }} and .toHaveLength(1). Did you mean to use {{ allQuery }}?`, + "replace-query-with-all": `Replace {{ query }} with {{ allQuery }}`, }, + hasSuggestions: true, }; function isAntonymMatcher(matcherNode, matcherArguments) { @@ -48,6 +51,24 @@ function usesToHaveLengthZero(matcherNode, matcherArguments) { ); } +/** + * Extract the DTL query identifier from a call expression + * + * () -> + * screen.() -> + */ +function getDTLQueryIdentifierNode(callExpressionNode) { + if (!callExpressionNode || callExpressionNode.type !== "CallExpression") { + return null; + } + + if (callExpressionNode.callee.type === "Identifier") { + return callExpressionNode.callee; + } + + return callExpressionNode.callee.property; +} + export const create = (context) => { const alternativeMatchers = /^(toHaveLength|toBeDefined|toBeNull|toBe|toEqual|toBeTruthy|toBeFalsy)$/; @@ -83,21 +104,64 @@ export const create = (context) => { // only report on dom nodes which we can resolve to RTL queries. if (!queryNode || (!queryNode.name && !queryNode.property)) return; - // toHaveLength() is only invalid with 0 or 1 - if (matcherNode.name === "toHaveLength" && matcherArguments.length) { + // *By* query with .toHaveLength(0/1) matcher are considered violations + // + // | Selector type | .toHaveLength(1) | .toHaveLength(0) | + // | ============= | =========================== | ===================================== | + // | *By* query | Did you mean to use *AllBy* | Replace with .not.toBeInTheDocument() | + // | *AllBy* query | Correct | Correct + // + // @see https://github.com/testing-library/eslint-plugin-jest-dom/issues/171 + // + if (matcherNode.name === "toHaveLength" && matcherArguments.length === 1) { const lengthValue = getLengthValue(matcherArguments); - // isNotToHaveLengthZero represents .not.toHaveLength(0) which is a valid use of toHaveLength - const isNotToHaveLengthZero = - usesToHaveLengthZero(matcherNode, matcherArguments) && negatedMatcher; - const isValidUseOfToHaveLength = - isNotToHaveLengthZero || - !["Literal", "Identifier"].includes(matcherArguments[0].type) || - lengthValue === undefined || - lengthValue > 1; - - if (isValidUseOfToHaveLength) { + const queryName = queryNode.name || queryNode.property.name; + + const isSingleQuery = + queries.includes(queryName) && !/AllBy/.test(queryName); + const hasViolation = isSingleQuery && [1, 0].includes(lengthValue); + + if (!hasViolation) { return; } + // If length === 1, report violation with suggestions + // Otherwise fallback to default report + if (lengthValue === 1) { + const allQuery = queryName.replace("By", "AllBy"); + return context.report({ + node: matcherNode, + messageId: "invalid-combination-length-1", + data: { + query: queryName, + allQuery, + }, + loc: matcherNode.loc, + suggest: [ + { + messageId: "replace-query-with-all", + data: { query: queryName, allQuery }, + fix(fixer) { + return fixer.replaceText( + queryNode.property || queryNode, + allQuery + ); + }, + }, + { + desc: "Replace .toHaveLength(1) with .toBeInTheDocument()", + fix(fixer) { + // Remove any arguments in the matcher + return [ + ...Array.from(matcherArguments).map((argument) => + fixer.remove(argument) + ), + fixer.replaceText(matcherNode, "toBeInTheDocument"), + ]; + }, + }, + ], + }); + } } // toBe() or toEqual() are only invalid with null @@ -195,6 +259,12 @@ export const create = (context) => { context, node.object.object.arguments[0].name ); + + // Not an RTL query + if (!queryNode || queryNode.type !== "CallExpression") { + return; + } + const matcherNode = node.property; const matcherArguments = node.parent.arguments; @@ -202,7 +272,7 @@ export const create = (context) => { const expect = node.object.object; check({ negatedMatcher: true, - queryNode: (queryNode && queryNode.callee) || queryNode, + queryNode: queryNode.callee, matcherNode, matcherArguments, expect, @@ -212,16 +282,25 @@ export const create = (context) => { [`MemberExpression[object.callee.name=expect][property.name=${alternativeMatchers}][object.arguments.0.type=Identifier]`]( node ) { - const queryNode = getAssignmentForIdentifier( + // Value expression being assigned to the left-hand value + const rightValueNode = getAssignmentForIdentifier( context, node.object.arguments[0].name ); + + // Not a DTL query + if (!rightValueNode || rightValueNode.type !== "CallExpression") { + return; + } + + const queryIdentifierNode = getDTLQueryIdentifierNode(rightValueNode); + const matcherNode = node.property; const matcherArguments = node.parent.arguments; check({ negatedMatcher: false, - queryNode: (queryNode && queryNode.callee) || queryNode, + queryNode: queryIdentifierNode, matcherNode, matcherArguments, }); @@ -237,14 +316,17 @@ export const create = (context) => { return; } - const queryNode = - arg.type === "AwaitExpression" ? arg.argument.callee : arg.callee; + const queryIdentifierNode = + arg.type === "AwaitExpression" + ? getDTLQueryIdentifierNode(arg.argument) + : getDTLQueryIdentifierNode(arg); + const matcherNode = node.callee.property; const matcherArguments = node.arguments; check({ negatedMatcher: false, - queryNode, + queryNode: queryIdentifierNode, matcherNode, matcherArguments, });