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.

Sign up for GitHub

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

Jump to bottom

Allow more customization of network mocking urls #53

Open
danwenzel opened this issue May 20, 2019 · 8 comments · May be fixed by #61
Open

Allow more customization of network mocking urls #53

danwenzel opened this issue May 20, 2019 · 8 comments · May be fixed by #61
Labels
enhancement

Comments

@danwenzel
Copy link
Contributor

nock has several customization options that would be nice to implement in ember-cli-fastboot-testing's network mocking. Just to mention a few:

  • Setting of the hostname - Currently, we are limited to using the same hostname as the test runner. Oftentimes we have a separate api host that we'd like to override.
  • Customizing the path - Currently, we can only use exact path matches - it would be great to be able to use RegEx or the includes option.
  • Customizing the query string - Similar to above, it would be really helpful to explicitly specify the query string, use regex, or use nock's .query(true) option to ignore the query string altogether. Otherwise, it's only a match if the query string is appended to the url with the exact full string.

We also have this issue: https://github.com/embermap/ember-cli-fastboot-testing/issues

It seems like we either need a way to use nock's syntax when creating these mocks, or expand the API to allow several different customization options. Maybe something like:

await mockServer.get('/api/posts/1', { <body>}, { <options> } )

(we'd have to move the status code into an option, and maybe deprecate the string as a 3rd param)

I'd be happy to work on a PR for this once we land on the approach.
@ryanto
Copy link
Member

ryanto commented May 20, 2019

Nice, totally agree!

There's some work being done in #51 that exposes a better API for post requests. We can do something similar for gets as well.

I thinking we should have MockGet (similar to MockPost from the above PR) that exposes these options via an API. We can pick and choose the options from nock we want to support, I think hostname and query(true) make sense.

Do you have an example of how you'd like to use RegExp w/ includes? Same for anything RegExp related in the query string. I'd love to see how that could be used in a test.

@danwenzel
Copy link
Contributor Author

Awesome that this is getting worked on, @ryanto!

hostname and query(true) are definitely the ones we'd use the most.

As far customizing the path, the best example I can think of is dynamic segments. So something like /users/:id/profile, etc.

And for the query string, maybe something where we're checking for a certain key, but don't care what the value is. Maybe: /rentals?token=...

Ideally, we'd have the ability to use the function syntax in the reply and access the original request for these dynamic scenarios. But that may be out of scope for this.

@CvX
Copy link
Collaborator

CvX commented Jun 3, 2019

I've finally fixed the fastboot tests in my app, and the hostname configuration is definitely a must. I ended up simply adding it as a first argument to all mocking methods, just to keep the ball rolling (master...CvX:hostname).

I could also use the "disabling real http requests" (with nock.disableNetConnect()), to make sure absolutely no actual requests are made behind my back 😉). And maybe nock.recorder.rec() to take a peek at the data returned by the backend.

This made me wonder if it wouldn't be better to expose the whole nock functionality, via comlink-like interface:

import { nock } from 'ember-cli-fastboot-testing/test-support';

test('…', async function() {
  await nock.disableNetConnect();
  await nock('https://api.example.com')
    .get('/posts)
    .reply(200, …);
});

This would allow to use every feature nock has to offer, without having to patch it in, one by one, to the ECFT. The downside is the tight coupling to nock, so e.g. if this addon was to replace the mocking library with some other package - that would be a breaking change for addon's users. I think that's a fair tradeoff though.

@danwenzel
Copy link
Contributor Author

Having direct access to the nock object would be awesome. @ryanto - thoughts? would that even be possible?

@ryanto
Copy link
Member

ryanto commented Jun 14, 2019

Hey! Sorry for the late reply.

One of the reasons I didn't give direct access to the nock object is because the tests are written in the browser, but nock runs in a node process. I was unsure of how to serialize all those object and function calls across the browser/server boundary. That's the short of why the mocking API is so limited.

@CvX it sounds like comlink will give us a nice API for crossing that boundary? If so, I'm all for this! Giving full access to nock seems like it will solve a whole lot of these recent issues, and I'm not so worried about the tight coupling at this point since I don't really see the need for us needing to support multiple node mocking libraries.

Thoughts?

@CvX
Copy link
Collaborator

CvX commented Jun 19, 2019

Alright, I initially thought I can get a proof-of-concept done in a single evening, but it took me a bit over two days. 😅

Comlink, in its current state, doesn't support the builder pattern (e.g. await thing().otherThing().somethingElse();). I'm going to submit a PR to comlink with my changes, after I clean up the code.

In the mean time, I have some other minor changes to ecft. I'll also start the PR with this new mocking interface to get things rolling.

@ryanto
Copy link
Member

ryanto commented Jun 19, 2019

Wow, awesome!

@CvX
Copy link
Collaborator

CvX commented Jun 20, 2019

Comlink PR is ready: GoogleChromeLabs/comlink#311

And here's a preview of ember-cli-fastboot-testing changes: master...CvX:nock-proxy

@CvX CvX linked a pull request Jun 21, 2019 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[WIP] Expose nock via comlink CvX/ember-cli-fastboot-testing
3 participants
@CvX @ryanto @danwenzel