valendres / playwright-msw

A Mock Service Worker API for Playwright
MIT License
162 stars 22 forks source link

Support for onUnhandledRequest #52

Open jmbeach opened 1 year ago

jmbeach commented 1 year ago

I'd like to use the msw configuration option onUnhandledRequest: 'error'. However, this is an option that gets passed to the listen() method and I don't know if this is supported by playwright-msw. Any advice?

brandon-pereira commented 1 year ago

I'd also love to see this feature!

valendres commented 1 year ago

Hey @jmbeach and @brandon-pereira, sorry for the delay. At this stage this configuration is unsupported by playwright-msw.

The reason for this is that playwright-msw only attempts to intercept API calls that are explicitly defined within your handlers, e.g. if you have a MSW handler for /users/:id, it will only try to handle that request. Any request that does not match the paths in one of your handlers will not be intercepted by Playwright at all. The way playwright-msw achieves this is by iterating over each handler and calling Playwright's page.route for each unique path. e.g. rest.get("/users/:id") would translate to page.route("/users/*").

Another way of putting this is that unlike the official msw, playwright-msw does not attempt to intercept all API calls. Therefore it is not possible to know when one has not been handled. In theory, this is achievable by calling page.route("**/*"), but there are some side effects of doing this:

  1. For every API call made, the library would have to call either route.fulfill, route.continue or route.abort. Each of these operations results on a log entry. Unlike cypress, there is no way to omit playwright operations from the logs. The sheer amount of log entries can make debugging tests quite difficult.
  2. Static resources such as javascript bundle chunks, CSS files and images are also network requests and would get intercepted by the page.route("**/*") call. If there wasn't a MSW handler defined for an image or some other static resource, it would result in an error.

Interestingly, prior to version 2.0, the library did use **/* to match all requests, but it was changed to be more targeted in an effort to reduce noise in the playwright reports. It's possible that this method could be conditionally re-introduced such that is only used if onUnhandledRequest is specified as a configuration option. This would mean that this feature can be supported, but at the cost of increased log spam when enabled.

I have two questions:

  1. Would you accept increased noise if you could handle unhandled requests?
  2. What is the reason for wanting this feature? if I can understand this, I might be able to think of another solution that does not have this tradeoff
jmbeach commented 1 year ago

It sounds like this would be pretty involved and I only passively wanted it. I didn't get how playwright-msw worked, but I figured it just made calls to the official msw, in which case, you'd just expose the configuration property that allows users to do this.

I wouldn't want to add it if it could potentially give other users of the library a worse experience (with more noise).

To answer your question though, I turn the option on so that I don't forget to mock any requests. When I use playwright, it's typically to do more of an integration test instead of an end-to-end test so I'm not wanting to use real api calls.

brandon-pereira commented 1 year ago

For me it's very similar to @jmbeach, we are running these as integration tests in our CI server. Sometimes, some of the requests appear to not be mocked (haven't debugged why, probably query params or headers mismatch). Currently, these misses result in going out to our real server, and the response of that server will have different data than the mocked response. This then causes the integration test to fail because it's different result than we're expecting.

The ideal scenario for me would be that these tests fail, so that we can fix the mock to correctly resolve (or flag that a new api is not mocked, as an example) rather than failing due to the DOM output not being as expected.

I don't think this is absolutely necessary either, but some ability to potentially have a "debug" mode where its very verbose and allows us to debug this would be easier, currently, there is no way (that I know of) to properly show which responses were cache hit vs cache miss other than looking at the network planel in the playwright windows dev tools. Please correct me if i'm wrong!

By the way, I really appreciate the thorough answer and explanation, thank you! 🙏

valendres commented 1 year ago

Thanks for taking the time to explain the importance of this feature @jmbeach and @brandon-pereira. Having requests hit a real server and causing tests to be flaky is definitely undesirable. In my opinion, while there is some effort required to implement this feature, it is worthwhile to mitigate this issue. As such, I've started working on this feature already. I'll have more to share on this front in the near future :)

there is no way (that I know of) to properly show which responses were cache hit vs cache miss other than looking at the network planel in the playwright windows dev tools.

That's correct. There's no way to do this right now.

I'm keen to get some input though, if one were to pass in onUnhandledRequest: 'error'., what would you expect to happen:

  1. An error is thrown and the test immediately fails, or...
  2. A message is logged to console and test proceeds as normal. Whether the tests passes is up to the assertions.

I'm currently leaning towards #2 as this is how I believe the official MSW behaves. If we want the test to fail, we can pass a callback to onUnhandledRequest and throw an error. Thoughts?

brandon-pereira commented 1 year ago

I'm currently leaning towards https://github.com/valendres/playwright-msw/pull/2 as this is how I believe the official MSW behaves. If we want the test to fail, we can pass a callback to onUnhandledRequest and throw an error. Thoughts?

I would agree with this! onUnhandledRequest: 'error' would result in some logs in the console, whereas onUnhandledRequest: (req) => throw new Error(`${req.url}` not mocked`) would result in the test failing. Ideally the req returned in the callback is the same request from the msw implementation.

Please let me know if I can help out with implementing this feature :)

bel0v commented 1 year ago

I second @jmbeach and @brandon-pereira's case for mocking all XHR requests for functional (not e2e) tests — the main feature request here being able to tell which requests we forgot to mock. Perhaps this could even be a sort of special report or check or log level 🤔

effectivetom commented 1 year ago

I'd also love to see this implemented to aid debugging, and would be happy to help out.

nathanhannig commented 2 months ago

I would like to see this as well, to avoid external URL requests that were forgotten to be mocked, I would like to mock all requests so that we are not including network requests in our tests to make them quicker