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

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

Closed
ncphillips opened this issue Feb 6, 2019 · 116 comments

Comments

@ncphillips
Copy link

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

When upgrading a large code base

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

@ncphillips

This comment has been minimized.

Copy link
Author

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

@gaearon

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

"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

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

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

@ncphillips

This comment has been minimized.

Copy link
Author

commented Feb 6, 2019

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

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

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Author

commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

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.

@FredyC

This comment has been minimized.

Copy link

commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

@gaearon

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

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

@ncphillips

This comment has been minimized.

Copy link
Author

commented Feb 8, 2019

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

This comment has been minimized.

Copy link

commented Feb 8, 2019

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();
  });
});

@threepointone threepointone self-assigned this Feb 8, 2019

@hisapy

This comment has been minimized.

Copy link

commented Feb 8, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

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

This comment has been minimized.

Copy link

commented Feb 11, 2019

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

This comment has been minimized.

Copy link

commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

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

This comment has been minimized.

Copy link

commented Feb 11, 2019

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.

@j13l

This comment has been minimized.

Copy link

commented Feb 11, 2019

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:

  • Do we need to use act() only on components using hooks?
  • Should we consider the code running after act() as sync? I get what act does, but I don't get how the next line (the assertion) waits for its execution.
@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Could you share a codesandbox repro? Hard to say whether you’re using the alpha correctly

@andrecalvo

This comment has been minimized.

Copy link

commented Jun 24, 2019

@threepointone what is the correct/different way to use the alpha?

@526117

This comment has been minimized.

Copy link

commented Jun 24, 2019

@526117

This comment has been minimized.

Copy link

commented Jun 24, 2019

@526117

This comment has been minimized.

Copy link

commented Jun 24, 2019

@cif

This comment has been minimized.

Copy link

commented Jun 25, 2019

@FredyC You can also put this somewhere in your compiler path as a temp fix until DefinitelyTyped is updated:

import { DebugPromiseLike } from 'react-test-renderer';
export function act(callback: () => Promise<void> | undefined): DebugPromiseLike | {};

EDIT/DISCLAIMER: This might not be as straight forward as it seemed depending on your TS config, this can help https://stackoverflow.com/a/45437448.

The good news is, once you have TS agreeing the signature is valid, all is working great in 19.0-alpha with async hook tests

@andrecalvo

This comment has been minimized.

Copy link

commented Jun 26, 2019

Could you share a codesandbox repro? Hard to say whether you’re using the alpha correctly

Hey @threepointone, here's a Codesandbox. If you check the console you'll see the error I get when I run this on my project. You'll notice I've commented out the fetchMock stuff as it didn't seem to get along well within the Codesandbox environment, it's irrelevant anyway as you can still see the error I'm getting.

https://codesandbox.io/embed/react-async-fetch-hook-test-failing-6wpkz

@FredyC

This comment has been minimized.

Copy link

commented Jun 26, 2019

@andrecalvo That's easy, you forgot to use react-test-renderer@16.9.0-alpha.0 ;) Since you are importing act from it, it's kinda obvious.

@andrecalvo

This comment has been minimized.

Copy link

commented Jun 26, 2019

@FredyC Aha, I was using the wrong version of react-test-renderer. Updating that but also using
waitForNextUpdate(); after my act fixed my issue.

I also had to switch a line as described here #14769 (comment)

@meza

This comment has been minimized.

Copy link

commented Jun 29, 2019

I'm getting the same error with react-test-renderer when the hook has an async function inside that it awaits on.

@andyrichardson

This comment has been minimized.

Copy link

commented Jul 1, 2019

So I've tried out the latest alpha and the added functionality is great!

It still doesn't look to cover use cases where async actions are being triggered inside a component on update. I'm assuming the only option is to call a setTimeout when the promise being called can't be accessed inside of a test?

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Do you have an example repro? Either as a git repo or codesandbox

@andyrichardson

This comment has been minimized.

Copy link

commented Jul 1, 2019

@threepointone I don't have an example using the latest alpha but here's the current workaround we're using.

@LeeCheneler

This comment has been minimized.

Copy link

commented Jul 24, 2019

Temporary workaround for people who can't upgrade to an alpha version of React like me:

/**
 * Suppress React 16.8 act() warnings globally.
 * The react teams fix won't be out of alpha until 16.9.0.
 */
const consoleError = console.error;
beforeAll(() => {
  jest.spyOn(console, 'error').mockImplementation((...args) => {
    if (!args[0].includes('Warning: An update to %s inside a test was not wrapped in act')) {
      consoleError(...args);
    }
  });
});
@mcmillion

This comment has been minimized.

Copy link

commented Jul 27, 2019

This did seem to be working (not spitting out warnings) in the alpha until recently? Had to install deps again on a new laptop and now I'm getting warnings again in 16.9.0.alpha.0

@gaearon

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

We just published 0.0.0-db3ae32b8 of all packages. It is likely going to be a release candidate for 16.9.0 so you can give it a try if you'd like before it gets promoted to stable.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

16.9 got released, including async act, and updated documentation https://reactjs.org/blog/2019/08/08/react-v16.9.0.html Closing this, cheers.

@mharidy

This comment has been minimized.

Copy link

commented Aug 14, 2019

upgrading react and react-dom to 16.9 doesn't fix the problem, the only change console.error node_modules/react-dom/cjs/react-dom.development.js:545 insted of
console.error node_modules/react-dom/cjs/react-dom.development.js:506

@FredyC

This comment has been minimized.

Copy link

commented Aug 14, 2019

@mharidy See #16348 where I tried to improve that and now it continues over at facebook/jest#8819

@ernestohegi

This comment has been minimized.

Copy link

commented Aug 17, 2019

https://reactjs.org/blog/2019/08/08/react-v16.9.0.html#async-act-for-testing this solved all my problems when using custom async hooks. No more act warnings and tests are passing! Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.