[testing recipes] change data fetching example to manually resolve promises

[threepointone]

Some people might get confused that async act would wait for any promise to resolve, when really it only flushes already resolved promises. This changes the example to be obvious about resolving the mock with data manually. Slightly uglier, but that's what libs/abstractions are for I suppose.

[reactjs-bot]

Deploy preview for reactjs ready!

Built with commit b043b40

https://deploy-preview-2233--reactjs.netlify.com

[alexkrolick]

How about using jest.mockResolvedValue here or jest.mockResolvedValueOnce inline?

If you want to show act() wrapping an async call triggered indirectly, maybe put a button click or something instead of a render sideEffect?

[kentcdodds]

Here's what I recommend (and I recommend doing this in a global setup, but it's fine to just have it in the test file for this example like this I think):

beforeEach(() => {
  jest.spyOn(window, 'fetch').mockImplementation((...args) => {
    console.warn('window.fetch is not mocked for this call', ...args)
    return Promise.reject(new Error('This must be mocked!'))
  })
})

afterEach(() => {
  window.fetch.mockRestore()
})

Then in your test, you can just rely on the fact that window.fetch is a mock function. As for making it work nicely with act and resolving the promise before continuing things, your current implementation is fine. What would you think of doing something like making a little deferred function:

function deferred() {
  let resolve, reject
  const promise = new Promise((res, rej) => {
    resolve = res
    reject = rej
  })
  return {resolve, reject, promise}
}

Then you could use that in the test like so:

test('renders user data', async () => {
  // ...
  const {resolve, promise} = deferred()
  window.fetch.mockImplementationOnce(() => {
    return Promise.resolve({json: () => promise})
  })
  // ...

  resolve(fakeUser)
  await act(promise)
  // ...

I think that should work.

This is so much easier with RTL, because you have the find* queries and wait utilities.

// no need for a deferred utility or mutable variable references

test('renders user data', async () => {
  // ...
  window.fetch.mockImplementationOnce(() => {
    return Promise.resolve({json: () => Promise.resolve(fakeUser)})
  })
  // ...

  await wait(() => expect(container.querySelector("summary").textContent).toBe(fakeUser.name));
  // ...

Or even better with the changes I was recommending for the RTL version:

test("renders user data", async () => {
  const fakeUser = {
    name: "Joni Baez",
    age: "32",
    address: "123, Charming Avenue",
  };
  window.fetch.mockImplementationOnce(() =>
    Promise.resolve({
      json: () => Promise.resolve(fakeUser),
    }),
  );

  const { getByLabelText, getByText } = render(<User id="123" />);

  expect(window.fetch).toHaveBeenCalledTimes(1);
  expect(window.fetch).toHaveBeenCalledWith("/123");

  // in any real world UI (pre-suspense as we are now), you'd have a loading indicator like so:
  await waitForElementToBeRemoved(() => getByText(/loading/i));

  // this requires aria-label to be added to the elements which is good for a11y
  expect(getByLabelText(/name/i)).toHaveTextContent(fakeUser.name);
  expect(getByLabelText(/age/i)).toHaveTextContent(fakeUser.age);
  expect(getByLabelText(/address/i)).toHaveTextContent(fakeUser.address);
});

In any case. If you want to change as little as possible, then I suggest at least adding this:

beforeEach(() => {
  jest.spyOn(window, 'fetch').mockImplementation((...args) => {
    console.warn('window.fetch is not mocked for this call', ...args)
    return Promise.reject(new Error('This must be mocked!'))
  })
})

afterEach(() => {
  window.fetch.mockRestore()
})

Reason being, if this test fails, then the global.fetch.mockRestore(); (which is currently in this test) would not run and the other tests in this file will fail in odd ways.

Sorry for butting in with opinions 😅 Feel free to ignore.

[threepointone]

all good feedback. I'll think some more.

[alexkrolick]

Left a comment inline about an alternative.

Manually resolving promises is risky if the app calls the mocked function the wrong number of times, leaving dangling promises in the test. It can also cause mixed up results if the fn is called again before the first one is resolved (due to the mutable resolve reference).

cc @kentcdodds 👀

[gaearon]

@threepointone Is this waiting for a review?

[threepointone]

No, but I can’t mark a PR as “changes planned”, and it’s hiding the last comment I made #2233 (comment)