-
-
Notifications
You must be signed in to change notification settings - Fork 733
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
feat: Support http.request signatures added in Node 10.9+ #1588
Changes from all commits
f598816
3c93c3d
e2e59e4
fd54a81
a03480b
9dac372
759783a
e8e90b5
fdd0688
17f203c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,6 @@ const common = require('./common') | |
const { inherits } = require('util') | ||
const Interceptor = require('./interceptor') | ||
const http = require('http') | ||
const { parse: urlParse } = require('url') | ||
const { URL } = require('url') | ||
const _ = require('lodash') | ||
const debug = require('debug')('nock.intercept') | ||
const globalEmitter = require('./global_emitter') | ||
|
@@ -147,7 +145,7 @@ function interceptorsFor(options) { | |
|
||
// First try to use filteringScope if any of the interceptors has it defined. | ||
let matchingInterceptor | ||
_.each(allInterceptors, function(interceptor, k) { | ||
_.each(allInterceptors, function(interceptor) { | ||
_.each(interceptor.scopes, function(scope) { | ||
const { filteringScope } = scope.__nock_scopeOptions | ||
|
||
|
@@ -238,7 +236,7 @@ function ErroringClientRequest(error) { | |
inherits(ErroringClientRequest, http.ClientRequest) | ||
|
||
function overrideClientRequest() { | ||
// Here's some background discussion about overridding ClientRequest: | ||
// Here's some background discussion about overriding ClientRequest: | ||
// - https://github.com/nodejitsu/mock-request/issues/4 | ||
// - https://github.com/nock/nock/issues/26 | ||
// It would be good to add a comment that explains this more clearly. | ||
|
@@ -247,8 +245,10 @@ function overrideClientRequest() { | |
// ----- Extending http.ClientRequest | ||
|
||
// Define the overriding client request that nock uses internally. | ||
function OverriddenClientRequest(options, cb) { | ||
if (!options) { | ||
function OverriddenClientRequest(...args) { | ||
const { options, callback } = common.normalizeClientRequestArgs(...args) | ||
|
||
if (Object.keys(options).length === 0) { | ||
// As weird as it is, it's possible to call `http.request` without | ||
// options, and it makes a request to localhost or somesuch. We should | ||
// support it too, for parity. However it doesn't work today, and fixing | ||
|
@@ -271,14 +271,12 @@ function overrideClientRequest() { | |
debug('using', interceptors.length, 'interceptors') | ||
|
||
// Use filtered interceptors to intercept requests. | ||
const overrider = RequestOverrider( | ||
this, | ||
options, | ||
interceptors, | ||
remove, | ||
cb | ||
) | ||
const overrider = RequestOverrider(this, options, interceptors, remove) | ||
Object.assign(this, overrider) | ||
|
||
if (callback) { | ||
this.once('response', callback) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
} | ||
} else { | ||
debug('falling back to original ClientRequest') | ||
|
||
|
@@ -373,20 +371,12 @@ function activate() { | |
|
||
// ----- Overriding http.request and https.request: | ||
|
||
common.overrideRequests(function( | ||
proto, | ||
overriddenRequest, | ||
options, | ||
callback | ||
) { | ||
common.overrideRequests(function(proto, overriddenRequest, args) { | ||
// NOTE: overriddenRequest is already bound to its module. | ||
let req, res | ||
|
||
if (typeof options === 'string') { | ||
options = urlParse(options) | ||
} else if (options instanceof URL) { | ||
options = urlParse(options.toString()) | ||
} else if (!options) { | ||
const { options, callback } = common.normalizeClientRequestArgs(...args) | ||
|
||
if (Object.keys(options).length === 0) { | ||
// As weird as it is, it's possible to call `http.request` without | ||
// options, and it makes a request to localhost or somesuch. We should | ||
// support it too, for parity. However it doesn't work today, and fixing | ||
|
@@ -399,23 +389,25 @@ function activate() { | |
'Making a request with empty `options` is not supported in Nock' | ||
) | ||
} | ||
|
||
// The option per the docs is `protocol`. Its unclear if this line is meant to override that and is misspelled or if | ||
// the intend is to explicitly keep track of which module was called using a separate name. | ||
// Either way, `proto` is used as the source of truth from here on out. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what we ought to do here but really appreciate this comment! |
||
options.proto = proto | ||
|
||
const interceptors = interceptorsFor(options) | ||
|
||
if (isOn() && interceptors) { | ||
let matches = false | ||
let allowUnmocked = false | ||
|
||
matches = !!_.find(interceptors, function(interceptor) { | ||
const matches = !!_.find(interceptors, function(interceptor) { | ||
return interceptor.matchAddress(options) | ||
}) | ||
|
||
allowUnmocked = !!_.find(interceptors, function(interceptor) { | ||
const allowUnmocked = !!_.find(interceptors, function(interceptor) { | ||
return interceptor.options.allowUnmocked | ||
}) | ||
|
||
if (!matches && allowUnmocked) { | ||
let req | ||
if (proto === 'https') { | ||
const { ClientRequest } = http | ||
http.ClientRequest = originalClientRequest | ||
|
@@ -430,13 +422,7 @@ function activate() { | |
|
||
// NOTE: Since we already overrode the http.ClientRequest we are in fact constructing | ||
// our own OverriddenClientRequest. | ||
req = new http.ClientRequest(options) | ||
|
||
res = RequestOverrider(req, options, interceptors, remove) | ||
paulmelnikow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (callback) { | ||
res.on('response', callback) | ||
} | ||
return req | ||
return new http.ClientRequest(options, callback) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
} else { | ||
globalEmitter.emit('no match', options) | ||
if (isOff() || isEnabledForNetConnect(options)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
'use strict' | ||
|
||
const http = require('http') | ||
const { test } = require('tap') | ||
const common = require('../lib/common') | ||
const matchBody = require('../lib/match_body') | ||
|
@@ -453,3 +454,32 @@ test('percentEncode encodes extra reserved characters', t => { | |
t.equal(common.percentEncode('foo+(*)!'), 'foo%2B%28%2A%29%21') | ||
t.done() | ||
}) | ||
|
||
test('normalizeClientRequestArgs throws for invalid URL', async t => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gr2m has expressed a preference for testing our helper functions through real use cases in the public API wherever possible, and it seems to pay off well. It also has the advantage of surfacing code that is no longer needed. Would you be up for expressing these tests in terms of calls to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes please 👍 |
||
// no schema | ||
t.throws(() => http.get('example.test'), { | ||
input: 'example.test', | ||
name: /TypeError/, | ||
}) | ||
}) | ||
|
||
test('normalizeClientRequestArgs can include auth info', async () => { | ||
const scope = nock('http://example.test') | ||
.get('/') | ||
.basicAuth({ user: 'user', pass: 'pw' }) | ||
.reply() | ||
|
||
http.get('http://user:pw@example.test') | ||
scope.isDone() | ||
}) | ||
|
||
test('normalizeClientRequestArgs with a single callback', async t => { | ||
// TODO: Only passing a callback isn't currently supported by Nock, | ||
// but should be in the future as Node allows it. | ||
const cb = () => {} | ||
|
||
const { options, callback } = common.normalizeClientRequestArgs(cb) | ||
|
||
t.deepEqual(options, {}) | ||
t.is(callback, cb) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not calling newRequest instead? This breaks the Node API as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done to mimic native Node behavior in case other libs are monkey-patching the function.
Can you provide more details on "This breaks the Node API as well"? Preferably in a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make a new issue. But writing the details here as well.
New Relic (https://github.com/newrelic/node-newrelic) is overwriting http.get and http.request like you do. But when http.get is calling http.request like here, a call to http.get will call http.request twice when using nock in a New Relic test.
To prove that this is not native Node behaviour (at least on Node 12):