ESLint: Add and enable eslint-plugin-testing-library
#45103
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Build Tooling
|
|
|
Size Change: +174 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
tyxla
force-pushed
the
add/eslint-plugin-testing-library
branch
2 times, most recently
from
84ee369 to
c06928e
Compare
tyxla
force-pushed
the
add/eslint-plugin-testing-library
branch
from
c06928e to
b912dea
Compare
tyxla
requested review from
jsnajdr,
ntsekouras,
Mamaduka,
gziolo,
ciampo,
mirka,
chad1008 and
youknowriad
|
This one is ready for review now 👀 |
Mamaduka
approved these changes
Oct 20, 2022
In principle, I agree with the reason behind this rule. In practice, we still use Do we have an estimate of how many errors we would need to fix, and how many of those errors would require disabling the rule? |
There are quite a bit of those warnings right now:
So one way would be to disable them completely for now and re-enable them once we've fully addressed all that can be addressed. That can help reduce the noise from the warnings. WDYT? |
Sounds like a plan! |
Done in 87df095. This actually makes the PR pretty innocent too, since it introduces no new ESLint warnings or errors, just enables rules that we're already following. Thank you both for taking a look! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR introduces the
eslint-plugin-testing-libraryESLint plugin and enables it for all non-native test files.Why?
It aims to help maintain following the best practices when writing tests with
testing-library. There's been a discussion around how restrictive those are, and I believe they ensure best practices while being not-so-restrictive, but I'm happy to take feedback.How?
We're enabling the plugin and most its recommended rules for all test files, and excluding all native tests because there's a separate library for it.
I'd suggest we disable a few of the recommended rules for now, because migrating some of them will be tricky and will require more than a few PRs (yes
testing-library/no-containerandtesting-library/no-node-access, I'm looking at you).Testing Instructions
expect( screen.queryByText( 'Hello' ) ).toBeInTheDocument();in your test, you'll get an error.Feedback
I'd love some feedback on whether making
no-containerandno-node-accesswarnings is okay, or if we should instead disable them completely. My argument towards making them warnings is raising awareness that there's a better way to query for elements, and ideally, get the existing violations fixed as we pass by those test files.