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

root url parsing needs to be more robust/lenient #1061

Open
Overdrivr opened this issue Feb 7, 2018 · 9 comments · May be fixed by #1692
Open

root url parsing needs to be more robust/lenient #1061

Overdrivr opened this issue Feb 7, 2018 · 9 comments · May be fixed by #1692

Comments

@Overdrivr
Copy link

The root URL parsing could benefit from improvements for the following cases:

'localhost' -> null//null:443localhost/foo

var scope = nock('localhost').get('/foo').reply(200);
console.log('active mocks: %j', scope.activeMocks());
// active mocks: ["GET null//null:443localhost/foo"]

'localhost:port' -> error

var scope = nock('localhost:1234').get('/foo').reply(200);
console.log('active mocks: %j', scope.activeMocks());
// Fails with
TypeError: Cannot read property 'replace' of null
    at new Scope (C:\Users\remib\Gitlab\cronobo-microservices\node_modules\nock\lib\scope.js:53:48)
    at startScope (C:\Users\remib\Gitlab\cronobo-microservices\node_modules\nock\lib\scope.js:26:10)

'localhost:port/url' -> localhost://1234:443/api/foo

var scope = nock('localhost:1234/api').get('/foo').reply(200);
// active mocks: ["GET localhost://1234:443/api/foo"]

'foo.com/api' -> null//null:443foo.com/api/foo

var scope = nock('foo.com/api').get('/foo').reply(200);
console.log('active mocks: %j', scope.activeMocks());
// active mocks: ["GET null//null:443foo.com/api/foo"]

I would expect either an error thrown if the root URL does not match exactly an expected pattern OR that it could parse correctly all those patterns that are valid root urls (well, valid is not maybe the more appropriate word here. But they can be performed successfully using request n co).

Since nock is primarily used for testing purposes, being lenient about it should be the best option IMO.

Also, the last case is pretty bad because since the root URL is not interpreted correctly, it is not intercepted at all. Which means a test environment could actually call the API you're trying to nock.
@gr2m
Copy link
Member

gr2m commented Feb 7, 2018

Yes I agree, happy to accept pull requests with tests to improve

Overdrivr added a commit to Overdrivr/nock that referenced this issue Feb 16, 2018
@Overdrivr
Implement failing tests of nock#1061
f1f05e2
@Overdrivr Overdrivr mentioned this issue Feb 16, 2018
Closed
@Overdrivr
Copy link
Author

I have implemented some failing tests to cover the problem. I am not sure I have formalized them in the way you wanted, as they purely reflect those specific cases + a few other ones.

Tests might be better written by actually testing if nock is really intercepting requests to those problematic root URLs. So consider this to be a WIP, I will get back at it later on.

If you already have any sort of feedback, it'll be really appreciated.

gr2m pushed a commit to Overdrivr/nock that referenced this issue Feb 23, 2018
@Overdrivr @gr2m
Implement failing tests of nock#1061
3fb240f
@stale
Copy link

stale bot commented Sep 14, 2018

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.

@stale stale bot added the stale label Sep 14, 2018
@Overdrivr
Copy link
Author

Will get back to it in a couple of weeks

@stale stale bot removed the stale label Sep 14, 2018
@stale
Copy link

stale bot commented Dec 13, 2018

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.

@stale stale bot added the stale label Dec 13, 2018
@gr2m
Copy link
Member

gr2m commented Dec 13, 2018

@Overdrivr we do a remote hackathon this Friday, wanna join? #1269

@stale stale bot removed the stale label Dec 13, 2018
@stale
Copy link

stale bot commented Mar 13, 2019

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.

@stale stale bot added the stale label Mar 13, 2019
@stale stale bot closed this as completed Mar 20, 2019
@paulmelnikow
Copy link
Member

This issue is still valid and has some tests in progress: #1065. I'm going to pin it for now.

@paulmelnikow paulmelnikow reopened this Aug 22, 2019
@stale stale bot removed the stale label Aug 22, 2019
@paulmelnikow
Copy link
Member

I'm not necessarily convinced we should make this more lenient, though we should definitely throw an error instead of no-opping when the input is invalid.

paulmelnikow added a commit that referenced this issue Aug 22, 2019
@paulmelnikow
feat: Throw errors when base URLs are specified incorrectly
ea75bca 
- Switch to the new URL API which throws errors for most invalid inputs
- Throw errors for URLs with unrecognized schemes
- Add tests for cases that previously had been handled incorrectly

Closes #1061
Supersedes #1065
@paulmelnikow paulmelnikow linked a pull request Aug 22, 2019 that will close this issue
Open
paulmelnikow added a commit that referenced this issue Sep 6, 2019
@paulmelnikow
feat: Throw errors when base URLs are specified incorrectly
2c8cb5f 
- Switch to the new URL API which throws errors for most invalid inputs
- Throw errors for URLs with unrecognized schemes
- Add tests for cases that previously had been handled incorrectly

Closes #1061
Supersedes #1065
@gr2m gr2m removed the pinned label Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Throw errors when base URLs are specified incorrectly [WIP] nock/nock
3 participants
@gr2m @Overdrivr @paulmelnikow