[test] Replace waitFor() with act()

[oliviertassinari]

A follow-up on #14124 (comment). I believe the use of waitFor() should be avoided as much as possible. We want to be able to test changes with incremental steps, so using waitFor feels fundamentally a last resort option.

I have removed a few to see how it would go.

A side note, there is maybe a need to move our act to be awaited mui/mui-public#174.

[mui-bot]

Deploy preview: https://deploy-preview-14851--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 191aa84

[oliviertassinari]

The initial fix. The rest is me curious if the others are needed too.

[LukasTy]

I believe the use of waitFor() should be avoided as much as possible. We want to be able to test changes with incremental steps, so using waitFor feels fundamentally a last resort option.

I'm not sure about this assumption. 🤔
I think that it has it's place, when async side effects are involved, but I do agree, that in most cases it can be avoided. 👍
Kent doesn't discourage it's usage, just provides some recommended usage examples.

[LukasTy]

Could you clarify why this has been added here? 🤔
There does not seem to be any async action within this act. 🤷

[oliviertassinari]

@LukasTy I used to think the same as you: How can this be any different? But I have noticed behavior changes.

If I remove the async in https://github.com/mui/base-ui/blob/463d61a3befa8676e564db695eee2ef645060cec/packages/mui-base/src/Tooltip/Root/TooltipRoot.test.tsx#L103 , it crashes the test, with this error:

The current testing environment is not configured to support act

That error is coming from: https://github.com/facebook/react/blob/1460d67c5b9a0d4498b4d22e1a5a6c0ccac85fdd/packages/react-reconciler/src/ReactFiberAct.js#L54

I have seen the same in testing-library/react-testing-library#1061 (comment).

Looking at the source, I don't quite understand why: https://github.com/facebook/react/blob/1460d67c5b9a0d4498b4d22e1a5a6c0ccac85fdd/packages/react/src/ReactAct.js#L109. Maybe it's because we use a fake clock.

@Janpot A preference on this? You flagged this as wrong in testing-library/eslint-plugin-testing-library#915

[LukasTy]

Nice. This means that I've been fighting with this error for a bit as well. 🤯 🙈
Adding async in the await act also helps me avoid the flaky error in my cases. 👍

[Janpot]

My preference is to follow what React suggests, which is to always use the async version as they plan to deprecate the sync version. That's why I opened that feature request. I noticed while fixing act calls in tests (e.g. remove ones that should be unnecessary and wrap calls that should be wrapped) that it magically made tings work more predictably 🙂.

I believe the use of waitFor() should be avoided as much as possible. We want to be able to test changes with incremental steps, so using waitFor feels fundamentally a last resort option.

I somewhat disagree with this statement though. I believe the primary reason (not the only) for having tests is to have a codebase that is optimized for refactoring. The most useful tests in that mindset are the ones that don't need an update when implementation changes. act makes tests rely on implementation details and waitFor takes those assumptions away. Not to say that you can't have any tests that test implementation details, just the bulk are testing behavior and the closer you can get your test to real user interaction, the more useful they become (real user events, wait for a11y selector, screenshots,...). Most of the time comes at a cost though 🙂.

[oliviertassinari]

which is to always use the async version as they plan to deprecate the sync version

@Janpot Agree, I opened mui/material-ui#44028 and mui/base-ui#706 for this reason. But I was wondering about

await act(() => {
// vs.
await act(async () => {

The most useful tests in that mindset are the ones that don't need an update when implementation changes.

I agree with this. My assumption is that for act() to not be enough and having to need waitFor(), it must mean that we are doing something creative, in 90+% of the time, it shouldn't be needed. I would expect, as a developer, to be able to take a component from MUI and write a test with prop changes or act() but without waitFor().

[Janpot]

I would expect, as a developer, to be able to take a component from MUI and write a test with prop changes or act() but without waitFor().

I see where you're coming from, but I expect the opposite. The way I see it is that the only reason this works is because we execute those events synchronously. Which is quite far from real world usage. To bring the test closer to real user behavior you'd have to think about it as if you were interacting with the page through the event loop. i.e. executing the .click() only schedules an event on the event loop and then returns, instead the current behavior as if we're actually in control of when the event fires. If that were the behavior, the only way to test whether a change happened is through waitFor. This is much closer to how you'd interact with a real browser, e.g. over the devtools protocol, and therefore much closer to real user interaction.

[oliviertassinari]

This is much closer to how you'd interact with a real browser, e.g. over the devtools protocol, and therefore much closer to real user interaction.

@Janpot If these were e2e tests, yes, I would expect the same. But those are not e2e tests: we don't fetch data from the network, we don't do real event interactions, so I expect deterministic behavior.

[Janpot]

But it's act that introduces the non-determinism. Tests randomly break on patch releases of dependencies, when the implementation slightly changes, when running them in browsers vs. node, When react updates, when passing it an async vs sync function,... I fail to see what this notion of deterministic behavior do for us.

It's not because we use user-event and .waitFor that our tests becomes an e2e-tests. I don't feel like there is much to gain from rigorously categorizing tests this way anyway. It's still a component as a unit that's under test. And it's still testing the exact same behavior. But what it's not doing is alter React behavior in a way that never occurs in the real world. And fire events in isolation that would never fire in isolation in the real world.

[oliviertassinari]

Tests randomly break on patch releases of dependencies, when the implementation slightly changes, when running them in browsers vs. node, When react updates, when passing it an async vs sync function

Ah, then act() is not doing its job 😀. It didn't used to be like this. It would reliably work.

I fail to see what this notion of deterministic behavior do for us.

For me, it's about, after doing an action, knowing that the state if reflected in the DOM without having to think about a complex selector for it (waitFor), as a developer, I want to use one that will always work (act). Then, I can console.log, inspect the DOM, etc. It will always show me up-to-date information, no stale state.

[oliviertassinari]

I'm not sure about this assumption

@LukasTy In this link, it's says:

waitFor is intended for things that have a non-deterministic amount of time between the action you performed and the assertion passing.

I'm trying to remove waitFor() from all the places where we have deterministic behavior, which I started this PR with:

-apiRef.current.setPage(1);
-await waitFor(() =>
-  expect(Object.keys(apiRef.current.state.rowSpanning.spannedCells).length).to.equal(1),
-);
+await act(async () => {
+  apiRef.current.setPage(1);
+});
+expect(Object.keys(apiRef.current.state.rowSpanning.spannedCells).length).to.equal(1);

as a perfect example of what we want to see.