mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
15.94k stars 518 forks source link

Requests is Node don't resolve immediately #528

Closed evanblack closed 3 years ago

evanblack commented 3 years ago

Environment

Name Version
msw 0.24.2
node 10.21.0
OS Mac OSX 10.15.7

Question

Pardon me if this is the wrong place to post. I am working on transferring all of our mocks to a wrapper library around your wonderful library. This is working great, but now that I have moved on to updating our UI unit tests I've run into a bit of a quandary.

Most of our tests do something like:

jest.spyOn(global, 'fetch').mockImplementation(url => Promise.resolve({
    json: () => Promise.resolve(data),
    status: 200 
}));

I believe this is a fairly common pattern that I've seen referenced many places. I've removed this functionality to accommodate setupServer, but now have multiple issues with act warnings and unexpected tree states. I understand that the Node requests will be intercepted in the middleware, and thus asynchronous requests are still being made with some latency. I've also read the "How is this library different?" section of node-request-interceptor, so understand that this particular project takes a stance.

It seems my only option at this point is to install and utilize react-testing-library's waitFor utility. It just seems like a lot of overhead to upgrade react-scripts/Jest to get all this working. I was hoping listen would be a little more accommodating to customizations, but maybe that's not in the cards.

kettanaito commented 3 years ago

Hey, @evanblack. Thanks for reaching out.

Can you please share an example test with us? The fact that you get some act warnings doesn't look like it's related to MSW, but perhaps there's something amiss in how the setupServer is called during test runs. Requests interception itself is provisioned synchronously and takes immediate effect once server.listen() is called.

evanblack commented 3 years ago

@kettanaito Thanks for following up.

I know the server.listen() call is synchronous, but the requests themselves that are intercepted still seem to have some latency. Let me see if I can put together a watered down reproduction today.

evanblack commented 3 years ago

@kettanaito Here's an example that demonstrates the problem with the LoginForm component from the React REST examples. I know this maybe isn't the most "ideal" testing setup.

import React, { useState, useCallback } from 'react';
import renderer from 'react-test-renderer';
import { setupServer } from 'msw/node';
import { rest } from 'msw';

export const LoginForm = () => {
  // Store the username so we can reference it in a submit handler
  const [username, setUsername] = useState('')

  // Create a state for the user data we are going to receive
  // from the API call upon form submit.
  const [userData, setUserData] = useState(null)

  // Whenever we change our username input's value
  // update the corresponding state's value.
  const handleUsernameChange = useCallback((event) => {
    setUsername(event.target.value)
  }, [])

  // Handle a submit event of the form
  const handleFormSubmit = useCallback(
    (event) => {
      // Prevent the default behavior, as we don't want
      // for our page to reload upon submit.
      event.preventDefault()

      // Perform a POST /login request and send the username
      fetch('/login', {
        method: 'POST',
        body: JSON.stringify({
          username,
        }),
      })
        .then((res) => res.json())
        // Update the state with the received response
        .then(setUserData)
                .catch((err) => console.error(JSON.stringify(err.message || err)))
    },
    [username]
  )

  if (userData) {
    return (
      <div>
        <h1>
          <span data-testid="firstName">{userData.firstName}</span>{' '}
          <span data-testid="lastName">{userData.lastName}</span>
        </h1>
        <p data-testid="userId">{userData.id}</p>
        <p data-testid="username">{userData.username}</p>
      </div>
    )
  }

  return (
    <form id="login-form" onSubmit={handleFormSubmit}>
      <div>
        <label htmlFor="username">Username:</label>
        <input
          id="username"
          name="username"
          value={username}
          onChange={handleUsernameChange}
        />
        <button type="submit">Submit</button>
      </div>
    </form>
  )
};

const handlers = [
  rest.post('/login', (req, res, ctx) => {
    const { username } = req.body

    return res(
      ctx.json({
        id: 'f79e82e8-c34a-4dc7-a49e-9fadc0979fda',
        username,
        firstName: 'John',
        lastName: 'Maverick',
      })
    )
  }),
]

const server = setupServer(...handlers)

// Establish API mocking before all tests.
beforeAll(() => {
    server.listen()
})
// Reset any request handlers that we may add during the tests,
// so they don't affect other tests.
afterEach(() => {
    server.resetHandlers()
})
// Clean up after the tests are finished.
afterAll(() => {
    server.close()
})

test('new test with msw', async () => {
    let container;

    await renderer.act(async () => {
        container = renderer.create(<LoginForm />);
    });

    await renderer.act(async () => {
        container.root.findByProps({ id: 'username' }).props.onChange({ target: { value: 'testing' }});
        container.root.findByProps({ id: 'login-form' }).props.onSubmit({ preventDefault: () => {} });
        // This line will make it work since it's enough time for the request to come back from Node
        // await new Promise(resolve => setTimeout(resolve, 100));
    });

    expect(container.toJSON()).toMatchSnapshot();

});

test('original test with immediate resolution', async () => {
    let container;
    jest.spyOn(global, 'fetch').mockImplementationOnce(url => Promise.resolve({
        json: () => Promise.resolve({
            id: 'f79e82e8-c34a-4dc7-a49e-9fadc0979fda',
            username: 'testing',
            firstName: 'John',
            lastName: 'Maverick',
        }),
        status: 200
    }));

    await renderer.act(async () => {
        container = renderer.create(<LoginForm />);
    });

    await renderer.act(async () => {
        container.root.findByProps({ id: 'username' }).props.onChange({ target: { value: 'testing' }});
        container.root.findByProps({ id: 'login-form' }).props.onSubmit({ preventDefault: () => {} });
    });

    expect(container.toJSON()).toMatchSnapshot();
    global.fetch.mockRestore();
});
kettanaito commented 3 years ago

Thank you for putting down that example!

I believe it's expected for the request to take a certain amount of time as opposed to stubbing fetch. When you stub a function, any call to that function will resolve with the mocked value immediately (unless specified otherwise). MSW doesn't stub modules, so when your component performs an HTTP request via fetch (considering JSDOM), that request travels its actual path:

fetch -> XMLHttpRequest (JSDOM) -> open/DONE -> response

It's logical to assume that the default request execution chain may take a longer time than a direct function stub. I don't have any more insights at the moment as to what exactly takes time, but it would be nice to profile that and see the issue if there's any. I certainly expect the two test cases to behave identically, apart from spyOn replacement with setupServer.

Please, if you have a chance, your investigation and feedback would be highly appreciated!

msutkowski commented 3 years ago

Wouldn't you have to await the form submission being that it is an async handler? I think mocking fetch was just masking the promise-related issues here, but I could be wrong!

await container.root.findByProps({ id: 'login-form' }).props.onSubmit({ preventDefault: () => {} });

Edit: I was curious about this, so I made a small repro where it works as expected based on the MSW template.

https://codesandbox.io/s/msw-test-react-test-renderer-qpd48?file=/test/App.test.js

kettanaito commented 3 years ago

You have a race condition between the form submit and the underlying assertion:

container.root
      .findByProps({ id: 'login-form' })
      .props.onSubmit({ preventDefault: () => {} })

// This assertion doesn't wait for the form submit.
expect(container.toJSON()).toMatchSnapshot()

Returning the fetch Promise from the handleFormSubmit callback and awaiting it in the test act makes the test pass reliably:

  // Handle a submit event of the form
  const handleFormSubmit = useCallback(
    (event) => {
      // Prevent the default behavior, as we don't want
      // for our page to reload upon submit.
      event.preventDefault()

      // Perform a POST /login request and send the username
+     return (
        fetch('/login', {
          method: 'POST',
          body: JSON.stringify({
            username,
          }),
        })
          .then((res) => res.json())
          // Update the state with the received response
          .then(setUserData)
          .catch((err) => console.error(JSON.stringify(err.message || err)))
      )
    },
    [username],
  )

test('new test with msw', async () => {
  let container

  await renderer.act(async () => {
    container = renderer.create(<LoginForm />)
  })

  await renderer.act(async () => {
    container.root
      .findByProps({ id: 'username' })
      .props.onChange({ target: { value: 'testing' } })
+   await container.root
      .findByProps({ id: 'login-form' })
      .props.onSubmit({ preventDefault: () => {} })
  })

  expect(container.toJSON()).toMatchSnapshot()
})

It's your responsibility to await asynchronous actions in tests. Even if a stubbed fetch resolves instantaneously, that just results in a false positive test, as the race condition between submitting and the assertion isn't resolved, it just happens to always run in the seemingly correct order.

Await the async logic, or await UI elements that derive from that logic's completion, and you will be fine.