Skip to content

Avoid using throwing queries with expect: no-expect-get-by-query #21

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.

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

Closed
thomaslombart opened this issue Oct 14, 2019 · 10 comments · Fixed by #22
Closed

Avoid using throwing queries with expect: no-expect-get-by-query #21

thomaslombart opened this issue Oct 14, 2019 · 10 comments · Fixed by #22
Assignees
Labels
new rule New rule to be included in the plugin

Comments

@thomaslombart
Copy link
Collaborator

thomaslombart commented Oct 14, 2019

Problem

According to the cheatsheet, getBy, getAllBy are queries that throw an error if there are no matches. Thus, it doesn't make sense to use expect with a query that throws:

expect(getByText("foo")).toBeInTheDocument()

I don't know if that's a thing but I'm more used to use:

getByText("hello")
// OR
expect(queryByText("foo")).toBeInTheDocument()

Solution

The rule (let's call it no-expect-throwing-query but I'm not sure of that name) would prevent getBy and getAllBy to be used with expect as they already throw an error.

Valid cases:

// ✅ Valid
getByPlaceholderText("foo")
expect(queryByText("foo")).toBeInTheDocument()
expect(queryAllByText("foo")).toHaveLength(3)
getAllByTitle("foo")

Invalid cases:

// ❌Invalid
expect(getByPlaceholderText("foo")).toBeInTheDocument()
expect(getAllByText("foo")).toHaveLength(3)

const foo = getByText("foo")
expect(foo).toBeInTheDocument()
@thomaslombart thomaslombart added the new rule New rule to be included in the plugin label Oct 14, 2019
@thomaslombart thomaslombart changed the title Avoid using getBy and findBy with expect: no-expect-throwing-query Avoid using throwing queries with expect: no-expect-throwing-query Oct 14, 2019
@Belco90
Copy link
Member

Belco90 commented Oct 14, 2019

Nice, this is recommended in testing library doc too so it would definitely be a good addition.

Anyway, there are couple of examples in your invalid cases I don't understand:
expect(findByText("foo")).not.toBeNull()
Why is this one wrong? There is no alternative in this case for async queries.

expect(findAllByText("foo").toHaveLength(3)
And this one? Again, there is no alternative for async queries.

So I think findBy variants are out of the scope of this issue as there is no alternative, and the point of this it's just getBy vs queryBy.

@thomaslombart thomaslombart changed the title Avoid using throwing queries with expect: no-expect-throwing-query Avoid using throwing queries with expect: no-expect-get-by-query Oct 14, 2019
@thomaslombart
Copy link
Collaborator Author

Yeah, true. I updated the comment!

@Belco90
Copy link
Member

Belco90 commented Oct 14, 2019

Could, everything lgtm now! Do you want to take care of this one? I'll try to build something for #20 meanwhile.

@thomaslombart
Copy link
Collaborator Author

Sure! 🙂

@thomaslombart thomaslombart self-assigned this Oct 14, 2019
@emmenko
Copy link
Contributor

emmenko commented Oct 15, 2019

FYI: my team and I started recently to also implement some eslint rules for RTL. In fact we have one that basically solves this problem

https://www.npmjs.com/package/eslint-plugin-testing-library-react

@Belco90 would you be open if we contribute to your list of rules, so that we deprecate our package? WDYT?

@Belco90
Copy link
Member

Belco90 commented Oct 15, 2019

Hey @emmenko! Absolutely, I'll be more than happy if you contribute to this plugin with that rule and even collaborate together in the future with more rules. The only thing is I see your plugin is focused in just react, but the rule should be generic enough to work also with dom, vue and angular for this plugin, which I think it already is but we would have to double check.

I'm not sure how to proceed from here, maybe @thomlom can tell us how far he got the rule so we can get it working together with your rule implementation?

@emmenko
Copy link
Contributor

emmenko commented Oct 15, 2019

Sure thing. Let me know if we have the green light to make a PR with our rule.

@Belco90
Copy link
Member

Belco90 commented Oct 15, 2019

Of course! There is no better way to review the rule being included here for sure.

@thomaslombart
Copy link
Collaborator Author

I haven't done nothing yet @Belco90, so you can open a PR @emmenko 🙂
Thanks for contributing!

@emmenko
Copy link
Contributor

emmenko commented Oct 16, 2019

I opened a PR with the new rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new rule New rule to be included in the plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants