Skip to content
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

Support for onUnhandledRequest #52

Open
jmbeach opened this issue Jan 3, 2023 · 10 comments
Open

Support for onUnhandledRequest #52

jmbeach opened this issue Jan 3, 2023 · 10 comments

Comments

@jmbeach
Copy link

jmbeach commented Jan 3, 2023

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
Copy link

I'd also love to see this feature!

@valendres
Copy link
Owner

valendres commented Feb 4, 2023

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
Copy link
Author

jmbeach commented Feb 5, 2023

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
Copy link

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
Copy link
Owner

valendres commented Feb 6, 2023

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
Copy link

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?

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
Copy link

bel0v commented Feb 28, 2023

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
Copy link

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

@nathanhannig
Copy link

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

@LeonGeldsch
Copy link

For anyone stumbling across this, I solved this by adding a catch all handler like mentioned earlier in this thread, but instead of catching all requests it only catches request that are sent to the api:

const catchAllHandler = http.all("yourapidomain.com/api/*", ({ request }) => {
    console.warn(`Warning: ${request.url} endpoint using ${request.method} method is not mocked.`);

    return Response.error(); // This will return a network error
});

adjust the api base url to fit your sites structure

This should show a warning in your playwright console for any unhandled request to your api.
Returning the error should make most tests time out but of course there can still be tests that send network requests to the api but don't rely on them actually succeeding to pass.

This way you can catch unhandled requests when running the tests manually and paying attention to the console, but to catch them all automatically it would be nice to see this package updated.

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

No branches or pull requests

7 participants