Failing tests for root URL parsing #1065
Conversation
| "root": "foo.com/api", | ||
| "basePath": "http://foo.com:80/api" | ||
| } | ||
| ] |
There was a problem hiding this comment.
maybe add two https testcase, one with port, one without? Otherwise looks good so far 👍
There was a problem hiding this comment.
I have added a few tests with protocols like you requested.
Also added a test to check that if a root url has a trailing backslash (like foo.com/) nocking will still work.
And finally I have added actual checks that node can intercept a request on those root urls. Currently it cannot.
Once those tests pass, I have the feeling nock will be much more enjoyable and easy to use.
gr2m
force-pushed
the
i1061-improve-root-url-parsing
branch
from
5218683
to
5900703
Compare
|
fyi I've rebased your branch on latest master as I got the tests fixed there. Do you want to take a stab and getting them green? |
|
I would but I honestly don't have the bandwidth right now for fixing this, especially given that I am not familiar with node-nock source code |
|
neither am I, I figure it out as I go :) no rush we can leave this open |
…/nock into i1061-improve-root-url-parsing
|
I fixed the parsing tests, I ended up removing the call to However it seems that it is not sufficient for nock to correctly intercept requests. So I have 14 passing over 27 tests. I am not sure why nock does not intercept requests correctly. Could you have a quick look at the code and let me know if you have an idea why ? BTW you should avoid rebasing contributors branches for ongoing tickets, because it makes local branches divert from remote, which then forces us to make a local merge if we forget to pull first ;) |
Yes, I’m very sorry for that, I mixed up PRs, I didn’t meant to rebase yours :(
Can’t promise when, but I'll try. It will take me some time, too. I’m not so familiar with the code base either I’m afraid, I took on maintainership recently. |
| this.urlPart = {}; | ||
|
|
||
| // Extract protocol | ||
| var protocolDelim = new RegExp(/\/{2}/); |
There was a problem hiding this comment.
Is there an advantage with using the RegExp constructor over just var protocolDelim = /\/{2}/?
Also what if someone does sth like nock('http://example.com//foo')? Maybe it would be better to split with /:\/{2}/?
Could you add a comment explaining why we don’t use url.parse() here, maybe reference the pull request?
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions. |
|
Sorry about the inactivity. I will have a couple of busy weeks and I'll get a go at it after then |
|
Thanks, @Overdrivr. No worries. :) |
|
@Overdrivr Any chance you've had some time for this? :) |
|
Sadly not :( |
|
I'll pin it. If you have time, it'd be great to see this merged in. :) |
|
Closing in favor of #1692. |
This is for ticket #1061