patternfly / patternfly-react-seed

A PatternFly seed for React web applications
http://patternfly-react-seed.surge.sh/
MIT License
89 stars 250 forks source link

Fire events that update state during the test are wrapped in act(...) #222

Open safarcik opened 6 months ago

safarcik commented 6 months ago

When testing, code that causes React state updates should be wrapped into act(...) This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act

safarcik commented 6 months ago

This PR should fix this error during the tests:

➜  patternfly-react-seed git:(main) npm run test                              

> patternfly-seed@0.0.2 test
> jest

  console.error
    Warning: An update to NavList inside a test was not wrapped in act(...).

    When testing, code that causes React state updates should be wrapped into act(...):

    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */
jpuzz0 commented 6 months ago

Hello and thank you for putting up this PR!

I don't think act is the route we really want to go here though, as RTL actually uses it internally and discourages its explicit use whenever possible.

Please see this documentation we have about preferred approaches to handling async issues in RTL based tests , if you have already tried our more preferred approaches and none of them resolve the issues please let me know as it may be that we need to update our documentation and guidance here.

I agree with @wise-king-sullyman. From what I can see, the only possible necessary use-case for act is if there is meant to be state changes as a result of window.dispatchEvent(new Event('resize'));, but even then, it looks like waitForElementToBeRemoved is a helper that has act built-in that can be used in the tests I'm seeing here. It seems like all act warnings could be prevented without using importing/using act in this PR.