[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:

• Search for createMount() usage
• Menu.test.js ([test] Convert Menu tests to testing-library #26915)
• Modal.test.js ([test] Convert Modal tests to testing-library #26912)
• Popover.test.js ([test] Convert Popover tests to testing-library #26913)
• SwipeableDrawer.test.js ([test] Convert SwipeableDrawer tests to testing-library #26916)
• Drawer.test.js ([test] Convert Drawer tests to testing-library #26837)
• Accordion.test.js ([test] Migrate Accordion to react-testing-library #22952)
• Backdrop.test.js ([test] Migrate Backdrop to react-testing-library #22931)
• ButtonBase.test.js ([test] Migrate ButtonBase to react-testing-library #23011)
• Container.test.js ([test] Migrate Container to react-testing-library #22919)
• CssBaseline.test.js ([test] Migrate CssBaseline to react-testing-library #22920)
• Fab.test.js ([test] Migrate Fab to react-testing-library #22959)
• Fade.test.js ([test] Migrate Fade to react-testing-library #22918)
• Grow.test.js ([test] Migrate Grow to react-testing-library #22917)
• IconButton.test.js ([test] Migrate IconButton to react-testing-library #22972)
• ImageListItem.test.js ([test] Migrate MenuList and ImageListItem to react-testing-library #22958)
• InputBase.test.js ([test] Migrate InputBase to react-testing-library #23014)
• Link.test.js ([test] Migrate more components to react-testing-library #22892)
• List.test.js ([test] Migrate List to react-testing-library #22929)
• MenuList.test.js ([test] Migrate MenuList and ImageListItem to react-testing-library #22958)
• MobileStepper.test.js ([test] Migrate MobileStepper to react-testing-library #22963)
• NativeSelect.test.js ([test] Migrate NativeSelect to react-testing-library #22970)
• NativeSelectInput.test.js ([test] Migrate NativeSelectInput to react-testing-library #22910)
• RadioGroup.test.js ([test] Migrate RadioGroup to react-testing-library #22953)
• Slide.test.js ([test] Migrate Slide to react-testing-library #22913)
• SpeedDial.test.js ([test] Migrate SpeedDial to react-testing-library #23021)
• SpeedDialIcon.test.js ([test] Migrate SpeedDialIcon to react-testing-library #22965)
• StepContent.test.js
• TableBody.test.js ([test] Migrate all Table components to testing-library #20914)
• TableCell.test.js ([test] Migrate all Table components to testing-library #20914)
• TableFooter.test.js ([test] Migrate all Table components to testing-library #20914)
• TableHead.test.js ([test] Migrate all Table components to testing-library #20914)
• TablePagination.test.js ([test] Migrate all Table components to testing-library #20914)
• TableRow.test.js ([test] Migrate all Table components to testing-library #20914)
• TabList.test.js
• TabIndicator.test.js ([test] Migrate TabIndicator to react-testing-library #22906)
• TextField.test.js ([test] Migrate TextField to react-testing-library #22944)
• useTheme.test.js ([test] Migrate useTheme,withTheme to react-testing-library #22928)
• withTheme.test.js ([test] Migrate useTheme,withTheme to react-testing-library #22928)
• Zoom.test.js ([test] Migrate Zoom to react-testing-library #22914)

Integration

• test/integration/Menu.test.js
• test/integration/MenuList.test.js
• test/integration/NestedMenu.test.js
• test/integration/Select.test.js
• test/integration/TableCell.test.js ([test] Migrate TableCell to react-testing-library #23095)
• test/integration/TableRow.test.js ([test] Migrate TableRow to react-testing-library #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 1b43a2c#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.

[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]

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());
    });

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

[eps1lon]

Still have tests using createMount

[eps1lon]

The remaining tests either need enzyme or are testing legacy code.

Thanks everybody for your help. This will help alot during the React 18 pre-release.