[test][material-ui] Remove unnecessary async act calls

[aarongarciah]

We're working on adopting eslint-plugin-testing-library (issue) and it errors when it finds unnecessary act calls wrapping Testing Library helpers like fireEvent, because these helpers already call act for us.

This PR removes unnecessary act calls from several test suites, making the code follow the "rules" of Testing Library and thus removing the ESLint errors. Also adds await / async to act calls in the modified files as recommended in the React docs.

Context: #41061 (comment)

(Thanks @Janpot for the tip!)

[mui-bot]

Netlify deploy preview

https://deploy-preview-42942--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against cb529fe

[aarongarciah]

@romgrk let me know if you're ok with these changes.

[aarongarciah]

Per React docs, act is meant to be awaited and the callback passed to it to be async:

We recommend using act with await and an async function. Although the sync version works in many cases, it doesn’t work in all cases and due to the way React schedules updates internally, it’s difficult to predict when you can use the sync version.

We will deprecate and remove the sync version in the future.

Source: https://react.dev/reference/react/act#await-act-async-actfn

[aarongarciah]

We should probably create an ESLint rule for this in the future if there's no "official" one.

[romgrk]

LGTM, as long as the tests pass I don't really have an opinion. But IIUC all the comments, the fireEvent calls should be awaited because they call act and React wants act to be async only, eventually.

[Janpot]

But IIUC all the comments, the fireEvent calls should be awaited because they call act and React wants act to be async only, eventually.

The fireEvent calls don't seem to be async though, they must rely on the sync behavior of act still. I quickly skimmed their repo but can't seem to find any info around this. Do you think we should await those calls (without wrapping them with act) to future proof the tests?

[aarongarciah]

fireEvent calls should be awaited because they call act and React wants act to be async only, eventually

I left a question in the Testing Library Discord to see if the plan is to update eventWrapper (which wraps all fireEvent methods afaik) to await all act calls: https://discord.com/channels/723559267868737556/723565903228305549/1262686782005182527

I wonder if we should wait for @testing-library/react to await all act calls, given it's the official testing recommendation, and given that tests work as expected (for now). Let's see if we get a response.

[aarongarciah]

The fireEvent calls don't seem to be async though, they must rely on the sync behavior of act still. I quickly skimmed their repo but can't seem to find any info around this. Do you think we should await those calls to future proof the tests?

This is the eventWrapper implementation in @testing-library/react, which wraps all fireEvent methods afaik: https://github.com/testing-library/react-testing-library/blob/main/src/pure.js#L60-L66

And this is where it's used in fireEvent in @testing-library/dom: https://github.com/testing-library/dom-testing-library/blob/main/src/events.js#L5-L19

[aarongarciah]

@⁠⁠eps1lon is working on await act. See testing-library/react-testing-library#1214. It looks like it will take some time seeing the amount of work it requires and the time the PR has been open.

[Janpot]

Looks great

[aarongarciah]

@romgrk @Janpot this is the response from @⁠eps1lon after being questioned about if we should do await act(async () => fireEvent(...)):

That would be quite the heavy lift for you. When there's a deprecation warning, we'll have a codemod and changes to RTL. Until then I wouldn't advise library maintainers to spend time on this just yet.

That may not necessarily apply to libraries using use heavily but for MUI I don't you'd meaningfully change what you're testing by moving to await act

We can forget about this for now. I'd recommend using await on our act calls though.