Skip to content

docs: replace "axiosMock" with "msw" in React Testing Library example #483

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

Merged
merged 5 commits into from
Jun 4, 2020

Conversation

kettanaito
Copy link
Contributor

Changes

  • Removes the usage of jest.mock and axiosMock.get.mockResolvedValueOnce() in favor of a more declarative approach (see the reasoning in the GitHub issue).
  • Updates the "Step-by-step" section to briefly explain why msw has been added, and what each of its usage steps means.

GitHub

Notes

This was the only usage example I've found that illustrates testing a component that performs requests. Please, if I missed something, would you suggest me where to look for similar examples in the documentation? Thanks!

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just two quick things.

)

beforeAll(() => server.listen())
afterAll(() => server.close())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an afterEach(() => server.resetHandlers()) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a safe option—yes, but unless you declare runtime handlers via server.use() there’s nothing to reset. I’d not suggest calling resetHandlers by default. Developers that need it will find it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. Now that you mention it, I think the server.use call should be inside the test rather than outside it. That's what people should be doing most of the time.

With that in mind, resetHandlers should be included.

And I think it should be used by default 😉

Copy link
Contributor Author

@kettanaito kettanaito Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I please double check on this?

I see your point of using resetHandlers and server.use inside a test suite, but I think I'm confused about what runtime handler to add via server.use, as the example tests exactly one API communication to "GET /greeting".

const server = setupServer(
  // centralized place for request handlers,
  // generally encouraged.
  rest.get('/greeting', resolver)
)

beforeAll(...)
afterAll(...)

test('loads and displays greeting', () => {
  // I need `afterEach(() => server.resetHandlers())`
  // only if I use `server.use()` in any of my tests.
  server.use(/* however, what should I add here? */)
})

Am I missing something in this? 🤔 Do you suggest to move rest.get('/greeting') from setupServer to server.use inside a test suite?

I'm slightly concerned that showcasing both initial (setupServer) and runtime handlers (server.use) can be overwhelming for the reader, so I'd try to keep their usage to a minimum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fair point. There are two things to consider:

  1. Handle all of our application's typical requests so we don't have to think about them on a per-test basis
  2. Add a handler for a specific test (to test an error state)

With that in mind, I think we should adjust this example so we have two tests. One for the success case, and one for the error case. That way the first one can rely on the default handlers and one that can add a runtime handler for the error.

Because I see using afterEach(() => server.resetHandlers()) as an important default whenever someone uses server.use and we should probably demonstrate that so people don't add a bunch of complicated single-test-specific handlers to their setupServer call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is perfect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the error handling test scenario to this example.

  1. Rewrote useState to useReducer in fetch.js to reflect the error and data relation.
  2. Added a new test suite, used server.use() with permanent override there.
  3. Added afterEach that calls server.resetHanders().

Please, could you double check if that fetch.js looks okay to you? Thanks.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this. Thank you!

@kentcdodds kentcdodds merged commit 3861509 into testing-library:master Jun 4, 2020
@kettanaito kettanaito deleted the msw branch June 4, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider showcasing "msw" package for API mocking
2 participants