Add an option to warn/error on unmocked requests

[JamesZoft]

Is your feature request related to a problem? Please describe. In my experience, when testing components, I've always mocked API responses. If I've forgotten to mock a response, it'll go something like: run test, assertion fails, wonder why, look at the html, realise I forgot to mock, add a mock. If MSW output a warning/error when a network request was made to an unmocked endpoint, it would be immediately obvious what's wrong.

Describe the solution you'd like There should be an option added to MSW to globally (and perhaps per-test) configure an option to warn/error on unmocked requests. Probably something that would go in an mswSetup.js file that goes in the jest config setup? I personally think this should be on by default, but I can imagine it possibly being a barrier to usage if it's on by default. If it isn't, it should be prominently displayed in the README and promoted, assuming you'd agree.

Describe alternatives you've considered n/a

Additional context Add any other context or screenshots about the feature request here.

[kettanaito]

Hey, @JamesZoft! Thanks for raising this topic.

I think this definitely has a practical application, so I'm willing to add such functionality to the library. I do not, however, think that it should be the default behavior, as MSW is positioned as a tool for both testing, development, and debugging in equal measure. I think this behavior should be opt-in via the library's API.

API

The name of the property that represents this behavior is to be discussed. Let's assume it's warnOnUncaptured for now.

Global setting

worker.start({
  warnOnUncaptured: true
})

server.listen({
  warnOnUncaptured: true
})

Per-test setting

There is no concept of runtime settings at the moment. The settings you provide affect the worker/server instance upon start. I'm not sure if this new behavior is a sufficient justification to add one. Anyway, it's a much broader topic and we can definitely raise it separately.

Questions

1. Should it be a warning on an error when MSW encounters a request that doesn't have a corresponding request handler?

[JamesZoft]

I’m fine with it being opt in as long as it’s visible as an option.

Perhaps we could do onUncaptured: ‘warn’ | ‘error’ as the type which would allow flexibility and strictness for those who might want it to error?

How does msw interact with tests/suites? When I run ‘jest’ is the one service worker for msw active the entire time or does it get torn down per suite?

Sent from my iPhone

On 3 Jun 2020, at 10:39, Artem Zakharchenko notifications@github.com wrote:

 Hey, @JamesZoft! Thanks for raising this topic.

I think this definitely has a practical application, so I'm willing to add such functionality to the library. I do not, however, think that it should be the default behavior, as MSW is positioned as a tool for both testing, development, and debugging in equal measure. I think this behavior should be opt-in via the library's API.

API

The name of the property that represents this behavior is to be discussed. Let's assume it's warnOnUncaptured for now.

Global setting

worker.start({ warnOnUncaptured: true }) server.listen({ warnOnUncaptured: true }) Per-test setting

There is no concept of runtime settings at the moment. The settings you provide affect the worker/server instance upon start. I'm not sure if this new behavior is a sufficient justification to add one. Anyway, it's a much broader topic and we can definitely raise it separately.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[kettanaito]

@JamesZoft, it will definitely be a visible option. I like your suggestion to let the developer decide what strictness of a message such behavior should have. I will think on that.

When you run tests in Node (i.e. using Jest), there are no Service Workers at all (it's a browser-specific API). MSW uses a library called node-request-interceptor that intercepts requests issued in Node, and then matches them against your request handlers, identically to what is happening when you use MSW for the client. That's why there's an API dedicated for Node: setupServer.

Whether to have a single server instance, or do it on per-test basis is completely up to you. Just don't forget to clean up via server.close().

[JamesZoft]

Coolio, ok! I think I’ve given all the thoughts I’ve got on this for the moment :)

Sent from my iPhone

On 4 Jun 2020, at 13:00, Artem Zakharchenko notifications@github.com wrote:

 @JamesZoft, it will definitely be a visible option. I like your suggestion to let the developer decide what strictness of a message such behavior should have. I will think on that.

When you run tests in Node (i.e. using Jest), there are no Service Workers at all (it's a browser-specific API). MSW uses a library called node-request-interceptor that intercepts requests issued in Node, and then matches them against your request handlers, identically to what is happening when you use MSW for the client. That's why there's an API dedicated for Node: setupServer.

Whether to have a single server instance, or do it on per-test basis is completely up to you. Just don't forget to clean up via server.close().

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[KnisterPeter]

This would really an amazing feature. :+1:

[cloud-walker]

What about something like logUnhandledRequests, which:

• can be a boolean or a function;
• its false by default;
• you can pass your own function if you want to customize the logging output

About the logging output.. if the feature provides a default one, we want to discuss the design of it?

[kettanaito]

Regarding logUnhandledRequests, I have a few questions:

can be a boolean or a function;

How can we distinguish between warn and error expectations towards unhandled requests? It seems that a boolean flag will not suffice. We can reach out to writing a custom function, but I find enum working the best here.

you can pass your own function if you want to customize the logging output

Hm, I'm not sure we should combine this feature with the ability to customize the logging output (sounds a separate feature). Currently it's possible to customize the logging output of the setupWorker execution, but setupServer doesn't have any output at all. I'd raise this as a separate feature, if it makes sense to you?

I like your idea of supplying this option with a function value. I can easily imagine something like this being useful:

server.listen({
  onUnhandledRequests: 'error' // or "warn", or custom function
})

[cloud-walker]

I like your idea of supplying this option with a function value. I can easily imagine something like this being useful:

Yeah, I still think that one option is enough, like:

server.listen({
  onUnhandledRequests: 'none' // or "error", or "warn", or custom function
})

Where none is default (to avoid breaking changes, I'm not sure that should be the default, but its a separate topic I guess).

I'm just talking out loud.. I feel like one option is enough, avoiding as much as possible the configuration hell!

[kettanaito]

I think you were right in giving it false as the default value. 'none' enum reads weirdly, but that may be just my subjective feeling. I believe that the absence of a behavior must be denoted with a falsy value (null/undefined/false). We can choose between these:

• onUnhandledRequest: false
• onUnhandledRequest: null
• onUnhandledRequest: undefined. This one I find confusing, as it reads as "the behavior on unhandled request is not defined". But it is defined, by default unhandled requests are bypassed, and performed as-is.

We can also consider an enum for the "bypass" behavior:

• onUnhandledRequest: 'bypass' or - onUnhandledRequest: 'fall-through'

[cloud-walker]

Maybe an enum it's ok. We just need to choose the right words. Maybe bypass is good!

[kettanaito]

Nice! Let's try to put down a specification for this feature? I'll keep this comment up-to-date to reflect our discussion.

Unhandled requests

Unhandled request is a request that has no corresponding request handler. It's possible to configure MSW to handle such requests differently.

onUnhandledRequest

Both worker and server instances accept the onUnhanledRequest configuration option.

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

• Default: 'bypass'

Example of usage:

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

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

---

I think it's a good thing to forbid returning a mocked response from the onUnhandledRequest function. That's not a request handler, that's a function that may be called when no request handlers are present.

[cloud-walker]

Ok! What's the content of the default log?

We want to differentiate it for warn and error?

Error means throw?

[KnisterPeter]

Throw on error would be good.

That's the only option to prevent the request and stay valid I think.

[kettanaito]

What's the content of the default log?

If you mean the logging of requests into browser's console, then:

• bypass doesn't log anything, the request has been performed as-is, you can find it in your "Network" tab. Relevant request Promise status will be printed into your console (if any) by the request issuing library (i.e. fetch).
• warn produces a developer-friendly warning that this particular request wasn't handled by your request handlers.
• error throws an exception with a similar text to warning.
• custom function, doesn't do any implicit behavior (neither throw, nor log), delegates these decisions to you.

[cloud-walker]

Yeah I was wondering the specs of the actual content of the warn / error message.

bypass doesn't log anything, the request has been performed as-is, you can find it in your "Network" tab. Relevant request Promise status will be printed into your console (if any) by the request issuing library (i.e. fetch).

This is the reason why I'm thinking that bypass is not a great default for at least the node environment, where we don't have a "Network" tab and the experience is tests failing "randomly" for missing mocks. But yeah, maybe its another issue!

@kettanaito you think I can pick up the PR myself? Or you are already on it?

[kettanaito]

@cloud-walker, I'd be thankful if you could pick up the pull request! This feature is planned for the next release, so my participation and reviews will be a top priority.

Yes, I share your concerns for this default behavior in Node, but I think we should have the same default behavior between browser and Node. Overall, I believe we should follow the philosophy that mocking is non-intrusive by default. That implies that anything you don't handle gets performed as-is.

[cloud-walker]

Perfect! I will definitely need your support on this PR as its my first in the repository ❤️

[cloud-walker]

I've watched the code, and I currently think that our "logging" functionality should be set on following locations:

• https://github.com/mswjs/msw/blob/8b8b37c35a621a09e2af9deebd7aaab2c9d4b154/src/node/setupServer.ts#L62
• https://github.com/mswjs/msw/blob/8b8b37c35a621a09e2af9deebd7aaab2c9d4b154/src/utils/handleRequestWith.ts#L72

Let me know if I'm wrong! (I will be busy for awhile, I will try to open a draft PR this evening / tomorrow 🤔 )

[jensmeindertsma]

@cloud-walker I'd be willing to help you with this if you'd like?

[cloud-walker]

Sure!

[kettanaito]

@cloud-walker, those places are spot-on! In case of the setupWorker, it should still send the MOCK_NOT_FOUND event to the worker, but before that check the onUnhandledRequest config option and decide what to do 👍

I highly recommend to get a rough draft of how you imagine this feature to work, and issue a pull request. We can iterate on it together, making sure we are moving towards the right direction from the start.

[jensmeindertsma]

@cloud-walker @kettanaito I've got the basic outline that we can discuss and iterate over. I'm willing to open a pull request, if that's OK with you?

It currently changes nothing about the behavior, it just adds if statements to the correct places for all of the options. Also I added types.

None of the actual warning logic is in place yet, I want to give that opportunity to someone else if someone wants to do it.

[cloud-walker]

Let's do this!

[jensmeindertsma]

I've opened a draft here: https://github.com/mswjs/msw/pull/257

[marcosvega91]

If you need help you can also tag me :)