Add an option to warn/error on unhandled requests #257

jensmeindertsma · 2020-07-01T14:53:17Z

GitHub

Resolves Add an option to warn/error on unmocked requests #191

Specification

This specification will be kept up to date as we iterate over the functionality.

An unhandled request is a request that has no corresponding request handler. It's possible to configure MSW to handle such requests differently. For example, MSW could notify the user with a warning or error message when a request has no corresponding request handler.

onUnhandledRequest

Both worker and server instances accept the onUnhanledRequest configuration option. This allows the user to choose a provided functionality or provide their own through a custom function:

type OnUnhandledRequest = 'bypass' | 'warn' | 'error' | (req: MockedRequest) => void

Important: all functionality is opt-in. This means that the default value for onUnhanledRequest is 'bypass'.

Usage example

const worker = setupWorker(...)
const server = setupServer(...)

worker.start({ onUnhandledRequest: 'bypass' })
server.listen({ onUnhandledRequest: 'error' })

Draft

This pull request is a work in progress. That means large portions of functionality have not been implemented yet, and the functionality and specification details are still open for debate.

jensmeindertsma · 2020-07-01T16:12:42Z

Will throwing an error on these lines display an error message as intended or should I do console.error?

cloud-walker · 2020-07-01T23:24:52Z

src/node/setupServer.ts

+          if (resolvedOptions.onUnhandledRequest === 'warn') {
+            // Produce a developer-friendly warning
+            return console.warn(
+              `A request to ${mockedRequest.url} was detected but not mocked because no request handler matching the URL exists.`,
+            )
+          }
+
+          if (resolvedOptions.onUnhandledRequest === 'error') {
+            // Throw an exception
+
+            // throw new Error(`A request to ${req.url} was detected but not mocked because no request handler matching the URL exists.`)
+            return
+          }
+
+          if (typeof resolvedOptions.onUnhandledRequest === 'function') {
+            resolvedOptions.onUnhandledRequest(mockedRequest)
+            return
+          }
+
+          // resolvedOptions.onUnhandledRequest === 'bypass'


I would personally avoid duplication of both the error / warn message and the onUnhandledRequest logic, by creating a util, like:

export const onMissingRequestHandler = ( request: MockedRequest, onUnhandledRequest: OnUnhandledRequest, ) => { if (typeof onUnhandledRequest === 'function') { onUnhandledRequest(request) return } if (['warn', 'error'].includes(onUnhandledRequest)) { const message = `[MSW] A request to ${request.url} was detected but not mocked because no request handler matching the URL exists.` if (onUnhandledRequest === 'warn') { console.warn(message) } else { throw new Error(message) } return } // if onUnhandledRequest is 'bypass' we have nothing to do. }

I've added [MSW] tag at the beginning of the message and refactored a bit the logic.

cloud-walker · 2020-07-01T23:27:08Z

src/utils/handleRequestWith.ts

+        if (options.onUnhandledRequest === 'warn') {
+          // Produce a developer-friendly warning
+          console.warn(
+            `A request to ${req.url} was detected but not mocked because no request handler matching the URL exists.`,
+          )
+          return channel.send({ type: 'MOCK_NOT_FOUND' })
+        }
+
+        if (options.onUnhandledRequest === 'error') {
+          // Throw an exception
+
+          // throw new Error(`A request to ${req.url} was detected but not mocked because no request handler matching the URL exists.`)
+          return channel.send({ type: 'MOCK_NOT_FOUND' })
+        }
+
+        if (typeof options.onUnhandledRequest === 'function') {
+          options.onUnhandledRequest(req)
+          return channel.send({ type: 'MOCK_NOT_FOUND' })
+        }
+
+        // options.onUnhandledRequest === 'bypass'


We can avoid channel.send duplication and onUnhandledRequest logic by creating the util described in a previous comment.

Not sure of how much is the effort, but we should test this logic on both worker.listen and server.start

Yes. Would you be willing to write the tests?

Absolutely agree, there's no need to duplicate the channel.send() and any other possible logic in case of an unhandled request. You can think of the new feature as:

if (!response) { onUnhandledRequest(req, resolvedOptions) // analyzes "resolvedOptions.onUnhandledRequest" return channel.send({ type: 'MOCK_NOT_FOUND' }) }

To clarify: I mean exactly what @cloud-walker suggested above: a custom function that encapsulates the logic (checks if the option value is a function, or one of the enums).

Yes. Would you be willing to write the tests?

How can contribute in this scenario? You give fork permissions to me? Or I should do another fork? 🤔

I will give you permission to push to my fork

I've merged the changes made by @kettanaito in the upstream repository so we're all up to date and I've given you access. Can you verify that it works? Haven't done this before.

kettanaito · 2020-07-02T07:54:47Z

src/node/glossary.ts

+import { OnUnhandledRequest } from '../onUnhandledRequest'
+
+export interface ListenOptions {
+  onUnhandledRequest?: OnUnhandledRequest


Considering we will be shipping the same behavior in terms of this option to both Browser and Node, do you find it useful to introduce a concept of "shared" configuration?

interface SharedOptions { onUnhandledRequest?: OnUnhandledRequest } type WorkerOptions = SharedOptions & { serviceWorker: WhatWeHaveNow // ... } type ServerOptions = SharedOptions

All those interfaces, apart from the SharedOptions, already exist in the respective setupWorker/setupServer declarations.

Perhaps we could call it IsomorphicOptions? Doesn't really matter to me.

"Shared" seems to describe that the options are shared between multiple units. I'm not sure what "isomorphic" means in relation to those settings.

kettanaito · 2020-07-02T07:59:21Z

src/node/setupServer.ts

@@ -10,6 +10,11 @@ import { getResponse } from '../utils/getResponse'
 import { parseRequestBody } from '../utils/parseRequestBody'
 import { isNodeProcess } from '../utils/isNodeProcess'
 import * as requestHandlerUtils from '../utils/requestHandlerUtils'
+import { ListenOptions } from './glossary'
+
+const DEFAULT_LISTEN_OPTIONS: DeepRequired<ListenOptions> = {


No need to use DeepRequired, it's one level object (for now).

jensmeindertsma · 2020-07-02T09:02:15Z

I added most of the changes that you requested, but there is still one problem: due to the use of Object.assign, onUnhandledRequest still has a possible type of undefined, after combining the passed options with the default options. This causes a type error when passing options.onUnhandledRequest to the function, because it doesn't accept undefined. To fix this, let's check whether options.onUnhandledRequest is defined inside the function, and assign a default value of 'bypass' here if it isn't. Are you OK with this idea @kettanaito ? It will move the default value for this options away from the DEFAULT_OPTIONS object.

cloud-walker · 2020-07-02T23:03:54Z

@kettanaito I've no idea on how to run the integration tests, running yarn test:integration all test cases are failing for.. reasons.

You have any advice on how to start? Thanks!

jensmeindertsma · 2020-07-03T11:05:40Z

They could be failing because of TypeScript errors?

jensmeindertsma · 2020-07-03T12:02:55Z

This change fixes the TypeScript issues and probably also solves the issue with the integration tests for @cloud-walker , but it does make the code a little less DRY. Please give your opinion on this change and whether you have a better suggestion.

kettanaito · 2020-07-06T08:19:13Z

You've done an amazing work, fellows! I've rebased the branch and will add some tests to cover this feature. Looks solid!

jensmeindertsma · 2020-07-06T13:34:21Z

Awesome!

kettanaito · 2020-07-07T07:35:17Z

I'm having troubles testing the "error" scenario, because it's designed to throw an exception. The trick here is that the exception is not thrown as a part of fetch call, but within the process that runs Jest. I've tried various ways to catch that exception, but all had no effect:

expect(myFunc).toThrow()
expect(myFunc()).rejects.toThrow()
expect(myFunc()).rejects.toEqual()
process.on('unhandledException')
process.on('unhandledRejection')

The culprit is that the "error" value is designed to fail the test, and it does, including the test on the MSW side. That is the expected behavior, so we need to somehow convert that failed test into a "passing" test.

Also having a little in-browser issues while testing, but should figure them out. Manual testing of the feature works great.

cloud-walker · 2020-07-07T16:20:00Z

@kettanaito you can share the code of the tests?

kettanaito · 2020-07-08T06:24:06Z

Sure. I've pushed the setupServer tests. Note that it would need mswjs/interceptors#29 to be released in order to pass.

There's a trick towards throwing in the request promise: if not handled, that exception will terminate the current running process (i.e. a test). Although it seems expected, I couldn't find a reliable way to assert that in tests. Instead, I've introduced a feature to node-request-interceptor that, if an exception is thrown in a request promise, it imitates the request error (similar to network error). This results into error being thrown by request issuing function (i.e. fetch).

kettanaito · 2020-07-09T08:53:43Z

I've pushed browser integration tests. The following tests are failing, I cannot seem to figure why:

error.test.ts
callback-throws.test.ts

Manual testing produces expected behavior. There must be something on the Puppeteer's/test setup side that fails to assert that behavior properly. If somebody could take a look at those tests, I'd be much thankful!

cloud-walker · 2020-07-09T15:50:35Z

I'm not an expert of puppeteer, but I can give an eye 👀

@kettanaito The command to run is npm run test:integration?

kettanaito · 2020-07-09T19:38:38Z

@cloud-walker that'd be awesome.

Yeah, to run a specific integration test use:

$ yarn test:integration test/msw-api/setup-worker/start/on-unhandled-request/warn.test.ts

You can also run a specific test scenario in a browser to play around with by this command:

$ yarn test:focused test/msw-api/setup-worker/start/on-unhandled-request/warn.mocks.ts

For some reason both error and callback-throws scenarios fail (they don't produce an error in the console during runtime). Manual testing works as expected. Something must be off.

cloud-walker · 2020-07-09T21:38:56Z

I get a console.error about the "msw" dep missing:

I should do something else? Like yarn link?

jensmeindertsma · 2020-07-10T07:54:05Z

Have you run yarn build yet?

cloud-walker · 2020-07-10T09:41:45Z

@kettanaito I get a SIGABRT error (by puppeteer I guess) or a SIGSDEV or SIGBUS at random on all the tests (bypass included). Seems to be in heisenbug..

My guess is that some cleanup of puppeteer is missing, but I'm not an expert of it T_T

kettanaito · 2020-07-10T10:57:19Z

Yes, you need to yarn build the package, as the built version is used in the integration tests.
Those exceptions are rather strange. Please, @cloud-walker, which OS are you using?

cloud-walker · 2020-07-10T13:11:31Z

I use MacOS mojave

kettanaito · 2020-07-12T07:07:54Z

There's one nuance I've noticed: since all requests come through Service Worker and MSW, if you specify onUnhandledRequest: 'error' this, most likely, will break your entire page. Requests to third-party integrations, local assets, and even internal endpoints (like requests that webpack polls for HMR) will error, since they don't have (and shouldn't have?) a corresponding request handler.

I can't say I disagree with such behavior, since the "error" value is quite self-descriptive, but I'd be cautious about this. What do you think, folks?

jensmeindertsma · 2020-07-12T07:12:05Z

Perhaps it would be better to include a required RegEx option with onUnhandledRequest where only routes that match the RegEx get "blocked" when using error. Also, the error option could mock a network error as well as throwing a error in the console. What do you think of this?

cloud-walker · 2020-07-12T10:25:42Z

There's one nuance I've noticed: since all requests come through Service Worker and MSW, if you specify onUnhandledRequest: 'error' this, most likely, will break your entire page. Requests to third-party integrations, local assets, and even internal endpoints (like requests that webpack polls for HMR) will error, since they don't have (and shouldn't have?) a corresponding request handler.

I can't say I disagree with such behavior, since the "error" value is quite self-descriptive, but I'd be cautious about this. What do you think, folks?

I personally think it's the expected behavior, at least for setupServer. During test development you want the code to fail as early as possible.

For setupWorker I agree, seems a bit odd. But I guess that the majority of users will choose warn here.

kettanaito · 2020-07-15T08:42:40Z

@jensmeindertsma, I'm not against supporting a RegExp as the value of onUnhandledRequest, but I would start with shipping the current state of the feature first. That is to see if there are more people in need of regular expressions. My concern is to keep this feature small and predictable.

Regarding making the error value behave as a network error, I think this would be confusing. The next release will also include the actual network error emulation via res.networkError() (#253), which you can utilize on per-handled basis. You can achieve a similar behavior to unhandled requests and res.networkError() using a fall-through handler:

// Handled request
rest.get('/user', (req, res) => res()),

// Fall-through handler matching any other request
res.get('*', (req, res) => res.networkError('Any other request raises a network error!')

The error option for unhandled requests is meant to raise an error in your test run, not in the mocked response.

jensmeindertsma · 2020-07-15T08:58:07Z

Is there still work to be done to get the current version of this feature shipped?

kettanaito · 2020-07-15T09:30:24Z

There are two test suits that fail:

error.test.ts
callback-throws.test.ts

We need to either construct them better, or make sure they pass in the current state before shipping this.

cloud-walker · 2020-07-15T14:49:03Z

@kettanaito you think it's possible to reproduce the bugged scenarios on a dummy repo? Just to remove the real project noise?

kettanaito · 2020-07-21T08:31:07Z

I'm not sure, @cloud-walker. That noise may contain a valuable context. Once we bring a potentially solved test scenario in, it may start failing anyway. I'd say it's a good practice to approach tests exactly where they're being run.

This certainly takes more time, but I'm confident it's worth it. Sometimes Puppeteer is tricky, but most of the times it's just me now knowing how to use it. For example, I wonder why it doesn't get the console errors that are clearly there if you run Puppeteer in a headfull mode. The overall page.on('console') listener seems to be flaky at times, yet the error production must be consistent.

I'm a strong believer that no test is a thousand times better than a flaky one. I'll give this one more go, and if I cannot find a reliable way to test certain error scenarios they simply won't be added.

kettanaito · 2020-07-22T08:15:55Z

After significant amount of effort I conclude error and callback-throws test scenarios as unreliable. The list of intercepted console errors sometimes is an empty array [] received from Puppeteer, and I can't find a reason why.

That being said, those scenarios do produce an expected behavior during manual testing.

I'm removing those tests and checking if the CI passes with the remaining browser and server tests. I've still added the *.mocks.ts files that represent the tested behavior, for manual testing mainly.

…ption

kettanaito

Thank you everybody for participating in developing this feature!
It looks great, let's publish it and give it a try. Approved 🎉

jensmeindertsma · 2020-07-22T08:56:32Z

🎉

This enables us to use onUnhandledRequest introduced in mswjs/msw#257

* chore(dependencies): bump msw to 0.20.5 This enables us to use onUnhandledRequest introduced in mswjs/msw#257 * chore(msw setup): throw on un-intercepted requests * fix(reports/apiService): errors during submit are not being detected as such We could only detect this by stop mocking fetch and letting msw intercept the request on the network level. Now we know the apiService and the thunk invoking it did not rethrow the error caught by ky. * test(reports): test successful submit using msw closes https://github.com/FixMyBerlin/fixmy.platform/issues/250 relates to https://github.com/FixMyBerlin/fixmy.platform/issues/424 * chore(dependencies): update jest and ts.jest to v26 * chore(jest setup): cleanup * refactor(OverviewMapState unit tests): replace fetch-mock with msw * refactor(OverviewMapState unit tests): increase readability * refactor(SubmitReportState unit tests): increase readability * refactor(survey unit tests): replace fetch-mock with msw * refactor(api unit tests): replace fetch-mock with msw * chore(jest setup): only use one setup file This simplifies our setup as it has been desired. It makes not much sense to split up the logic running before each test into two files. It is not 100% clear, but setupFilesAfterEnv seems to offer everything setupFiles does PLUS globals like `expect` or `beforeAll` can be used. See https://stackoverflow.com/questions/47587689/whats-the-difference-between-setupfiles-and-setuptestframeworkscriptfile * Fix article unit test * Add new fetch error to expected actions * Revert commenting out code * fix(OverviewMapState tests) bugs where introduced in 8a49c12 and e2583aa * refactor(SubmitReportState.unit.test): prettier Co-authored-by: Vincent Ahrend <mail@vincentahrend.com>

jensmeindertsma mentioned this pull request Jul 1, 2020

Add an option to warn/error on unmocked requests #191

Closed

cloud-walker suggested changes Jul 1, 2020

View reviewed changes

kettanaito added this to the 0.20.0 milestone Jul 2, 2020

kettanaito reviewed Jul 2, 2020

View reviewed changes

kettanaito mentioned this pull request Jul 7, 2020

Cancel a request when the middleware throws an exception mswjs/interceptors#28

Closed

kettanaito mentioned this pull request Jul 8, 2020

Updates "node-request-interceptor" to version 0.3.0 #267

Merged

Jens Meindertsma and others added 11 commits July 22, 2020 10:23

Added basic outline for new unhandled request warnings

c53b4c0

Fixed issue where MOCK_NOT_FOUND event was not sent to worker

38d7f29

Added missing req parameter to CustomFunction type

24ded9e

Added warning functionality & support for custom function

e839bef

Updated custom function usage for better safety in JavaScript

5942187

Implement requested changes

35061fe

Fixed TypeScript issues

a17ea6b

Adjusts "onUnhandledRequest" logic

17470b1

setupServer: Adds integration tests for "onUnhandledRequest" listen o…

19b1500

…ption

Adds browser integration for "onUnhandledRequest" option

31a5bc1

Adds browser test scenarios for "error" and "callback-throws" cases

fa0c0f7

kettanaito marked this pull request as ready for review July 22, 2020 08:30

kettanaito approved these changes Jul 22, 2020

View reviewed changes

kettanaito merged commit 6001b79 into mswjs:master Jul 22, 2020

jensmeindertsma deleted the feature/warn-on-unhandled branch July 22, 2020 08:56

s-pic added a commit to FixMyBerlin/fixmy.frontend that referenced this pull request Aug 27, 2020

chore(dependencies): bump msw to 0.20.5

6242da0

This enables us to use onUnhandledRequest introduced in mswjs/msw#257

Add an option to warn/error on unhandled requests #257

Add an option to warn/error on unhandled requests #257

Conversation

jensmeindertsma commented Jul 1, 2020 • edited by kettanaito Loading

GitHub

Specification

onUnhandledRequest

Usage example

Draft

jensmeindertsma commented Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensmeindertsma Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensmeindertsma commented Jul 2, 2020

cloud-walker commented Jul 2, 2020

jensmeindertsma commented Jul 3, 2020

jensmeindertsma commented Jul 3, 2020

kettanaito commented Jul 6, 2020

jensmeindertsma commented Jul 6, 2020

kettanaito commented Jul 7, 2020

cloud-walker commented Jul 7, 2020

kettanaito commented Jul 8, 2020

kettanaito commented Jul 9, 2020

cloud-walker commented Jul 9, 2020

kettanaito commented Jul 9, 2020

cloud-walker commented Jul 9, 2020

jensmeindertsma commented Jul 10, 2020

cloud-walker commented Jul 10, 2020

kettanaito commented Jul 10, 2020

cloud-walker commented Jul 10, 2020

kettanaito commented Jul 12, 2020

jensmeindertsma commented Jul 12, 2020

cloud-walker commented Jul 12, 2020

kettanaito commented Jul 15, 2020 • edited Loading

jensmeindertsma commented Jul 15, 2020

kettanaito commented Jul 15, 2020

cloud-walker commented Jul 15, 2020

kettanaito commented Jul 21, 2020

kettanaito commented Jul 22, 2020 • edited Loading

kettanaito left a comment

Choose a reason for hiding this comment

jensmeindertsma commented Jul 22, 2020

jensmeindertsma commented Jul 1, 2020 •

edited by kettanaito

Loading

jensmeindertsma commented Jul 1, 2020 •

edited

Loading

jensmeindertsma Jul 2, 2020 •

edited

Loading

kettanaito commented Jul 15, 2020 •

edited

Loading

kettanaito commented Jul 22, 2020 •

edited

Loading