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

add matcher option to cy.route - addresses #387 #3984

Closed
wants to merge 8 commits into from

Conversation

godspeedelbow
Copy link

Addresses #387

Allows you to write routes like this:

cy.route({
  matcher: (request) => !! request.body.match(//)
})

Notes:

  • matcher is an option that can be passed when setting up a route using the cy.route(options) api.
  • matcher should be a function (if it's not a function, it's ignored) and it is passed two arguments: xhr and the route options. The latter so that you don't have to rely on closure to validate your route, e.g. you could do something like this matcher: (xhr, route) => xhr.method === route.method
  • matcher should return true if the route matches the xhr request, and false if it doesn't. "truthy" (e.g. "it's a match!") is intepreted as true, "falsy" (e.g. undefined) is interpreted as false.
  • matcher takes precedence over normal route matching. If a matcher function is passed, it will be evaluated and the normal matching is ignored. There is no fallback to the original route matching.

To do:

  • There are no tests because I couldn't find me way through the zoo of tests. Also, docs are not up date or things don't work as they should (ie. I cannot run a single test, tests fail to run entirely ReferenceError: cy is not defined, etc). Maybe somebody can tell me where to put the test and where the instructions are for running that test?

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2019

CLA assistant check
All committers have signed the CLA.

@godspeedelbow godspeedelbow changed the title add matcher option to cy.route add matcher option to cy.route - addresses #387 Apr 17, 2019
@godspeedelbow
Copy link
Author

@jennifer-shehane where would I add a test for this?

@godspeedelbow
Copy link
Author

What should I do to elicit a response on this PR?

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need a PR in cypress-documentation to document the new option before this can be merged.

@@ -272,6 +272,7 @@ create = (options = {}) ->
testStr(fullyQualifiedUrl, options.stripOrigin(fullyQualifiedUrl))

xhrMatchesRoute: (xhr, route) ->
return route.matcher(xhr, route) if _.isFunction(route.matcher)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests for the desired behavior?

The specs for cy.server, cy.route, etc. are all here: https://github.com/cypress-io/cypress/blob/develop/packages/driver/test/cypress/integration/commands/xhr_spec.coffee

@flotwig
Copy link
Contributor

flotwig commented Apr 30, 2019

The Cypress project for the driver is in packages/driver/test. To run the driver tests, you can use npm run cypress:open in the packages/driver directory.

You'll also need to set it up to compile your changes, instructions for building the driver are here: https://github.com/cypress-io/cypress/blob/develop/packages/driver/README.md#building

Let me know if you have difficulty with this, it should just work.

@godspeedelbow
Copy link
Author

Hey @flotwig, thanks for getting back to me!

I added 4 tests. I also noticed that those tests failed because options passed to cy.route no longer contained an url key-value when using the matcher option. I fixed xhr.coffee to handle that case. I'll look at the docs now.

Please let me know if you need anything else on this PR.

@godspeedelbow
Copy link
Author

@flotwig I added the tests for xhr but I realised that I hadn't actually checked on the request body value. When trying to add these, I realised I actually don't have access to the request body. I think I am too unfamiliar with some of the Cypress concepts to know how to get access to requestBody in userland.

What I found out, is that the xhr object that gets passed to the matcher function doesn't contain the request body. I've seen that there's a requestBody in the xml_http_request.coffee proxy which gets set in the XHR.prototype.send override but the "proxied" XHR doesn't make it into xhrMatchesRoute.

Could you maybe give me some pointers to how I get access to the request body inside matcher?

@jennifer-shehane jennifer-shehane requested a review from flotwig May 7, 2019 05:16
@flotwig
Copy link
Contributor

flotwig commented May 7, 2019

@godspeedelbow I took a look at your branch to see how the request body could be passed in, but I actually couldn't figure it out either. I added a (failing) test that ensures that matcher receives the request body.

XHR.prototype.send should be attaching the request body to the XHR as you've found, but actually, the breakpoint I set in XHR.prototype.send was never hit at all in the test I created. I also tried setting a breakpoint in jQuery's xhr.js, where it calls XHR.send, but it was never reached 🤔 So I'm not sure what's going on with this.

@godspeedelbow
Copy link
Author

I think this happens because xhr.open(method, url, async, user, password) is called first and then xhr.send(body).

However, in Cypress one complication is that in xhr.prototype.open, the request body is not yet known but we do need to determine the matching route to create the stub the url if needed.

However, in xhr.prototype.send we do know the request body and we pass that so that it'll end up in the matcher function eventually.

One option is: we don't pass the request body in XHR.prototype.open: route = server.getRouteForXhr(@) but we do pass it in XHR.prototype.send: route = server.getRouteForXhr(@, requestBody). However, this leads to a flaky API wrt the matcher function: it gets first called without and then later again with the request body. But this is so undesirable that I think this proves that the matcher options is DOA. We shouldn't go down this path.

So, as an alternative, what I started thinking instead is maybe we should pass requestBody to the response function. Arguably, this doesn't allow you to match the url on requestBody but it does allow you to tune the response based on the requestBody. I think this serves equally well the use case.

In investigating this I ran into another wall and that is that the options.response function gets evaluated before the route is matched, in cy.route. we would need to rewire this so that the response gets evaluated only the XHR comes in, not before.

@flotwig Do you have any idea how we could do that?

@flotwig
Copy link
Contributor

flotwig commented May 10, 2019

@godspeedelbow Sadly, I'm not too familiar with this part of the code, but maybe someone else will chime in. @cypress-io/test-runner, any thoughts?

@godspeedelbow
Copy link
Author

Closing this. I don't see how to this with current architecture.

@sunnywx
Copy link

sunnywx commented Jul 17, 2019

Closing this. I don't see how to this with current architecture.

@godspeedelbow Hi, i think your solution is useful, why not reopen this PR? When using cypress to test an app with single api endpoint (i.e graphql), cy.route couldn't route to different stubbed response. I checked the source code, when debugged in devtool, found the following snippet in runner/cypress_runner.js:

 xhrMatchesRoute: function(xhr, route) {
    return server.methodsMatch(route.method, xhr.method) && server.urlsMatch(route.url, xhr.url);
}

If we can hook into this method, then we can use cy.route to handle different stubbed data with same api endpoint.

Waiting for your feedback :)

@godspeedelbow
Copy link
Author

Hi Xi! I am not entirely sure how the code works anymore. "It's complicated", is all I remember. I decided I am just going to wait for Cypress 2 which should land with better support for stubbing.

Feel free to continue on this however!

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

Successfully merging this pull request may close these issues.

5 participants