-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Testing: Remove enzyme completely #44494
Conversation
Size Change: 0 B Total Size: 1.26 MB ℹ️ View Unchanged
|
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.
Excellent work on replacing Enzyme with React Testing Library.
All code changes look great. We should ensure that we have documented the case when some project use enzyme
so they know what to install and configure so they can continue using their current tests.
There is one open question for future work. Should we make RTL a part of the default Jest preset we ship with @wordpress/scripts
?
Fantastic work, @tyxla! |
8e5c8c2
to
f6d12ec
Compare
We could, potentially. What benefit would that yield, though on top of always including |
I've rebased after merging the dependency PRs, and I think all feedback has been addressed and the PR is ready for another look 👀 . |
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.
❤️
It seems like we were implicitly relying on |
11abc19
to
e36a112
Compare
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.
LGTM 👍 Awesome!
Nice :) |
Here's a draft dev note, feel free to edit at will, everyone: Dropping Enzyme in favor of
|
fwiw enzyme absolutely will be compatible with react 18 at some point. |
Good to know, thanks @ljharb! I've edited the dev note to account for this. |
I’ll also point out that the effort y’all expended on this transition could have probably helped me get enzyme compatible with react 17/18 twice over :-/ |
It's worth mentioning that this hasn't been an intentional crusade against Enzyme. Historically, Enzyme has been an indispensable utility in our testing toolchain, and we've been infinitely thankful to have it. Given that we had decided to go ahead with using However, recently, we've found a lot of value in using Eventually, we could have a use for On a personal note, I'm curious if any progress has been made on the migration to offer first-class support for React 17 and 18. What kind of help would you be looking for to get it to the finish line? |
Understood. I'm not aware of much that RTL has that enzyme's I just wanted to make it clear for future readers that enzyme's lack of movement is almost exclusively due to about 3-4 years worth of time where virtually no money, and only a few humans' time, have been offered. To get React 17 support over the finish line (which is an obvious prereq for 18 support), I'd need nonzero people to devote the time to ramp up on the project and its testing conventions, and pair with me on getting the 17 adapter passing tests and released - after which we could build the 18 adapter off of that, and evaluate how much work that would take. To avoid further noise for gutenberg, if anyone's interested in discussing further, please file an issue on enzyme, or reach out to me on any of the numerous mediums I have an account, and we can chat! |
What?
This PR gets rid of good old
enzyme
now that we've migrated all tests to use@testing-library/react
.Fixes #17249.
Why?
Enzyme has been a major blocker for upgrading to React 18, so it's great to see it gone!
How?
We're getting rid of the enzyme dependency, its React 17 adapter and snapshot serializers.
This PR includes #44492 and #44493 and should be rebased after they land.We're also consequently getting rid of the last usage of
underscore
, as explained here.Testing Instructions
Verify all checks are green.
Notes
This change definitely deserves a dev note.