-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ci: Prevent future enzyme imports with pre-commit lint rule #8219
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
6a7d25b
to
01cab47
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8219 +/- ##
=======================================
Coverage 39.65% 39.65%
=======================================
Files 1233 1233
Lines 29820 29820
Branches 2840 2840
=======================================
Hits 11824 11824
Misses 17307 17307
Partials 689 689 ☔ View full report in Codecov by Sentry. |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5d5dedfd-db08-40f9-b644-cc4a72d712d6 |
eed1e54
to
81e864e
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is really needed, thanks.
The fact that it requires to update tests when we modify a file is, most of the times, not a big deal as test is only just a snapshot match. Very few tests use Enzyme and have extended logic at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this requires design system review
@georgewrmarshall I figured it out. We had believed the CODEOWNER rule had triggered us for review on this one hence why we were confused but it looks like @Cal-L was actually requesting it (not CODEOWNERS) and was likely just making sure our team saw this change. |
Added do not merge label - Will sync with @pedronfigueiredo to explore fitness functions instead |
Hm.. I didn't explicitly request review from DS. There might be something happening in the automation. cc @desi |
Closing in favor of fitness function PR - #8789 |
Description
This change is intended to prevent future imports of enzyme since it has not been maintained React 16. The recommended unit testing library for components is to use is React Testing Library. This is enforced by adding the
no-restricted-imports
lint rule on pre-commit that lets devs know to use RTL instead.Caveat - If an existing file using enzyme is changed, the lint rule will fail on the commit command. Since most of our component test files are lightweight, devs should try their best to update the changed file to use RTL.
Next iteration - Retype enzyme import to show strikeout on usage (deprecate)
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
enzyme-lint.mov
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist