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

React 18. set state in finally throws "act" warning, though test is passing #1051

Closed
JoeyAtSS opened this issue Apr 19, 2022 · 32 comments · Fixed by #1137
Closed

React 18. set state in finally throws "act" warning, though test is passing #1051

JoeyAtSS opened this issue Apr 19, 2022 · 32 comments · Fixed by #1137
Labels
bug Something isn't working

Comments

@JoeyAtSS
Copy link

JoeyAtSS commented Apr 19, 2022

  • @testing-library/react version: 13.1.1
  • Testing Framework and version: jest 27.5.1
  • DOM Environment: jest-dom 27.5.1

Relevant code or config:

  const handleSavedCardPayment = () => {
    const paymentInfo = {
      amount: formDataRef.current.amount,
      userId,
      accountId: formDataRef.current.selectedCardAccountId,
    };

    dispatch(makePayment(paymentInfo))
      .then(handlePaymentSuccess)
      .catch(noop) // already caught by thunk
      .finally(() => {
        formDataRef.current = null;
        setSubmitting(false);
      });
  };

What you did:

NOTE: This error only shows up after updating to react 18 and testing library to 13.1.1, this was not an issue in earlier version.

setSubmitting toggles the button disability. When the promise is complete, it turns the re-enables the button. so the test is doing a waitFor button to be re-enabled.

the promise looks more like this

dispatch(makePayment(paymentInfo)) returns a promise, with a catch inside.

so the whole promise chain looks like this
promisedFunction().then(() => do something).catch(error => show error). then(() => componentLevel).catch(do nothing here).finally(reset state)

What happened:

image

the "act" error is thrown, even though i've added a waitFor and that is passing to prove that that statement has already been rendered.

await waitFor(() => expect(screen.getByRole('button', { name: /submit/i })).not.toBeDisabled());

Problem description:

set state in finally seems to cause testing library to think act is incomplete, even though it is.

currently solving the error by putting the set state inside of catch
image

However, there's the WET code, i had to put the set state into handlePaymentSuccess too, to achieve the same results.

@intercaetera
Copy link

I encountered something similar while using Formik, I suspect this is related. Here is a repo which reproduces this issue:

This only happens on the new version of RTL and React 18.

@clothoo
Copy link

clothoo commented Apr 28, 2022

I'm using react-hook-form and experience the same act warning. Using either waitFor or waitForElementToBeRemoved doesn't remove the warning.

The only thing that worked is to wrap act with sleep.

  await act(async () => {
    await new Promise((resolve) => {
      setTimeout(resolve, 50);
    });
  });

credit to https://bufferings.hatenablog.com/entry/2021/11/18/015809

lifeisegg123 added a commit to r2don/react-query-toolkit that referenced this issue May 1, 2022
  - migrate react-testing-library to support react 18 ([unsolved warning](testing-library/react-testing-library#1051))
  - vitest to 0.10.0
lifeisegg123 added a commit to r2don/react-query-toolkit that referenced this issue May 1, 2022
  - migrate react-testing-library to support react 18 ([unsolved warning](testing-library/react-testing-library#1051))
  - vitest to 0.10.0
lifeisegg123 added a commit to r2don/react-query-toolkit that referenced this issue May 1, 2022
- migrate react-testing-library to support react 18 ([unsolved warning](testing-library/react-testing-library#1051))
  - vitest to 0.10.0
@hlmnd

This comment was marked as off-topic.

@eps1lon
Copy link
Member

eps1lon commented May 21, 2022

It doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://react.new), or a link to a repository on GitHub.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

@intercaetera
Copy link

@eps1lon Is my example in this comment not sufficient to demonstrate the issue?

@bedrich-schindler
Copy link

bedrich-schindler commented Jun 8, 2022

I have observed the same behavior as @JoeyAtSS. After update to the latest React 18 and the latest RTL, we had to change following across whole application, otherwise error from above is shown:

image

We also observed #1057. Unfortunately, I haven't observed pattern in which it happens.

@guiphc
Copy link

guiphc commented Jun 24, 2022

Hey guys, i made some tests because I am having the same problem:
This is what I got.

"@testing-library/react": "^13.3.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"@testing-library/user-event": "^14.2.1",

// it pass without act() warning
fireEvent.change(loginField, { target: { value: '' } })
expect(loginField).toHaveValue('')

// it gives the act() warning
fireEvent.change(loginField, { target: { value: '' } })
await waitFor(async () => {
  expect(loginField).toHaveValue('')
})

// don't wait the render - error
fireEvent.click(loginButton)
expect(screen.queryAllByText('Campo obrigatório')).toHaveLength(2)

// it pass all fine :)
fireEvent.click(loginButton)
await waitFor(async () => {
  expect(screen.queryAllByText('Campo obrigatório')).toHaveLength(2)
})

This way it works fine:

// it pass without act() warning
fireEvent.change(loginField, { target: { value: '' } })
expect(loginField).toHaveValue('') 

But if I change to userEvent, I get the act() warning all over again.

userEvent.clear(loginField)
expect(loginField).toHaveValue('')

@bobsilverberg
Copy link

I am having a similar problem after updating to react 18.2.0 and @testing-library/react 13.3.0. Some of my calls to dispatch store actions need to be wrapped in await act(async () => {...} ). I thought that @testing-library/react took care of the act wrapping for us.

@szimek
Copy link

szimek commented Jul 4, 2022

@eps1lon I replicated our issue here: https://codesandbox.io/s/rtl-react-18-act-issue-forked-l0bcj2?file=/src/__tests__/App.test.js.

You can see this error when you run tests and check the output in the console. In our case it's triggered by a component that uses react-query with dynamic import.

We're getting these warnings/errors in other places as well (e.g. we had 2 getByText(...).click() calls right after each other and after updating to React 18, I had to wrap each in a separate act call), but I wasn't able to create a minimal example for these.

@thepuzzlemaster
Copy link

thepuzzlemaster commented Jul 7, 2022

I'm running into the same problem upgrading my App to React 18, and RTL to 13.3.0.

In my case, I've even got a test which triggers an act warning for every letter pressed from await user.type() triggering an onChange handler, which just calls a setter from a useState hook.

Even if I add await waitFor(() => expect(myInput).toHaveValue(finalValue) it still throws the warnings (amongst hundreds of other act warnings in my other tests).

@Aerophite
Copy link

Aerophite commented Jul 13, 2022

Based on my tests, it looks like cleanup isn't being called at the end of every test as it used to. For me, if I manually unmount the component or call the cleanup function, the act errors go away.


Edit:

Note that calling cleanup in an afterEach will not work. You will have to actually call it at the bottom of the test. Something like this

test('should do something', () => {
  render(<Component />);
  // test stuff
  cleanup();
});

@ben-su
Copy link

ben-su commented Jul 13, 2022

We found that pinning the version of @testing-library/dom to the current version 8.16.0 using yarn resolutions drastically reduces the amount of act warnings for us.

@bertiecroll
Copy link

Having struggled with a lot of test was not wrapped in act(...) warnings in our tests as well, I noticed we had added @testing-library/dom in our project dependencies. On re-reading the @testing-library/user-event install guide it advises not to do this:

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.

On removing the dependency from our package.json all the warnings were gone. This may not be a fix for all the described issues, though adding comment here incase this helps any others having similar struggles.

For info, currently using:

"@testing-library/jest-dom": "5.16.4",
"@testing-library/react": "13.3.0",
"@testing-library/user-event": "14.3.0",
"react": "18.2.0",
"react-dom": "18.2.0"

@esslamben
Copy link

Just to add to this, we've noticed our tests riddled with act errors since moving to react 18. I've been able to solve most by swapping getBy to findBys and using waitFors however it has been a pain updating pretty much every test. In addition, I also had to enable globalThis.IS_REACT_ACT_ENVIRONMENT = true; so that I could use act errors to fix the state updates that I couldn't control via a findBy.

@martdavidson
Copy link

martdavidson commented Aug 2, 2022

Like others have mentioned, we're also seeing this pervasively after upgrading to react 18 and latest RTL.

"@testing-library/jest-dom": "5.16.4",
"@testing-library/react": "13.3.0",
"@testing-library/user-event": "14.4.0",
"react": "18.2.0",
"react-dom": "18.2.0",

Hoping for a solution that doesn't involve wrapping all our tests in additional act or adding cleanup() to every test or what have you.

Edit: In my case, it seems it's caused by remix-run/react-router#7634 as we're also upgrading to react router v6 as part of this.

The useNavigate() hook does not return a stable reference, and using it it in the dependency array of a useEffect causes any useEffect to run again if the location changes. The end result of this is that if a test navigates as part of the user interaction, the useEffect gets triggered again and if it updates state, it may be doing so after the test has passed and completed.

Just putting this here in case it helps any one else dropping by.

@nstepien
Copy link

nstepien commented Aug 5, 2022

// We just want to run `waitFor` without IS_REACT_ACT_ENVIRONMENT
// But that's not necessarily how `asyncWrapper` is used since it's a public method.
// Let's just hope nobody else is using it.
asyncWrapper: async cb => {
const previousActEnvironment = getIsReactActEnvironment()
setReactActEnvironment(false)

It looks like this is the culprit, it disables the "act environment" while running waitFor.
This doesn't make sense to me, any particular reason for doing this @eps1lon?

@eps1lon
Copy link
Member

eps1lon commented Aug 11, 2022

This doesn't make sense to me, any particular reason for doing this @eps1lon?

Because we're not acting when we wait for something to happen. Anything wrapped in act will only flush updates when we exit its scope. For example, if we would wait asynchronously for 5s which is wrapped in act then no state update would be flushed during that period defeating the purpose of waitFor

More detailed explainer: #937 (comment)

@nstepien
Copy link

Anything wrapped in act will only flush updates when we exit its scope.

Is this documented somewhere?

if we would wait asynchronously for 5s which is wrapped in act then no state update would be flushed during that period defeating the purpose of waitFor

Do we need to flush state updates? Don't they run anyway when they can?

@eps1lon
Copy link
Member

eps1lon commented Aug 11, 2022

Is this documented somewhere?

I guess the documentation for act() could be more explicit about this. It's somewhat implied by "state updates have been processed at the end" but I can see how this is fairly ambigious

Do we need to flush state updates? Don't they run anyway when they can?

If inside an act scope no state updates, effects etc are flushed. Only at the end. Which is why it wouldn't make sense to wrap a waiting period in act

@thepuzzlemaster
Copy link

I appreciate all the additional info here, both from maintainers and other users. I have never really wrapped my head around act(). I understand when it should be used, but I don't fully understand what it's actually doing. And I think because of that, I'm a bit unclear on what the right thing to do here is - to remove all of these act() warnings which started showing up after upgrading to React 18.

Is it something we as consumers should be doing differently in our tests? Something that needs to change on the React side? Something that needs to change in react-testing-library? Some combination of things? Something else entirely? Not clear yet?

I'm essentially delaying my upgrade to React 18, as a result of this issue, but it's not quite clear to me what the eventual fix will be. So if someone is able to add some clarity there, I would really appreciate that. 🙏

@nstepien
Copy link

I have a test roughly like this:

    setup();
    await userEvent.click(someButton);
    await waitFor(() => 
      expect(getCell()).toHaveTextContent('...');
    );

Clicking the button triggers an function with an API call roughly like this:

    async function onClick() {
      try {
        setLoading(true);

        const data = await api('POST', ...);

        // update the swr cache with the new data
        // this is what will update the DOM
        await mutate(...);
      } catch (error) {
        setError(error);
      } finally {
        setLoading(false);
      }
    }

The test works fine, but React still gives me an act warning about the setLoading(false) in the finally block.
Calling unmount() doesn't help either.

What I had to do was waitFor the loading state to be set to false by testing it

    setup();
    await userEvent.click(someButton);
    await waitForElementToBeRemoved(someSpinner);
    expect(getCell()).toHaveTextContent('...');

Now the act warning is gone, but this isn't ideal IMO as

  1. I don't care to test for the loading state itself
  2. I don't get a warning for this with React 17

🤷‍♂️

mstuartf added a commit to newscorp-ghfb/NewsKit that referenced this issue Aug 15, 2022
- https://floating-ui.com/docs/react-dom#testing
- https://testing-library.com/docs/user-event/intro/#difference-to-fireevent
- testing-library/react-testing-library#1051
- Switch from fireEvent to userEvent for floating-ui tests.
- Because floating-ui styling is applied async, a state update occurs in a Promise microtask leading to errors and warnings after upgrading React.
- Migrating to userEvent solves this issue.
- Some snapshot updates required as async styling unrelated to test that was not previously applied now is.
@CharlieCharlieCharlieCharlie

We are able to spot-fix tests by wrapping in act, but tests then fail previously passing cases with strange outputs that do not match manual testing.

Not to mention in our large-ish codebase, its nigh impossible to manually update the 1000+ tests we have...

@robin-drexler
Copy link

robin-drexler commented Nov 5, 2022

To be sure I'm not missing anything, I created two small codesandboxes with the same test.

In @testing-lib/react@^12 with react@^17, the following simple test does not produce an act warning. (presumably because waitFor is wrapped in an act)

https://codesandbox.io/s/act-warning-waitfor-react-17-btzykv

import { render, screen, waitFor } from "@testing-library/react";
import App from "./App";
import userEvent from "@testing-library/user-event";

describe("app", () => {
  it("turns on", async () => {
    render(<App />);

    await userEvent.click(screen.getByText("Turn on"));

   // I'm aware of find*, but the point is about `waitFor`
    await waitFor(async () => {
      screen.getByText("It is on");
    });

   // that's just here so the test doesn't finish prematurely 
    await new Promise((resolve) => {
      setTimeout(resolve, 300);
    });
  });
});

The component under test, shows some text on click after a few ms

import { useState } from "react";
import "./styles.css";

export default function App() {
  const [isOn, setIsOn] = useState(false);

  return (
    <div className="App">
      {isOn && <div>It is on</div>}
      <button
        onClick={() => {
          setTimeout(() => {
            setIsOn(true);
          }, 1000);
        }}
      >
        Turn on
      </button>
    </div>
  );
}

The same setup with @testing-library/react@^13 and react@18, now produces a warning.

https://codesandbox.io/s/act-warning-wait-for-react-18-w4e521

image

@eps1lon is this really the intended behaviour from now on or am I missing/misunderstanding something?

It also seems like waitFor sets global.IS_REACT_ACT_ENVIRONMENT to be false while it executes, which can cause the The current testing environment is not configured to support act(...) warning when one tries to manually wrap certain calls with act (I still need to wrap my head around this one 😅)

Example sandbox for this behaviour: https://codesandbox.io/s/act-warning-wait-for-react-18-forked-nc2mm8?file=/src/App.test.js

@szymonnowak-st
Copy link

szymonnowak-st commented Dec 14, 2022

It was already mentioned by others, but making sure that we only have one version of @testing-library/dom library helped us significantly reduce number of these warnings.

If you use yarn, you can run yarn why @testing-library/dom to see how many versions of this library you have and why. If you have more than one, it's probably best to uninstall all libraries that depend on it and reinstall them.

@cristianrodri
Copy link

I just upgraded @testing-library/dom to the latest version. yarn add -D @testing-library/dom@latest

@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2023

@eps1lon Is my example in #1051 (comment) comment not sufficient to demonstrate the issue?

@intercaetera It's not runnable. Even after fixing the lockfile (npm ci failed), npm test I got a Validation Error. Repros need to be minimal and easily reproducible.

The repro from @szimek worked i.e. I could reproduce the bug with their repro. It also had problems though since it didn't have a lockfile. NPM couldn't even install it anymore. Only yarn succeeded.
@szimek I could fix the missing-act warning with #1137 but the test is still failing which seems correct. The image is already rendered so the waitFor is not actually waiting for the data to render.

The 3rd repro from @robin-drexler finally worked (though still missing a lockfile). However, it's a clear example of a missing act. The warning got fixed by wrapping the state update in act:

-    await new Promise((resolve) => {
-      setTimeout(resolve, 300);
+    await act(async () => {
+      await new Promise((resolve) => {
+        setTimeout(resolve, 300);
+      });

In the future, please make sure repros are minimal and runnable indefinitely in the futre (e.g. they have a lockfile ensuring the same install, they have a Node.js version.

The repro from @szimek looks like a duplicate to #1125 so I'll close this issue once we land #1137

@eps1lon eps1lon linked a pull request Feb 16, 2023 that will close this issue
7 tasks
@eps1lon eps1lon added bug Something isn't working and removed needs more information labels Feb 16, 2023
@eps1lon
Copy link
Member

eps1lon commented Feb 16, 2023

Fixed in #1137
Released in @testing-library@14.0.0

@eps1lon eps1lon closed this as completed Feb 16, 2023
@annidai
Copy link

annidai commented Feb 17, 2023

I'm getting more act warnings now with @testing-library@14.0.0.
Tested with await waitFor, and still throwing act warnings with userEvent that triggers a state change.

Tested version:
"jest": "29.4.3",
"@testing-library/jest-dom": "5.16.5",
"@testing-library/react": "14.0.0",
"@testing-library/user-event": "14.4.3",

@zigang93
Copy link

@testing-library@14.0.0 still have same act warning

@smellai
Copy link

smellai commented Feb 20, 2023

@testing-library@14.0.0 still have same act warning

+1

@zigang93
Copy link

@eps1lon here is what I found out..

29.4.3 have act warning..
"jest": "^29.4.3"
"jest-environment-jsdom": "^29.4.3"

but 29.3.1 is working perfectly
"jest": "^29.3.1"
"jest-environment-jsdom": "^29.3.1"
@smellai give it a try, hope can help to solve it

@eps1lon
Copy link
Member

eps1lon commented Feb 20, 2023

For people still having issues, please file a new one and include a minimal, cloneable reproduction. Just a list of dependencies is not sufficient.

@testing-library testing-library locked as resolved and limited conversation to collaborators Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.