[test] Introduce @testing-library/react

[eps1lon]

Introduces a new testing utility that will use react-testing-library instead of enzyme. The DOM helpers that enzyme provided (text(), hasClass() etc) are available via chai-dom

Sample diff:

-const wrapper = mount(
+const { container } = mount(
  <Ripple classes={classes} timeout={{}} rippleX={0} rippleY={0} rippleSize={11} />,
);
-const rippleWrapper = wrapper.find('span').first();
-assert.strictEqual(rippleWrapper.hasClass(classes.ripple), true);
-assert.strictEqual(rippleWrapper.hasClass(classes.fast), false);
+const ripple = container.querySelector('span');
+expect(ripple).to.have.class(classes.ripple);
+expect(ripple).not.to.have.class(classes.fast);

react-testing-library vs. enzyme

If anybody insists on providing examples I can collect all the function component or strict mode test PRs but I guess the recent refactorings have proven that tests written with enzyme are a major pain point. Even ignoring bugs (or missing features ) in enzyme it's to easy to test implementation details of react which doesn't seem to help debugging an issue. A component itself is already shallow enough that we don't really need to assert on instance variables or the component in order to debug a test. Most of the time a broken test was just the test being broken and not the component.

Most of the time enzyme is just used to hit a specific line to increase coverage. But that's not improving robustness of the component. Most of the time it doesn't do anything meaningful. If we don't touch a line we need to evaluate why this line exists and how we test this instead of writing a test that will break every time we touch that line.

We don't have to migrate all of our tests at once and some will be pretty hard to convert because of the nature of the component. But I would hope that once we have migrated complex test we end up with a more robust test. It's also not a pressing issue. Just something we can gradually do and with which we might attract new contributors.

react-testing-library doesn't have a convenient setProps for good reasons. Though they don't really apply to library tests. We would end up writing noisy wrappers that would just consist of a button that triggers a state update that passes new props. I added a convenience wrapper which reduces the amount of code we have to rewrite.

no more wrapper.update

I don't think I ever fully understood why this was sometimes necessary. Will reduce the amount of magic that happens in the tests

no more implicit act

This sounds like a step back but it's important that we explicitly test this. There's a semantic difference between useLayoutEffect and useEffect which will become more evident in concurrent react. I don't know how this will look but I simply don't trust enzyme anymore to correctly determine when to use act(() => {}) and await act(async () => {}).

assert.strictEqual vs expect

That's entirely optional at this point. We can just as well write

-expect(ripple).to.have.class(classes.ripple);
+assert.strictEqual(ripple.classList.contains(classes.ripple), true);

I still want to make an argument for the expect style with helpers instead of assertion style with strictEqual focus.

1. better assertion error messages
   expect will log expected span.MuiTouchRipple-child.MuiTouchRipple-childLeaving to have class 'MuiTouchRipple-rippleVisible'
   while assert will log AssertionError: expected false to equal true
   The latter does not assist understanding the issue. This is especially prevalent during debugging: Test failed? What was expected, what appeared instead? Oh no idea, need to inspect/log those first.
2. Required API knowledge is the same. One requires knowledge about DOM APIs one about the assertion API
3. Better onboarding
   Most open source libraries close to us (react, enzyme, ant-design, eslint) use the expect style. While chai is not as widely adopted the expect style should be more familiar over the assert style.
4. It "just" reads better
   I mean nobody communicates with others a test in the form of "I assert that classList contains this class is strictly equal to true". "expect(ripple).to.have.class()" is apart from syntax almost exactly what you would say to a person when you describe how a unit works: "expect ripple to have the given class"

/cc @mui-org/core-contributors

Long term plan

Remove enzyme tests entirely. Since enzyme relies on React internals any update to react can potentially break our tests. We should reduce the test churn (ironically by introducing churn).

Explore jest (assert -> expect) makes the switch a bit easier.

[mui-pr-bot]

No bundle size changes comparing ac39205...bd7a8f5

Generated by 🚫 dangerJS against bd7a8f5

[oliviertassinari]

@eps1lon So we better understand the implication. Do you have an estimation of the time this effort is going to take? What will a Material-UI user get as a value out of this migration?

You have mentioned the problems we had with the older enzyme tests, where we were testing the internals. Didn't the hooks migration effort force us to move away from these wrong patterns? Do we still have problematic logic?

If we move forward in this effort, I think that we should wait 6 months so we can dedicate resources on building more components.

cc @mui-org/core-contributors

[eps1lon]

Do you have an estimation of the time this effort is going to take?

They're already pretty close to how you would write those with react-testing-library. It comes down to making them robust so that they don't break with the next refactoring.

What will a Material-UI user get as a value out of this migration?

I don't have to spend time debugging enzyme issues and file fixes upstream. The free time can be spend on actual material-ui code.

Do we still have problematic logic?

Mostly usage of enzyme helpers like html(), text(), simulate() which are neither StrictMode compatible nor very safe. Especially simulate doesn't do what it says and is even discouraged by enzyme maintainers.

It's also about the mere existence of the enzyme API. The API is an easy escape hatch that creates a lot of technical debt which we should prevent.

If we move forward in this effort, I think that we should wait 6 months so we can dedicate resources on building more components.

I will repeat that this is not something that has to be done in one commit. enzyme tests can easily live next to react-testing-library test. New test should probably be written with react-testing-library. We can outsource those like we did with the TS demos or hooks migration issues.

[joshwooding]

I don’t see anything negative here, it seems that the community is using react-testing-library so we know it’s a good library to use, plus it provides the benefits listed above which I agree with.

[kgregory]

Most of the time enzyme is just used to hit a specific line to increase coverage. But that's not improving robustness of the component. Most of the time it doesn't do anything meaningful. If we don't touch a line we need to evaluate why this line exists and how we test this instead of writing a test that will break every time we touch that line.

This paragraph is spot on and very important.

I agree with @joshwooding and feel that react-testing-library is a good library with sane guiding principles. I'm excited for this.

[oliviertassinari]

@eps1lon Ok, to get down to the bottom of my concern: It's about the timing orchestration.

I'm asking about the resource it would cost us to perform the migration as I doubt it's something we can automate. I believe we will have to dedicate X hours to update the 2294 tests we have, X to be estimated. On the other hand, the value it provides to our users is not immediate, better tests don't mean more features. I believe it's an investment that needs a year or two to yield a positive ROI. Correct me If you think I'm wrong. Doing it in a single commit or over a period of 6 months won't change the total resource required to perform it. Will people be able to help out in the migration? I would suspect fewer will be interested compared to the TypeScript migration that they get direct value after performing. Also, having people migrating the tests requires a non-negligible amount of review time resource.

assert vs expect

I have no objection with that. I actually think it's a good idea considering that Jest is the most used solution and that most people are using expect.
Also, my motivation for using assert.strictEqual as much as possible applies to the same for using expect().toBe(). It's all about ===, sacrificing as much "pretty" error messages we can get away with, optimizing for a smaller API surface external contributors need to know.

react-testing-library vs. enzyme

I agree that enzyme allows for writing nonoptimal tests. I also believe that you can write correct tests with it. Enzyme is historically the leading solution but react-testing-library is gaining traction. I don't have a specific preference between the two.

---

I have no specific objection with the change on its own. However, I want to make sure we dedicate resource to solving more people problems rather than refining problem we already "solved", even if suboptimal. Having a side test migration effort ongoing will distract us. How can we move forward?
I think that the following would work:

• We don't touch the old tests for the next 6 months, no community effort work asked.
• The new component tests are written with react-testing-library, the slider will be a great starting point
• Once we have built enough components so people feel the Material-UI is a feature rich as Ant Design, we migrate the old component tests.

Would it work for you?

[eps1lon]

didn't want to disable this globally in case this allows new bugs to be introduced. Used this opportunity to use all the mocha related rules only on test files.

[eps1lon]

Are there any new arguments against this?

I don't think it's worth discussing what the user benefit is. I would have to start way earlier to explain why testing is important and why bad testing is actually harmful.

enzyme is currently blocking some features again because it relies so heavily on React internals (see StrictMode + simulate).

I've rewritten some RadioGroup tests and they are just so much more readable than anything we currently tests it amazes me how long we thought enzyme tests were ok. And writing them is also that much more convenient because you get helpful debug messages

[eps1lon]

Posting some examples of refactored tests while working on getting rid of simulate (strictMode violation):

+    function getMenu() {
+      const control = wrapper.getByLabelText('When device is locked');
+      return wrapper.container.querySelector(`#${control.getAttribute('aria-controls')}`);
+    }
it('should not be open', () => {
-      const popover = wrapper.find(Popover);
-      assert.strictEqual(popover.props().open, false, 'should have passed open=false to Popover');
-      const menuEl = document.getElementById('simple-menu');
-      assert.strictEqual(menuEl, null);
+    it.only('should not be open by default', () => {
+      expect(getMenu()).not.to.be.visible;
     });

[oliviertassinari]

enzyme is currently blocking some features again because it relies so heavily on React internals (see StrictMode + simulate).

@eps1lon The enzyme tests can be refactored to follow the react-testing-library philosophy. What's the blocker?

diff --git a/packages/material-ui-lab/src/Slider/Slider.test.js b/packages/material-ui-lab/src/Slider/Slider.test.js
index 7fcc1cd10..888155861 100644
--- a/packages/material-ui-lab/src/Slider/Slider.test.js
+++ b/packages/material-ui-lab/src/Slider/Slider.test.js
@@ -8,6 +8,7 @@ import {
   wrapsIntrinsicElement,
 } from '@material-ui/core/test-utils';
 import describeConformance from '@material-ui/core/test-utils/describeConformance';
+import TestUtils from 'react-dom/test-utils';
 import Slider from './Slider';

 function touchList(touchArray) {
@@ -31,7 +32,7 @@ describe('<Slider />', () => {

   before(() => {
     classes = getClasses(<Slider value={0} />);
-    mount = createMount({ strict: false });
+    mount = createMount({ strict: true });
   });

   after(() => {
@@ -111,10 +112,11 @@ describe('<Slider />', () => {
   describe('when mouse leaves window', () => {
     it('should move to the end', () => {
       const handleChange = spy();
+      const ref = React.createRef();

-      const wrapper = mount(<Slider onChange={handleChange} value={50} />);
+      const wrapper = mount(<Slider ref={ref} onChange={handleChange} value={50} />);

-      wrapper.simulate('mousedown');
+      TestUtils.Simulate.mouseDown(ref.current);
       document.body.dispatchEvent(new window.MouseEvent('mouseleave'));

       assert.strictEqual(handleChange.callCount, 1);

Are there any new arguments against this?

I don't have any new arguments. The concerns in are still valid. The biggest one is the amount of resources it's gonna require us to move our 2339 test cases vs how these resources can be allocated otherwise. I believe we can have a better ROI investing these resources on different topics. There is no question that react-testing-library is growing:

• https://www.rank2traffic.com/testing-library.com
• https://npm-stat.com/charts.html?package=%40testing-library%2Freact&package=react-testing-library&package=enzyme&package=react-dom&from=2018-06-19&to=2019-06-19.

It would be a good move for a new project. For us, I don't think that we should commit to it yet, at least, not for the next 6 months.

cc @mui-org/core-contributors What do you guys think?

[eps1lon]

-      wrapper.simulate('mousedown');
+      TestUtils.Simulate.mouseDown(ref.current);

That's equivalent. enzyme uses TestUtils.Simulate under the hood.

It would be a good move for a new project. For us, I don't think that we should commit to it yet, at least, not for the next 6 months.

I'm not sure how I can explain this anymore: This is not a proposal for a sprint. Something we just do on side when we touch the tests in questions i.e. when they break because of enzyme. I am not proposing to switch out every test.

[eps1lon]

currently rewriting test/integration/Menu.test.js and the test for initial item selection fails without #16294 i.e. enzyme's API encourages a test that produces a false positive.

And it uncovered another potential a11y issue (aria-controls should point to the menu not the wrapper). So it's not that unlikely that other tests currently produce false positives.

Edit: That was premature. The problem is a different one. Will follow-up in #16294.

[oliviertassinari]

👍