mswjs / msw

Industry standard API mocking for JavaScript.
https://mswjs.io
MIT License
16.04k stars 525 forks source link

msw in testcafe says its started, but responds with 404 #259

Closed benmonro closed 4 years ago

benmonro commented 4 years ago

Describe the bug

I am attempted to use msw in conjunction with testcafe (the best e2e testing tool for node, yeah even better than cypress, shamless plug).

testcafe has good docs about running service workers & https features in tests which I am following.

When I run the test, I can see it says [MSW] Mocking enabled but then none of the fetches are working.

image

Environment

Please also provide your browser version. chrome 83

To Reproduce

Steps to reproduce the behavior:

  1. Clone https://github.com/benmonro/msw-server/tree/feature/testcafe (note repro for this is on a branch called feature/testcafe)
  2. (cd app && yarn)
  3. yarn
  4. yarn testcafe
  5. testcafe will launch a browser, Once the browser loads you can launch chrome dev tools and see the 404 as shown in the above screenshot.

If you'd like to see this working in the actual app:

  1. (cd app && yarn dev)
  2. you can then add a todo or click a todo to remove one.

that all goes through msw and works fine. it only seems to be problematic over testcafe's proxy server.

Expected behavior

expecting fetch calls to be mocked.

Screenshots

If applicable, add screenshots to help explain your problem. (see above)

kettanaito commented 4 years ago

Hey, @benmonro! Thanks for reporting this, and for the detailed reproduction scenario with the repo 🎉 I'll try to look into it as soon as I can and let you know what I find. Meanwhile, if you discover something, do not hesitate to update this thread.

kettanaito commented 4 years ago

I've debugged the reproduction repository, posting some of the insights below. I'll sort my findings by the probability to cause the issue.

Service Worker script is inactive

To verify: add any console.log statement within the body of the "fetch" event in the mockServiceWorker.js script. That event should trigger on any request on the page, considering the page's scope is controlled by the worker. The console is not called, thus, the "fetch" event is not called either.

Why?

Screen Shot 2020-07-10 at 10 55 46

Although the Service Worker registers successfully, I cannot predict how Hammerhead's changes may affect its lifecycle.

By default the worker registers at / (root) scope. In your case it's http://localhost:1337/. Since the worker has access to all the routes relative to the registered scope, it should cover the URL that TestCafe creates during the test. However, there may be an issue with that, as http://localhost:1337/yghhCXKav/http://localhost:3000/ is a confusing URL.

I'm not familiar with TestCafe/Hammerhead, but I'd strongly recommend you to disable the editing of mockServiceWorker.js file by Hammerhead, and see how it behaves.

Relative routes are invalid

Once your testing framework spawns a URL like http://localhost:1337/yghhCXKav/http://localhost:3000/ your request handler's URL become relative to the / (root), meaning:

rest.get('/todos')
// http://localhost:1337/todos

However, the actual request is performed relative to the entire URL: http://localhost:1337/yghhCXKav/http://localhost:3000/todos

One of the ways I've tried to overcome this is by adding the * (asterisks) before the leading slash of the route:

rest.get('*/todos')

This reads as "any URL ending with /todos". Nevertheless, this doesn't get the request URL matched, and I think the previous point about an inactive Service Worker is the reason. Once you can get around that, you'd most likely want to adjust request handlers' URLs as well, in one way or another.

Overall, I find such URL structure during tests utterly confusing. There may be much more implications than Service Worker not registering, if your apps suddenly renders in such nested URL. I'd consider looking into TestCafe and any ways to make sure your app's URL structure resembles its actual production structure as close as possible.

msw/node is required in index.ts

I strongly recommend to remove the import of msw/node in src/index.ts. That file is used by browser.ts to get the list of endpoints in a browser's runtime. However, as it imports msw/node, it includes Node-specific modules into browser's runtime (doesn't matter if you use those modules or not).

You should split your browser/node setups of MSW into separate files and require them only in the proper environment. Here's a good example of that.

Local untrusted SSL

Service Workers are demanding when it comes to SSL. Make sure that your local certificate is trusted by your browser/system. According to spec, Service Worker won't register on an untrusted host. In your case it does register, but I'd still recommend to either ensure the SSL is always trusted, or use http:// for testing.


At the moment I cannot provide more details, as I lack the experience with TestCafe. I hope the insights above are enough to get you started in the right direction. Let me know in this thread what you find out. Thanks.

kettanaito commented 4 years ago

Hey, @benmonro. Please, how is it going with this issue? Have you been successful in debugging this further?

benmonro commented 4 years ago

We have been trying not nothing yet? One thing that I tried but haven't had any luck with is foreign fetch.

benmonro commented 4 years ago

@kettanaito we made some progress today by disabling a few things in testcafe-hammerhead (which we would have to make configurable before opening a PR against hammerhead). Perhaps you might be able to shed some light. a different set of repro steps though.

in a separate directory, clone this fork of my branch of testcafe-hammerhead https://github.com/benmonro/testcafe-hammerhead/tree/feature/msw-support

in that dir do yarn then yarn gulp http-playground then back in the directory where you cloned in the original repro steps: cd app && yarn dev

in the browser go to http://localhost:1400

this will run testcafe's proxy w/out having to go thru testcafe and makes it a lot easier to debug stuff.

from here type http://localhost:3000 and click the button

from here we have disabled some things that testcafe-hammerhead does to inject stuff into the service workers etc and in fact we can see it hit the MSW listener, but it now does not seem to match the /todos route, even tho it sees the resolver for it, maybe i'm missing something?

benmonro commented 4 years ago

@kettanaito good news! Just today I finally got testcafe and MSW playing nicey-nice. image (the 2 little todos in there come from MSW)

however, in order to do this I did have to use your suggestion of prefixing all of the routes w/ */

I noticed that when msw goes to match it's prefixing the origin of the page to do the match. was curious if you would be open to allowing a config option for a proxyUrl or something like that so we don't have to do the */ trick. thoughts?

kettanaito commented 4 years ago

Hey, I'm glad to see it running!

The usage of asterisk proves that the issue is with the proxy URL that Testcafe prepends. While asterisk would work, it's generally not recommended, as it literally matches <ANYTHING>/todos, which may lead to unexpected matches.

I recommend to look into the Testcafe API and grab its prepended URL during the test runtime. Then use that URL when resolving a request handler's route. Here's an example:

const IS_TEST = process.env.NODE_ENV === 'test'

function withUrl(url) {
  const prefix = IS_TEST ? testCafeApi.getUrl() : ''
  return `${prefix}${url}`
}

rest.get(withUrl('/todo'), (req, res, ctx) => res())
benmonro commented 4 years ago

@kettanaito I was curious what you think about changing this line:

https://github.com/mswjs/msw/blob/master/src/mockServiceWorker.js#L173

to be

          return resolve(getOriginalResponse())

making that change has been the only way to get testcafe to work. thoughts?

kettanaito commented 4 years ago

I don't think that's a good idea. INTERNAL_ERROR represents any exception within a request handler. MSW gracefully handles those exceptions as 500 response. That is the same behavior if an exception happens on the server, which MSW partially represents. Retrieving the original response in case of internal error is less obvious and overall seems off.

Changing that line to resolve with the original response is effectively a hack that forces MSW to respond with an actual response when it shouldn't due to an unhandled exception.

If it falls through the INTERNAL_ERROR case, you should solve the exception, not the response type from the library. Since the issue of Testcafe usage is the custom proxy that prepends a URL, the solution lies in either removing that proxy, or configuring your request handlers to treat that prepended URL as expected.

kettanaito commented 4 years ago

I believe once you either configure Testcafe to lift the URL override, or account for the different location hostname in your request handlers there will be no issue.

benmonro commented 4 years ago

@kettanaito the thing that's not clear to me is that I'm not defining that route. i only have a handful of graphql endpoints. seems to me the action should be MOCK_NOT_FOUND, rather than NETWORK_ERROR, no?

kettanaito commented 4 years ago

Please, what route are you speaking about?

Sorry, I fail to remember if there was a NETWORK_ERROR produced during this issue. The only way MSW dispatches that event internally is if you call res.networkError:

https://github.com/mswjs/msw/blob/4b2f057ee63f8b3a7afcca511d709f5691665756/src/utils/handleRequestWith.ts#L116-L126

Bear in mind NetworkError is a custom internal error instance and not the general in-browser network error.

I believe the network error you are referring to is a browser error stating that the requested resource hasn't been found. This is the correct behavior, because:

  1. You request /abc relatively to the hostname, which in case of Testcafe gets overridden by its proxy.
  2. MSW attempts to lookup /abc, but cannot, because such route doesn't exist.
  3. MSW dispatches MOCK_NOT_FOUND internal event, signaling the worker that there is no mock for the current request.
  4. Worker performs that request as-is.

You can also debug this better in 0.20.0 with onUnhandledRequest option for setupWorker.

worker.start({ onUnhandledRequest: 'warn' })

You should see that http://localhost:XXXX/_hasSum/http://localhost:3000/request/path is reported as a request without corresponding request handler.

benmonro commented 4 years ago

It's /messaging. I will try to make it a more reliable reproduction

benmonro commented 4 years ago

here's the specific error I'm seeing (and seeing a lot). Working on getting you a repro so we can find a better solution. image

benmonro commented 4 years ago

what's strange is I 100% for sure did not define any endpoints to handle that URL (/messaging), so I don't understand why it's not going through MOCK_NOT_FOUND

benmonro commented 4 years ago

@kettanaito ok i have a reproducible example for you and actually, i think this may be a bug in msw actually on this line:

https://github.com/mswjs/msw/blob/master/src/graphql.ts#L134

it's assuming that req.body is a parsed json object, but it's not, so req.body.query is not present.

to reproduce, follow steps 1-4 in the original post here.

however, once the browser loads, open the devtools window and enable pause on exceptions in the sources tab and reload the page.

Note that the original issue w/ the proxy URL is now handled, thanks to a solution given to me by the testcafe team. you can find that in testcafe/registerServiceWorker.js, which testcafe injects into the page during testing to ensure that mockServiceWorker.js does not get that funky altered URL, but instead is regitered at the root. You'll know it's working because you can see todos listed in the webpage. the problem now is that MSW is throwing an exception when it tries to parse this request that testcafe needs to send back to it's proxy server. shall we re-open this issue? or create a new one?

kettanaito commented 4 years ago

@benmonro thank you on those details and reproduction steps! Could you please open a new issue and include “Must provide source” error message in its title?

benmonro commented 4 years ago

@kettanaito I have a fix, but I'm not sure where to put a test to capture the fix, would you mind pointing me in the right place? I just have to add a null check for req.body.query and it seems to fix it.