Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

safarcik
Copy link
Contributor

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

…(...)

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
Copy link
Contributor Author

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 */

@dlabaj dlabaj requested a review from jpuzz0 April 25, 2024 17:53
@dlabaj dlabaj added the bug Something isn't working label Apr 25, 2024
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jpuzz0
Copy link

jpuzz0 commented Apr 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants