facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.33k stars 46.37k forks source link

Unhelpful warning for `act` for react-dom@16.8 #14769

Closed ncphillips closed 5 years ago

ncphillips commented 5 years ago

Do you want to request a feature or report a bug?

Feature/Improvement

What is the current behavior?

If there is test code that should be wrapped in act(...) then the current warning is given:

 console.error node_modules/react-dom/cjs/react-dom.development.js:506
    Warning: An update to null 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

When upgrading a large code base, this is basically useless.

What is the expected behavior?

Provide at least the test name if not the line number of code that triggered the warning.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

react@16.8.0 react-dom@16.8.0

bvaughn commented 5 years ago

Provide at least the test name if not the line number of code that triggered the warning.

console.error should display line numbers (and a call stack) which should include the test code that triggered this warning. Is this not happening?

bvaughn commented 5 years ago

When upgrading a large code base

Follow-up question: Does this imply that you have a large codebase making use of hooks?

ncphillips commented 5 years ago

It is happening, but the error is in react-dom

image

Follow-up question: Does this imply that you have a large codebase making use of hooks?

Yup. At least I think the codebase is large, maybe just medium 🤷‍♂️. There's about 800 /.tsx?/ files. We (perhaps foolishly) have been using the alpha version in production for the last month and have been experimenting with hooks in a variety of places. It just so happens that one of those places is a widely used component that is being rendered in a lot of tests.

bvaughn commented 5 years ago

I wouldn't expect that. Do other console.error logs have their call stacks truncated as well?

To be clear, the error is logged from within react-dom – but the call stack should show the other frames that got to that point (e.g. where in react-dom was warningWithoutStack called) up to, and including, your individual test.

threepointone commented 5 years ago

Could you also share your testing setup? Do you use enzyme/react-testing-library? How do you do async testing?

gaearon commented 5 years ago

"An update to null" looks weird. Can you try to create a reduced reproducing test case?

It is supposed to include the name of your component.

gaearon commented 5 years ago

Also, what test runner are you using? Can you create a minimal repro?

ncphillips commented 5 years ago

We're using jest with react-testing-library.

I'll try to carve out some time later to get repro.

threepointone commented 5 years ago

react testing library just released an update that integrates act by default with its render/fireEvent calls, so it should pass a bunch of your tests. Any remaining tests probably have explicit mutations, and you should wrap them with act() calls

ncphillips commented 5 years ago

I did forget to update react-testing-library however upgrading to 5.5.1 did not fix the warnings. Maybe it did fix some, but not all. I don't have a count of how many warnings there were from running the tests.

Note: all our tests do pass, they just get those warnings.

threepointone commented 5 years ago

Without act calls, it's likely your tests might not even be accurate. I updated a whole bunch of fb code for the fb codebase and it wasn't too bad - went through every warning, went to the test that the stack pointed to, and wrapped relevant calls with act() (and caught a couple of bugs in the process)

I'm curious why your stack is getting clipped.

danielkcz commented 5 years ago

I think this issue is more about making that warning more useful. I've been fighting with same thing yesterday. For a reference have a look at travis output. It's also Jest + react-testing-library.

There is simply no means on figuring out what piece of code is causing that warning because Jest shows only a line where the console.error was produced. At least these are bundled per a test file for some initial clue, but it's not enough.

I think ideally this bulky warning should be there just once and then show only a more succinct variant for each occurrence that tells more about the location of it.

ghengeveld commented 5 years ago

Having the same issue while trying to update the specs for useAsync to v16.8.1. The warning just refers to react-dom, not my own code.

schermafbeelding 2019-02-07 om 11 50 53

Besides this, I still have not found a way to actually fix the tests. Wrapping with act doesn't work. Using jest.useFakeTimers suggested here doesn't work either. Please provide decent documentation on how to write tests for custom hooks.

ghengeveld commented 5 years ago

For the record, here's how I was testing the useAsync hook (simplified):

const Async = ({ children, ...props }) => {
  const renderProps = useAsync(props)
  return children(renderProps)
}

test("returns render props", async () => {
  const promiseFn = () => Promise.resolve("done")
  const component = <Async promiseFn={promiseFn}>{({ data }) => data || null}</Async>
  const { getByText } = render(component)
  await waitForElement(() => getByText("done"))
})

useAsync calls a whole bunch of different hooks underneith.

bvaughn commented 5 years ago

Please provide decent documentation on how to write tests for custom hooks.

I know it's a bit annoying at the moment, but please be patient with us. We're working on this (both the docs, and the underlying issue).

ghengeveld commented 5 years ago

Good to know, and sorry for the harsh-sounding comment, you're doing an excellent job.

gaearon commented 5 years ago

Please provide decent documentation

If anything, being demanding in issues is a good way to discourage work on something. For example I've spent a lot of effort on this documentation (about a month of work), and comments like this make me not want to even open it anymore. Just something to keep in mind.

gaearon commented 5 years ago

@ncphillips We'd still appreciate a reproducing case to see how you got null in the warning message.

ncphillips commented 5 years ago

Sorry @gaearon it's proving difficult to reproduce 🤔

It's hard to identify which tests are causing the warning, because they're all passing and since I don't have a warning count, I wouldn't even know if I fixed one.

I'm starting up a repo now to try and reproduce, but I don't have much time today.

panjiesw commented 5 years ago

I created a simple repro for the warning message here https://github.com/panjiesw/act-warning. It uses jest (from CRA) and react-testing-library v5.5.3

In that repro, the test passed but the warning is still there and only showed component's name, without stack trace and line number

The component code:

import React, { useReducer, useEffect } from 'react';

function reducer(state, action) {
  switch (action.type) {
    case 'value':
      return {
        ...state,
        value: action.payload,
      };
    case 'isBar':
      return {
        ...state,
        isBar: action.payload,
      };
    default:
      return state;
  }
}

const Input = () => {
  const [state, dispatch] = useReducer(reducer, { value: '', isBar: 'no' });

  useEffect(() => {
    let mounted = true;
    Promise.resolve().then(() => {
      if (mounted) {
        dispatch({
          type: 'isBar',
          payload: state.value === 'bar' ? 'yes' : 'no',
        });
      }
    });
    return () => {
      mounted = false;
    };
  }, [state.value]);

  const onChange = e => {
    dispatch({ type: 'value', payload: e.target.value });
  };

  return (
    <div>
      <input data-testid="foo" value={state.value} onChange={onChange} />
      <div data-testid="check">isBar: {state.isBar}</div>
    </div>
  );
};

export default Input;

The test:

import React from 'react';
import { render, fireEvent, cleanup } from 'react-testing-library';
import Input from './Input';

afterEach(cleanup);

jest.useFakeTimers();

test('Input using hooks', done => {
  const w = render(<Input />);

  fireEvent.change(w.getByTestId('foo'), {
    target: { value: 'bar' },
  });

  process.nextTick(() => {
    expect(w.getByTestId('foo').value).toEqual('bar');
    expect(w.getByTestId('check').textContent).toEqual('isBar: yes');
    done();
  });
});
hisapy commented 5 years ago

Hi devs,

In my case, I'm testing a simple component that useState with mount from enzyme.

Notice that I'm not testing the hooks. I'm just mounting the component and simulating some onChange and onClick and then asserting what props changed.

Everything passes except that I get the warning:

console.error ../../node_modules/react-dom/cjs/react-dom.development.js:506
    Warning: An update to _default 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 the docs it says:

To reduce the boilerplate, we recommend using react-testing-library which is designed to encourage writing tests that use your components as the end users do.

What does it means for enzyme users? I have a ton of tests that are working but a ton of these warnings as well ... Perhaps, is there a way to silence them?

ghengeveld commented 5 years ago

It's not just Enzyme, you'd have the same issue with react-testing-library right now. The team is aware of the issue and working on improving the situation, but having just launched a revolutionary new API means they are probably scrambling to fix a whole lot of other issues as well. It's frustrating if you want to keep up to date with the latest and greatest, but that's how it is. For now I'm just going to disable the affected tests and fly with it.

swapkats commented 5 years ago

How could I disable this warning. I use jest and enzyme. I understand the implications but would really like to suppress this warning as it causes unnecessary scrolling and sort of throws me off my workflow.

hisapy commented 5 years ago

I added this to my test files

// Silence until enzyme fixed to use ReactTestUtils.act()
// If not silenced we get the following warning:
// console.error../../ node_modules / react - dom / cjs / react - dom.development.js: 506
// Warning: An update to _default 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

const originalError = console.error;

beforeAll(() => {
  console.error = jest.fn();
});

afterAll(() => {
  console.error = originalError;
});

describe("onSubmit login", () => {
  // tests here
})

If you want to suppress the warnings globally instead of file by file, you might want to try the above in a jest setup file

gaearon commented 5 years ago

Sorry folks — again, we’ll be looking into different scenarios soon and adjust the heuristics.

If you have a test suite full of warnings immediately after a new API that has previously been called an alpha was released then I’m sure you can tolerate living with warnings in tests for a few more days. :-)

but would really like to suppress this warning

We do not recommend to silence it hoping that Enzyme will fix them for you.

In some cases, you will need to actually wrap your code into act() sections. Again, we’ll need to adjust the heuristics and provide more guidance. But I would avoid writing large test suites around APIs while they are in alpha versions if you can’t tolerate some warnings or aren’t sure how to suppress them.

flucivja commented 5 years ago

Hello, the problem I have is that even I wrap code in act I receive the warning and I am not sure how to wrap code to avoid it (I tried several combinations without luck).

Here is gist I created to reproduce issue: https://gist.github.com/flucivja/36a324f183c212d0e1cf449631e725a3

At the first I thought it is enzyme issue but then I did same without it and warning was still there.

ajur58 commented 5 years ago

Not sure if this is the best place for feedback on act(), sorry if it's off-topic.

While encountering the issue with the warnings, heading to the docs & ending up in this thread, I wondered about two things:

gaearon commented 5 years ago

Do we need to use act() only on components using hooks?

It is best practice to always use it — but with classes it's too late to warn due to existing code. That ship has sailed. With Hooks, we have a chance to enforce best practice right from the beginning. Of course, testing code written during alphas would need to adapt, but that's the cost of using alphas.

Should we consider the code running after act() as sync?

Yes. Nothing waits for execution, act just defers React mutations until the nested callback exits, and then makes them happen in a batch.

marcinczenko commented 5 years ago

I know that people in this thread understand what is the problem, but playing (or being) stupid myself, I want to make sure that we have some super basic example, independent from all project-specific snippets people paste here and there. If I am not on the wrong foot, this should be a representative example of what causes the act warning in the corresponding test"

Main.js

import React, { useState, useEffect } from 'react'

const Main = () => {
  const [value, setValue] = useState(0)

  useEffect(() => {
    const timeout = setTimeout(() => {
      setValue(1) // <=== this is causing the warning, and it should not
    }, 1000)
    return () => {
      clearTimeout(timeout)
    }
  })

  return (
    <div>
      <p>Current value is: {value}</p>
    </div>
  )
}

export { Main }

Main.test.js

import React from 'react'
import { render, waitForElement } from 'react-testing-library'
import { Main } from './Main'

describe('Main', () => {
  it('is quite annoying', async () => {
    const { getByText } = render(<Main />)

    await waitForElement(() => getByText(/value is: 1/i))
  })
})

react-testing-library says the problem is on react side (https://github.com/kentcdodds/react-testing-library/issues/285#issuecomment-461833739) and they refer this thread.

Again, I am just trying to make sure I am looking at the right place, and maybe as a side effect it will help people that just discovered this problem and are traveling between enzyme, react-testing-library, and react, to find the cure.

threepointone commented 5 years ago

I was literally writing out these details in a separate post, but I'm typing this out here to relieve some of y'all's anxiety :)

So first, the immediate, really really short term 'workarounds' if you can't hold on for a couple of days - @marcinczenko (and to everyone else, really) in your case, you should be able to pass tests by using fake timers. This is how fb does all its tests and should work in your case.

import React from 'react'
import { render, waitForElement } from 'react-testing-library'
import { Main } from './Main'
jest.useFakeTimers()
describe('Main', () => {
  it('is not so annoying', () => {
    const { getByText } = render(<Main />)
    act(() => { jest.runAllTimers() })
    let element = getByText(/value is: 1/i)
  })
})

What of promises then? Even when using promises, if you can mock the returned promise's as something like

{
  then : jest.fn((callback) => act(() => { callback(mockData) }
}

Then, triggering an update would 'set state' in the same 'tick' as you triggered it, and your tests would pass.

However, when using async/await, javascript resolves its promises in a task queue separate of control from user land. (Happy to explain this in more detail separately, but take my word for it right now) So examples like kent's - https://github.com/kentcdodds/react-testing-library/issues/281#issuecomment-461150694 would be hard to test. The 'fix' for this is heavy

Again, this is how we do it in fb, and it works across all our tests.

Good so far. But we can do a lot better. The above suggestions aren't a satisfying solution for many, so I'm working on a fix right now. We will probably allow async logic inside an act(...) block. THIS EXAMPLE IS ONLY A SKETCH, DETAILS MAY CHANGE -

act(async () => {
  render(<Main />)
  await waitForElement(() => getByText(/value is: 1/i))
})

That should make it a much better experience. The batching behaviour is slightly less enforced, but it should do for most usecases, and we'll have a chance to make it much better with concurrent mode.

Fair? Happy? I'm trying to get this in fairly soon, so it shouldn't be painful for too long; days, not weeks. Promise.

(There's a separate issue where warnings aren't showing stack traces for a lot of people, and I'll fix that separately)

marcinczenko commented 5 years ago

Thanks @threepointone. It is nice and educative. I have no problem to wait for the ultimate solution, I just started to get lost with the flood of messages and who is fixing what ;). Here and there I will try the workarounds. Unfortunately there are cases where I cannot mock the async call when component mounts (I mean, I can technically, but this would break the purpose of some of my tests - so I do not want). And heavy fix is fine if one cannot wait, but that feels like an overkill (in nextjs I can easily control babel configuration, but in create-react-app not unless I eject or fork react-scripts or use customize-cra, etc). So for those cases I will be happy to wait.

threepointone commented 5 years ago

I think we could have done a better job of communicating this, but I got caught up shipping this internally and let this slip through. I apologise for that.

ajur58 commented 5 years ago

Thanks for the suggested solutions @threepointone.

Without wanting to rush things, but out of curiosity: should act() also do its magic with components wrapped in context?

I created a small example where act() doesn't seem to do anything, but if I manually call update() on a wrapped component, it works.

Would be happy to get feedback: https://codesandbox.io/s/n5yrrxy6y4

gaearon commented 5 years ago

As the first improvement, the 16.8.2 release adds a component stack trace to each message so you can see which component is causing it.

We’ll have more to share when the async version is ready (it’s not yet). This might take another week or so to resolve.

quasicomputational commented 5 years ago

I also had some difficulty figuring out just what I was meant to do, especially because the link in the message pointed to the docs of a module I wasn't even using. Some concrete suggestions for additions to the docs that I think would've been helpful for me:

    const renderer = TestRenderer.create(<Swizzy />);
    doSomeAssertions(renderer.toJSON());
threepointone commented 5 years ago

@quasicomputational

You can read some of the original motivation at https://github.com/facebook/react/pull/14744

quasicomputational commented 5 years ago

Thanks for the elucidation. I hope that these points can make it into the docs, so that future people won't have to puzzle it out by trial and error. I'd definitely recommend mentioning act prominently in the documentation for update and create, since users will be looking at those anyway. There are also some worked examples on the TestRenderer page which don't use act.

If I can bikeshed the description, I think that act's documentation should say something like "This will flush React's event queue before it returns, ensuring that your tests do not see an inconsistent state.", which I think should let people understand where it would be needed (assuming I've understood things correctly, of course).

Also, doesn't act solve #14050? That's quite useful if so, and the good news should be more widely known.

Concering TestRenderer.create, I asked specifically because act doesn't allow you to return a value from it (Warning: The callback passed to TestRenderer.act(...) function must not return anything. You returned: [object Object]). If I want to inspect the rendered tree and make assertions, those should be outside the function passed to act (right?), but I must have access to the TestRenderer in order to make those assertions. Is there something I am missing here, or will I need to do something like, e.g.,

    const renderer = TestRenderer.create(null);
    act(() => renderer.update(<Swizz />));
    assertions(renderer.toJSON());

or perhaps

    let renderer;
    act(() => { renderer = TestRenderer.create(<Swizz />) });
    assertions(renderer.toJSON());

in just about every test? This is somewhere where I was looking to the example code for guidance and coming up empty.

threepointone commented 5 years ago

The docs will get better very soon, thank you for the feedback!

cellog commented 5 years ago

could a temp workaround for the async issue be to mock useState to return a setState that is wrapped in act? Or are there issues I should think about before trying this? To be clear about my question, I'm trying to test a custom hook that calls an async function in useEffect and calls the setState function returned from useState inside the async function.

threepointone commented 5 years ago

@cellog have you tried the steps in https://github.com/facebook/react/issues/14769#issuecomment-462528230? I wouldn't recommend mocking setState in that manner.

cellog commented 5 years ago

@threepointone thanks for the fast reply. I am hoping to avoid a major configuration change for the tests, so I guess I'll make the code test-aware (pass in a callback I can use to wrap my setState calls with act, basically, and then run assertions in the callback) until act understands async.

cellog commented 5 years ago

@threepointone I came across an elegant solution to this problem. If the async functions are defined in a separate file, using factory functions to pass in the variables from the hook for the closure, you can mock them out and then test the async portion completely separately. No mess, no weird promise polyfills. I think hooks should not have any async stuff directly defined in the hook file for this reason.

danielkcz commented 5 years ago

@threepointone Just want to note, that act warning in 16.8.2 is pretty much unhelpful too :) Yes, there is a call stack leading to the component, but considering there is usually one test file per a component... If it would be only possible to somehow locate the line that caused the unguarded update, that would be awesome :)

  console.error node_modules/react-dom/cjs/react-dom.development.js:506
    Warning: An update to ConfirmButton 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 ConfirmButton

And the same thing is repeated like 10 times not really giving any hint where to look for the problem.

Besides, I kinda cannot wrap my head around why there are actual warnings when tests are passing 🤔Doesn't that imply false positives in regards that act is not needed there?

bvaughn commented 5 years ago

Besides, I kinda cannot wrap my head around why there are actual warnings when tests are passing 🤔Doesn't that imply false positives in regards that act is not needed there?

The warnings are a signal that you may not be testing what you think you're testing. This could be important in cases where passing tests could be false positives.

danielkcz commented 5 years ago

The warnings are a signal that you may not be testing what you think you're testing. This could be important in cases where passing tests could be false positives.

Whoa. I've revised some tests that had this issue and indeed there was a bug 😮. After fixing it warning is gone even without act. I am starting to like this very much ❤️ Although it's rather hard to find it.

danielkcz commented 5 years ago

Well, so I've encountered one case where I am unable to figure why is the warning shown. Unfortunately, CodeSandbox behaves differently here and works just fine. If someone is willing to give it a try, you can download the project and run it locally. There is only a single test which is passing, but a warning is shown.

https://codesandbox.io/s/812mvpzm60

PS: At first glance, you might just say to wrap the clock.tick to the act which surely works. However, point is that since timers are mocked, the whole thing runs synchronously which is proven by test passing.

threepointone commented 5 years ago

@FredyC wrapping clock.tick with act() is indeed the recommendation!

threepointone commented 5 years ago

I wrote a doc that explains how to use act(...) in various scenarios https://github.com/threepointone/react-act-examples. It includes examples and the babel setup for harder cases. I'm still editing working on this, but it could be useful to you folks in this group. Please take some time out to read the document and the included test file. Happy to hear feedback if you have any!

danielkcz commented 5 years ago

@FredyC wrapping clock.tick with act() is indeed the recommendation!

Ok, but I would like to know why :) Given that test is passing, what is act helping me with in that case? Isn't it false positive as I've mentioned above? Perhaps it can serve to improve heuristics. It wouldn't be a very helpful tool if we have to be wrapping things with it kinda randomly without any clue.

I've read the whole doc, definitely most appreciated, it's very educative ❤️ Even though it doesn't answer my question it seems.

threepointone commented 5 years ago

A single runAllTimers() inside an act() call might seem like a false positive, but consider if you had many updates happening within that time span, and if these updates trigger new effects too. Without the act() call, you wouldn't be guaranteed that the UI has updated to show what a user would see. That would be a buggy test! The very first example in the doc demonstrates this.