Closed getdave closed 1 year ago
RTL still has its own set of issues when testing code with hooks as highlighted in https://kentcdodds.com/blog/write-fewer-longer-tests:
I'm still getting the act warning, even though I'm using React Testing Library's utilities! React's act utility is built-into React Testing Library. There are very few times you should have to use it directly if you're using React Testing Library's async utilities:
- When using
jest.useFakeTimers()
- When using
useImperativeHandle
and calling functions directly that call state updaters.- When testing custom hooks and calling functions directly that call state updaters.
In the past @nerrad refactored a big group of tests to stop using Enzyme because they were a few months behind React update: https://github.com/WordPress/gutenberg/issues/7005#issuecomment-406041544
https://github.com/WordPress/gutenberg/issues/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.
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.
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.
Remember at one point React officially recommended enzyme too ;).
@nerrad Take your point. It seems a lot more lightweight than Enzyme though.
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.
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)?
...anything that is testing "..the way Gutenberg is used by users" is a better candidate for e2e test
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 getByLabelText()
actually warns you if it doesn't find an associated form control!).
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.
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.
RTL still has its own set of issues when testing code with hooks
@gziolo Yes that is true. However, you can more easily work around these due to not having written all your tests using shallow
and then suddenly being forced to use mount
. For example RTL's act()
is just a pass-through wrapper around the underlying React Core Testing Lib act
.
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.
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)?
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 useSelect
tests would be good to convert here? That at least could validate that RTL has no issues with these type of components?
Related:
https://github.com/WordPress/gutenberg-examples/pull/84#discussion_r313265902
It looks like
react-test-renderer
is a very deep dependency of @wordpress/scripts. We should ensure it has tighter integration so it works out of the box in all cases.
I was about to create a new issue as noted in https://github.com/WordPress/gutenberg/issues/17016#issuecomment-524176170 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 @wordpress/scripts
.
While working on #17875, I noticed that most unit tests on Toolbar
broke even though the UI worked and outputted the same HTML as before. Besides using shallow
, tests were trying to .find()
specific React components in the tree by their displayName
s. As far as I know, react-test-renderer
suffers from a similar problem with findByType
, which isn't ideal.
I updated the tests so they're less coupled to implementation details by using mount
and .find
ing by valid DOM selectors (https://github.com/WordPress/gutenberg/pull/17875/commits/5e9a5482600e176d6d5172163252fe702ae21ef9#diff-65573d09cbd9a91cf8541c8005f24910). But Enzyme really doesn't encourage or facilitate this approach.
For 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 react-testing-library
was the best option. But I'd like to do a better comparison between the different methods, and try other approaches that I haven't had much opportunity to work with, like react-test-renderer
and react-dom/test-utils
. Here's the result.
The closer the method is to "Manual testing" the better. In this case, react-testing-library
truly demonstrated to be the best option. And I think we should consider adopting it on Gutenberg.
Method | Tests |
---|---|
Manual testing | β β β β β |
enzyme/shallow |
β ββββ |
enzyme/mount |
β β β ββ |
react-test-renderer |
β β βββ |
react-dom/test-utils |
β β β β β |
react-testing-library |
β β β β β |
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 React.createElement
(or JSX), which looks like this:
{
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 ReactDOM.render()
, the one that ends up in the user's browser.
This experiment emerged while I was working on @wordpress/components
' Toolbar
. I'm going to use a simpler version of that component as the example here.
<Toolbar
controls={[{ title: "control1" }, { title: "control2" }]}
onControlClick={title => console.log(title)}
/>
This should render two controls with the texts control1
and control2
respectively, and, when the user clicks on some of them, it should log its text to the console. Simple as that!
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
enzyme/shallow
shallow
will not expand child elements, so it won't find an element deep in the React tree. I'm trying to follow best practices here, but find()
lets you find by component name (e.g. find("ToolbarButton")
), which would break if the component name changes. We have several tests written like that.
enzyme/mount
It's the same as enzyme/shallow
, with the same concerns around find()
, but it does expand child elements so we can find [children='control1']
deep in the tree.
react-test-renderer
It's similar to enzyme/mount
, except that it's harder to find elements. Again, I'm trying to follow best practices, but it has a findByType
method that encourages you to find by the component name, and we're using this method in some parts of our codebase.
react-dom/test-utils
In terms of API, this is the worse. But, unlike the previous approaches, it deals with the rendered DOM tree, which makes tests more reliable.
react-testing-library
By far, the best API. It uses the DOM tree and real DOM events. It doesn't let you access state
, props
or any sort of implementation detail. So, it's hard to make mistakes and test implementation details even if you want.
All tests pass!
β
Manual testing
β
enzyme/shallow
β
enzyme/mount
β
react-test-renderer
β
react-dom/test-utils
β
react-testing-library
button
into a custom componentenzyme/shallow
fails because it doesn't expand Control
, so it can't find the underlying [children=title]
, which is the button
element.
β
Manual testing
β enzyme/shallow
β
enzyme/mount
β
react-test-renderer
β
react-dom/test-utils
β
react-testing-library
react-test-renderer
fails because it finds the span
element, not button
. It tries to call props.onClick
, which doesn't work.
β
Manual testing
β enzyme/shallow
β
enzyme/mount
β react-test-renderer
β
react-dom/test-utils
β
react-testing-library
Since enzyme/mount
deals with the React tree, it finds Control
, which is the component that has children
now. It fails because onControlClick
is called with the default React SyntheticMouseEvent
instead of the control1
string.
β
Manual testing
β enzyme/shallow
β enzyme/mount
β react-test-renderer
β
react-dom/test-utils
β
react-testing-library
Only react-testing-library
passes this because it's the only which dispatches actual DOM events.
β
Manual testing
β enzyme/shallow
β enzyme/mount
β react-test-renderer
β react-dom/test-utils
β
react-testing-library
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 https://github.com/WordPress/gutenberg/pull/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 mount
, and don't give you any robust confidence about how your components are working. All they'll do is ensure that in no-change refactors, things are still working. However, this is far far from the common case of what changes are.
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)
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.
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 ReactDOM.renderToString
, then add jsdom, then render the HTML via jsdom, and then hydrate it, but then you're only testing "how your app works with full asset and JS support", and not actually testing how your app works when JS breaks or fails to load (which happens way more often than people realize on non-first-world internet connections or devices).
All they'll do is ensure that in no-change refactors, things are still working.
What do you mean by "no-change refactors"?
I demonstrated that integration testing with react-testing-library
and react-dom/test-utils
provides much more value and confidence than testing with Enzyme, specially when using shallow
. Maybe I used Enzyme incorrectly, so I would appreciate any feedback on that.
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 jsdom
, right? How would it be better than simply using react-dom/server
?
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 .find
in enzyme; find by component reference, not by display name (which is indeed brittle, as you indicated). The issue there though is that you don't need to find anything "deep in the tree". In your example test for Toolbar
, all you need to test with shallow is that with the right props, they render the appropriate ToolbarButton
, ToolbarGroup
, etc - the responsibility to test ToolbarButton
lies solely with ToolbarButton
. Props are the external API of a component, so they're part of Toolbar
's API, and they're part of ToolbarButton
's API as well. Testing implementation details is certainly subpar, but it's not necessarily a footgun to be avoided - the value of tests is not "they don't break when you make changes", it's "when something breaks, you know why and where, as rapidly as possible". Tests that are slower to write and more importantly, slower to execute, are not meeting that bar.
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. react-dom/server
only spits out a string of HTML - how do you test that without mounting it in jsdom, or, throwing it into cheerio/jquery? enzyme lets you test it in the same way that you'd test your code in the browser - making your tests just as portable as you hope your components will be.
I've really enjoyed using react-testing-library
in a small team of more junior developers because:
.state()
and resulted in significant improvements in the way tests were written (less brittle to changes in the internal implementation and actually testing logic rather than just the state) without having to invest loads of time in training and educating developersI 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).
forces you to think about and test the public interface, not the implementation of all the one-off components which are likely to change in the next refactor
@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. Testing the actual thing, rather than meticulously mocking bits and testing for obscure prop/state.
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.
I highlighted this issue as a topic for discussion in the WordPress weekly Core JS chat scheduled for later today at 14.00 UTC
@gziolo Did anything come out of this?
I highlighted this issue as a topic for discussion in the WordPress weekly Core JS chat scheduled for later today at 14.00 UTC
@gziolo Did anything come out of this?
Here is the summary of the meeting: https://make.wordpress.org/core/2019/10/31/javascript-chat-summary-october-29nd-2019/
Related part:
There is an ongoing discussion about different approaches to testing UI components on GitHub. It isnβt a new topic. We have already considered removing
enzyme
in the past when some React APIs werenβt covered, but we gave up because of its wide usage.We agreed that we can live with
enzyme
in old files following Code Refactoring guidelines, but we should plan to make it easier to contribute with tests when working on new features. @nerrad emphasized that it would be good to iron out what testing approach we want to recommend going forward. If anything the discussion in that issue highlights, we should include it in the Testing Overview documentation as the very first step.@gziolo proposed we consider the E2E testing approach with the instance of Storybook as the testing environment given that it is a static site working like a single page application. @itsjonq shared that heβs done storybook E2E testing using Cypress in the past. It was something that could be automated by CI (Travis) and it worked great.
Detailed discussion available on WordPress Slack (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1572359851227800
I found this to be the case when using something like Cypress. It's wonderful. Testing the actual thing, rather than meticulously mocking bits and testing for obscure prop/state.
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 RichText
component in https://github.com/WordPress/gutenberg/pull/17303. I don't think you could replicate such advances testing scenarios with the libraries which were discussed above and fall under the unit testing group.
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: https://medium.com/javascript-in-plain-english/i-tested-a-react-app-with-jest-testing-library-and-cypress-here-are-the-differences-3192eae03850
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 https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/pull/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 @testing-library
:
I'm planning to be putting more effort into this in the future, primarily motivated by the fact that enzyme
is a blocker for upgrading to React 18.
I'm planning to be putting more effort into this in the future, primarily motivated by the fact that
enzyme
is a blocker for upgrading to React 18.
You can check https://github.com/WordPress/gutenberg/pull/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 enzyme
migration.
I did a quick search and seems like there are only 3 tests left that are still using enzyme
:
block-switcher
autosave-monitor
post-preview-button
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 enzyme
removal is being suggested here:
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".