-
Notifications
You must be signed in to change notification settings - Fork 145
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
Unnecessary presence checks on findBy queries #743
Comments
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 |
I like this idea and have started my first open-source PR 🎉 |
Lovely because I honestly don't know when I was going to get to this...
…On Mon, Sep 11, 2023 at 5:17 PM Anthony Devick ***@***.***> wrote:
@nathanmmiller <https://github.com/nathanmmiller> ^
—
Reply to this email directly, view it on GitHub
<#743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI6QW3Z3VWUDP5WWW5LEDRDXZ5WUDANCNFSM6AAAAAAWNX2BAY>
.
You are receiving this because you were mentioned.Message ID:
<testing-library/eslint-plugin-testing-library/issues/743/1714517562@
github.com>
|
Why does the plugin need to recommend the explicit one? Can the community/consumers decide? Just asking because sometimes people blindly follow what the |
@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 |
I'd actually be curious on @kentcdodds thoughts on this one now that the above was brought up. To me, I would always do:
over
...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,
...feels more intentional - the whole 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:
🤷♂️ I will personally keep the assertion, but feel free to do what you like. |
Closes #743 * feat(prefer-implicit-assert): adding new rule * feat(prefer-implicit-assert): increment number of rules * feat(prefer-implicit-assert): reduce duplication * feat(prefer-implicit-assert): add recommened rule by library, more test cases, update docs * feat(prefer-implicit-assert): added findBy link * feat(prefer-implicit-assert): added line and column checks * feat(prefer-implicit-assert): use existing utils * feat(prefer-implicit-assert): remove unnecessary checks
🎉 This issue has been resolved in version 6.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
The text was updated successfully, but these errors were encountered: