Skip to content

Prevent act warning on async hook #14

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

Closed
mpeyper opened this issue Mar 19, 2019 · 34 comments
Closed

Prevent act warning on async hook #14

mpeyper opened this issue Mar 19, 2019 · 34 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@mpeyper
Copy link
Member

mpeyper commented Mar 19, 2019

Currently, there is a warning about updating a hook outside of an act call when the update happens as the result of an async call (i.e. callback, promise, async/await, etc.)

There is an example of this in our own test suite which generates the warning when we run our tests.

Firstly I'd like this warning to go away. I've tried all the suggestions in @threepointone's examples repo to no avail. There has also been a rather lengthy discussion in the react-testing-library repo on the topic.

Secondly, if there is a way to remove the warning, the solution(s) should be documented in our docs to help others.

Finally, is there anything we could be doing/wrapping in our API so that users do not have to call act in their own code as often, similarly to the work that has already been done in react-testing-library?

I'm not 100% certain on whether there is anything we can actually do, so if you're more familiar with the act function and its uses, I'd love to get your input.

@mpeyper mpeyper added enhancement New feature or request help wanted Extra attention is needed labels Mar 19, 2019
@panjiesw
Copy link

panjiesw commented Mar 19, 2019

What a timing.

Experienced this just minutes ago.

It's fine and generates no warning if the async call is triggered by a previous update, because we can call 2 act, first to update state triggering the async call, and the second to call jest.runAllTimers().

But if the async call is immediately run in the first render, then it'll generate the warning, even with jest.runAllTimers() inside act after the render declration.

@mpeyper
Copy link
Member Author

mpeyper commented Mar 19, 2019

Sorry, I'm not sure I follow. As far as I can tell, any time the setter of a useState (and friends) call is called in a way that cannot be wrapped in an act call, the warning is present, regardless of which updated triggered the actual async call itself.

Or am I misunderstanding you?

@panjiesw
Copy link

panjiesw commented Mar 19, 2019

It's one of technique in @threepointone's examples repo which involves polyfilling Promise using promise NPM package.

You can see in my fork of this repo. I changed asyncHook.test.js to my use case, add a test case illustrating this and skip other tests. This test case doesn't generate warning.

But there are times when async call need to be made immediately in useEffect like in your original test cases, there waitForNextUpdate() is useful but generates warning like described in this issue.

EDIT: looks like I'm misunderstanding something or my illustration's not valid after all

@panjiesw
Copy link

So it's indeed a miss in my test. I've updated my fork closer to how I did it.

We don't need waitForNextUpdate() if using combination of:

  1. global promise polyfill
  2. jest fake timers
  3. and call jest.runAllTimers() inside act

Will wait for proper way without polyfill when act is more polished as mentioned in facebook/react#14769

@ab18556
Copy link
Contributor

ab18556 commented Mar 20, 2019

There is already a PR on react that should fix this facebook/react#14853
I wouldn't bother finding a workaround until this is released. It is only a warning after all ;)

@lvl99
Copy link

lvl99 commented Mar 20, 2019

@ab18556 it is a warning, however it blocks CI builds if CI=true on create-react-app projects. CRA treats any warnings as errors and aborts the build, even if the tests passed.

@kevinwolfcr
Copy link

Same is happening for me when trying to test an asynchronous callback:

const handleIncrementAsync = useCallback(
  async () => {
    await sleep(2000);
    setCount(count + 1);
  },
  [count]
);

And the corresponding test case:

test("should handleIncrementAsync", async () => {
  const { result, waitForNextUpdate } = renderHook(() => useCounter());
  act(() => result.current.handleIncrementAsync());
  await waitForNextUpdate();
  expect(result.current.count).toBe(1);
});

With the same error message:

Warning: The callback passed to ReactTestUtils.act(...) function must not return anything.

It looks like you wrote ReactTestUtils.act(async () => ...), or returned a Promise from the callback passed to it. Putting asynchronous logic inside ReactTestUtils.act(...) is not supported.

It is also difficult to expect a promise thrown from an async hook, let's hope a solution lands soon :).

PD:

I made a code sandbox demonstrating this issue. Tests pass but it's still awkward to see this warning 🤷🏻‍♂️.

@ab18556
Copy link
Contributor

ab18556 commented Mar 20, 2019

@lvl99 I get your point, but since there is already a PR to address it, I don't feel adding a temporary workaround would really worth the effort.

According to this post, it should be released soon.
facebook/react#14769 (comment)

@lvl99
Copy link

lvl99 commented Mar 20, 2019

@ab18556 no prob. I wasn't suggest it as a workaround, just giving some extra context on how it still blocks certain things, even if it is just a warning.

I'm currently dealing with a pipeline that refuses to build and are eagerly waiting for the react fix to land 😂

@mpeyper
Copy link
Member Author

mpeyper commented Mar 21, 2019

If the proper fix is on the way from the React team, then I agree it's not worth spending time on a short term work around.

CRA treats any warnings as errors and aborts the build, even if the tests passed.

@lvl99 would suppressing the output allow the build to pass?

function suppressWarnings(callback) {
  const consoleWarn = console.warn.bind(console)
  global.console.warn = () => {}
  callback().finally(() => {
    global.console.warn = consoleWarn
  })
}

test("should handleIncrementAsync", suppressWarnings(async () => {
  const { result, waitForNextUpdate } = renderHook(() => useCounter());
  act(() => result.current.handleIncrementAsync());
  await waitForNextUpdate();
  expect(result.current.count).toBe(1);
});

Note: untested

Not ideal, but might allow you to keep moving while you wait for a fix.

@mpeyper mpeyper mentioned this issue Mar 21, 2019
22 tasks
@threepointone
Copy link

we released a new alpha 16.9.0-alpha.0 that includes async act() to address these issues. longer docs are incoming, but it works as one would expect. example, taking @kevinwolfcr's example, rewritten with the new api

test("should handleIncrementAsync", async () => {
  const { result, waitForNextUpdate } = renderHook(() => useCounter());
  await act(async () => {
    result.current.handleIncrementAsync();
    await waitForNextUpdate();
  });
  expect(result.current.count).toBe(1);   
});

@mpeyper
Copy link
Member Author

mpeyper commented Apr 6, 2019

@threepointone

I have updated to the 16.9.0-alpha.0 version locally and updated and ran the tests and all the warnings go away, which is lovely.

I then made a change to waitForNextUpdate from

{
  // ...
  waitForNextUpdate: () => new Promise((resolve) => addResolver(resolve)),
  // ...
}

to

{
  // ...
  waitForNextUpdate: () => act(() => new Promise((resolve) => addResolver(resolve))),
  // ...
}

which allowed all the test cases in this library pass with no warning without having to make any changes, except on where multiple waitForNextUpdate are made in parallel (to test that they would all resolve on the next updated), where a warning states:

Warning: You seem to have overlapping act() calls, this is not supported. Be sure to await previous act() calls before making a new one. 

I like that the warning went away without having to change the tests, but I'm concerned that the act calls do not explicitly wrap the things making the updates (i.e. it only acts around the waiting part, not the whatever triggered the async update to occur), and that there is the potential to have overlapping act calls (not sure anyone would do this in the wild, or what the consequences there might be for doing it).

Happy to hear any suggestions or feedback.

@threepointone
Copy link

Do you have a git repo? Hard to say without looking at actual usecases.

but I'm concerned that the act calls do not explicitly wrap the things making the updates (i.e. it only acts around the waiting part, not the whatever triggered the async update to occur)

act isn’t just about suppressing the warning, it’s meant to give shape to your tests, and it batches and flushes effects and updates accordingly. I’m not sure what the concern is here.

there is the potential to have overlapping act calls (not sure anyone would do this in the wild, or what the consequences there might be for doing it).

I hope my explainer doc makes it clearer next week, but tldr like above, it’s meant to give shape to your actions before asserting.

If you make a git repo with the minimal usecases and concerns, I’d be happy to have a look.

@mpeyper
Copy link
Member Author

mpeyper commented Apr 6, 2019

Do you have a git repo?

I don't, but I'll try to use the examples already in this issue to tell the story well enough.

act isn’t just about suppressing the warning, it’s meant to give shape to your tests, and it batches and flushes effects and updates accordingly. I’m not sure what the concern is here.

I guess this is my actual concern. By making waitForNextUpdate automatically act as it's waiting, it does suppress the warning in many cases, but at the cost of removing explicitly shaping the test. I worry that many will prefer the convenience and end up introducing subtle issues into their tests. It's also possible I'm thinking too much about this and it's not a problem at all.

For example the test:

test("should handleIncrementAsync", async () => {
  const { result, waitForNextUpdate } = renderHook(() => useCounter());
  await act(async () => {
    result.current.handleIncrementAsync();
    await waitForNextUpdate();
  });
  expect(result.current.count).toBe(1);   
});

is fine, but the change to waitForNextUpdate I proposed above would allow it to be written as

test("should handleIncrementAsync", async () => {
  const { result, waitForNextUpdate } = renderHook(() => useCounter());
  result.current.handleIncrementAsync();  // no immediate update, so no warning, but initiates an async update that will happen later
  // the "gap" I'll refer to later is here
  await waitForNextUpdate(); // wraps the promise in `act` so the async update happens to be within is, suppressing the warning
  expect(result.current.count).toBe(1);   
});

This version is easier to write (and more likely to be closer to developer's first attempt at writing this test), especially when the library does it all by default so in many cases, the developer may not be aware that the act calls are being handled for them. If handleIncrementAsync ever changed to have an immediate update before initiating the async update the warning would suddenly show up and the test would need updating (unlike the first example). I'm also not sure if any subtle bugs can be introduced in the "gap" between the call initiating the async action and the call to create a acting promise waiting for it to occur.

I guess I'm trying to find the balance between helping the developers by taking care of act for them and helping so much it actually hurts them.

@threepointone
Copy link

aha, ok I think I get what you're saying. I actually think this is fine since it did enforce the shape (for reference, this is the shape I'm talking about https://twitter.com/threepointone/status/1114276002023264261), but let me think about it some more and get back to you.

@mpeyper
Copy link
Member Author

mpeyper commented Apr 24, 2019

@threepointone how is the 16.9.0-alpha going? Is there any ETA on when it might be ready for prime time?

@alexlafroscia
Copy link

alexlafroscia commented May 9, 2019

I'm seeing an issue where react-hooks-testing-library warns about an update not being wrapped in act even when using the async-act-friendly alpha version of React.

I have a Code Sandbox here using the alpha versions of react, react-dom and react-test-renderer. The tests pass but I get a ton of "noise" in the console about act not being used.

https://codesandbox.io/embed/mzpm6n2v78

@mpeyper
Copy link
Member Author

mpeyper commented May 16, 2019

Sorry @alexlafroscia, I missed this one.

Until the stable release of React, you'll need to wrap the waitForUpdate call with act`:

await act(() => waitForNextUpdate());

This no longer produces the warning for the first update, but, and I'm not sure if this is intentional or not, but your useEffect does not specify any dependencies, so the state change triggers another fetch, which triggers another, etc. and these updates do not get wrapped in act so they produce the warning.

This will likely not be an issue when running the tests in your checkout, but in codesandbox they continue to execute even after the tests complete, so the warnings get printed to the console. You can make the fetch only execute once by changing the effect to:

useEffect(function() {
  async function performFetch() {
    const result = await fakeFetch();
    setResult(result);
  }

  performFetch();
}, []); // note the empty array

Whether or not this is right for you will depend on your requirements, so I'll leave that up to you.

[Here is an updated sandbox}(https://codesandbox.io/s/asynchooktesting-n718c) without the warning, or repeating fetch calls.

I hope this helps and isn't too late. Let me know if there is something else I can help you with.

@alexlafroscia
Copy link

Thanks so much @mpeyper! Not too late at all; I really appreciate your help!

@mayacode
Copy link

mayacode commented Jun 6, 2019

I have a feeling like I'm missing something.

I've tried all presented solutions and nothing works for me.

I have the custom hook which uses useState and useEffect to fetch data and save them in the state.

import { useEffect, useState } from 'react';
import axios from 'axios';

export function getStats() {
  const [stats, setStats] = useState([]);
  useEffect(() => {
    axios
      .get('/api/stats/user-id')
      .then(response => setStats(response.data.content))
      .catch(error => console.log('fail', error));
  }, []);

  return stats;
}

I try to test it this way (but I've tried everything what was in this thread, including react 16.9 alpha version):

import { renderHook, act } from 'react-hooks-testing-library';
import axios from 'axios';

import { getStats } from '../hooks';

jest.mock('axios');

describe('DashboardView hooks', () => {
  describe('getStats', () => {
    beforeEach(() => {
      jest.useFakeTimers();
    });

    afterEach(() => {
      jest.useRealTimers();
    });

    it('', async () => {
      const resp = { data: { content: [ { a: 1 } ] } };
      axios.get.mockResolvedValue(resp);
      const { result } = renderHook(() => getStats());

      expect(result.current).toEqual([]);

      act(() => {
        jest.runAllTimers()
      })

      expect(result.current).toEqual([ { a: 1 } ]);
    });
  });
});

Test fails. Unfortunately I always get at least the warning about Warning: An update to TestHook inside a test was not wrapped in act(...). and in few situations also Warning: The callback passed to ReactTestUtils.act(...) function must not return anything..

I would really appreciate the help. I feel like I stuck and nothing what I use works...

@andrecalvo
Copy link

I've just come across this thread and think it may berelated to an example I'm trying to get help with here => #facebook/react#14769 (comment). Would appreciate any pointers. Thanks

@mpeyper
Copy link
Member Author

mpeyper commented Jun 25, 2019

Hi @mayacode,

sorry I missed your comment. I also have not had much success with faking timers to remove the warning. Sorry, I cannot help more. If you want to set up a codesandbox with the failing example, I'll take a closer looks and see if I can spot something for you,

@mpeyper
Copy link
Member Author

mpeyper commented Jun 25, 2019

Hi @andrecalvo,

It's a bit hard to help debug without a codesandbox or equivalent to work with. If you have installed the 16.9.0-alpha.0, then you can try

  it("should call the api bla bla", async () => {
    const { result, waitForNextUpdate } = renderHook(() => useDataApi());
    const [{ data, isLoading, error }, setApiUrl] = result.current;

    await act(async () => {
      // not sure this needs to be in the `act` callback, but I'm unfamiliar with `fetchMock` myself, so maybe?
      fetchMock.mock("test.com", {
        reviews: [
          {
            category: "testing"
          },
          {
            category: "testing"
          }
        ]
      });
      setApiUrl("test.com");
      await waitForNextUpdate()
    });

    // I assume you will add some assertions here at some point?
  });

@cif
Copy link

cif commented Jun 25, 2019

I've been having mixed results with async hooks on a new project for the past month or so. I was able to limp along like this for awhile:

await act(() => {
  submitHandler();
});
expect(validation).toHaveBeenCalled();

The above worked for asserting that mocks were being called, but turned up all sorts of errors and didn't reflect hook state post async operations correctly.

Since I am using typescript, I had to overwrite types/react-test-renderer/index.t.ds to include:

export function act(callback: () => Promise<void> | Promise<undefined>): DebugPromiseLike | {};

If you are using typescript and need to do that, this can help: https://stackoverflow.com/a/45437448

Here's a fully working example of async hook test using this lib with no output errors at all:

import { act } from 'react-test-renderer';
import { useForm } from '../../src/hooks/useForm';
import { renderHook } from '@testing-library/react-hooks';
import Ajv = require('ajv');

describe('useForm custom hook' , () => {
  it('should not submit when validation fails', async () => {
      const fetcher = jest.fn();
      const collector = jest.fn();
      const validator: Ajv.ValidateFunction = new Ajv().compile({
        "required": ["foo"]
      });
      const history = { };
      const { result } = renderHook(
        () => useForm(fetcher, history as any, collector, validator)
      );

      collector.mockReturnValue({ bad: 'datas' });
      expect(result.current.validationErrors.length).toBe(0);
      await act(async () => { // <-- had to both await act and wrap callback in async. 
        await result.current.submitHandler({ preventDefault: jest.fn() });
      });
      expect(result.current.validationErrors.length).toBeGreaterThan(0);
      expect(result.current.inFlight).toBe(false);
      expect(fetcher).not.toBeCalled();
    });

});

Hopefully someone else finds this useful!

@andrecalvo
Copy link

Hi @andrecalvo,

It's a bit hard to help debug without a codesandbox or equivalent to work with. If you have installed the 16.9.0-alpha.0, then you can try

  it("should call the api bla bla", async () => {
    const { result, waitForNextUpdate } = renderHook(() => useDataApi());
    const [{ data, isLoading, error }, setApiUrl] = result.current;

    await act(async () => {
      // not sure this needs to be in the `act` callback, but I'm unfamiliar with `fetchMock` myself, so maybe?
      fetchMock.mock("test.com", {
        reviews: [
          {
            category: "testing"
          },
          {
            category: "testing"
          }
        ]
      });
      setApiUrl("test.com");
      await waitForNextUpdate()
    });

    // I assume you will add some assertions here at some point?
  });

using waitForNextUpdate(); worked here. Thanks.

@alexandrzavalii
Copy link

alexandrzavalii commented Nov 5, 2019

the only thing that worked for me was wrapping wait inside act.
I am on React 16.11 and @testing-library/react: 9.3.1

  await act( async () => {
   const { container }  =  await getRenderedResult(component);
    await wait(() => {
      expect(getTarget(container)).toMatchSnapshot();
    })
  })

@amarkrsinha1997
Copy link

Sorry @alexlafroscia, I missed this one.

Until the stable release of React, you'll need to wrap the waitForUpdate call with act`:

await act(() => waitForNextUpdate());

This no longer produces the warning for the first update, but, and I'm not sure if this is intentional or not, but your useEffect does not specify any dependencies, so the state change triggers another fetch, which triggers another, etc. and these updates do not get wrapped in act so they produce the warning.

This will likely not be an issue when running the tests in your checkout, but in codesandbox they continue to execute even after the tests complete, so the warnings get printed to the console. You can make the fetch only execute once by changing the effect to:

useEffect(function() {
  async function performFetch() {
    const result = await fakeFetch();
    setResult(result);
  }

  performFetch();
}, []); // note the empty array

Whether or not this is right for you will depend on your requirements, so I'll leave that up to you.

[Here is an updated sandbox}(https://codesandbox.io/s/asynchooktesting-n718c) without the warning, or repeating fetch calls.

I hope this helps and isn't too late. Let me know if there is something else I can help you with.

I couldn't find any documentation for waitForNextUpdate. Will be better if can add this in the documentation itself.

@mpeyper
Copy link
Member Author

mpeyper commented Feb 27, 2020

@amarkrsinha1997

  1. https://react-hooks-testing-library.com/usage/advanced-hooks#async
  2. https://react-hooks-testing-library.com/reference/api#waitfornextupdate

Let me know if anything is missing.

@agriffis
Copy link

I'm so confused. I get this warning with a simple test:

test('works', async () => {
  const {result} = renderHook(() => useMyHook())
})

Internally, the hook kicks off some fetches that are handled by mock-service-worker.

I've read all the docs, and I'm running React 16.13.1, and I just can't figure out what I should be doing differently.

@mpeyper
Copy link
Member Author

mpeyper commented Jun 29, 2020

Hi @agriffis, sorry you're having trouble.

It's a little hard to help without seeing the internals of the hooks and the full test. The warnings are almost always triggered by a state update that occurs asynchronously with the test and not waiting at the necessary points, but without seeing the code I'm just guessing.

You may also want to head over to the new discord channel as there's more folks over there to lend a hand (it's basically just me looking at these issues).

@agriffis
Copy link

agriffis commented Jul 1, 2020

I found what was causing this. My hook launches multiple fetches and rerenders as each fetch returns. However my test is only interested in the earlier responses. The errors were due to fetches that completed after the test finished.

The hook will clean up properly if unmounted, so I changed it:

test('works', async () => {
  const {result, unmount, wait} = renderHook(() => useMyHook())
  try {
    expect(...)
    await wait(() => result.interestingThing)
    expect(...)
  } finally {
    unmount()
  }
})

I also wrote a utility to reuse this pattern, but frankly I didn't use it, since the utility is just as much boilerplate:

const renderHookWithUnmount = async (hook, test) => {
  const rendered = renderHook(hook)
  try {
    await test(rendered)
  } finally {
    rendered.unmount()
  }
}

Anyway, hopefully this helps someone else: If you're getting this error from a complex hook with multiple outstanding fetches/promises, you might need to unmount (assuming that your hook will avoid additional setStates after unmount)

@thisismydesign
Copy link

thisismydesign commented Oct 21, 2020

I'm seeing the same warning when testing a context.

import { useSubscription, SubscriptionProvider } from "./Subscription";

const wrapper = ({ children }) => (
  <SubscriptionProvider>{children}</SubscriptionProvider>
);

const { result } = renderHook(() => useSubscription(), {
  wrapper,
});

expect(result...

My provider uses a state and updates it:

export const SubscriptionProvider = ({ children }) => {
  const [state, setState] = useState();

  useEffect(() => {
    setState(....);
  }, [setState]);

  ...
};

I assume this is what's causing the warning but I didn't find a way to get rid of it in this thread. Any suggestions?

Edit:

In fact, waitForNextUpdate doesn't resolve, even though the state verifiably changes.

@mpeyper
Copy link
Member Author

mpeyper commented Oct 21, 2020

Hi @thisismydesign,

The code you've shown should not be producing that error. Is there more to it then that?

Given the name of things I'm assuming the effect code is more complex?

What does useSubscription look like?

@jeffersoneagley
Copy link

jeffersoneagley commented Dec 9, 2020

I just had something similar suddenly start happening with a codebase that's using MockServiceWorker, but strangely, we haven't changed much other than adding one more test and one more MSW handler... 🤷‍♂️

I'm using contexts similar to @thisismydesign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests