-
Notifications
You must be signed in to change notification settings - Fork 232
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
Comments
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 But if the async call is immediately run in the first render, then it'll generate the warning, even with |
Sorry, I'm not sure I follow. As far as I can tell, any time the setter of a Or am I misunderstanding you? |
It's one of technique in @threepointone's examples repo which involves polyfilling You can see in my fork of this repo. I changed But there are times when async call need to be made immediately in EDIT: looks like I'm misunderstanding something or my illustration's not valid after all |
So it's indeed a miss in my test. I've updated my fork closer to how I did it. We don't need
Will wait for proper way without polyfill when |
There is already a PR on react that should fix this facebook/react#14853 |
@ab18556 it is a warning, however it blocks CI builds if |
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:
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 🤷🏻♂️. |
@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. |
@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 😂 |
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.
@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. |
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);
}); |
I have updated to the I then made a change to {
// ...
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 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 Happy to hear any suggestions or feedback. |
Do you have a git repo? Hard to say without looking at actual usecases.
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 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. |
I don't, but I'll try to use the examples already in this issue to tell the story well enough.
I guess this is my actual concern. By making 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 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 I guess I'm trying to find the balance between helping the developers by taking care of |
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. |
@threepointone how is the |
I'm seeing an issue where I have a Code Sandbox here using the alpha versions of |
Sorry @alexlafroscia, I missed this one. Until the stable release of React, you'll need to wrap the 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 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 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. |
Thanks so much @mpeyper! Not too late at all; I really appreciate your help! |
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 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 I would really appreciate the help. I feel like I stuck and nothing what I use works... |
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 |
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, |
Hi @andrecalvo, It's a bit hard to help debug without a codesandbox or equivalent to work with. If you have installed the 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?
}); |
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 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! |
using |
the only thing that worked for me was wrapping wait inside act.
|
I couldn't find any documentation for waitForNextUpdate. Will be better if can add this in the documentation itself. |
I'm so confused. I get this warning with a simple test:
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. |
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). |
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) |
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, |
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 |
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 |
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.The text was updated successfully, but these errors were encountered: