Using "onUnhandledRequest": "error" does not fail tests #946
Comments
bennettdams
changed the title
bennettdams
changed the title
|
Hey, @bennettdams. Thank you for opening the issue and for providing the reproduction repository. You're awesome. I can confirm the behavior you're experiencing and conclude that's an issue. I'm sharing some context and technical details on it below, both for posterity and for us to discuss. Don't hesitate to share your opinion on the best way to tackle this problem. Context
The issueThe challenge here is to do both of the following:
Terminating a pending requestThis is done in a fairly straightforward way: whenever
Request resolution is entirely encapsulated within the interceptor's call: msw/src/node/createSetupServer.ts Lines 59 to 80 in 11bb244 Exceptions in this block trigger the logic flow I've described above. From the MSW's perspective no exception has happened because interceptors gracefully handled it. Thus, you don't see that exception propagating and failing your test, even though it should.
Developer experienceIt's safe to assume that a great developer experience when using the "error" option for
While we're good with displaying a verbose error message, we currently don't throw the exception in the test runner's context (see the explanation above). This leads to the test runner thinking that the test passed (given no other criteria to fail it). ConclusionI'd like to conclude a few statements based on the investigation above.
Propagating the exceptionBefore throwing the unhandled request exception, the pending request must be terminated as described above (already present behavior). That is not to leave any pending promises afterward. This, in turn, means that the
In a nutshell, we need to perform something along these lines: const interceptor = createInterceptor({
modules: interceptors,
async resolver(request) {
const mockedRequest = parseIsomorphicRequest(request)
// "handleRequest" is where the exception is thrown.
// But we cannot propagate it here, that'd still be
// handled by the "resolver" function of the interceptors
// and treated as an arbitrary request/response exception
// aborting the pending request without propagating the exception.
return handleRequest<MockedInterceptedResponse>(
mockedRequest,
currentHandlers,
resolvedOptions,
emitter,
{
transformResponse(response) {
return {
status: response.status,
statusText: response.statusText,
headers: response.headers.all(),
body: response.body,
}
},
},
)
},
})
// <-- Need to propagate the exception to the higher scope.
// At the same time ensuring that it's propagated only after
// the pending request gets terminated (it still should).
// OPTION 1: Exception instance and a new callback.
interceptor.on('unhandledException', (error) => {
if (error instanceof UnhandledRequestError) {
// If an exception reported by the interceptors
// was the exception thrown by "onUnhandledRequest",
// propagate it to the upper scope.
throw error
}
})Unfortunately, when propagated, it'll result in the general unhandled promise rejection: (node:13492) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)This will not fail the test. |
kettanaito
added
DX
scope:browser
On exception's scopeLet's go from the expected behavior here. We expect the it('some test', () => {
// Either direct exceptions
throw new Error('Like this')
// Or exceptions thrown anywhere
// in the child scope.
render(<Component />)
})Since the user won't throw exceptions in the test, we are implying a child scope here. Note that the exception must still be thrown, and that's where the biggest difficulty comes. MSW itself has access only to two scopes:
Throwing an error in the test setup's scope would be unexpected, as you lose the test-failure association. Thus, the only remaining scope where the library has any influence is the request client scope. You see, request clients don't throw exceptions. And they are right in that: you don't wish for your entire app to blow if a single And that leads us to a great discovery: MSW already throws the exception in the request client's scope upon unhandled requests! That's precisely the thing that terminates pending requests as I described above. You may access that exception using the error handling strategy appropriate for your request client (that depends on how the client exposes those exceptions to you). |
|
@kettanaito Thanks a lot for your detailed answer, but I'm lacking knowledge to give you better feedback for that. One thing I can ask though:
Do you have an example on how to do that? Let's take the easiest "request client": Usually, you have some kind of reusable helper function to execute requests (which e.g. uses // http-utils.ts
async function fetchSomething() {
// imagine a more sophisticated implementation here
try {
return await fetch("https://jsonplaceholder.typicode.com/todos/1")
} catch (error) {
throw error
}
}
// SomeComponent.tsx
useEffect(() => {
async function test() {
const response = await fetchSomething()
console.log("Response: ", response)
}
test();
}, []);Where would you access the exception, when the request is "interrupted" by MSW? As you said, as a developer we maybe don't want to throw errors here, to not blow the entire app. How would we handle MSW's exception then to let the test fail? I think one of the great benefits of MSW is that I can write my tests without thinking about requests that are used for a test every time, because I took care of that in my As a compromise, if this is not working, I'd be also fine with having one single test whose only purpose is failing when an unhandled request occured in any of the other tests, based on MSW. That will not show me which test exactly is responsible for sending that request, but at least I know that I have an unhandled request somewhere and can start digging myself. Is there a solution for this right now? // msw-setup.ts
...
export const server = setupServer(...handlers);
.....and then use that in a dedicated test: describe("Requests", () => {
it("throw an error when an unhandled request is found", () => {
const mswSpy = jest.spyOn(server, "onUnhandledRequest")
expect(mswSpy).not.toHaveBeenCalled();
});
}); |
|
We cannot base this approach on any exact request client, unfortunately. People may use Take a look at this example: /**
* @jest-environment jsdom
*/
import 'whatwg-fetch'
it('fetch (plain)', () => {
fetch('https://do.not.exist')
})
it('fetch (async/await)', async () => {
await fetch('https://do.not.exist')
})
it('XMLHttpRequest', () => {
const xhr = new XMLHttpRequest()
xhr.open('GET', 'https://do.not.exist')
xhr.send()
})Only the second test will fail requesting a resource that doesn't exist. And it will fail only because it disrupts the await flow and only because
Status code is a property on response. When there's an exception during a request, it won't be performed, thus, you get no response. This behavior is correct.
While the test is not marked as failed, any assertions that depend on data will fail. The request is still terminated even as of now, so if you rely on any data from your
I strongly advise you against this. Tests are here to reflect the intention of the software you write. They must not act as logical gateways to orchestrate other tests. |
I think this point is very important and comes down to why I think the tests should fail on unhandled requests anyways: I want to fill my |
|
The behavior you expect is reasonable and I think the library should also behave that way. At the moment I don't see how we can achieve that, so we must research the options before moving forward. My discoveries above are a great starting point but they don't spotlight any possible solutions to this issue. We may want to look into how other similar libraries are failing tests, I'm sure they have their analogs of |
|
Looking into how other libraries deal with this, // my.test.js
const nock = require('nock')
nock.disableNetConnect()That, however, doesn't fail the test, it exits the process. This is a disruptive action and I'd much prefer for tests to continue running and only the affected tests to be marked as failed. |
|
Hello @kettanaito, what is the status of your investigation ? |
|
Hey, @balzdur. There hasn't been any additional investigation than the one above. I did stumble upon what seems the behavior we want when rewriting the ClientRequest interceptor but I don't have tests to confirm that. Anyone is welcome to dive into this to see how we can support failing individual tests (or the entire test suite) whenever Note on semanticsI wonder if such test failures are semantic in regards to your assertions. Think of it this way: if your test setup (i.e. the Since MSW is, effectively, a party of your testing setup, I'd say that it makes more sense to fail the entire test suite rather than try to figure out how to associate requests with the test code that issued them (spoiler: that's likely impossible). That being said, it still poses a challenge due to how the library is architectures. As the first step, I'd try propagating the exception from |
kettanaito
changed the title
|
I also wonder what would happen if MSW responds to unhandled requests with 500 (or 405 if that makes more sense)? That should halt individual tests, as that server error would propagate to the request client and cause an exception. Even if such an error is handled by the client, it still won't produce the expected state. The only downside here is that false-positive tests are still possible with this approach. |
|
I think moving the .listen call around can help slightly with the error messages, but still not great. If we have the listen call at the module top level the stack trace points to a line number at the listen call (which isn't inside the test) If we put the listen call in the test itself, then it will provide a line number so we can more easily track down which test failed, but still not great. edit: looking at how Apollo does it, they have a similar issue: apollographql/apollo-client#5917 |
|
I spent more time playing with this, and this is what I have so far: const server = setupServer()
// Start server before all tests
beforeAll(() => server.listen())
// Close server after all tests
afterAll(() => server.close())
// Reset handlers after each test `important for test isolation`
afterEach(() => server.resetHandlers())
// keep track of inflightRequests because onUnhandledRequest is called after
// `afterEach`.
const inflightRequests = new Map<string, MockedRequest<DefaultBodyType>>()
server.events.on("request:start", req => {
inflightRequests.set(req.id, req)
})
server.events.on("request:end", req => {
inflightRequests.delete(req.id)
})
// check if we had any unhandled requests
afterEach(() => {
const errors = [...inflightRequests.values()]
inflightRequests.clear()
if (errors.length) {
// TODO: figure out if we can include where in the code the request was called
const err = Error(
errors
.map(
req => `found an unhandled ${req.method} request to ${req.url.href}`,
)
.join("\n\n"),
)
// clear the useless stack trace
err.stack = undefined
throw err
}
})This results in tests with output like:
Edit: it doesn't work, the Edit2: with some promises I think it now works: import { setupServer } from "msw/node"
import { DefaultBodyType, MockedRequest } from "msw"
type Deferred<T> = {
promise: PromiseLike<T>
done: () => void
error: () => void
}
function deferred<T>(data: T): Deferred<T> {
// based from: https://stackoverflow.com/a/69027809/3720597
let resolve: (value: T | PromiseLike<T>) => void
let reject: (reason?: unknown) => void
const promise = new Promise<T>((res, rej) => {
resolve = res
reject = rej
})
return {
promise,
done: () => {
resolve(data)
},
error: () => {
reject(data)
},
}
}
const server = setupServer()
beforeAll(() => server.listen())
afterAll(() => server.close())
afterEach(() => server.resetHandlers())
const inprogress = new Map<string, Deferred<MockedRequest<DefaultBodyType>>>()
server.events.on("request:start", req => {
inprogress.set(req.id, deferred(req))
})
server.events.on("request:match", req => {
inprogress.get(req.id)?.done()
})
server.events.on("request:unhandled", req => {
inprogress.get(req.id)?.error()
})
afterEach(async () => {
const errors = (
await Promise.allSettled([...inprogress.values()].map(x => x.promise))
)
.filter((x): x is PromiseRejectedResult => x.status === "rejected")
.map((x): MockedRequest<DefaultBodyType> => x.reason)
inprogress.clear()
if (errors.length) {
// TODO: figure out if we can include where in the code the request was called
const err = Error(
errors
.map(
req => `found an unhandled ${req.method} request to ${req.url.href}`,
)
.join("\n\n"),
)
// clear the useless stack trace
err.stack = undefined
throw err
}
}) |
Basically: - Replace web pack setup with vite and upgrade various dependencies and cull unnecessary ones. - Also remove TSLint. Will replace with typescript-eslint rules eventually. - Got rid of the hacky landing page static generation. Various road bumps along the way: - https://javascript.plainenglish.io/how-to-set-up-path-resolving-in-vite-ad284e0d9eae - fix sass imports vitejs/vite#5764 (comment) - Then needed to rewrite the alias for typescript again to match the types - Replace `process`. With the `import.meta.env` thing - https://stackoverflow.com/questions/64677212/how-to-configure-proxy-in-vite - Fix imports for static files from `requires()` - Had to fix proxying for `/avatar` and `/api` in dev - Ran into remarkjs/react-markdown#339 (comment) - Upgrade markdown react to fix - Migrate from `react-helmet` to fix some deprecation warnings - Vite has a different jsx config, so no need to import React - microsoft/TypeScript#41882 - Vitest issue: - https://github.com/vitest-dev/vitest/blob/57c2367196b3fd978a04fa38ebdac7a5b6ef9b16/packages/vite-node/src/client.ts#L178-L179 - Couldn’t import react-dnd, upgraded - react-dnd/react-dnd#3368 - `import { isUndefined } from "util"` didn’t work - Favicon via https://github.com/darkobits/vite-plugin-favicons to replace web pack equivalent - Issue with React router browser history in vitest, it exploded until jsdom was setup - Question: https://stackoverflow.com/questions/71703933/what-is-the-difference-between-vite-and-vite-preview - Vitest vscode integration broken :/ - vitest-dev/vscode#44 - Tried happy-dom but it doesn’t work, lots of issues, supposed to be faster - Took a while to get MSW in a good place, had to write some stuff so missing endpoint mocks fail the test - I think there's room for improvement here in terms of developer experience - Test with react testing library and API calls - https://www.npmjs.com/package/nock - Doesn’t fail test on unknown mock - https://stackoverflow.com/questions/69430232/jest-nock-how-to-fail-a-test-if-a-non-mocked-request-is-made - MSW - Doesn’t fail test on unknown mock - mswjs/msw#946 - Relay’s mockEnvironment - couldn't easily find thorough examples - Apollo mock provider - Doesn’t fail test on unknown mock - Added a visualize plugin similar to a web pack analyzer thing, but it’s slightly off with the numbers it gives for sizes: - btd/rollup-plugin-visualizer#96 - TODO: - ESLINT rules, replace what tslint provided, eslint-cake anyone? - https://typescript-eslint.io/rules/no-floating-promises/ - And more!!! - Replace lodash with lodash-es - Or maybe: https://github.com/onebay/vite-plugin-imp - Although lodash-es seems cleaner - SSR for landing page?
|
@sbdchd I have the same problem and at least for my very simple test suite the following hack worked: onUnhandledRequest: (req) => {
test(`${req.url} is not handled`, () => {});
},
|
|
@nico1510, you can use life-cycle methods to have a similar effect: test('my test', () => {
server.on('request:unhandled', () => {
throw new Error('Must handle request')
})
})Would this work the same way? Depends whether that exception will propagate to the |
|
@kettanaito hmm but how is this different than using the server.events.on('request:unhandled', () => {
throw new Error('Must handle request');
});and somehow it's not even executed 🤔 To give a little more context about why one would like to make tests fail on unhandled requests. Your point here makes a lot of sense to me:
But... In reality multiple developers work on the same code base and someone might add an unhandled call to some of the components under test. The developer who first wrote the test could not have predicted this and now there might be unexpected changes in the way his code is executed. Even if those changes don't make the tests fail I see this as a problem because at some point they might. E.g. if someone adds another request which depends on the first unhandled request he has now the burden of mocking both requests.
Users might handle those errors in their request libraries in multiple different ways e.g. by showing an error fallback or retrying requests or whatever... So I think if someone wants to have this behaviour then they could just implement this themselves via the |
Yeah, I'm totally on board with this. It's rather a technical challenge to make this happen. I'm open to suggestions and, hopefully, pull requests. I've started at #951 but never got anywhere with this. As you've correctly pointed out, the exceptions are thrown in So, we either need a way to hook into the individual |
|
Yes that makes sense and after your explanations I'm starting to think that it shouldn't even be of any concern to |
|
@kettanaito My teams is relatively new to MSW and have come up against this in our test suite. We're using Jest for our testing. To address the issue we're spying on the const consoleSpy = jest.spyOn(global.console, "warn")
describe('Some tests', () => {
beforeAll(() => server.listen())
afterEach(() => {
server.resetHandlers()
expect(consoleSpy).not.toHaveBeenCalled()
})
afterAll(() => server.close())
// tests
})While it would be good to have tighter integration with Jest (and others) which could be a lot of effort perhaps a less integrated solution would be good for the interim: describe('Some tests', () => {
beforeAll(() => server.listen())
afterEach(() => {
server.checkUnhandledRequests()
server.resetHandlers()
})
afterAll(() => server.close())
// testsIf there are unhandled requests the |
|
Hi, @michael-harrison. That's a neat implementation. It would mess up the stack trace a bit though, as the exception would happen in the |
|
@kettanaito Thanks, it's just a thought at the moment. TLDR; the v1.1.0 update release a few days ago resolves the issue for us. I assume by messed up you're meaning the stack trace will not point you to the exact line in the test that has a API call with a missing response. Yes this is true however, Jest will report the test that failed. I did a quick test with jest for proof of concept: import {afterEach, describe, it} from "@jest/globals"
let throwMe
describe('Some tests', () => {
afterEach(() => {
if (throwMe) {
throw new Error('You failed')
}
})
describe('when some condition 1', () => {
it('will have the expected result', async () => {
// Test
throwMe = true
})
})
describe('when some condition 2', () => {
it('will have the expected result', async () => {
// Test
throwMe = false
})
})
})Output from the test FAIL tests/ui/testThrow.spec.js
Some tests
when some condition 1
✕ will have the expected result
when some condition 2
✓ will have the expected result (1 ms)
● Some tests › when some condition 1 › will have the expected result
You failed
5 | afterEach(() => {
6 | if (throwMe) {
> 7 | throw new Error('You failed')
| ^
8 | }
9 | })
10 |
at Object.<anonymous> (tests/ui/testThrow.spec.js:7:13)
Test Suites: 1 failed, 1 total
Tests: 1 failed, 1 passed, 2 total
Snapshots: 0 total
Time: 0.765 s, estimated 1 sThis may not be the same for all test frameworks. That aside, since encountering this issue (about 4 days ago) I noted that an updated for MSW has been released V1.0.0 > V1.1.0 3 days ago. I've done the update and I can confirm that the update resolves this issue for my team; the following now causes tests to fail when there are unhandled requests: beforeAll(() => server.listen({onUnhandledRequest: 'error'}))Which yields something like Error sending the query 'myQuery' FetchError {
message: 'request to http://core.test.lan/app/graphql failed, reason: [MSW] Cannot bypass a request when using the "error" strategy for the "onUnhandledRequest" option.',
type: 'system',
errno: undefined,
code: undefined
}The stack trace allows us to dig down into the library calling the API library where the call came from. |
|
That is pretty nice. We haven't shipped this support explicitly but maybe something in the context changed, which now results in those unhandled exceptions propagating as request errors. Come to think of it, they should've always been treated as response errors but I frankly haven't touched this part of the library in quite some time so I don't remember. |
|
@kettanaito thanks for the amazingly detailed explanation of the problem. after reading all the comment I ended up using this solution. import {
vi,
describe,
beforeAll,
afterEach,
afterAll,
test,
expect,
} from 'vitest';
import { setupServer } from 'msw/node';
import { rest } from 'msw';
import Axios from 'axios';
const getData = (url: string) => {
return Axios.get(url).then(({ data }) => data);
};
const onUnhandledRequest = vi.fn();
const server = setupServer(
rest.get('https://jsonplaceholder.typicode.com/todos/1', (_, res, ctx) => {
return res(
ctx.status(200),
ctx.json({
a: 'a',
})
);
})
);
describe('onUnhandledRequest workaround', () => {
beforeAll(() => {
server.listen({
onUnhandledRequest,
});
});
afterEach(() => {
server.resetHandlers();
expect(onUnhandledRequest).not.toHaveBeenCalled();
onUnhandledRequest.mockClear();
});
afterAll(() => {
server.close();
});
test('request handler matched', async () => {
const data = await getData('https://jsonplaceholder.typicode.com/todos/1');
expect(data).toMatchObject({
a: 'a',
});
});
test('request handler not matched', async () => {
const data = await getData('https://do.not.have.a.handler');
expect(data).toMatchObject({
a: 'a',
});
});
});this should be straight-forward to implement with any test runner. for vite I am able to see which test is failing as below which is enough for me.
|
UpdateThere's a bit of news regarding this. When using global fetch in Node.js, you will get individual test failures because the "error" strategy will manifest as the Ultimately, this will always come down to how the request client you're using is handling/forwarding unhandled exceptions. Most likely, those will be treated as network errors and will, once again, fail the relevant requests and tests. We can't do anything else really on MSW's side here. MSW sits below your request client on the environment level so we don't have access to the individual test's context to affect it in any way. I'm reluctant to consider things like module IDs because those are likely not to be reliable. I'm closing this issue in its current state. I recommend upgrading to |
|
Since 2.2.0, I believe we can use Pull requests are welcome! |
Describe the bug
When a server ist configured to listen with
onUnhandledRequest: "error", the test where this error occurs does not fail.Based on this discussion: #943
Environment
https://github.com/bennettdams/msw-nextjs-bug/blob/master/package.json
This happens for a fresh install with all default configuration from the Next.js & MSW docs.
msw: 0.35.0nodejs: 14.18.0npm: 6.14.15To Reproduce
Reproduction repo: https://github.com/bennettdams/msw-nextjs-bug
To try it out:
npm inpm run testSteps to reproduce the behavior from zero:
Install initial Create Next App with TypeScript
npx create-next-app@latest --use-npm --ts .bennettdams/msw-nextjs-bug@2cf426a
Create
srcfolder at root and movepagesfolder to itbennettdams/msw-nextjs-bug@78b657e
Install & configure Jest, testing libs, etc. based on Next's docs:
npm install --save-dev jest babel-jest @testing-library/react @testing-library/jest-dom identity-obj-proxy react-test-rendererbennettdams/msw-nextjs-bug@aaf1fb3
Create a simple test file based on Next's docs:
bennettdams/msw-nextjs-bug@bd9ba77
Install MSW and follow setup for mocks/handlers/server via docs:
npm install msw --save-devbennettdams/msw-nextjs-bug@cdd07c1
Integrate MSW with Jest
bennettdams/msw-nextjs-bug@3973836
Install and utilize
whatwg-fetch- needed for Polyfill with Next.jsnpm install -D whatwg-fetchbennettdams/msw-nextjs-bug@5cbe84a
Change server config to
onUnhandledRequest: "error"and add some simple fetch execution in the tested componentbennettdams/msw-nextjs-bug@f77bb0f
==> The test does not fail, even though the tests shows the error.
Expected behavior
When
onUnhandledRequestis configured witherrorand an unhandled request is found, the test shoud fail.The text was updated successfully, but these errors were encountered: