[ButtonBase] Improve test coverage

[eps1lon]

• improve type coverage by type checking the ButtonBase unit test (investigating viability of Migrate to TypeScript #15984)
• improve a11y coverage by moving to rtl
• use strict mode compatible test API (remove enzyme's simulate)
• enables a previously skipped test

All these points are coupled together (rtl is a bit more type friendly to the new testing approach, rtl enables new test etc).

Enables thinking about when to test for DOM properties vs HTML attributes so that was good practice.

[eps1lon]

This is passed to useImperativeHandle.

[mui-pr-bot]

No bundle size changes comparing ed4a76c...cc69b86

Generated by 🚫 dangerJS against cc69b86

[oliviertassinari]

How was your experience moving the tests from enzyme to testing-library?

[oliviertassinari]

Would using react-dom/test-utils solve the issue?

[eps1lon]

They don't create native events for you but dispatch a given one. I haven't looked for a package that creates "old events" ergonomically. I couldn't even find a way to create events in chrome 49 manually without react failing to identify the key.

It seems like key is the prioritized property by react to create the synthetic event which makes this the simplest solution.

[oliviertassinari]

So using react-dom/test-utils doesn't have an impact on how React polyfills the events.
What allowed the tests to pass previously in Chrome 49?

[eps1lon]

It wasn't an actual event just something that matched it structurally for the important parts. It works most of the time since we're not interested in the full event object but it makes the test a bit more robust against implementation changes while they are more brittle towards browser implementation.

I would definitely rethink this if we would support more older browsers but since it's only chrome 49 and we will likely phase that out in v5 I'm comfortable with that.

It also raises awareness about behavior that would require user/dev action in certain browsers like enabling touch events in edge.

[eps1lon]

How was your experience moving the tests from enzyme to testing-library?

Learned a bit about the DOM. Writing experience was pleasant since vscode works really well with typed libraries. Whenever I made a mistake refactoring the proper expect matchers helped identify the problem without me having to add another console.log. rtl also helps printing out the DOM tree when it can't find an element .e.g.

const button = getByRole('button'); will log

cannot find element with [role="button"] in 
<div>
  <button type="button">Hello</button>
</div>

so you immediately know you should've used getByText¹ and don't have to debug manually.

Some things are obviously hard like the centerRipple but we don't have a test for that anyway.

Tests are now more descriptive and it's easier with them to explain to someone how something should behave i.e. every time you do something that nobody will ever do in their codebase/browser you leave some question marks. It's obviously easier in some cases like focus compared to an Enter keyDown which has like 1 or 2 pages of spec text.

All we can do is try to improve instead of giving up immediately because it won't be perfect.

[eps1lon]

Forgot to add the ¹: Going back and forth whether role="button" should match <button /> as well but a native button has different behavior than just role="button" so I guess the current behavior is ok. I wish browsers would apply behavior based on roles as well.

[oliviertassinari]

Sweet, thanks for sharing the journey.

[eps1lon]

Forgot to add the 1: Going back and forth whether role="button" should match <button /> as well but a native button has different behavior than just role="button" so I guess the current behavior is ok. I wish browsers would apply behavior based on roles as well.

Turns out I was wrong about that. The documentation for getByRole was incomplete. expect(render(<button />).getByRole('button')).toBeInTheDocument() works perfectly fine since getByRole considers default roles.