Unnecessary presence checks on findBy queries
nathanmmiller opened this issue · 10 comments
Name for new rule
no-find-by-presence
Description of the new rule
The rule should avoid things like expect(await screen.findBy*()).toBeInTheDocument(), because that's functionally the same as await screen.findBy*().
Perhaps this is a new rule (I proposed a name) or maybe it should just be part of the existing find-by rule.
Testing Library feature
await findBy
Testing Library framework(s)
All, presumaly.
What category of rule is this?
Suggests an alternate way of doing something
Optional: other category of rule
No response
Code examples
expect(await screen.findByRole("button")).toBeInTheDocument();
expect(await screen.findByText("hello")).toBeDefined();
expect(await screen.findByLabel("zoop!")).toBeTruthy();
Anything else?
I am happy to contribute this rule - but if so, it may take me a month or two to do so, as things are busy. Please let me know if you'd like me to contribute or not.
Do you want to submit a pull request to make the new rule?
Yes
Hi @nathanmmiller! I think it could be a good idea to implement this rule, so give it a try if you are willing to contribute to this plugin!
We need to make sure we explain in its docs that it would be the opposite of prefer-explicit-assert, clarifying only one should be enabled, and the plugin actually recommends the explicit one. This rule could be named prefer-implicit-assert then.
I like this idea and have started my first open-source PR 🎉 prefer-implicit-assert #815
and the plugin actually recommends the explicit one
Why does the plugin need to recommend the explicit one? Can the community/consumers decide? Just asking because sometimes people blindly follow what the "library recommends" as the "gold standard"?
@adevick We made that decision based on KCD's "Common mistakes with React Testing Library" post.
@adevick We made that decision based on KCD's "Common mistakes with React Testing Library" post.
That makes sense I guess that last one to me was more of his personal opinion For this reason, many people skip the assertion. This really is fine honestly, but I personally, ... but to each their own.
I'd actually be curious on @kentcdodds thoughts on this one now that the above was brought up.
To me, I would always do:
expect(screen.getByRole("button")).toBeInTheDocument()
over
screen.getByRole("button")
...for the same reason KCD says - it's clear that there's an intentional assertion, not just an accidental remnant of some refactor.
But, also to me,
await screen.findByRole("button")
...feels more intentional - the whole await part of it makes me feel like we're purposely asserting something ("this will show up").
That said, I don't have an extreme preference here. If this rule exists, I will turn it on, even if it's not "recommended," for precisely the reasoning above. I can see that others wouldn't because it's not "recommended" but 🤷.
I don't feel very strongly about this. As noted in the article:
Importance: low
🤷♂️ I will personally keep the assertion, but feel free to do what you like.
🎉 This issue has been resolved in version 6.1.0 🎉
The release is available on:
Your semantic-release bot 📦🚀