Material-ui: [test] Migrate tests from enzyme to react-testing-library

[oliviertassinari]

15 months ago, we have introduced react-testing-library in the codebase: #15732. Since, then, we have been progressively migrating the tests from enzyme to react-testing-library. This was done as a background task, so far. Especially when we were migrating a component from class names to hooks or when we needed to fix a bug.

However, we have seen community members interested in helping with this effort, e.g. @baterson in #22441, @marcosvega91 in #20914, @emilyuhde in #17942, or @eladmotola in #22906.

I have opened this issue so we can keep track of the tests that are left to be migrated and avoid conflicts:

• [x] Accordion.test.js (#22952)
• [x] Backdrop.test.js (#22931)
• [x] ButtonBase.test.js (#22911)
• [x] Container.test.js (#22919)
• [x] CssBaseline.test.js (#22920)
• [ ] Drawer.test.js
• [x] Fab.test.js (#22959)
• [x] Fade.test.js (#22918)
• [x] Grow.test.js (#22917)
• [x] IconButton.test.js (#22972)
• [x] ImageListItem.test.js (#22958)
• [x] InputBase.test.js (#23014)
• [x] Link.test.js (#22892)
• [x] List.test.js (#22929)
• [ ] Menu.test.js
• [x] MenuList.test.js (#22958)
• [x] MobileStepper.test.js (#22963)
• [ ] Modal.test.js
• [x] NativeSelect.test.js (#22970)
• [x] NativeSelectInput.test.js (#22910)
• [ ] Popover.test.js
• [x] RadioGroup.test.js (#22953)
• [x] Slide.test.js (#22913)
• [x] SpeedDial.test.js (#23021)
• [x] SpeedDialIcon.test.js (#22965)
• [x] StepContent.test.js
• [ ] SwipeableDrawer.test.js
• [x] TableBody.test.js (#20914)
• [x] TableCell.test.js (#20914)
• [x] TableFooter.test.js (#20914)
• [x] TableHead.test.js (#20914)
• [x] TablePagination.test.js (#20914)
• [x] TableRow.test.js (#20914)
• [x] TabList.test.js
• [x] TabIndicator.test.js (#22906)
• [x] TextField.test.js (#22944)
• [x] useTheme.test.js (#22928)
• [x] withTheme.test.js (#22928)
• [x] Zoom.test.js (#22914)

Integration

• [x] test/integration/Menu.test.js
• [x] test/integration/MenuList.test.js
• [x] test/integration/NestedMenu.test.js
• [x] test/integration/Select.test.js
• [x] test/integration/TableCell.test.js (#23095)
• [x] test/integration/TableRow.test.js (#23105)

[Avi98]

@oliviertassinari can I pick some of the tests and migrate it react-testing-library

[JorgeAndd]

@oliviertassinari Can I give it a shot at migrating any of these files?

[itamar244]

Need help in testing Modal.test.js.

The following test depends on querying the Fade component and checking if it has specific prop.
I searched for a different approach of testing this with react-testing-library in the project and in the internet and couldn't find a fitting solution.

[GiselaMD]

@itamar244 I'm also trying to find a solution for that, the other tests have the same "find component" query

[NMinhNguyen]

@oliviertassinari how do you want to handle tests that use describeConformance? Should those particular tests just be left as is?

[oliviertassinari]

@NMinhNguyen Best to ask @eps1lon. I have ported a few of them to rtl on the data grid, but that won't cut it.

[bewong89]

🤔 I was wondering this myself, I took a stab at forking describeConformance to use react-testing-library as I thought the abstraction it provides was still useful https://github.com/mui-org/material-ui/pull/22939/commits/1b43a2ce4a21d35ce8eca852f4d47ad3b3c270f5#diff-af11c81585e9558fff1a83088a7d2580

That basically makes it opt-in per component.

If this is an approach we want I'm happy to clean that code up and push through a seperate PR for describeConformance.

The other option we have is to duplicate those tests across components.

[eps1lon]

describeConformance needs enzyme because we explicitly implement "props inheritance". You can't test that with testing-library. No harm done using enzyme there.

[The24thDS]

@oliviertassinari I believe the table components have been migrated to react-testing-library already. #20914

[eladmotola]

@oliviertassinari I think StepContent.test.js is already migrated to react-testing-library

[Avi98]

Hi, I'm taking Drawer.test.js file for migration.

[oliviertassinari]

Last month, react-testing-library had more downloads than enzyme in the ecosystem!

[deiga]

TabList.test.js seems to already be migrated as far as can be, since describeConformance requires enzyme

[nicholas-l]

Should the integration tests be included in this list? (packages/material-ui/test/integration/*)

[oliviertassinari]

@nicholas-l Yes, thanks for raising it. I have added them to the list. I think that we should move them to the corresponding components.

[nicholas-l]

@oliviertassinari Do you mean into the same file as the unit tests or move the file to .int.js (or *.int.test.js) in the component folder?

[oliviertassinari]

@nicholas-l Well, forget about it, let's solve one problem at the time.

[deiga]

@oliviertassinari These tests seem to already use react-testing-library

• test/integration/Menu.test.js
• test/integration/MenuList.test.js
• test/integration/NestedMenu.test.js
• test/integration/Select.test.js

[deiga]

describeConformance needs enzyme because we explicitly implement "props inheritance". You can't test that with testing-library. No harm done using enzyme there.

Is it really necessary to test "props inheritance"? Isn't that kind of implied by using React? Or am I missing some point here?

[oliviertassinari]

@deiga Yes, it's necessary, a component can break the inheritance.

[eps1lon]

Is it really necessary to test "props inheritance"? Isn't that kind of implied by using React?

I don't think that's a useful discussion to have. You can always argue like "isn't that implied by JS". If we would rely on "implied by React" then we'd actually use enzyme and not testing-library.

Or am I missing some point here?

We use tests to generate documentation. We actually solve two problems at once: maintaining documentation and testing implementation. While outdated documentation isn't that bad of a problem to have it's always annoying to deal with (outdated docs being around, issue being reported, someone has to fix it etc).

I don't understand why it's so important to you that we ban enzyme completely from our code base? It still has some value for certain edge cases especially for component libraries.

The main problem with enzyme is it's large API surface that is easy to abuse. The goal is to have a useful test suite where it's hard to write bad tests. Once every test suite is ported to testing-library we can hide enzyme entirely inside describeConformance or other dedicated helpers where writing bad tests is harder.

[deiga]

Sure, that makes sense 👍

I usually like to push extra hard to get rid of "too many" dependencies, and wanted to prod the reasoning behind keeping enzyme :)

[vividh]

May I claim migrating one of the remaining components, say Menu.test.js?

[willRicard]

Same here, I would like to migrate Popover.test.js

[oliviertassinari]

Definitely, you can go ahead :)

[samrae7]

@Avi98 are you still doingDrawer.test.js? If not I will

[vividh]

I'm having a harder time with migrating Menu than I expected. Could I get some help with the following issue?

Migrated the following unit test, but container.firstChild is null.

describe('prop: PopoverClasses', () => {
  it('should be able to change the Popover style', () => {
    const { container } = render(<Menu {...defaultProps} PopoverClasses={{ paper: 'bar' }} />);
    expect(container.firstChild).to.have.class('bar');
  });
});

1) <Menu />
       prop: PopoverClasses
         should be able to change the Popover style:
     TypeError: Cannot read property 'classList' of null
      at Proxy.<anonymous> (node_modules/chai-dom/chai-dom.js:80:10)
      at Proxy.methodWrapper (node_modules/chai/lib/chai/utils/addMethod.js:57:25)
      at Context.<anonymous> (packages/material-ui/src/Menu/Menu.test.js:105:7)
      at processImmediate (node:internal/timers:462:21)

[anasufana]

Would this be an appropriate solution for testing the Modal Backdrop Fade transition with testing-library? As other people have suggested, I'm not sure there is a more elegant way of doing it with testing-library.

 const fadeTransition = (transition = 225) =>
      `opacity ${transition}ms cubic-bezier(0.4, 0, 0.2, 1) 0ms`;

it('should render a backdrop with a fade transition', () => {
      ...
      expect(backdrop.style).to.have.property('transition', fadeTransition());
    });



md5-d46cfc480f3e0aeeff6f7f327ed11864



it('should pass prop to the transition component', () => {
     ...
      expect(backdrop.style).to.have.property('transition', fadeTransition(200));
    });

[samrae7]

Update on drawer.test.js - because this component uses the modal component it is hard to test with react-testing library and so I haven't proceeded. I'm guessing this is the reason it was left until now