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

userEvent.click triggers warning that it must be wrapped in act in beforeEach #497

Closed
amandapouget opened this issue Nov 18, 2020 · 25 comments

Comments

@amandapouget
Copy link
Contributor

  • @testing-library/user-event version: 12.2.2
  • Testing Framework and version: Jest 26.6.3
  • DOM Environment: whatever default ships with Jest 26.6.3

Relevant code or config

    beforeEach(() => {
        userEvent.click(button.getByTestId('foobar'));
    });

What you did:
Ran the test

What happened:

Warning: An update to Foobar 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 */
    
    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
        in Foobar

Reproduction repository:

Problem description:
We don't have this problem when clicking the same button inside the test itself. But in beforeEach, it flips out and demands that we wrap in act and await. Then the warning goes away.

Suggested solution:
Either fix it or document it in the description for userEvent.click.

@kentcdodds
Copy link
Member

Hi @amandapouget,

We don't have this problem when clicking the same button inside the test itself. But in beforeEach, it flips out and demands that we wrap in act and await.

Here's a blog post to explain what that warning is all about: https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning

And here's another blog post to suggest you change the structure of your test to avoid issues like this: https://kentcdodds.com/blog/avoid-nesting-when-youre-testing

This may help as well: https://kentcdodds.com/blog/write-fewer-longer-tests

The recommendation is to restructure your test and not do stuff like this in those setup/teardown hooks (like beforeEach and afterEach). Good luck!

@amandapouget
Copy link
Contributor Author

Can you update the documentation for userEvent.click to say either:
a) do not use in beforeEach, or
b) you must wrap in act when using in beforeEach

Right now the docs say that the only time you need to wrap it is when you are using delay, and this example shows that is simply not true.

@kentcdodds
Copy link
Member

You're more than welcome to make a PR for recommendation a.

Right now the docs say that the only time you need to wrap it is when you are using delay, and this example shows that is simply not true.

This is incorrect. userEvent.click is already wrapped in act. The reason the warning is going away fro you isn't because of act but because of async/await. If you read the blog posts I provided hopefully they will explain why.

@amandapouget
Copy link
Contributor Author

amandapouget commented Nov 18, 2020

I am reading through them but struggling to find the relevant line of code that explains the functional difference.

When I do:

it('something, () => {
   userEvent.click(<anything>)
});

I do not get a warning. But if move that same line into a beforeEach, I do.

@amandapouget
Copy link
Contributor Author

So it sounds like I need to open a PR that says, "You do not need to await userEvent.click unless a) you are using delay or b) you are using it in before/after blocks".

Correct?

@kentcdodds
Copy link
Member

The recommendation should be to not use this in before/after blocks at all and you only ever need to use await when using type with a delay. And you never need to use act because all the utilities are already wrapped in act.

@amandapouget
Copy link
Contributor Author

amandapouget commented Nov 18, 2020

Whether or not to use before/after blocks is a stylistic choice.

Users do not expect there to be a functional difference between putting something in beforeEach and duplicating the same line in every it statement.

Your posts make a compelling stylistic case for "never use before/after" in general, but there's nothing I see in these posts that claims there is a functional difference.

Yet, there seems to be some functional difference with userEvent.click since you have to await it in before/after but do not in it.

I plan to open a PR that says, "Stylistically, we do not believe in using before/after blocks. See [blog posts about stylistic reasons]. Functionally, if you do choose to use a before/after block, you must await userEvent.click to avoid warning messages."

@kentcdodds
Copy link
Member

Whether or not to use before/after blocks is a stylistic choice. Users do not expect there to be a functional difference...

This is also incorrect. As your example demonstrates it makes a semantic difference.

You can feel free to open a PR like that, but it will not be merged if it doesn't communicate our intended recommendation.

@amandapouget
Copy link
Contributor Author

Feel free to change the language of the PR as needed.

@amandapouget
Copy link
Contributor Author

Yes, my example does illustrate there is some kind of functional difference.

I didn't see anything in your blog posts that highlighted the functional difference that is relevant to the behavior in this case. So I still don't get why, even after reading all three posts.

If you can point me to where in the blog post, you talk about that functional difference, I will more clearly reference or summarize that difference in the PR.

@kentcdodds
Copy link
Member

I'm afraid that I don't have any more time to explain it now, but if you'd like to come to my office hours sometime I could walk you through it then :)

@amandapouget
Copy link
Contributor Author

Sure, sounds good.
I read the post again and still didn't see anything there that explains the difference for this case.

I'll come to office hours and then maybe we can add a sentence of explanation to either the blog post, the README, or this issue. So it won't just say "don't use because style", it says "don't use because functional reason".

That way a future dev has a fighting chance at discovering the why and you don't have to explain it to anyone else. :-)

@ph-fritsche
Copy link
Member

While I don't see good reason to use userEvent inside of beforeEach, it certainly is possible and does not require some extra act per se:
https://codesandbox.io/s/quiet-fast-clsk9?file=/src/App.test.js

I guess your test setup - if performed in specific order - covers up some delayed state change.
You might want to examine what exactly makes your code run synchronously / in specific order in one case and asynchronously in the other. Maybe a mocked window.setTimeout?

@zigang93
Copy link

zigang93 commented Jun 13, 2022

I am very struggle with act warning when using userEvent@14 for few days, I tried to understand the best practice of writing good test suits for react app.. but due to breaking changes on userEvent@14, I had to change to async await method when using userEvent.click() or etc.. that was my first struggle I had when follow others answer at stack overflow which already outdated..

I had read this blog https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning but because of my project is using the mui packages.. I had lost my code control to fix the bug..

The error that I get was An update to ForwardRef(FormControl) inside a test was not wrapped in act(...). .. I had double check my code.. clearly is from mui side..

so the alternative way is using fireEvent,then the most confuse things is that why fireEvent no warning at all but userEvent do have warning for act.. base on my research. both of them should wrapped with act..

writing test for front end is really difficult compare to backend..

@lcundiff
Copy link

lcundiff commented Jun 14, 2022

The recommendation should be to not use this in before/after blocks at all and you only ever need to use await when using type with a delay. And you never need to use act because all the utilities are already wrapped in act.

While I don't see good reason to use userEvent inside of beforeEach, it certainly is possible and does not require some extra act per se: https://codesandbox.io/s/quiet-fast-clsk9?file=/src/App.test.js

One use case would be to test the separate parts of the UI after a button press. You could also copy & paste code for each test and wait for it to finish, but you could say that for almost any use case for beforeEach. I believe the intuitive name implies it should be used for code that needs to be parsed before every test.

I'm not clear on why it is bad practice to fire events in before functions and I think people would benefit from an explanation on a highly visited issue like this. The article referenced is against mutable variables and nesting testing in general, so it didn't answer the question specifically. It also was written by the same developer that attached the article, leaving room for developer bias (although it was a informative read). The article also recommends not using mutable variables in before/after functions, yet the React Example for beforeEach uses a mutable variable. So doesn't appear to be bad practice among the react team.

If an explanation could be given, or give an option for user event to be waited on within the built-in 'act', I think that would be helpful. In terms of UX principle, if multiple developers are using a function 'wrong' than the design should be reconsidered, not blaming the user.

@ph-fritsche
Copy link
Member

  1. Some technical information on this issue is outdated. This issue was based on [email protected]. In the latest (and currently only supported version) user-event@14 all methods are asynchronous.
  2. As demonstrated in the Codesandbox example above, the issue is based on incorrect information and was actually a non-issue to begin with.
    (Note that Codesandbox was also updated since, you might need to recreate it locally if you want to verify.)
  3. user-event should not be wrapped in act. If you get an act warning, your code does something it either shouldn't or that you are not aware of - like a delayed asynchronous state update.
  4. Such an update might be the result of the test in question or a leaking test running earlier.

One use case would be to test the separate parts of the UI after a button press. [...] I believe the intuitive name implies it should be used for code that needs to be parsed before every test.

An interaction that involves moving parts of the component under test should never be before the test. A failure of this step is a failure of the component under test.

You could also copy & paste code for each test and wait for it to finish, but you could say that for almost any use case for beforeEach

And those cases not covered by "almost" should go into a beforeEach. Rule of thumb: If there's a symmetrical afterEach/afterAll, the code should go into a before hook. If there isn't, it is a mistake most of the time.

There is no need to copy&paste code. Write a renderThisPartOfMyApp function and invoke it in the first line of the test. The next dev who has to work with the test might thank you, because they can follow the function name to its declaration and don't need to jump through code to find beforeEach hooks that might or might not be there.

I'm not clear on why it is bad practice to fire events in before functions and I think people would benefit from an explanation on a highly visited issue like this. [...] developer bias

This recommendation is of course biased. There is no technical barrier preventing you from writing you whole test in a beforeEach hook. It's just that each line in a before hook has the potential to obfuscate a problem or - like this issue demonstrates only too well - to make people look for problems where there is none. That's why we recommend to avoid this.


If you have further questions:
Please, let's keep this issue board tidy and use it for discussing technical details on definite issues.
We've got a Discussions board with Q&A section and a Discord server for everything else.

@ronny-lark
Copy link

ronny-lark commented Apr 27, 2023

I just tried using userEvents over fireEvents and am utterly confused about this.
My tests are not nested in the least, yes I'm getting the act warning over this line in my component:

    > 24 |   const onClick = () => setEditing(true);

This is the simplest line I can possible imagine for changing state yet jest asks me to wrap it.
All my tests work perfectly fine with a synchronous fireEvent.click, but the following won't work without wrapping it in act of waitFor:

const user = setup()
render(<Component />)
const component = screen.get...
await user.click(component)

No idea what's going on.

@maurer2
Copy link

maurer2 commented Apr 27, 2023

Hello, I believe the findBy-queries are asynchronous, so you need to use await, e.g.

const component = await screen.findBy...

@ronny-lark
Copy link

I meant get. Fixed in my comment. I stress again that my tests are fully synchronous using fireEvent and work just fine.

@veloware
Copy link

I just tried using userEvents over fireEvents and am utterly confused about this. My tests are not nested in the least, yes I'm getting the act warning over this line in my component:

    > 24 |   const onClick = () => setEditing(true);

This is the simplest line I can possible imagine for changing state yet jest asks me to wrap it. All my tests work perfectly fine with a synchronous fireEvent.click, but the following won't work without wrapping it in act of waitFor:

const user = setup()
render(<Component />)
const component = screen.get...
await user.click(component)

No idea what's going on.

Ive noticed in my tests the exact same thing, and its ever since I added await to the userEvent... line as the test is now async based on some other factors. Now after adding the await before the userEvent, ive had to wrap in act -

await act(async() => {
   await userEvent.type(...)
})

seems very verbose, i'm also confused by this, but it seems to work....

@mikeybags
Copy link

mikeybags commented May 25, 2023

I just tried using userEvents over fireEvents and am utterly confused about this. My tests are not nested in the least, yes I'm getting the act warning over this line in my component:

    > 24 |   const onClick = () => setEditing(true);

This is the simplest line I can possible imagine for changing state yet jest asks me to wrap it. All my tests work perfectly fine with a synchronous fireEvent.click, but the following won't work without wrapping it in act of waitFor:

const user = setup()
render(<Component />)
const component = screen.get...
await user.click(component)

No idea what's going on.

Ive noticed in my tests the exact same thing, and its ever since I added await to the userEvent... line as the test is now async based on some other factors. Now after adding the await before the userEvent, ive had to wrap in act -

await act(async() => {
   await userEvent.type(...)
})

seems very verbose, i'm also confused by this, but it seems to work....

I've run into the same thing as well, and had to do the same thing to get rid of the warnings.

@ph-fritsche
Copy link
Member

ph-fritsche commented Jun 16, 2023

Do not just add act() calls until React shuts up! https://ph-fritsche.github.io/blog/post/react-act
☝️ This article explains why adding act is usually not the correct fix when there is an act warning.

Either your code has an unexpected state update or your setup isn't correct.

If you use one of the framework wrappers, it is important that @testing-library/dom is resolved to the same installation required by the framework wrapper of your choice.
Usually this means that if you use one of the framework wrappers, you should not add @testing-library/dom to your project dependencies.

As mentioned earlier this issue already contains outdated and unrelated information. Please use the Q&A section or join our Discord server if you need help setting up your test environment or writing tests.

@kanjurer
Copy link

kanjurer commented Jul 7, 2023

Did anyone figure out why this works:

await act(async () => {
   await user.click(showDetailsButton);
});

and this does not:

await user.click(showDetailsButton);

@VinokurovAlexander
Copy link

Did anyone figure out why this works:

await act(async () => {
   await user.click(showDetailsButton);
});

and this does not:

await user.click(showDetailsButton);

seems like you have a different versions of @testing-library/dom package in project, take a look on this discussion, it helped me

@kanjurer
Copy link

Thank you, it fixed my issue

dbkr added a commit to element-hq/matrix-react-sdk that referenced this issue Sep 20, 2024
I have no idea why this is flaking. There are warnings about
things not being wrapped in act() which may be relevant... this makes
the warnings happy, although apparently should not be necessary.
testing-library/user-event#906 and
testing-library/user-event#497 are
depressing reading (making the versions the same didn't help). I think
my conclusion might be to do this until we're able to upgrade to the
latest testing-library, then re-evaluate.

It still may or may not fix the flake.
github-merge-queue bot pushed a commit to element-hq/matrix-react-sdk that referenced this issue Sep 23, 2024
I have no idea why this is flaking. There are warnings about
things not being wrapped in act() which may be relevant... this makes
the warnings happy, although apparently should not be necessary.
testing-library/user-event#906 and
testing-library/user-event#497 are
depressing reading (making the versions the same didn't help). I think
my conclusion might be to do this until we're able to upgrade to the
latest testing-library, then re-evaluate.

It still may or may not fix the flake.
github-merge-queue bot pushed a commit to element-hq/matrix-react-sdk that referenced this issue Sep 23, 2024
I have no idea why this is flaking. There are warnings about
things not being wrapped in act() which may be relevant... this makes
the warnings happy, although apparently should not be necessary.
testing-library/user-event#906 and
testing-library/user-event#497 are
depressing reading (making the versions the same didn't help). I think
my conclusion might be to do this until we're able to upgrade to the
latest testing-library, then re-evaluate.

It still may or may not fix the flake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests