mui / mui-public

The public mono-repository of MUI (as an organization), see mui/mui-private for the opposite.
80 stars 16 forks source link

[code-infra] In react tests, clean up our act() #174

Open oliviertassinari opened 4 months ago

oliviertassinari commented 4 months ago

1. Don't wrap fireEvent with act

Example: https://github.com/mui/material-ui/blob/0bddcac5beeb535c36fa2055082cc8324e59c282/packages/mui-material/src/Button/Button.test.js#L612-L615

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

+    fireEvent.keyDown(document.body, { key: 'TAB' });
     act(() => {
-      fireEvent.keyDown(document.body, { key: 'TAB' });
       button.focus();
     });

2. act should be awaited

As per https://github.com/mui/material-ui/pull/41061#discussion_r1631251653

From the docs:

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.

This is causing hard to debug issues. We should make our tests more robust and remove all sync usage of act.

     const button = getByRole('button');

     fireEvent.keyDown(document.body, { key: 'TAB' });
-    act(() => {
+    await act(async () => {
       button.focus();
     });

Also investigate if we can leverage eslint to discourage sync usage of act, see https://github.com/testing-library/eslint-plugin-testing-library/issues/915

Search keywords:

oliviertassinari commented 1 month ago

On point 2. I wonder how true "This is causing hard to debug issues." is. They seem to report errors when an await is missing: https://github.com/facebook/react/blob/1460d67c5b9a0d4498b4d22e1a5a6c0ccac85fdd/packages/react/src/ReactAct.js#L122

Edit: looks like it's not true, it's needed: https://github.com/testing-library/react-testing-library/issues/1061#issuecomment-1140112314.

Janpot commented 1 month ago

From https://github.com/mui/material-ui/pull/44028#pullrequestreview-2352318687, potential eslint rule to help use with 2:

'no-restricted-syntax': [
  'error',
  {
    selector: "CallExpression[callee.name='act'] > ArrowFunctionExpression:not([async=true])",
    message: 'Synchronous form of `act` is not allowed. Use `await act(async () => {...})` instead.',
  },
  {
    selector: "CallExpression[callee.name='act']:not(AwaitExpression > CallExpression[callee.name='act'])",
    message: '`act` must be awaited. Use `await act(async () => {...})` instead.',
  },
],

We could plan some time to do a full sweep of the codebase, in my limited experience the sync version doesn't always work as expected. I wouldn't be surprised if https://github.com/mui/mui-x/pull/14668 magically starts working if we remove all sync act calls.