Skip to content

renderHook result values with waitFor not working #1030

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
kylebake opened this issue Aug 1, 2022 · 20 comments
Closed

renderHook result values with waitFor not working #1030

kylebake opened this issue Aug 1, 2022 · 20 comments
Labels
bug Something isn't working compat: testing-library help wanted Extra attention is needed

Comments

@kylebake
Copy link

kylebake commented Aug 1, 2022

Describe the bug

Using the new renderHook function included in this library does not behave the same way when it comes to awaiting different return values as the other implementations (referencing specifically react-hooks + testing-library/react).

Trying to waitFor different values to change from the hook directly does not seem to be functioning as expected.

Expected behavior

If https://github.com/testing-library/react-hooks-testing-library is going to be deprecated in favor of this library, I'd expect to retain the same functionality the previous library offered since it was created to test complex hooks without the need for testing the components using these hooks as well. I'd also expect similar functionality to the testing-library/react implementation.

waitFor should be able to handle waiting for the values of the hook to match the expectation passed to it

Steps to Reproduce

Consider the following hook:

const useCount = () => {
  const [count, setCount] = useState(0)

  useEffect(() => {
    const retrieveCount = async () => {
      await new Promise(resolve => {
        setTimeout(() => {
          resolve(undefined)
        }, 500)
      })

      setCount(10)
    }

    retrieveCount()
  }, [])

  return count
}

If we wanted to try and waitFor count to be 10, there doesn't appear to be a way to do this with testing-library/react-native as we can with the others:

  it('waits until count has been set', async () => {
    const { result } = renderHook(() => useCount())
   
    // this line will always fail. `result.current` is always 0, no matter the options/rerendering/etc
    await waitFor(() => expect(result.current).toBe(10))

    // this syntax will also seemingly not do anything, which worked previously in the react-hooks library
    await waitFor(() => result.current === 10)

    expect(true).toBeTrue()
  })

The above example will work just fine in testing-library/react, however.

Screenshots

Versions

@mdjastrzebski
Copy link
Member

@kylebake RNTL implementation of both renderHook & waitFor closely matches React/Dom Testing Library one (essentially it probably is copy+paste+tweak), with minimal changes due to using React Test Renderer instead of React DOM renderer and not using having Js DOM objects like document.

If you have a bit of spare time and interest, you could try running diff for renderHook and waitFor source files between RNTL & RTL maybe this will shed some light why RTL's version is working and RNTL's version is not.

@mdjastrzebski mdjastrzebski added bug Something isn't working help wanted Extra attention is needed compat: testing-library hooks labels Aug 1, 2022
@kylebake
Copy link
Author

kylebake commented Aug 1, 2022

Happy to help out and dig more into it 👍 I'm in the process of upgrading react-native for our app, which is why I'm uncovering some of this. I'll let you know if I find anything over this week

@kylebake
Copy link
Author

kylebake commented Aug 3, 2022

A bit more info while I'm still investigating: it definitely appears to be that renderHook isn't rerendering the hook when asynchronous tasks happen. How I tested this theory:

If you take the example hook from my first comment, and test it with this code, result.current is always 0:

    const { result, rerender } = setupHook()

    setInterval(() => {
      console.log(`result: ${result.current}`)
    }, 1000)
    await waitFor(() => expect(result.current).toBe(10), { timeout: 15000 })

However, running the following test, waitFor handles the situation as I would expect and passes after 3s:

    let count = 0

    setTimeout(() => {
      count = 10
    }, 3000)
    await waitFor(() => expect(count).toBe(10), { timeout: 15000 })

I'm going to focus my attention on renderHook for the time being, as it seems to be the root issue

@pierrezimmermannbam
Copy link
Collaborator

pierrezimmermannbam commented Aug 19, 2022

@kylebake I think the issue here is not with the renderHook implementation but rather that waitFor isn't able to wait for promises using timers, these promises are never resolved (see this other issue #1067). To fix it you can use fake timers, run the pending timer and then use waitFor to resolve the promise. Regarding waitFor not being able to handle this situation I'm not sure what's causing this but I wonder if it's not rather a jest issue

@mdjastrzebski
Copy link
Member

@pierrezimmermannbam do you mean that setTimeout (and setInterval) never gets called when not real timers? Or that they get called but waitFor is somehow not able to observe the results of them being called. Either way, that sounds like a serious bug.

@kylebake
Copy link
Author

The promises are resolving in my use cases, I'm able to set breakpoints and see the code get past the async logic. The tests don't seem to be able to see this, however. I'd also push back on it being a jest issue since I'm able to get all of this working without any code/jest changes if I use testing-library/react.

A super hacky workaround that I've found is to continuously update the component/hook in the test and only then can the waitFor eventually see the assertion. My working example right now:

  const hook = renderHook(() => useCount())
  const timer = setInterval(() => {
    hook.rerender()
  }, 500)
 
  // now this waitFor will correctly see the result change to 10
  await waitFor(() => expect(hook.result.current).toBe(10)) 

Sidenote: I can spend some more time on this issue this weekend, been busy with getting our react native version upgraded to unblock us. I'll get a small repo stood up as well so it's easier for us to test against.

@tinahir
Copy link

tinahir commented Sep 14, 2022

Please try this with @testing-library/react-hooks

it('test', async () => {
  const { result, waitForNextUpdate } = renderHook(() => useHook());

  act(() => {
    result.current.fetch();
  });
  expect(result.current.state).toBe(undefined);

  await waitForNextUpdate();
  expect(result.current.state).toBe('GOOD'); //or at least "BAD"
});

@pierrezimmermannbam
Copy link
Collaborator

@kylebake are you using react 18 ? Promises mixed with useEffect do not work when using react 18, this issue is being discussed in #1093 and #1067 is another duplicate for the same issue. I think this will be fixed but in the meantime I believe your test case should work if you use fake timers

@mdjastrzebski
Copy link
Member

@kylebake can you confirm if this issue persists with RNTL v11.2.0?

@kylebake
Copy link
Author

Looks like I'm no longer able to reproduce the issue with v11.2.0, thanks for all the collaboration on this! Apologies for my slow responses, I'll try to help out more moving forward with this repo if I can

@mdjastrzebski
Copy link
Member

Awesome, thanks for checking that @kylebake

@lubbo
Copy link

lubbo commented Oct 12, 2022

Looks like I'm no longer able to reproduce the issue with v11.2.0, thanks for all the collaboration on this! Apologies for my slow responses, I'll try to help out more moving forward with this repo if I can

I'm still reproducing the problem in v11.2.0!

@AugustinLF
Copy link
Collaborator

@lubbo could you please post a repro using https://github.com/callstack/react-native-testing-library/tree/main/examples/basic please? Without that, we can't figure out what is happening, since the issue has been solved for most (it seems) users.

Are you using react 18 by the way? Because there could still be problem with older versions of react, but I'm not sure we'll be able to prioritise work to fix them.

@bkdev98
Copy link

bkdev98 commented Nov 2, 2022

Hi @AugustinLF here is my repro using the basic example: https://github.com/bkdev98/rn-testing-lib-waitfor-issue. The tests/asyncHooks.spec.tsx failed and waitFor does not await for new state to be updated.
https://github.com/bkdev98/rn-testing-lib-waitfor-issue/blob/main/__tests__/asyncHooks.spec.tsx
Test result:
Screen Shot 2022-11-02 at 13 05 43

@mdjastrzebski
Copy link
Member

mdjastrzebski commented Nov 2, 2022

@bkdev98 you should use waitFor with an expectation, i.e. something that throws when the expectation is failed. Returning false does not work, as waitFor considers that a legit return value.

That should be relatively easy to fix:

await waitFor(() => expect(result.current).toBeTruthy());

@bkdev98 if that does not fix the issue for you, please adjust your code to use except as shown above and create a new GH issues for your case.

@pierrezimmermannbam
Copy link
Collaborator

@bkdev98 I made the following changes in your test and it now works

describe("useProductDetail", () => {
  beforeEach(() => {
    jest.useFakeTimers();
  });

  test("should instantly show initial product data", async () => {
    jest.useFakeTimers();

    const { result } = renderHook(() => useTestAsyncHook());

    await waitFor(() => expect(result.current).toBe(true), { timeout: 2500 });

    expect(result.current).toEqual(true);
  });
});

As @mdjastrzebski mentioned the callback provided to waitFor should throw when the expectation is not met and also since you have a setTimeout of 2000ms, you need to increase the waitFor timeout which is of 1000ms per default so that fake timers are advanced by 2000ms, else it will timeout after 1000s

@pierrezimmermannbam
Copy link
Collaborator

We probably should revisit the documentation on waitFor, it says that the default timeout is 4500ms which is false and it does not clearly state that the expectation should throw so that it may work. Also I think it would be nice to mention that it behaves differently when using fake timers

@bkdev98
Copy link

bkdev98 commented Nov 2, 2022

It works!! Thank you for your help! ❤️

@mdjastrzebski
Copy link
Member

@pierrezimmermannbam would you have time to update waitFor documentation around timeout and making more explicit how the API works (throwing vs boolean)?

@pierrezimmermannbam
Copy link
Collaborator

@mdjastrzebski sure I'll try to submit a pr by the end of the week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compat: testing-library help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants