waitForNextUpdate is missing updates

[brainkim]

• react-hooks-testing-library version: 7.0.1
• react version: 17.0.2
• react-dom version (if applicable): 17.0.2
• react-test-renderer version (if applicable): N/A
• node version: 14.17.3
• npm (or yarn) version: 6.14.3

Relevant code or config:

const React = require('react');
const Observable = require('zen-observable');
const {renderHook} = require('@testing-library/react-hooks');

function useObservable(observable) {
  const [state, setState] = React.useState(null);
  React.useEffect(() => {
    const subscription = observable.subscribe((state) => setState(state));
    return () => subscription.unsubscribe();
  }, [observable, setState]);

  return state;
}

it('lets you wait each update', async () => {
  let observer;
  const observable = new Observable((observer1) => (observer = observer1));
  const {result, waitForNextUpdate} = renderHook(
    () => useObservable(observable),
  );

  expect(result.current).toBe(null);
  setTimeout(() => {
    observer.next(1);
    Promise.resolve().then(() => {
      observer.next(2);
    });
  });

  console.log(result.all.length); // => 1
  await waitForNextUpdate();
  console.log(result.all.length); // => 3
  expect(result.current).toBe(1); // throws an error!
});

What you did:

Hi, I’m currently trying to adopt testing-library/react-hooks to clean up Apollo Client’s test suite. One problem I’m having is that waitForNextUpdate() sometimes misses renders, especially when multiple state updates happen in additional microtasks.

What happened:

waitForNextUpdate() misses an update.

Reproduction:

See the code above.

Problem description:

See above.

Suggested solution:

The waitForNextUpdate() function seems to call wait() but I don’t understand why we can’t just return a promise whose resolve function is added to addResolver()? https://github.com/testing-library/react-hooks-testing-library/blob/main/src/core/asyncUtils.ts#L86-L98

[mpeyper]

Hi @brainkim, the reason waitForNextUpdate calls wait was to tie it into the shared act and timeout logic.

I'm assuming the reason you are not getting the discreet results is because React is not returning from act until all the microtasks have flushed. This is why we introduced result.all, so that discreet changes could be asserted if required, but in general I advise folks to not be too concerned about the intermediate results and to think of them as implementation details of their hooks.

If you truly want to assert in between the updates, using a promise that resolves after a short delay (e.g. new Promise(resolve => setTimeout(resolve, 10)).then(...)) instead of Promise.resolved should allow it, although it's not always practical when the promise comes from within the code being tested instead of the test code itself.

[brainkim]

This is why we introduced result.all, so that discreet changes could be asserted if required, but in general I advise folks to not be too concerned about the intermediate results and to think of them as implementation details of their hooks.

Yes I’m in full agreement here. The transitory renders which happen within a microtask loop shouldn’t be tested, especially because React 18 doing batched renders for every update is about to break the tests of everyone who’s testing this stuff anyways.

The actual reason I wanted react-hooks-testing-library to not miss updates is because of flakiness issues in the test suite, where tests would occasionally pass or fail based on which update waitForNextUpdate() resumed on. I can’t actually reproduce this outside of Apollo Client because the cause is likely somewhere deep in what I will lovingly refer to as “the bowels” of the codebase, but I was hoping that a more precise waitForNextUpdate() would fix that. I’m running into race conditions where perhaps timers in Apollo Client and the wait timers are firing slightly out of order. Insofar as waitForNextUpdate() relies on wait(), which as far as I can tell, does 50ms interval-based polling, I was wondering if we could just skip the polling part. Maybe a longer default interval would help too?

Anyways, thanks for the library, and thanks for your response.

[mpeyper]

interval checking is actually disabled for waitForNextUpdate so the underlying wait call removes it from the race. The only thing that will resolve waitForNextUpdate is the TestComponent rendering which is triggered when the hook being tested updates its state or a wrapper component rerendering.

[brainkim]

Ah, didn’t see that. So the loop runs at the speed of act()?

[mpeyper]

Yep. it boils down to a race between the next render and the timeout, wrapped in act.

[mpeyper]

@brainkim is there anything else to look at or talk about here or can I close this?

[brainkim]

So, I’m too lazy to debug it right now, but does the waitForNextUpdate() function still rely on some kind of looping/polling mechanism, which fires the resolver function repeatedly? I’m still trying to understand the flake I‘m seeing in unit tests, and wondering if resolving the waitForNextUpdate promise directly might get rid of the flake. Feel free to close otherwise! And thanks for the library.

[mpeyper]

Not that I can think of. When you peal back the layers, it really isn't very clever.

If you can point me to a particularly flaky test, I'm happy to dig into it for you.

[istvandesign]

Hello, I have the same issue, or similar issue

act(() => {
result.current.mutateAsync({name: x})
})
  await waitForNextUpdate();
  expect(result.current.isLoading).toBeTruthy();
  await waitForNextUpdate();
  expect(result.current.isSuccess).toBeTruthy();

is very flaky, I can make it about 90% flaky if I use the --runInBand flag.