Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(prefer-in-document): false positive on .toHaveLength(1) matcher with *AllBy* query #311

Merged
merged 9 commits into from
Aug 5, 2023
Merged

fix(prefer-in-document): false positive on .toHaveLength(1) matcher with *AllBy* query #311

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
160426e
fix(prefer-in-document): make toHaveLength(1) with AllBy queries valid
SevenOutman Oct 9, 2022
bc2aa4d
fix(prefer-in-document): make toHaveLength(0) with AllBy queries valid
SevenOutman Oct 9, 2022
c3562fb
feat(prefer-in-document): give suggestions when reporting invalid com…
SevenOutman Oct 9, 2022
6b626ae
docs(prefer-in-document): add explanation for using .toHaveLength mat…
SevenOutman Oct 9, 2022
155d0dc
refactor(prefer-in-document): edit wording assertion -> matcher
SevenOutman Oct 9, 2022
72ec55e
refactor(prefer-in-document): use messageId in suggestions
Nov 15, 2022
cca8137
fix test errors
EricKim987 Jul 5, 2023
e0e18ab
add some test cases
EricKim987 Jul 7, 2023
275f469
docs: regenerate documentation
G-Rath Aug 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/rules/prefer-in-document.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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);

Expand Down
236 changes: 110 additions & 126 deletions src/__tests__/lib/rules/prefer-in-document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => [
Expand Down Expand Up @@ -101,6 +128,31 @@ 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);`,

// *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);`,
];
const invalid = [
invalidCase(
Expand Down Expand Up @@ -135,18 +187,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
)`
),
G-Rath marked this conversation as resolved.
Show resolved Hide resolved
invalidCase(
`expect(screen.getAllByRole('foo')).toHaveLength(0,2,3//comment
)`,
Expand Down Expand Up @@ -179,143 +219,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(
Expand Down Expand Up @@ -397,10 +399,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()`
Expand All @@ -421,14 +419,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');
Expand Down Expand Up @@ -541,12 +531,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({
Expand Down