-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Drop and remove enzyme in favour of React Testing Utils or lightweight wrapper lib #17249
Comments
RTL still has its own set of issues when testing code with hooks as highlighted in https://kentcdodds.com/blog/write-fewer-longer-tests:
|
In the past @nerrad refactored a big group of tests to stop using Enzyme because they were a few months behind React update: #11214 is another example where we had issues with Enzyme. |
Remember at one point React officially recommended enzyme too ;). I generally like the idea of a light wrapper test utility such as RTL because it brings the promise of slightly easier test writing for newer contributors. On the other hand, I think it's good for a project like WordPress to use the "bare-metal" test utilities because they will generally always be up-to-date with latest React features and will remain the "official" test utilities to use.
In general I agree with this statement. However, I think that criteria should also help demarcate which test approach we take. In other words, anything that is testing "..the way Gutenberg is used by users" is a better candidate for e2e tests. Other tests can focus more on singular component (and utility) logic verification. Since e2e tests generally take longer to run, I think there's still room for the more quicker running component tests that help catch regressions due to changes in logic expectations etc while working on a component. |
@nerrad Take your point. It seems a lot more lightweight than Enzyme though.
This statement feels logical to me. However, my main concern would be will the average contributor be familiar with the lower level test APIs? I would think that most folks will use and be familiar with either:
Given that we should try to make the barrier to entry for writing tests as low as possible we should probably consider continuing support for one or the other. I know there will be arguments that "it [low level test APIs] is not that hard to learn", but if I'm contributing a patch should I have to learn the low level APIs to be able to contribute (genuine question)?
Not necessarily. e2e tests are horribly slow and much harder to write (I know, I've written a few - probably not as many as you though!). Component tests are still relatively isolated "units" of code but straddle the boundary between confidence and speed. I'm not approaching this sentiment focusing on trying to replicate a user in the browser (ie: mouse X, Y coordinates...etc). It's more about having the right mindset and approach to tests. If the way you are interacting with the rendered component in your tests mirrors the way it might be used by a user when it is eventually in a browser then that encourages better testing practices (for example
That's good to hear. I totally agree that we still need smaller true "unit" tests for the low lying code. But having worked with RTL today, I can already see it's a far nicer and more productive experience than Enzyme. |
@gziolo Yes that is true. However, you can more easily work around these due to not having written all your tests using I'll clearly defer to both of your greater experience here. However, I'd be keen to arrive at a consensus and for me to help move this forward. |
It's a hard question to answer because contributors might come from different backgrounds. Approaching it from the context of a typical WordPress developer mostly used to php and starting to pick up more advanced js (react etc), they might not have encountered either enzyme or RTL yet so it doesn't really matter what is used for tests, it's still something to learn. On the other hand, if we approach it from the context of someone super familiar with javascript who has typically used enzyme or RTL in their own projects, then ya there'd be a bit of a barrier to using react test utilities if they've never written tests with them, but then again, I would assume it would be a minor inconvenience at best. My only hesitation with adopting RTL is the potential for a future enzyme like scenario where we have to do workarounds because the library is behind in updating for newer react features. Other than that, I'm in agreement it'd be nice to have something like it available for test writing (given the positives you've already outlined). So for the first issue, I wonder if it'd be a good idea to try converting some tests covering a complicated component ("complicated" in terms of feature set it's using). Maybe the |
Related: WordPress/gutenberg-examples#84 (comment)
I was about to create a new issue as noted in #17016 (comment) but this one seems like a better one 😄My point was that there should be an easy way to test components offered which is baked into |
While working on #17875, I noticed that most unit tests on I updated the tests so they're less coupled to implementation details by using |
Evaluating different React testing approachesFor the past few years that I've been working with React, I've tried different ways to test components. Intuitively, since I started using it, I felt that ResultThe closer the method is to "Manual testing" the better. In this case,
What to test?When testing software, you want to assert on the output (including side effects) for a given input. If we update the code but it keeps producing the same output given the same input, tests shouldn't fail. If they do, they're likely testing implementation details, and it'll make the development process more difficult. That said, it's tempting to view the output of a React Component as the React Element object returned by {
type: "button",
props: {
children: "title"
}
} After all, that's what a function component returns. Save for rare cases, this output isn't useful, testing it doesn't provide any value. What you really want to test is the output produced by Test caseThis experiment emerged while I was working on <Toolbar
controls={[{ title: "control1" }, { title: "control2" }]}
onControlClick={title => console.log(title)}
/> This should render two controls with the texts At first, I'm gonna implement it in a simple way. Then update the implementation in several different ways without changing the API nor the behavior. As long as the requirements are met, the HTML structure doesn't matter too much, although only one implementation change will modify it. All the implementation versions and the test suites can be found on this CodeSandbox: https://codesandbox.io/s/evaluating-different-react-testing-approaches-33d2i Manual testing
|
Pinging @ljharb who might have some insights to contribute, especially related to Enzyme 🙂 |
I'm playing with StoryShots integration with Storybook which would auto-generate snapshot tests for all stories. I have mixed feelings about the idea, but I couldn't resist to give it a spin. It looks like many existing unit tests for UI components do something similar already, so it could further simplify writing tests. See #18031 for more details. |
In general, I'm confused by the OP. enzyme does not encourage poor testing practices; test IDs and "simulate", however, are very poor testing practices, to use some relevant examples. Shallow rendering every component ensures that server rendering can work, and it helps ensure that when your tests fail, you immediately get a pointer to which part of which component has failed. The alternatives mentioned are roughly the same as enzyme's Obviously you should use whichever testing solutions you think work best for you - but the approach used by react-testing-library is what Airbnb was using, and found extremely inferior, before enzyme was created as a better solution. Code coverage (for which 100% should be a minimum, not a goal) shot up from 8% to 70% over a year of requiring it and pushing enzyme as a solution, at Airbnb, and incidents and production bugs plummeted. While I agree that last-resort "smoke tests", testing user flows, are quite important, you can achieve 99% of the confidence you want with fast to write, fast to run, shallow unit tests. In general, anything that fails in a broad integration test is a sign of a failure to write proper unit tests - it's a safety net, not a first resort. (As for React's official recommendation, essentially, they opted not to communicate with me about keeping enzyme up to date with React's featureset, and have not put much effort into the shallow renderer for years, so their change of recommendation is more a reflection of the lack of cooperation than a commentary on testing approaches) |
Are there any conversations etc you can link to that document what Airbnb found extremely inferior? Having some context around this assertion might highlight some things we aren't aware of. |
No, I'm afraid it was all internal discussions - but in particular, serverside rendering is a best practice for many reasons, and any testing solution that requires a DOM be present is utterly failing to test "how a component server renders". You can certainly |
What do you mean by "no-change refactors"? I demonstrated that integration testing with That said, I totally agree that unit tests are faster to write and more helpful for JS functions where you assert on the return value. But that's not the case for components. According to Dan Abramov, even Facebook doesn't "do that much unit testing of components at Facebook" (ref). Regarding testing how components render on the server, I definitely see value on that. But that requires that you run a separate test process without |
I mean a refactor that does not have any observable behavior change or bug fixes or new additions. Certainly you should avoid passing a string to Additionally, tests that can pass while the underlying functionality is broken (ie, you can move a test ID around such that the test still passes but the JS or CSS that targets it is broken) are brittle tests - that are worse than no tests at all because they give false confidence. Facebook has a dearth of unit testing internally, and that's going to begin to bite them as they explore server rendering on their website redesign that's currently in progress - unit testing is a better approach when server rendering. Yes, that's right, to test server rendering you'd run your tests using node in normal node, without jsdom. |
I've really enjoyed using
|
I highlighted this issue as a topic for discussion in the WordPress weekly Core JS chat scheduled for later today at 14.00 UTC. See the full agenda at: https://docs.google.com/document/d/18cSUIb4pgrVMLRq87NlTzcGjHndrTW4ivYh2oIAVkUs/edit |
My personal experience with Enzyme has been quite painful as well, especially as libraries and apps start becoming more complex. You really feel the pain with enzyme when context and/or any from of HOC is introduced. When you're there, you spend a lot of time mocking out stuff. Eventually, you reach a certain point where you've mocked out too much stuff, and you're barely testing anything (important).
@jameslnewell makes an excellent point. This touches upon Kent C. Dodd's (author of react-testing-library) point of testing the actual thing, not implementation details (also something that @diegohaz mentioned above). I found this to be the case when using something like Cypress. It's wonderful. Note: I'm not saying we should use Cypress. My vote is for a robust approach that tests things that actually render out. Things that the user would see on their browser. Rather than what exists in the React component tree. |
@gziolo Did anything come out of this? |
Here is the summary of the meeting: Related part:
Detailed discussion available on WordPress Slack (link requires registration): |
As you can read from the summary shared in my previous comment, I'm also intrigued whether these days we could use an approach similar to E2E tests but isolated to components through Storybook. I mean this would have to work like it would be offline without any external services, focused only on UI interactions. There is some prior art, @ellatrix explored using Jest + Puppeteer with the |
One more thing. In the last issue of React weekly newsletter they highlighted this blog post which compares different approaches of testing the todo app: |
Enzyme is now maintained by the community: https://www.infoq.com/news/2020/03/airbnb-drops-ownership-enzyme/ And accordingly to the article, Airbnb "revealed that the React Testing Library has been increasingly used within the organization". |
Relabeling, this seems to have had quite a bit of technical feedback, so removing that label. I also think this is more of a tracking issue about the direction of testing, so applying that label. |
I think that the ship has sailed. @diegohaz landed #20428 that enabled React Testing Library nearly 3 months ago :) I migrated 6 existing test suites to use React Testing Library as part of Jest migration to v25 in #20766. There were 14 tests failing that were largely written with Enzyme so I used it as an opportunity to give it a try. The experience was great and it feels like we should stop using Enzyme for new tests. I don't think we should drop Enzyme for the sake of having only one testing approach though. The ideal approach would be to gradually replace tests with RTL as part of regular work when we change functionality that makes the test fail, or they no longer work with the updated version of Jest or Enzyme. To make the adoption of RTL smoother, we should:
Should we close this issue and open follow-ups? |
The approach sounds great! |
cc'ing @tyxla who has been recently migrating some tests to RTL |
Here are the recent PRs I've been working on with regards to migration to
I'm planning to be putting more effort into this in the future, primarily motivated by the fact that |
You can check #32765 to see how Riad was able to unblock the upgrade. Actually, I see he refactored a huge number of tests. Maybe, it would make sense to cherry-pick those changes first 😄 |
I did a quick pass on it, tl;dr: it's a rebase hell 😉 Also he did remove a few of them as well. Maybe we can do another pass soon when we advance a bit further with the |
I did a quick search and seems like there are only 3 tests left that are still using Once we migrate them all, we can then remove all references of enzyme in the project (removing dependencies and snapshot-serializers). |
This is still on my list, I've been doing a bunch of those before my AFK last week, and I'm planning to work on these last ones this week. |
Got an update here: the last 3 ones are being addressed here:
and the |
So great to see it happened finally! Mega kudos to everyone that made that happen 🎉 |
We currently use
enzyme
for a lot of our tests. However, it is not keeping pace with the developments in the React and encourages poor testing practices.We should be focusing on making tests resemble the way Gutenberg is used by users. Unfortunately, Enzyme's APIs encourage/enable "poor" testing practices such as testing component implementation details and shallow rendering which doesn't provide confidence that your "feature" works when all the components are wired together.
Describe the solution you'd like
An official announcement formally moving away from Enzyme to either
Updating documentation to
shallow()
which is a library-specific API which enables poor testing practicesIncrementally converting all the existing Enzyme based tests over to the new testing solution - this does not have to be done in a single hit and we could document the key tests to target for migration and leave the others to be migrated "as and when".
The text was updated successfully, but these errors were encountered: