From f9a84a0efab298dba5dc945f0098542420ab9b5b Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Mon, 30 Mar 2020 20:43:38 -0600 Subject: [PATCH 01/13] chore: remove body matching using "*" using definitions (#1964) * chore: remove body matching using "*" using definitions BREAKING CHANGE: Skipping body matching using "*" is no longer supported. Set the definition `body` to `undefined` instead. --- lib/scope.js | 23 +++-------------------- tests/test_define.js | 26 -------------------------- 2 files changed, 3 insertions(+), 46 deletions(-) diff --git a/lib/scope.js b/lib/scope.js index 73875d793..91ad43039 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -9,7 +9,6 @@ const assert = require('assert') const url = require('url') const debug = require('debug')('nock.scope') const { EventEmitter } = require('events') -const util = require('util') const Interceptor = require('./interceptor') let fs @@ -306,13 +305,6 @@ function tryJsonParse(string) { } } -// Use a noop deprecate util instead calling emitWarning directly so we get --no-deprecation and single warning behavior for free. -const emitAsteriskDeprecation = util.deprecate( - () => {}, - 'Skipping body matching using "*" is deprecated. Set the definition body to undefined instead.', - 'NOCK1579' -) - function define(nockDefs) { const scopes = [] @@ -335,17 +327,6 @@ function define(nockDefs) { options.reqheaders = reqheaders options.badheaders = badheaders - let { body } = nockDef - - if (body === '*') { - // In previous versions, it was impossible to NOT filter on request bodies. This special value - // is sniffed out for users manipulating the definitions and not wanting to match on the - // request body. For newer versions, users should remove the `body` key or set to `undefined` - // to achieve the same affect. Maintaining legacy behavior for now. - emitAsteriskDeprecation() - body = undefined - } - // Response is not always JSON as it could be a string or binary data or // even an array of binary buffers (e.g. when content is encoded). let response @@ -375,7 +356,9 @@ function define(nockDefs) { } }) - scope.intercept(npath, method, body).reply(status, response, rawHeaders) + scope + .intercept(npath, method, nockDef.body) + .reply(status, response, rawHeaders) scopes.push(scope) }) diff --git a/tests/test_define.js b/tests/test_define.js index 7b345e33d..ea32bb15f 100644 --- a/tests/test_define.js +++ b/tests/test_define.js @@ -304,29 +304,3 @@ test('define() uses badheaders', t => { ) req.end() }) - -test('define() treats a * body as a special case for not matching the request body', async t => { - t.plan(4) - - nock.define([ - { - scope: 'http://example.test', - method: 'POST', - path: '/', - body: '*', - response: 'matched', - }, - ]) - - process.once('warning', warning => { - t.equal(warning.name, 'DeprecationWarning') - t.match(warning.message, 'Skipping body matching using') - }) - - const { statusCode, body } = await got.post('http://example.test/', { - body: 'foo bar', - }) - - t.equal(statusCode, 200) - t.equal(body, 'matched') -}) From 11b76f8a22e211c063b92a709bddb5129b479d1b Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Mon, 30 Mar 2020 21:09:48 -0600 Subject: [PATCH 02/13] refactor: replace LoDash isPlainObject, mapValues, and isMap (#1963) - `isMap` was added to `util.types` in Node 10.0. https://nodejs.org/api/util.html#util_util_types_ismap_value - `isPlainObject` and `mapValue` both came from LoDash's master branch, which is the pending v5. - Replace LoDash dependency with `lodash.set` to cover the one use we still have. Ref: #1285 --- lib/common.js | 118 ++++++++++++++++++++++++++++++++------------- lib/interceptor.js | 3 +- lib/match_body.js | 5 +- package-lock.json | 8 +-- package.json | 2 +- 5 files changed, 92 insertions(+), 44 deletions(-) diff --git a/lib/common.js b/lib/common.js index 7679c0a38..259c6e211 100644 --- a/lib/common.js +++ b/lib/common.js @@ -1,9 +1,10 @@ 'use strict' -const _ = require('lodash') const debug = require('debug')('nock.common') -const url = require('url') +const set = require('lodash.set') const timers = require('timers') +const url = require('url') +const util = require('util') /** * Normalizes the request options so that it always has `host` property. @@ -194,7 +195,7 @@ function isJSONContent(headers) { * Duplicates throw an error. */ function headersFieldNamesToLowerCase(headers) { - if (!_.isPlainObject(headers)) { + if (!isPlainObject(headers)) { throw Error('Headers must be provided as an object') } @@ -243,11 +244,11 @@ function headersInputToRawArray(headers) { } // [].concat(...) is used instead of Array.flat until v11 is the minimum Node version - if (_.isMap(headers)) { + if (util.types.isMap(headers)) { return [].concat(...Array.from(headers, ([k, v]) => [k.toString(), v])) } - if (_.isPlainObject(headers)) { + if (isPlainObject(headers)) { return [].concat(...Object.entries(headers)) } @@ -359,7 +360,7 @@ function addHeaderLine(headers, name, value) { * @fieldName {String} field name - string with the case-insensitive field name */ function deleteHeadersField(headers, fieldNameToDelete) { - if (!_.isPlainObject(headers)) { + if (!isPlainObject(headers)) { throw Error('headers must be an object') } @@ -559,7 +560,7 @@ const dataEqual = (expected, actual) => * { 'foo[bar][0]': 'baz' } -> { foo: { bar: [ 'baz' ] } } */ const expand = input => - Object.entries(input).reduce((acc, [k, v]) => _.set(acc, k, v), {}) + Object.entries(input).reduce((acc, [k, v]) => set(acc, k, v), {}) /** * Performs a recursive strict comparison between two values. @@ -572,7 +573,7 @@ function deepEqual(expected, actual) { return expected.test(actual) } - if (Array.isArray(expected) || _.isPlainObject(expected)) { + if (Array.isArray(expected) || isPlainObject(expected)) { if (actual === undefined) { return false } @@ -588,6 +589,51 @@ function deepEqual(expected, actual) { return expected === actual } +/** + * Checks if `value` is a plain object, that is, an object created by the + * `Object` constructor or one with a `[[Prototype]]` of `null`. + * https://github.com/lodash/lodash/blob/588bf3e20db0ae039a822a14a8fa238c5b298e65/isPlainObject.js + * + * @param {*} value The value to check. + * @return {boolean} + */ +function isPlainObject(value) { + const isObjectLike = typeof value === 'object' && value !== null + const tag = Object.prototype.toString.call(value) + if (!isObjectLike || tag !== '[object Object]') { + return false + } + if (Object.getPrototypeOf(value) === null) { + return true + } + let proto = value + while (Object.getPrototypeOf(proto) !== null) { + proto = Object.getPrototypeOf(proto) + } + return Object.getPrototypeOf(value) === proto +} + +/** + * Creates an object with the same keys as `object` and values generated + * by running each own enumerable string keyed property of `object` thru + * `iteratee`. (iteration order is not guaranteed) + * The iteratee is invoked with three arguments: (value, key, object). + * https://github.com/lodash/lodash/blob/588bf3e20db0ae039a822a14a8fa238c5b298e65/mapValue.js + * + * @param {Object} object The object to iterate over. + * @param {Function} iteratee The function invoked per iteration. + * @returns {Object} Returns the new mapped object. + */ +function mapValue(object, iteratee) { + object = Object(object) + const result = {} + + Object.keys(object).forEach(key => { + result[key] = iteratee(object[key], key, object) + }) + return result +} + const timeouts = [] const intervals = [] const immediates = [] @@ -614,29 +660,33 @@ function removeAllTimers() { clearTimer(clearImmediate, immediates) } -exports.normalizeClientRequestArgs = normalizeClientRequestArgs -exports.normalizeRequestOptions = normalizeRequestOptions -exports.normalizeOrigin = normalizeOrigin -exports.isUtf8Representable = isUtf8Representable -exports.overrideRequests = overrideRequests -exports.restoreOverriddenRequests = restoreOverriddenRequests -exports.stringifyRequest = stringifyRequest -exports.isContentEncoded = isContentEncoded -exports.contentEncoding = contentEncoding -exports.isJSONContent = isJSONContent -exports.headersFieldNamesToLowerCase = headersFieldNamesToLowerCase -exports.headersFieldsArrayToLowerCase = headersFieldsArrayToLowerCase -exports.headersArrayToObject = headersArrayToObject -exports.headersInputToRawArray = headersInputToRawArray -exports.deleteHeadersField = deleteHeadersField -exports.forEachHeader = forEachHeader -exports.percentEncode = percentEncode -exports.percentDecode = percentDecode -exports.matchStringOrRegexp = matchStringOrRegexp -exports.formatQueryValue = formatQueryValue -exports.isStream = isStream -exports.dataEqual = dataEqual -exports.setTimeout = setTimeout -exports.setInterval = setInterval -exports.setImmediate = setImmediate -exports.removeAllTimers = removeAllTimers +module.exports = { + contentEncoding, + dataEqual, + deleteHeadersField, + forEachHeader, + formatQueryValue, + headersArrayToObject, + headersFieldNamesToLowerCase, + headersFieldsArrayToLowerCase, + headersInputToRawArray, + isContentEncoded, + isJSONContent, + isPlainObject, + isStream, + isUtf8Representable, + mapValue, + matchStringOrRegexp, + normalizeClientRequestArgs, + normalizeOrigin, + normalizeRequestOptions, + overrideRequests, + percentDecode, + percentEncode, + removeAllTimers, + restoreOverriddenRequests, + setImmediate, + setInterval, + setTimeout, + stringifyRequest, +} diff --git a/lib/interceptor.js b/lib/interceptor.js index 8d1cccba0..202c43bb3 100644 --- a/lib/interceptor.js +++ b/lib/interceptor.js @@ -2,7 +2,6 @@ const debug = require('debug')('nock.interceptor') const stringify = require('json-stringify-safe') -const _ = require('lodash') const querystring = require('querystring') const { URL, URLSearchParams } = require('url') @@ -483,7 +482,7 @@ module.exports = class Interceptor { // Normalize the data into the shape that is matched against. // Duplicate keys are handled by combining the values into an array. queries = querystring.parse(queries.toString()) - } else if (!_.isPlainObject(queries)) { + } else if (!common.isPlainObject(queries)) { throw Error(`Argument Error: ${queries}`) } diff --git a/lib/match_body.js b/lib/match_body.js index 6eda62dd5..73f753a23 100644 --- a/lib/match_body.js +++ b/lib/match_body.js @@ -1,6 +1,5 @@ 'use strict' -const _ = require('lodash') const querystring = require('querystring') const common = require('./common') @@ -70,8 +69,8 @@ function mapValuesDeep(obj, cb) { if (Array.isArray(obj)) { return obj.map(v => mapValuesDeep(v, cb)) } - if (_.isPlainObject(obj)) { - return _.mapValues(obj, v => mapValuesDeep(v, cb)) + if (common.isPlainObject(obj)) { + return common.mapValue(obj, v => mapValuesDeep(v, cb)) } return cb(obj) } diff --git a/package-lock.json b/package-lock.json index 1c0f480b2..1efc2d94d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3935,7 +3935,8 @@ "lodash": { "version": "4.17.15", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.15.tgz", - "integrity": "sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==" + "integrity": "sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==", + "dev": true }, "lodash.capitalize": { "version": "4.2.1", @@ -3981,9 +3982,8 @@ }, "lodash.set": { "version": "4.3.2", - "resolved": "https://registry.npmjs.org/lodash.set/-/lodash.set-4.3.2.tgz", - "integrity": "sha1-2HV7HagH3eJIFrDWqEvqGnYjCyM=", - "dev": true + "resolved": "http://artifactory.shuttercorp.net/artifactory/api/npm/npm-composite/lodash.set/-/lodash.set-4.3.2.tgz", + "integrity": "sha1-2HV7HagH3eJIFrDWqEvqGnYjCyM=" }, "lodash.toarray": { "version": "4.4.0", diff --git a/package.json b/package.json index ea222e26d..00de7a5a9 100644 --- a/package.json +++ b/package.json @@ -24,7 +24,7 @@ "dependencies": { "debug": "^4.1.0", "json-stringify-safe": "^5.0.1", - "lodash": "^4.17.13", + "lodash.set": "^4.3.2", "propagate": "^2.0.0" }, "devDependencies": { From 5c1b937b71edde6e7acac780c663c746566389b9 Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Sat, 4 Apr 2020 11:45:08 -0600 Subject: [PATCH 03/13] fix(router): bring behavior of `abort()` inline with native Node (#1960) Nocks behavior around events and errors relating to aborting and ending requests has not lined up with Node for while. I dug into an investigation of how it differs, if there were git commits/pull-requests explaining why they differ, and trying to answer the question of should Nock change to line up better. The single largest deviation were the errors emitted around aborting or already aborted requests. Emitting an error on abort was added to fix issue #439. The conversation talks about how Node emits an error post abort. > ... in practice the `http` module does emit an `ECONNRESET` error after aborting. ... - Jan 7, 2016 However, this is only true between the 'socket' and 'response' events. Otherwise calling abort does not emit an error of any kind. Determining exactly what Nock should do is a bit of moving target. As Node's internals of HTTP, Net, Stream, and micro task timing have changed a lot since iojs/v4, when Nock added these errors, and v13, current at this time. My conclusion is that none of Nock's deviations from Node's _documented_ behavior are user features that should be maintained. If anything, bringing Nock closer to Node better fulfills original requests. https://github.com/nock/nock/pull/282 The entire `test_abort.js` file was scraped and rewritten by writing assertions against Node behavior, without Nock first. Then adding Nock into each test to ensure events and errors were correct. BREAKING CHANGE: Calling `request.abort()`: - Use to always emit a 'socket hang up' error. Now only emits the error if `abort` is called between the 'socket' and 'response' events. - The emitted 'abort' event now happens on `nextTick`. - The socket is now only destroyed if the 'socket' event has fired, and now emits a 'close' event on `nextTick` that propagates though the request object. - `request.aborted` attribute is set to `true` instead of a timestamp. Changed in Node v11.0 https://github.com/nodejs/node/pull/20230 Calling `write`, `end`, or `flushHeaders` on an aborted request no longer emits an error. However, writing to a request that is already finished (ended) will emit a 'write after end' error. Playback of a mocked responses will now never happen until the 'socket' event is emitted. The 'socket' event is still artificially set to emit on `nextTick` when a ClientRequest is created. This means in the following code the Scope will never be done because at least one tick needs to happen before any matched Interceptor is considered consumed. ```js const scope = nock(...).get('/').reply() const req = http.get(...) scope.done() ``` --- lib/intercepted_request_router.js | 145 ++++++++++------- lib/playback_interceptor.js | 35 ++-- lib/socket.js | 24 ++- tests/test_abort.js | 256 ++++++++++++++++-------------- tests/test_back.js | 18 ++- tests/test_client_request.js | 3 +- tests/test_header_matching.js | 2 +- tests/test_intercept.js | 60 ++----- tests/test_request_overrider.js | 39 ++++- tests/test_socket_delay.js | 2 +- tests/test_stream.js | 87 +++++----- 11 files changed, 369 insertions(+), 302 deletions(-) diff --git a/lib/intercepted_request_router.js b/lib/intercepted_request_router.js index babf4f678..ae61a9f1d 100644 --- a/lib/intercepted_request_router.js +++ b/lib/intercepted_request_router.js @@ -39,16 +39,19 @@ class InterceptedRequestRouter { } this.response = new IncomingMessage(this.socket) - this.playbackStarted = false this.requestBodyBuffers = [] + this.playbackStarted = false + + // For parity with Node, it's important the socket event is emitted before we begin playback. + // This flag is set when playback is triggered if we haven't yet gotten the + // socket event to indicate that playback should start as soon as it comes in. + this.readyToStartPlaybackOnSocketEvent = false this.attachToReq() } attachToReq() { - const { req, response, socket, options } = this - - response.req = req + const { req, socket, options } = this for (const [name, val] of Object.entries(options.headers)) { req.setHeader(name.toLowerCase(), val) @@ -71,7 +74,7 @@ class InterceptedRequestRouter { // The same Socket is shared between the request and response to mimic native behavior. req.socket = req.connection = socket - propagate(['error', 'timeout'], req.socket, req) + propagate(['close', 'error', 'timeout'], req.socket, req) req.write = (...args) => this.handleWrite(...args) req.end = (...args) => this.handleEnd(...args) @@ -90,6 +93,11 @@ class InterceptedRequestRouter { // Some clients listen for a 'socket' event to be emitted before calling end(), // which causes nock to hang. process.nextTick(() => { + if (req.aborted) { + return + } + + socket.connecting = false req.emit('socket', socket) // https://nodejs.org/api/net.html#net_event_connect @@ -99,6 +107,10 @@ class InterceptedRequestRouter { if (socket.authorized) { socket.emit('secureConnect') } + + if (this.readyToStartPlaybackOnSocketEvent) { + this.maybeStartPlayback() + } }) } @@ -109,26 +121,42 @@ class InterceptedRequestRouter { }) } + // from docs: When write function is called with empty string or buffer, it does nothing and waits for more input. + // However, actually implementation checks the state of finished and aborted before checking if the first arg is empty. handleWrite(buffer, encoding, callback) { debug('write', arguments) const { req } = this - if (!req.aborted) { - if (buffer) { - if (!Buffer.isBuffer(buffer)) { - buffer = Buffer.from(buffer, encoding) - } - this.requestBodyBuffers.push(buffer) - } - // can't use instanceof Function because some test runners - // run tests in vm.runInNewContext where Function is not same - // as that in the current context - // https://github.com/nock/nock/pull/1754#issuecomment-571531407 - if (typeof callback === 'function') { - callback() - } - } else { - this.emitError(new Error('Request aborted')) + if (req.finished) { + const err = new Error('write after end') + err.code = 'ERR_STREAM_WRITE_AFTER_END' + this.emitError(err) + + // It seems odd to return `true` here, not sure why you'd want to have + // the stream potentially written to more, but it's what Node does. + // https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L662-L665 + return true + } + + if (req.aborted) { + return false + } + + if (!buffer || buffer.length === 0) { + return true + } + + if (!Buffer.isBuffer(buffer)) { + buffer = Buffer.from(buffer, encoding) + } + this.requestBodyBuffers.push(buffer) + + // can't use instanceof Function because some test runners + // run tests in vm.runInNewContext where Function is not same + // as that in the current context + // https://github.com/nock/nock/pull/1754#issuecomment-571531407 + if (typeof callback === 'function') { + callback() } common.setImmediate(function () { @@ -142,7 +170,7 @@ class InterceptedRequestRouter { debug('req.end') const { req } = this - // handle the different overloaded param signatures + // handle the different overloaded arg signatures if (typeof chunk === 'function') { callback = chunk chunk = null @@ -155,51 +183,48 @@ class InterceptedRequestRouter { req.once('finish', callback) } - if (!req.aborted && !this.playbackStarted) { - req.write(chunk, encoding) - this.startPlayback() - } - if (req.aborted) { - this.emitError(new Error('Request aborted')) - } + req.write(chunk, encoding) + req.finished = true + this.maybeStartPlayback() return req } handleFlushHeaders() { debug('req.flushHeaders') - const { req } = this - - if (!req.aborted && !this.playbackStarted) { - this.startPlayback() - } - if (req.aborted) { - this.emitError(new Error('Request aborted')) - } + this.maybeStartPlayback() } handleAbort() { debug('req.abort') - const { req, response, socket } = this + const { req, socket } = this if (req.aborted) { return } - req.aborted = Date.now() - if (!this.playbackStarted) { - this.startPlayback() - } - const err = new Error() - err.code = 'aborted' - response.emit('close', err) - - socket.destroy() - req.emit('abort') - - const connResetError = new Error('socket hang up') - connResetError.code = 'ECONNRESET' - this.emitError(connResetError) + // Historically, `aborted` was undefined or a timestamp. + // Node changed this behavior in v11 to always be a bool. + req.aborted = true + req.destroyed = true + + // the order of these next three steps is important to match order of events Node would emit. + process.nextTick(() => req.emit('abort')) + + if (!socket.connecting) { + if (!req.res) { + // If we don't have a response then we know that the socket + // ended prematurely and we need to emit an error on the request. + // Node doesn't actually emit this during the `abort` method. + // Instead it listens to the socket's `end` and `close` events, however, + // Nock's socket doesn't have all the complexities and events to + // replicated that directly. This is an easy way to achieve the same behavior. + const connResetError = new Error('socket hang up') + connResetError.code = 'ECONNRESET' + this.emitError(connResetError) + } + socket.destroy() + } } /** @@ -233,6 +258,21 @@ class InterceptedRequestRouter { } } + maybeStartPlayback() { + const { req, socket, playbackStarted } = this + + // In order to get the events in the right order we need to delay playback + // if we get here before the `socket` event is emitted. + if (socket.connecting) { + this.readyToStartPlaybackOnSocketEvent = true + return + } + + if (!req.aborted && !playbackStarted) { + this.startPlayback() + } + } + startPlayback() { debug('ending') this.playbackStarted = true @@ -274,7 +314,6 @@ class InterceptedRequestRouter { // wait to emit the finish event until we know for sure an Interceptor is going to playback. // otherwise an unmocked request might emit finish twice. - req.finished = true req.emit('finish') playbackInterceptor({ diff --git a/lib/playback_interceptor.js b/lib/playback_interceptor.js index ffe4d050c..875954589 100644 --- a/lib/playback_interceptor.js +++ b/lib/playback_interceptor.js @@ -257,10 +257,6 @@ function playbackInterceptor({ interceptor.markConsumed() - if (req.aborted) { - return - } - response.rawHeaders.push( ...selectDefaultHeaders( response.rawHeaders, @@ -277,24 +273,24 @@ function playbackInterceptor({ response.headers = common.headersArrayToObject(response.rawHeaders) - process.nextTick(() => - respondUsingInterceptor({ - responseBody, - responseBuffers, - }) - ) + respondUsingInterceptor({ + responseBody, + responseBuffers, + }) } function respondUsingInterceptor({ responseBody, responseBuffers }) { - if (req.aborted) { - return - } - function respond() { if (req.aborted) { return } + // Even though we've had the response object for awhile at this point, + // we only attach it to the request immediately before the `response` + // event because, as in Node, it alters the error handling around aborts. + req.res = response + response.req = req + debug('emitting response') req.emit('response', response) @@ -342,7 +338,16 @@ function playbackInterceptor({ } } - start() + // If there are no delays configured on the Interceptor, calling `start` will + // take the request all the way to the 'response' event during a single microtask + // execution. This setImmediate stalls the playback to ensure the correct events + // are emitted first ('socket', 'finish') and any aborts in the in the queue + // or called during a 'finish' listener can be called. + common.setImmediate(() => { + if (!req.aborted) { + start() + } + }) } module.exports = { playbackInterceptor } diff --git a/lib/socket.js b/lib/socket.js index 02f0cb7bd..6fd4b73e3 100644 --- a/lib/socket.js +++ b/lib/socket.js @@ -18,7 +18,7 @@ module.exports = class Socket extends EventEmitter { this.readable = true this.pending = false this.destroyed = false - this.connecting = false + this.connecting = true // totalDelay that has already been applied to the current // request/connection, timeout error will be generated if @@ -69,12 +69,28 @@ module.exports = class Socket extends EventEmitter { ).toString('base64') } + /** + * Denotes that no more I/O activity should happen on this socket. + * + * The implementation in Node if far more complex as it juggles underlying async streams. + * For the purposes of Nock, we just need it to set some flags and on the first call + * emit a 'close' and optional 'error' event. Both events propagate through the request object. + */ destroy(err) { + if (this.destroyed) { + return this + } + this.destroyed = true this.readable = this.writable = false - if (err) { - this.emit('error', err) - } + + process.nextTick(() => { + if (err) { + this.emit('error', err) + } + this.emit('close') + }) + return this } } diff --git a/tests/test_abort.js b/tests/test_abort.js index b2986ff89..8a19932a0 100644 --- a/tests/test_abort.js +++ b/tests/test_abort.js @@ -1,157 +1,181 @@ 'use strict' -const nock = require('..') const { expect } = require('chai') const http = require('http') const sinon = require('sinon') +const nock = require('..') require('./setup') -it('`req.abort()` should cause "abort" and "error" to be emitted', done => { - const scope = nock('http://example.test') - .get('/') - .delayConnection(500) - .reply() - - const onAbort = sinon.spy() - const req = http - .get('http://example.test/') - .once('abort', onAbort) - .once('error', err => { - // Should trigger last - expect(err.code).to.equal('ECONNRESET') - expect(onAbort).to.have.been.calledOnce() - scope.done() - done() - }) - process.nextTick(() => req.abort()) -}) +// These tests use `setTimeout` before verifying emitted events to ensure any +// number of `nextTicks` or `setImmediate` can process first. -it('abort is emitted before delay time', done => { - const scope = nock('http://example.test') - .get('/') - .delayConnection(500) - .reply() - - const start = Date.now() - const req = http - .get('http://example.test/') - .once('abort', () => { - const actual = Date.now() - start - expect(actual).to.be.below(250) - scope.done() +// Node will emit a `prefinish` event after `socket`, but it's an internal, +// undocumented event that Nock does not emulate. + +// The order of tests run sequentially through a ClientRequest's lifetime. +// Starting the top by aborting requests early on then aborting later and later. +describe('`ClientRequest.abort()`', () => { + it('Emits the expected event sequence when `write` is called on an aborted request', done => { + const scope = nock('http://example.test').get('/').reply() + + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') + req.abort() + req.write('foo') + + setTimeout(() => { + expect(emitSpy).to.have.been.calledOnceWithExactly('abort') + expect(scope.isDone()).to.be.false() done() - }) - .once('error', () => {}) // Don't care. - // Don't bother with the response + }, 10) + }) - setTimeout(() => req.abort(), 10) -}) + it('Emits the expected event sequence when `end` is called on an aborted request', done => { + const scope = nock('http://example.test').get('/').reply() -it('Aborting an aborted request should not emit an error', done => { - const scope = nock('http://example.test').get('/').reply() + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') + req.abort() + req.end() - let errorCount = 0 - const req = http.get('http://example.test/').on('error', () => { - errorCount++ - if (errorCount < 3) { - // Abort 3 times at max, otherwise this would be an endless loop. - // https://github.com/nock/nock/issues/882 - req.abort() - } + setTimeout(() => { + expect(emitSpy).to.have.been.calledOnceWithExactly('abort') + expect(scope.isDone()).to.be.false() + done() + }, 10) }) - req.abort() - - // Allow some time to fail. - setTimeout(() => { - expect(errorCount).to.equal(1, 'Only one error should be sent') - scope.done() - done() - }, 10) -}) -it('Aborting a not-yet-ended request should end it', done => { - // Set up. - const scope = nock('http://example.test').post('/').reply() + it('Emits the expected event sequence when `flushHeaders` is called on an aborted request', done => { + const scope = nock('http://example.test').get('/').reply() + + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') + req.abort() + req.flushHeaders() - const req = http.request({ - host: 'example.test', - method: 'post', - path: '/', + setTimeout(() => { + expect(emitSpy).to.have.been.calledOnceWithExactly('abort') + expect(scope.isDone()).to.be.false() + done() + }, 10) }) - req.on('error', () => {}) - // Act. - req.abort() + it('Emits the expected event sequence when aborted immediately after `end`', done => { + const scope = nock('http://example.test').get('/').reply() - // Assert. - scope.done() + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') + req.end() + req.abort() - done() -}) + setTimeout(() => { + expect(emitSpy).to.have.been.calledOnceWithExactly('abort') + expect(scope.isDone()).to.be.false() + done() + }, 10) + }) -it('`req.write() on an aborted request should trigger the expected error', done => { - const scope = nock('http://example.test').get('/').reply() + it('Emits the expected event sequence when aborted inside a `socket` event listener', done => { + const scope = nock('http://example.test').get('/').reply() - const req = http.get('http://example.test/') + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') - req.once('error', err => { - // This is the expected first error event emitted, triggered by - // `req.abort()`. - expect(err.code).to.equal('ECONNRESET') + req.on('socket', () => { + req.abort() + }) + req.on('error', err => { + expect(err.message).to.equal('socket hang up') + expect(err.code).to.equal('ECONNRESET') + }) + req.end() - req.once('error', err => { - // This is the abort error under test, triggered by `req.write()` - expect(err.message).to.equal('Request aborted') - scope.done() + setTimeout(() => { + const events = emitSpy.args.map(i => i[0]) + expect(events).to.deep.equal(['socket', 'abort', 'error', 'close']) + expect(scope.isDone()).to.be.false() done() - }) + }, 10) }) - process.nextTick(() => req.abort()) - process.nextTick(() => req.write('some nonsense')) -}) - -it('`req.end()` on an aborted request should trigger the expected error', done => { - const scope = nock('http://example.test').get('/').reply() + it('Emits the expected event sequence when aborted multiple times', done => { + const scope = nock('http://example.test').get('/').reply() - const req = http.get('http://example.test/') + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') - req.once('error', err => { - // This is the expected first error event emitted, triggered by - // `req.abort()`. - expect(err.code).to.equal('ECONNRESET') + req.on('error', () => {}) // listen for error so it doesn't bubble + req.on('socket', () => { + req.abort() + req.abort() + req.abort() + }) + req.end() - req.once('error', err => { - // This is the abort error under test, triggered by `req.end()` - expect(err.message).to.equal('Request aborted') - scope.done() + setTimeout(() => { + const events = emitSpy.args.map(i => i[0]) + // important: `abort` and `error` events only fire once and the `close` event still fires at the end + expect(events).to.deep.equal(['socket', 'abort', 'error', 'close']) + expect(scope.isDone()).to.be.false() done() - }) + }, 10) }) - process.nextTick(() => req.abort()) - process.nextTick(() => req.end()) -}) + it('Emits the expected event sequence when aborted inside a `finish` event listener', done => { + const scope = nock('http://example.test').get('/').reply() -it('`req.flushHeaders()` on an aborted request should trigger the expected error', done => { - const scope = nock('http://example.test').get('/').reply() + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') - const req = http.get('http://example.test/') + req.on('finish', () => { + req.abort() + }) + req.on('error', err => { + expect(err.message).to.equal('socket hang up') + expect(err.code).to.equal('ECONNRESET') + }) + req.end() + + setTimeout(() => { + const events = emitSpy.args.map(i => i[0]) + expect(events).to.deep.equal([ + 'socket', + 'finish', + 'abort', + 'error', + 'close', + ]) + expect(scope.isDone()).to.be.false() + done() + }, 10) + }) - req.once('error', err => { - // This is the expected first error event emitted, triggered by - // `req.abort()`. - expect(err.code).to.equal('ECONNRESET') + // The Interceptor is considered consumed just prior to the `response` event on the request, + // all tests below assert the Scope is done. - req.once('error', err => { - // This is the abort error under test, triggered by `req.flushHeaders()` - expect(err.message).to.equal('Request aborted') + it('Emits the expected event sequence when aborted inside a `response` event listener', done => { + const scope = nock('http://example.test').get('/').reply() + + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') + + req.on('response', () => { + req.abort() + }) + req.end() + + setTimeout(() => { + const events = emitSpy.args.map(i => i[0]) + expect(events).to.deep.equal([ + 'socket', + 'finish', + 'response', + 'abort', + 'close', + ]) scope.done() done() - }) + }, 10) }) - - process.nextTick(() => req.abort()) - process.nextTick(() => req.flushHeaders()) }) diff --git a/tests/test_back.js b/tests/test_back.js index d0752cd68..3a2388afa 100644 --- a/tests/test_back.js +++ b/tests/test_back.js @@ -54,10 +54,11 @@ function nockBackWithFixture(t, scopesLoaded) { nockBack('good_request.json', function (done) { t.equal(this.scopes.length, scopesLength) - http.get('http://www.example.test/') - this.assertScopesFinished() - done() - t.end() + http.get('http://www.example.test/', () => { + this.assertScopesFinished() + done() + t.end() + }) }) } @@ -360,10 +361,11 @@ test('nockBack record tests', nw => { nw.test('it loads your recorded tests', t => { nockBack('good_request.json', function (done) { t.true(this.scopes.length > 0) - http.get('http://www.example.test/').end() - this.assertScopesFinished() - done() - t.end() + http.get('http://www.example.test/', () => { + this.assertScopesFinished() + done() + t.end() + }) }) }) diff --git a/tests/test_client_request.js b/tests/test_client_request.js index 260772218..3f5b301f0 100644 --- a/tests/test_client_request.js +++ b/tests/test_client_request.js @@ -79,11 +79,10 @@ describe('Direct use of `ClientRequest`', () => { } const req = new http.ClientRequest(reqOpts, res => { expect(res.statusCode).to.equal(201) + scope.done() done() }) req.end() - - scope.done() }) it('should throw an expected error when creating with empty options', () => { diff --git a/tests/test_header_matching.js b/tests/test_header_matching.js index b41e3aad0..e34673354 100644 --- a/tests/test_header_matching.js +++ b/tests/test_header_matching.js @@ -568,7 +568,7 @@ it('header manipulation', done => { .get('/accounts') .reply(200, { accounts: [{ id: 1, name: 'Joe Blow' }] }) - const req = http.get({ host: 'example.test', path: '/accounts' }, res => { + const req = http.request({ host: 'example.test', path: '/accounts' }, res => { res.on('end', () => { scope.done() done() diff --git a/tests/test_intercept.js b/tests/test_intercept.js index 90a5988f6..ec024fbe2 100644 --- a/tests/test_intercept.js +++ b/tests/test_intercept.js @@ -255,34 +255,6 @@ test('on interceptor, filter path with function', async t => { scope.done() }) -// TODO: Move to test_request_overrider. -test('abort request', t => { - const scope = nock('http://example.test').get('/hey').reply(200, 'nobody') - - const req = http.request({ - host: 'example.test', - path: '/hey', - }) - - req.on('response', res => { - res.on('close', err => { - expect(err.code).to.equal('aborted') - scope.done() - }) - - res.on('end', () => expect.fail()) - - req.once('error', err => { - expect(err.code).to.equal('ECONNRESET') - t.end() - }) - - req.abort() - }) - - req.end() -}) - test('chaining API', async t => { const scope = nock('http://example.test') .get('/one') @@ -708,27 +680,19 @@ test('get correct filtering with scope and request headers filtering', t => { .reply(200, responseText, { 'Content-Type': 'text/plain' }) const onData = sinon.spy() - const req = http.get( - { - host: 'bar.example.test', - method: 'GET', - path: '/path', - port: 80, - }, - res => { - res.on('data', data => { - onData() - expect(data.toString()).to.equal(responseText) - }) - res.on('end', () => { - expect(onData).to.have.been.calledOnce() - scope.done() - t.end() - }) - } - ) + const req = http.get('http://bar.example.test/path', res => { + expect(req.getHeaders()).to.deep.equal({ host: requestHeaders.host }) - expect(req.getHeaders()).to.deep.equal({ host: requestHeaders.host }) + res.on('data', data => { + onData() + expect(data.toString()).to.equal(responseText) + }) + res.on('end', () => { + expect(onData).to.have.been.calledOnce() + scope.done() + t.end() + }) + }) }) test('different subdomain with reply callback and filtering scope', async t => { diff --git a/tests/test_request_overrider.js b/tests/test_request_overrider.js index ad2b1d569..5565d62a6 100644 --- a/tests/test_request_overrider.js +++ b/tests/test_request_overrider.js @@ -50,9 +50,10 @@ describe('Request Overrider', () => { const req = http.get('http://example.test') - scope.done() - - req.on('response', () => done()) + req.on('response', () => { + scope.done() + done() + }) }) it('write callback called', done => { @@ -220,6 +221,21 @@ describe('Request Overrider', () => { req.end('foobar', reqEndCallback) }) + it('should emit an error if `write` is called after `end`', done => { + nock('http://example.test').get('/').reply() + + const req = http.request('http://example.test') + + req.on('error', err => { + expect(err.message).to.equal('write after end') + expect(err.code).to.equal('ERR_STREAM_WRITE_AFTER_END') + done() + }) + + req.end() + req.write('foo') + }) + // http://github.com/nock/nock/issues/139 it('should emit "finish" on the request before emitting "end" on the response', done => { const scope = nock('http://example.test').post('/').reply() @@ -544,6 +560,23 @@ describe('Request Overrider', () => { }) }) + it('calling Socket#destroy() multiple times only emits a single `close` event', done => { + nock('http://example.test').get('/').reply(200, 'hey') + + const req = http.get('http://example.test') + req.once('socket', socket => { + const closeSpy = sinon.spy() + socket.on('close', closeSpy) + + socket.destroy().destroy().destroy() + + setTimeout(() => { + expect(closeSpy).to.have.been.calledOnce() + done() + }, 10) + }) + }) + it('socket has getPeerCertificate() method which returns a random base64 string', done => { nock('http://example.test').get('/').reply() diff --git a/tests/test_socket_delay.js b/tests/test_socket_delay.js index a27533990..ab9f87468 100644 --- a/tests/test_socket_delay.js +++ b/tests/test_socket_delay.js @@ -89,6 +89,7 @@ describe('`socketDelay()`', () => { res.once('end', () => { expect(body).to.equal(responseText) + scope.done() done() }) }) @@ -98,7 +99,6 @@ describe('`socketDelay()`', () => { }) req.end() - scope.done() }) }) diff --git a/tests/test_stream.js b/tests/test_stream.js index 7b38c715e..50fb0be72 100644 --- a/tests/test_stream.js +++ b/tests/test_stream.js @@ -43,35 +43,29 @@ it('pause response after data', done => { // multiple 'data' events. .reply(200, response) - http.get( - { - host: 'example.test', - path: '/', - }, - res => { - const didTimeout = sinon.spy() + http.get('http://example.test', res => { + const didTimeout = sinon.spy() - setTimeout(() => { - didTimeout() - res.resume() - }, 500) + setTimeout(() => { + didTimeout() + res.resume() + }, 500) - res.on('data', data => res.pause()) + res.on('data', () => res.pause()) - res.on('end', () => { - expect(didTimeout).to.have.been.calledOnce() - scope.done() - done() - }) - } - ) + res.on('end', () => { + expect(didTimeout).to.have.been.calledOnce() + scope.done() + done() + }) - // Manually simulate multiple 'data' events. - response.emit('data', 'one') - setTimeout(() => { - response.emit('data', 'two') - response.end() - }, 0) + // Manually simulate multiple 'data' events. + response.emit('data', 'one') + setTimeout(() => { + response.emit('data', 'two') + response.end() + }, 0) + }) }) // https://github.com/nock/nock/issues/1493 @@ -83,30 +77,22 @@ it("response has 'complete' property and it's true after end", done => { // multiple 'data' events. .reply(200, response) - http.get( - { - host: 'example.test', - path: '/', - }, - res => { - const onData = sinon.spy() + http.get('http://example.test', res => { + const onData = sinon.spy() - res.on('data', onData) + res.on('data', onData) - res.on('end', () => { - expect(onData).to.have.been.called() - expect(res.complete).to.be.true() - scope.done() - done() - }) - } - ) + res.on('end', () => { + expect(onData).to.have.been.called() + expect(res.complete).to.be.true() + scope.done() + done() + }) - // Manually simulate multiple 'data' events. - response.emit('data', 'one') - setTimeout(() => { + // Manually simulate multiple 'data' events. + response.emit('data', 'one') response.end() - }, 0) + }) }) // TODO Convert to async / got. @@ -325,14 +311,13 @@ it('error events on reply streams proxy to the response', done => { res => { res.on('error', err => { expect(err).to.equal('oh no!') + scope.done() done() }) + + replyBody.end(() => { + replyBody.emit('error', 'oh no!') + }) } ) - - scope.done() - - replyBody.end(() => { - replyBody.emit('error', 'oh no!') - }) }) From d523df2ed09f3cbc186c0e200571f87f7ee25d4d Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Sun, 5 Apr 2020 13:26:25 -0600 Subject: [PATCH 04/13] feature: remove Scope.log in favor of more debug (#1966) Consolidates the behavior around debugging and logging to exclusively use [`debug`](https://github.com/visionmedia/debug). The use of `scope.log(console.log)` was problematic and confusing for consumers as that appeared to be the correct approach when debugging unless they found the correct section in the README. The use of the logger vs directly using `debug` in the core of Nock was inconsistent, especially when matching. Now any failure to match a request will be directed to `debug` and if a single Interceptor is in context, the namespace in the log will include the host from the Scope. This allows for filtering of logs to stdout as well as differentiating colors from `debug`, when enabled. BREAKING CHANGE: `Scope.log` has been removed. Use the `debug` library when [debugging](https://github.com/nock/nock#debugging) failed matches. --- README.md | 31 ++++++++++++-------- lib/intercept.js | 2 +- lib/intercepted_request_router.js | 4 ++- lib/interceptor.js | 42 ++++++++++++++++++--------- lib/playback_interceptor.js | 20 +++++++------ lib/scope.js | 11 ++++--- tests/test_logging.js | 48 ++++++++++++++++--------------- types/index.d.ts | 1 - types/tests.ts | 6 +--- 9 files changed, 94 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index 04c3f274e..046b02ec9 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,6 @@ For instance, if a module performs HTTP requests to a CouchDB server or makes HT - [.pendingMocks()](#pendingmocks) - [.activeMocks()](#activemocks) - [.isActive()](#isactive) -- [Logging](#logging) - [Restoring](#restoring) - [Activating](#activating) - [Turning Nock Off (experimental!)](#turning-nock-off-experimental) @@ -1048,16 +1047,6 @@ if (!nock.isActive()) { } ``` -## Logging - -Nock can log matches if you pass in a log function like this: - -```js -const scope = nock('http://google.com') - .log(console.log) - ... -``` - ## Restoring You can restore the HTTP interceptor to the normal unmocked behaviour by calling: @@ -1548,8 +1537,26 @@ It does this by manipulating the modules cache of Node in a way that conflicts w Nock uses [`debug`](https://github.com/visionmedia/debug), so just run with environmental variable `DEBUG` set to `nock.*`. +```console +user@local$ DEBUG=nock.* node my_test.js +``` + +Each step in the matching process is logged this way and can be useful when determining why a request was not intercepted by Nock. + +For example the following shows that matching failed because the request had an extra search parameter. + ```js -$ DEBUG=nock.* node my_test.js +nock('http://example.com').get('/').query({ foo: 'bar' }).reply() + +await got('http://example.com/?foo=bar&baz=foz') +``` + +```console +user@local$ DEBUG=nock.scope:example.com node my_test.js +... +nock.scope:example.com Interceptor queries: {"foo":"bar"} +1ms +nock.scope:example.com Request queries: {"foo":"bar","baz":"foz"} +0ms +nock.scope:example.com query matching failed +0ms ``` ## Contributing diff --git a/lib/intercept.js b/lib/intercept.js index a98577fa7..c07fd57c5 100644 --- a/lib/intercept.js +++ b/lib/intercept.js @@ -159,7 +159,7 @@ function interceptorsFor(options) { // If scope filtering function is defined and returns a truthy value then // we have to treat this as a match. if (filteringScope && filteringScope(basePath)) { - debug('found matching scope interceptor') + interceptor.scope.logger('found matching scope interceptor') // Keep the filtered scope (its key) to signal the rest of the module // that this wasn't an exact but filtered match. diff --git a/lib/intercepted_request_router.js b/lib/intercepted_request_router.js index ae61a9f1d..fe8faebae 100644 --- a/lib/intercepted_request_router.js +++ b/lib/intercepted_request_router.js @@ -310,7 +310,9 @@ class InterceptedRequestRouter { ) if (matchedInterceptor) { - debug('interceptor identified, starting mocking') + matchedInterceptor.scope.logger( + 'interceptor identified, starting mocking' + ) // wait to emit the finish event until we know for sure an Interceptor is going to playback. // otherwise an unmocked request might emit finish twice. diff --git a/lib/interceptor.js b/lib/interceptor.js index 202c43bb3..a2af6bc16 100644 --- a/lib/interceptor.js +++ b/lib/interceptor.js @@ -1,6 +1,5 @@ 'use strict' -const debug = require('debug')('nock.interceptor') const stringify = require('json-stringify-safe') const querystring = require('querystring') const { URL, URLSearchParams } = require('url') @@ -180,8 +179,8 @@ module.exports = class Interceptor { } } - debug('reply.headers:', this.headers) - debug('reply.rawHeaders:', this.rawHeaders) + this.scope.logger('reply.headers:', this.headers) + this.scope.logger('reply.rawHeaders:', this.rawHeaders) this.body = body @@ -227,13 +226,23 @@ module.exports = class Interceptor { } } - debug("request header field doesn't match:", key, header, reqHeader) + this.scope.logger( + "request header field doesn't match:", + key, + header, + reqHeader + ) return false } match(req, options, body) { - if (debug.enabled) { - debug('match %s, body = %s', stringify(options), stringify(body)) + // check if the logger is enabled because the stringifies can be expensive. + if (this.scope.logger.enabled) { + this.scope.logger( + 'attempting match %s, body = %s', + stringify(options), + stringify(body) + ) } const method = (options.method || 'GET').toUpperCase() @@ -243,7 +252,7 @@ module.exports = class Interceptor { const { proto } = options if (this.method !== method) { - debug( + this.scope.logger( `Method did not match. Request ${method} Interceptor ${this.method}` ) return false @@ -275,6 +284,7 @@ module.exports = class Interceptor { ) if (!reqHeadersMatch) { + this.scope.logger("headers don't match") return false } @@ -282,26 +292,32 @@ module.exports = class Interceptor { this.scope.scopeOptions.conditionally && !this.scope.scopeOptions.conditionally() ) { + this.scope.logger( + 'matching failed because Scope.conditionally() did not validate' + ) return false } - const reqContainsBadHeaders = this.badheaders.some( + const badHeaders = this.badheaders.filter( header => header in options.headers ) - if (reqContainsBadHeaders) { + if (badHeaders.length) { + this.scope.logger('request contains bad headers', ...badHeaders) return false } // Match query strings when using query() if (this.queries === null) { - debug('query matching skipped') + this.scope.logger('query matching skipped') } else { // can't rely on pathname or search being in the options, but path has a default const [pathname, search] = path.split('?') const matchQueries = this.matchQuery({ search }) - debug(matchQueries ? 'query matching succeeded' : 'query matching failed') + this.scope.logger( + matchQueries ? 'query matching succeeded' : 'query matching failed' + ) if (!matchQueries) { return false @@ -404,8 +420,8 @@ module.exports = class Interceptor { } const reqQueries = querystring.parse(options.search) - debug('Interceptor queries: %j', this.queries) - debug(' Request queries: %j', reqQueries) + this.scope.logger('Interceptor queries: %j', this.queries) + this.scope.logger(' Request queries: %j', reqQueries) if (typeof this.queries === 'function') { return this.queries(reqQueries) diff --git a/lib/playback_interceptor.js b/lib/playback_interceptor.js index 875954589..df105ef19 100644 --- a/lib/playback_interceptor.js +++ b/lib/playback_interceptor.js @@ -83,6 +83,8 @@ function playbackInterceptor({ response, interceptor, }) { + const { logger } = interceptor.scope + function emitError(error) { process.nextTick(() => { req.emit('error', error) @@ -114,7 +116,7 @@ function playbackInterceptor({ // Clone headers/rawHeaders to not override them when evaluating later response.rawHeaders = [...interceptor.rawHeaders] - debug('response.rawHeaders:', response.rawHeaders) + logger('response.rawHeaders:', response.rawHeaders) if (interceptor.replyFunction) { const parsedRequestBody = parseJSONRequestBody(req, requestBodyString) @@ -213,10 +215,10 @@ function playbackInterceptor({ // Transform the response body if it exists (it may not exist // if we have `responseBuffers` instead) if (responseBody !== undefined) { - debug('transform the response body') + logger('transform the response body') if (interceptor.delayInMs) { - debug( + logger( 'delaying the response for', interceptor.delayInMs, 'milliseconds' @@ -230,7 +232,7 @@ function playbackInterceptor({ } if (common.isStream(responseBody)) { - debug('response body is a stream') + logger('response body is a stream') responseBody.pause() responseBody.on('data', function (d) { response.push(d) @@ -291,16 +293,16 @@ function playbackInterceptor({ req.res = response response.req = req - debug('emitting response') + logger('emitting response') req.emit('response', response) if (common.isStream(responseBody)) { - debug('resuming response stream') + logger('resuming response stream') responseBody.resume() } else { responseBuffers = responseBuffers || [] if (typeof responseBody !== 'undefined') { - debug('adding body to buffer list') + logger('adding body to buffer list') responseBuffers.push(responseBody) } @@ -309,11 +311,11 @@ function playbackInterceptor({ const chunk = responseBuffers.shift() if (chunk) { - debug('emitting response chunk') + logger('emitting response chunk') response.push(chunk) common.setImmediate(emitChunk) } else { - debug('ending response stream') + logger('ending response stream') response.push(null) // https://nodejs.org/dist/latest-v10.x/docs/api/http.html#http_message_complete response.complete = true diff --git a/lib/scope.js b/lib/scope.js index 91ad43039..d1fb314dd 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -39,7 +39,6 @@ class Scope extends EventEmitter { this.transformPathFunction = null this.transformRequestBodyFunction = null this.matchHeaders = [] - this.logger = debug this.scopeOptions = options || {} this.urlParts = {} this._persist = false @@ -50,13 +49,18 @@ class Scope extends EventEmitter { this.port = null this._defaultReplyHeaders = [] + let logNamespace = String(basePath) + if (!(basePath instanceof RegExp)) { this.urlParts = url.parse(basePath) this.port = this.urlParts.port || (this.urlParts.protocol === 'http:' ? 80 : 443) this.basePathname = this.urlParts.pathname.replace(/\/$/, '') this.basePath = `${this.urlParts.protocol}//${this.urlParts.hostname}:${this.port}` + logNamespace = this.urlParts.host } + + this.logger = debug.extend(logNamespace) } add(key, interceptor) { @@ -218,11 +222,6 @@ class Scope extends EventEmitter { return this } - log(newLogger) { - this.logger = newLogger - return this - } - persist(flag = true) { if (typeof flag !== 'boolean') { throw new Error('Invalid arguments: argument should be a boolean') diff --git a/tests/test_logging.js b/tests/test_logging.js index fe42ca8b4..f60001453 100644 --- a/tests/test_logging.js +++ b/tests/test_logging.js @@ -12,11 +12,10 @@ describe('Logging using the `debug` package', () => { let logFn beforeEach(() => { logFn = sinon.stub(debug, 'log') - debug.enable('nock.interceptor') + debug.enable('nock*') }) afterEach(() => { - sinon.restore() - debug.disable('nock.interceptor') + debug.disable('nock*') }) it('match debugging works', async () => { @@ -26,36 +25,39 @@ describe('Logging using the `debug` package', () => { await got.post('http://example.test/deep/link', { body: exampleBody }) const isMocha = process.argv.some(arg => arg.endsWith('mocha')) - // TODO For some reason this is getting slightly different arugments in Tap + // TODO For some reason this is getting slightly different arguments in Tap // vs Mocha. Remove this conditional when Tap is removed. if (isMocha) { + // the log function will have been a few dozen times, there are a few specific to matching we want to validate: + + // the log when an interceptor is chosen expect(logFn).to.have.been.calledWith( - sinon.match.string, + sinon.match('matched base path (1 interceptor)') + ) + + // the log of the Interceptor match + expect(logFn).to.have.been.calledWith( + // debug namespace for the scope that includes the host + sinon.match('nock.scope:example.test'), // This is a JSON blob which contains, among other things the complete // request URL. sinon.match('"href":"http://example.test/deep/link"'), // This is the JSON-stringified body. `"${exampleBody}"` ) - } - }) -}) - -describe('`log()`', () => { - it('should log host matching', async () => { - const logFn = sinon.spy() - - const scope = nock('http://example.test') - .get('/') - .reply(200, 'Hello, World!') - .log(logFn) - await got('http://example.test/') - - expect(logFn).to.have.been.calledOnceWithExactly( - 'matching http://example.test:80/ to GET http://example.test:80/: true' - ) + expect(logFn).to.have.been.calledWith( + sinon.match('query matching skipped') + ) - scope.done() + expect(logFn).to.have.been.calledWith( + sinon.match( + 'matching http://example.test:80/deep/link to POST http://example.test:80/deep/link: true' + ) + ) + expect(logFn).to.have.been.calledWith( + sinon.match('interceptor identified, starting mocking') + ) + } }) }) diff --git a/types/index.d.ts b/types/index.d.ts index 5d93ebc48..3632cf4bc 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -120,7 +120,6 @@ declare namespace nock { filteringRequestBody(regex: RegExp, replace: string): this filteringRequestBody(fn: (body: string) => string): this - log(out: (message: any, ...optionalParams: any[]) => void): this persist(flag?: boolean): this replyContentLength(): this replyDate(d?: Date): this diff --git a/types/tests.ts b/types/tests.ts index a6886c0c9..5b98fbb48 100644 --- a/types/tests.ts +++ b/types/tests.ts @@ -120,7 +120,6 @@ scope = scope.filteringRequestBody((path: string) => { return str }) -scope = scope.log(() => {}) scope = scope.persist() scope = scope.persist(false) scope = scope.replyContentLength() @@ -619,7 +618,7 @@ options = { allowUnmocked: true } scope = nock('http://example.test', options).get('/my/url').reply(200, 'OK!') // Expectations -let google = nock('http://example.test') +const google = nock('http://example.test') .get('/') .reply(200, 'Hello from Google!') setTimeout(() => { @@ -653,9 +652,6 @@ console.error('pending mocks: %j', nock.pendingMocks()) nock.activeMocks() // $ExpectType string[] nock('http://example.test').activeMocks() // $ExpectType string[] -// Logging -google = nock('http://example.test').log(console.log) - // Restoring nock.restore() From 5f48d9fe6b8841519dbc42b80e123a2480de9259 Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Sun, 5 Apr 2020 21:18:30 -0600 Subject: [PATCH 05/13] refactor(test): Mocha DSL for delays (#1967) --- tests/test_delay.js | 447 +++++++++++++++++++++----------------------- 1 file changed, 218 insertions(+), 229 deletions(-) diff --git a/tests/test_delay.js b/tests/test_delay.js index 4f86979f5..5155eb2e2 100644 --- a/tests/test_delay.js +++ b/tests/test_delay.js @@ -7,11 +7,9 @@ const http = require('http') const stream = require('stream') const assertRejects = require('assert-rejects') const sinon = require('sinon') -const { test } = require('tap') const nock = require('..') const got = require('./got_client') -require('./cleanup_after_each')() require('./setup') const textFilePath = path.resolve(__dirname, './assets/reply_file_1.txt') @@ -40,289 +38,280 @@ function checkDuration(start, durationMillis) { // .and.at.most(durationMillis + bufferMillis, 'delay upper bound exceeded') } -test('calling delay could cause timeout error', async () => { - const scope = nock('http://example.test') - .get('/') - .delay({ - head: 300, - }) - .reply(200, 'OK') +describe('`delay()`', () => { + let interceptor + let connSpy + let bodySpy + + // As a rule, the tests in this repo have a strategy of only testing the API and not spying on + // internals for unit tests. These next few tests break that rule to assert the proxy behavior of + // `delay()`. This is simply to reduce the need of double testing the behavior of `delayBody()` + // and `delayConnection()` and should not be used as an example for writing new tests. + beforeEach(() => { + interceptor = nock('http://example.test').get('/') + connSpy = sinon.spy(interceptor, 'delayConnection') + bodySpy = sinon.spy(interceptor, 'delayBody') + }) - await assertRejects( - got('http://example.test', { timeout: 100 }), - err => err.code === 'ETIMEDOUT' - ) + it('should proxy a single number argument', () => { + interceptor.delay(42) - scope.done() -}) + expect(connSpy).to.have.been.calledOnceWithExactly(42) + expect(bodySpy).to.have.been.calledOnceWithExactly(0) + }) -test('Body delay does not have impact on timeout', async () => { - const scope = nock('http://example.test') - .get('/') - .delay({ - head: 300, - body: 300, - }) - .reply(201, 'OK') + it('should proxy values from an object argument', () => { + interceptor.delay({ head: 42, body: 17 }) - const { body, statusCode } = await got('http://example.test/', { - timeout: { - response: 500, - }, + expect(connSpy).to.have.been.calledOnceWithExactly(42) + expect(bodySpy).to.have.been.calledOnceWithExactly(17) }) - expect(statusCode).to.equal(201) - expect(body).to.equal('OK') - scope.done() -}) + it('should default missing values from an object argument', () => { + interceptor.delay({}) -test('calling delay with "body" and "head" delays the response', t => { - nock('http://example.test') - .get('/') - .delay({ - head: 200, - body: 300, - }) - .reply(200, 'OK') - - const resStart = process.hrtime() + expect(connSpy).to.have.been.calledOnceWithExactly(0) + expect(bodySpy).to.have.been.calledOnceWithExactly(0) + }) - http.get('http://example.test', res => { - checkDuration(resStart, 200) + it('should throw on invalid arguments', () => { + expect(() => interceptor.delay('one million seconds')).to.throw( + 'Unexpected input' + ) + }) - // const dataStart = process.hrtime() - res.once('data', function (data) { - // TODO: there is a bug in Nock that allows this delay to be less than the desired 300ms. - // there is a known issues with streams, but this needs further investigation. - // checkDuration(dataStart, 300) - expect(data.toString()).to.equal('OK') - res.once('end', () => t.done()) + it('should delay the response when called with "body" and "head"', done => { + nock('http://example.test') + .get('/') + .delay({ + head: 200, + body: 300, + }) + .reply(200, 'OK') + + const start = process.hrtime() + + http.get('http://example.test', res => { + checkDuration(start, 200) + + res.once('data', function (data) { + checkDuration(start, 500) + expect(data.toString()).to.equal('OK') + res.once('end', done) + }) }) }) }) -test('calling delay with "body" delays the response body', async () => { - const scope = nock('http://example.test') - .get('/') - .delay({ body: 200 }) - .reply(200, 'OK') - - const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) - - expect(body).to.equal('OK') - scope.done() -}) - -test('calling delayBody delays the response', async () => { - const scope = nock('http://example.test') - .get('/') - .delayBody(200) - .reply(200, 'OK') - - const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) - - expect(body).to.equal('OK') - scope.done() -}) - -test('delayBody works with a stream', async () => { - const scope = nock('http://example.test') - .get('/') - .delayBody(200) - .reply(200, () => fs.createReadStream(textFilePath, { encoding: 'utf8' })) - - const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) +describe('`delayBody()`', () => { + it('should delay the clock between the `response` event and the first `data` event', done => { + nock('http://example.test').get('/').delayBody(200).reply(201, 'OK') + + // It would be preferable to get this start time inside the response callback, however, the delay starts just before + // the response event is emitted and the amount of time it takes to enter the callback varies wildly in the CI. + const start = process.hrtime() + http.get('http://example.test', res => { + res.once('data', () => { + checkDuration(start, 200) + done() + }) + }) + }) - expect(body).to.equal(textFileContents) - scope.done() -}) + it('should delay the overall response', async () => { + const scope = nock('http://example.test') + .get('/') + .delayBody(200) + .reply(200, 'OK') -test('delayBody works with a stream of binary buffers', async () => { - const scope = nock('http://example.test') - .get('/') - .delayBody(200) - // No encoding specified, which causes the file to be streamed using - // buffers instead of strings. - .reply(200, () => fs.createReadStream(textFilePath)) + const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) - const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) + expect(body).to.equal('OK') + scope.done() + }) - expect(body).to.equal(textFileContents) - scope.done() -}) + it('should not have an impact on a response timeout', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(300) + .delayBody(300) + .reply(201, 'OK') + + const { body, statusCode } = await got('http://example.test/', { + timeout: { + response: 500, + }, + }) -test('delayBody works with a delayed stream', async () => { - const passthrough = new stream.Transform({ - transform(chunk, encoding, callback) { - this.push(chunk.toString()) - callback() - }, + expect(statusCode).to.equal(201) + expect(body).to.equal('OK') + scope.done() }) - const scope = nock('http://example.test') - .get('/') - .delayBody(100) - .reply(200, () => passthrough) + it('should work with a response stream', async () => { + const scope = nock('http://example.test') + .get('/') + .delayBody(200) + .reply(200, () => fs.createReadStream(textFilePath, { encoding: 'utf8' })) - setTimeout(() => fs.createReadStream(textFilePath).pipe(passthrough), 125) + const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) - const { body } = await got('http://example.test/') + expect(body).to.equal(textFileContents) + scope.done() + }) - expect(body).to.equal(textFileContents) - scope.done() -}) + it('should work with a response stream of binary buffers', async () => { + const scope = nock('http://example.test') + .get('/') + .delayBody(200) + // No encoding specified, which causes the file to be streamed using + // buffers instead of strings. + .reply(200, () => fs.createReadStream(textFilePath)) -test('calling delay delays the response', async () => { - const scope = nock('http://example.test').get('/').delay(200).reply(200, 'OK') + const { body } = await resolvesInAtLeast(got('http://example.test/'), 200) - const { body } = await resolvesInAtLeast(got('http://example.test'), 200) + expect(body).to.equal(textFileContents) + scope.done() + }) - expect(body).to.equal('OK') - scope.done() -}) + it('should work with a delayed response stream', async () => { + const passthrough = new stream.Transform({ + transform(chunk, encoding, callback) { + this.push(chunk.toString()) + callback() + }, + }) -test('using reply callback with delay provides proper arguments', async () => { - const replyStub = sinon.stub().returns('') + const scope = nock('http://example.test') + .get('/') + .delayBody(100) + .reply(200, () => passthrough) - const scope = nock('http://example.test') - .post('/') - .delay(100) - .reply(200, replyStub) + setTimeout(() => fs.createReadStream(textFilePath).pipe(passthrough), 125) - await got.post('http://example.test', { body: 'OK' }) + const { body } = await got('http://example.test/') - expect(replyStub).to.have.been.calledOnceWithExactly('/', 'OK') - scope.done() + expect(body).to.equal(textFileContents) + scope.done() + }) }) -test('using reply callback with delay can reply JSON', async () => { - const scope = nock('http://example.test') - .get('/') - .delay(100) - .reply(200, () => ({ a: 1 })) - - const { body, headers, statusCode } = await got('http://example.test') +describe('`delayConnection()`', () => { + it('should cause a timeout error when larger than options.timeout', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(1000) + .reply(200, {}) - expect(body).to.equal('{"a":1}') - expect(headers).to.have.property('content-type', 'application/json') - expect(statusCode).to.equal(200) - scope.done() -}) - -test('delay with invalid arguments', t => { - const interceptor = nock('http://example.test').get('/') + await assertRejects( + got('http://example.test', { timeout: 10 }), + err => err.code === 'ETIMEDOUT' + ) - expect(() => interceptor.delay('one million seconds')).to.throw( - 'Unexpected input' - ) - t.end() -}) + scope.done() + }) -test('delay works with replyWithFile', async () => { - const scope = nock('http://example.test') - .get('/') - .delay(200) - .replyWithFile(200, textFilePath) + it('should delay the clock before the `response` event', done => { + nock('http://example.test').get('/').delayConnection(200).reply() - const { body, statusCode } = await resolvesInAtLeast( - got('http://example.test'), - 200 - ) + const req = http.request('http://example.test', () => { + checkDuration(start, 200) + done() + }) - expect(statusCode).to.equal(200) - expect(body).to.equal(textFileContents) - scope.done() -}) + req.end() + const start = process.hrtime() + }) -test('delay works with when you return a generic stream from the reply callback', async () => { - const scope = nock('http://example.test') - .get('/') - .delay(200) - .reply(200, () => fs.createReadStream(textFilePath)) + it('should delay the overall response', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(200) + .reply(200, 'OK') - const { body, statusCode } = await resolvesInAtLeast( - got('http://example.test'), - 200 - ) + const { body } = await resolvesInAtLeast(got('http://example.test'), 200) - expect(statusCode).to.equal(200) - expect(body).to.equal(textFileContents) - scope.done() -}) + expect(body).to.equal('OK') + scope.done() + }) -test('delay with replyWithError: response is delayed', async () => { - nock('http://example.test') - .get('/') - .delay(100) - .replyWithError('this is an error message') + it('should provide the proper arguments when using reply a callback', async () => { + const replyStub = sinon.stub().returns('') - await resolvesInAtLeast( - assertRejects(got('http://example.test'), /this is an error message/), - 100 - ) -}) + const scope = nock('http://example.test') + .post('/') + .delayConnection(100) + .reply(200, replyStub) -test('calling delayConnection delays the connection', async () => { - const scope = nock('http://example.test') - .get('/') - .delayConnection(200) - .reply(200, 'OK') + await got.post('http://example.test', { body: 'OK' }) - const { body } = await resolvesInAtLeast(got('http://example.test'), 200) + expect(replyStub).to.have.been.calledOnceWithExactly('/', 'OK') + scope.done() + }) - expect(body).to.equal('OK') - scope.done() -}) + it('should delay a JSON response when using a reply callback', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(100) + .reply(200, () => ({ a: 1 })) -test('using reply callback with delayConnection provides proper arguments', async () => { - const replyStub = sinon.stub().returns('') + const { body, headers, statusCode } = await got('http://example.test') - const scope = nock('http://example.test') - .post('/') - .delayConnection(100) - .reply(200, replyStub) + expect(body).to.equal('{"a":1}') + expect(headers).to.have.property('content-type', 'application/json') + expect(statusCode).to.equal(200) + scope.done() + }) - await got.post('http://example.test', { body: 'OK' }) + it('should work with `replyWithFile()`', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(200) + .replyWithFile(200, textFilePath) - expect(replyStub).to.have.been.calledOnceWithExactly('/', 'OK') - scope.done() -}) + const { body } = await resolvesInAtLeast(got('http://example.test'), 200) -test('delayConnection works with replyWithFile', async () => { - const scope = nock('http://example.test') - .get('/') - .delayConnection(200) - .replyWithFile(200, textFilePath) + expect(body).to.equal(textFileContents) + scope.done() + }) - const { body } = await resolvesInAtLeast(got('http://example.test'), 200) + it('should work with a generic stream from the reply callback', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(200) + .reply(200, () => fs.createReadStream(textFilePath)) - expect(body).to.equal(textFileContents) - scope.done() -}) + const { body } = await resolvesInAtLeast(got('http://example.test'), 200) -test('delayConnection works with when you return a generic stream from the reply callback', async () => { - const scope = nock('http://example.test') - .get('/') - .delayConnection(200) - .reply(200, () => fs.createReadStream(textFilePath)) + expect(body).to.equal(textFileContents) + scope.done() + }) - const { body } = await resolvesInAtLeast(got('http://example.test'), 200) + it('should work with a generic stream from the reply callback', async () => { + const scope = nock('http://example.test') + .get('/') + .delayConnection(200) + .reply(200, () => fs.createReadStream(textFilePath)) - expect(body).to.equal(textFileContents) - scope.done() -}) + const { body, statusCode } = await resolvesInAtLeast( + got('http://example.test'), + 200 + ) -test('request with delayConnection and request.timeout', async t => { - const scope = nock('http://example.test') - .get('/') - .delayConnection(1000) - .reply(200, {}) + expect(statusCode).to.equal(200) + expect(body).to.equal(textFileContents) + scope.done() + }) - await assertRejects( - got('http://example.test', { timeout: 10 }), - err => err.code === 'ETIMEDOUT' - ) + it('should delay errors when `replyWithError()` is used', async () => { + nock('http://example.test') + .get('/') + .delayConnection(100) + .replyWithError('this is an error message') - scope.done() - t.done() + await resolvesInAtLeast( + assertRejects(got('http://example.test'), /this is an error message/), + 100 + ) + }) }) From 2fc916acb06eaf18de37562cb6bf3cfe3b845f1c Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Tue, 14 Apr 2020 20:09:53 -0600 Subject: [PATCH 06/13] refactor(playback): convert all response bodies to streams (#1973) Consolidating all the possible response body objects into a stream has several advantages. - Reduces the code complexity before and after `respond` is called. - Reduces the code complexity around body delays. The need for the `DelayedBody` class is eliminated and a simple timeout can be used before resuming the steam. - The `replied` event is emitted when the response body is provided as a stream. - Delaying content-encoded response bodies is now possible. - The timer on delayed bodies now starts after the `response` event listeners are processed. BREAKING CHANGES: - The third argument passed to reply header callbacks is now the original response body. Previously, string bodies were passed to the callbacks as Buffers. - Interceptors are now marked as consumed _before_ the `request` event is emitted. --- lib/delayed_body.js | 52 -------- lib/interceptor.js | 9 +- lib/playback_interceptor.js | 215 ++++++++++++++------------------- tests/test_abort.js | 32 +++++ tests/test_content_encoding.js | 14 +-- tests/test_delay.js | 4 +- tests/test_events.js | 22 +++- tests/test_reply_headers.js | 3 +- 8 files changed, 161 insertions(+), 190 deletions(-) delete mode 100644 lib/delayed_body.js diff --git a/lib/delayed_body.js b/lib/delayed_body.js deleted file mode 100644 index 20359dd11..000000000 --- a/lib/delayed_body.js +++ /dev/null @@ -1,52 +0,0 @@ -'use strict' - -/** - * Creates a stream which becomes the response body of the interceptor when a - * delay is set. The stream outputs the intended body and EOF after the delay. - * - * @param {String|Buffer|Stream} body - the body to write/pipe out - * @param {Integer} ms - The delay in milliseconds - * @constructor - */ - -const { Transform } = require('stream') -const common = require('./common') - -module.exports = class DelayedBody extends Transform { - constructor(ms, body) { - super() - - const self = this - let data = '' - let ended = false - - if (common.isStream(body)) { - body.on('data', function (chunk) { - data += Buffer.isBuffer(chunk) ? chunk.toString() : chunk - }) - - body.once('end', function () { - ended = true - }) - - body.resume() - } - - // TODO: This would be more readable if the stream case were moved into - // the `if` statement above. - common.setTimeout(function () { - if (common.isStream(body) && !ended) { - body.once('end', function () { - self.end(data) - }) - } else { - self.end(data || body) - } - }, ms) - } - - _transform(chunk, encoding, cb) { - this.push(chunk) - process.nextTick(cb) - } -} diff --git a/lib/interceptor.js b/lib/interceptor.js index a2af6bc16..180b16a1e 100644 --- a/lib/interceptor.js +++ b/lib/interceptor.js @@ -71,8 +71,9 @@ module.exports = class Interceptor { scope.scopeOptions.badheaders || [] ) - this.delayInMs = 0 + this.delayBodyInMs = 0 this.delayConnectionInMs = 0 + this.delaySocketInMs = 0 this.optional = false @@ -598,7 +599,7 @@ module.exports = class Interceptor { * @return {Interceptor} - the current interceptor for chaining */ delayBody(ms) { - this.delayInMs += ms + this.delayBodyInMs += ms return this } @@ -618,7 +619,7 @@ module.exports = class Interceptor { * @returns {number} */ getTotalDelay() { - return this.delayInMs + this.delayConnectionInMs + return this.delayBodyInMs + this.delayConnectionInMs } /** @@ -628,7 +629,7 @@ module.exports = class Interceptor { * @return {Interceptor} - the current interceptor for chaining */ socketDelay(ms) { - this.socketDelayInMs = ms + this.delaySocketInMs = ms return this } } diff --git a/lib/playback_interceptor.js b/lib/playback_interceptor.js index df105ef19..46161f3a9 100644 --- a/lib/playback_interceptor.js +++ b/lib/playback_interceptor.js @@ -1,10 +1,10 @@ 'use strict' +const stream = require('stream') const util = require('util') const zlib = require('zlib') const debug = require('debug')('nock.playback_interceptor') const common = require('./common') -const DelayedBody = require('./delayed_body') function parseJSONRequestBody(req, requestBody) { if (!requestBody || !common.isJSONContent(req.headers)) { @@ -71,6 +71,44 @@ function selectDefaultHeaders(existingHeaders, defaultHeaders) { return result } +// Presents a list of Buffers as a Readable +class ReadableBuffers extends stream.Readable { + constructor(buffers, opts = {}) { + super(opts) + + this.buffers = buffers + } + + _read(size) { + while (this.buffers.length) { + if (!this.push(this.buffers.shift())) { + return + } + } + this.push(null) + } +} + +function convertBodyToStream(body) { + if (common.isStream(body)) { + return body + } + + if (body === undefined) { + return new ReadableBuffers([]) + } + + if (Buffer.isBuffer(body)) { + return new ReadableBuffers([body]) + } + + if (typeof body !== 'string') { + body = JSON.stringify(body) + } + + return new ReadableBuffers([Buffer.from(body)]) +} + /** * Play back an interceptor using the given request and mock response. */ @@ -92,6 +130,7 @@ function playbackInterceptor({ } function start() { + interceptor.markConsumed() interceptor.req = req req.headers = req.getHeaders() @@ -130,8 +169,8 @@ function playbackInterceptor({ // At this point `fn` is either a synchronous function or a promise-returning function; // wrapping in `Promise.resolve` makes it into a promise either way. Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody)) - .then(responseBody => continueWithResponseBody({ responseBody })) - .catch(err => emitError(err)) + .then(continueWithResponseBody) + .catch(emitError) return } @@ -144,8 +183,8 @@ function playbackInterceptor({ } Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody)) - .then(fullReplyResult => continueWithFullResponse({ fullReplyResult })) - .catch(err => emitError(err)) + .then(continueWithFullResponse) + .catch(emitError) return } @@ -157,21 +196,12 @@ function playbackInterceptor({ // of response buffers which should be mocked one by one. // (otherwise decompressions after the first one fails as unzip expects to receive // buffer by buffer and not one single merged buffer) - - if (interceptor.delayInMs) { - emitError( - new Error( - 'Response delay of the body is currently not supported with content-encoded responses.' - ) - ) - return - } - const bufferData = Array.isArray(interceptor.body) ? interceptor.body : [interceptor.body] const responseBuffers = bufferData.map(data => Buffer.from(data, 'hex')) - continueWithResponseBody({ responseBuffers }) + const responseBody = new ReadableBuffers(responseBuffers) + continueWithResponseBody(responseBody) return } @@ -196,92 +226,73 @@ function playbackInterceptor({ } } - return continueWithResponseBody({ responseBody }) + return continueWithResponseBody(responseBody) } - function continueWithFullResponse({ fullReplyResult }) { + function continueWithFullResponse(fullReplyResult) { let responseBody try { responseBody = parseFullReplyResult(response, fullReplyResult) - } catch (innerErr) { - emitError(innerErr) + } catch (err) { + emitError(err) return } - continueWithResponseBody({ responseBody }) + continueWithResponseBody(responseBody) } - function continueWithResponseBody({ responseBuffers, responseBody }) { - // Transform the response body if it exists (it may not exist - // if we have `responseBuffers` instead) - if (responseBody !== undefined) { - logger('transform the response body') - - if (interceptor.delayInMs) { - logger( - 'delaying the response for', - interceptor.delayInMs, - 'milliseconds' - ) - // Because setTimeout is called immediately in DelayedBody(), so we - // need count in the delayConnectionInMs. - responseBody = new DelayedBody( - interceptor.getTotalDelay(), - responseBody - ) - } + function prepareResponseHeaders(body) { + const defaultHeaders = [...interceptor.scope._defaultReplyHeaders] - if (common.isStream(responseBody)) { - logger('response body is a stream') - responseBody.pause() - responseBody.on('data', function (d) { - response.push(d) - }) - responseBody.on('end', function () { - response.push(null) - // https://nodejs.org/dist/latest-v10.x/docs/api/http.html#http_message_complete - response.complete = true - }) - responseBody.on('error', function (err) { - response.emit('error', err) - }) - } else if (!Buffer.isBuffer(responseBody)) { - if (typeof responseBody === 'string') { - responseBody = Buffer.from(responseBody) - } else { - responseBody = JSON.stringify(responseBody) - response.rawHeaders.push('Content-Type', 'application/json') - } - } - // Why are strings converted to a Buffer, but JSON data is left as a string? - // Related to https://github.com/nock/nock/issues/1542 ? - } + // Include a JSON content type when JSON.stringify is called on the body. + // This is a convenience added by Nock that has no analog in Node. It's added to the + // defaults, so it will be ignored if the caller explicitly provided the header already. + const isJSON = + body !== undefined && + typeof body !== 'string' && + !Buffer.isBuffer(body) && + !common.isStream(body) - interceptor.markConsumed() + if (isJSON) { + defaultHeaders.push('Content-Type', 'application/json') + } response.rawHeaders.push( - ...selectDefaultHeaders( - response.rawHeaders, - interceptor.scope._defaultReplyHeaders - ) + ...selectDefaultHeaders(response.rawHeaders, defaultHeaders) ) // Evaluate functional headers. common.forEachHeader(response.rawHeaders, (value, fieldName, i) => { if (typeof value === 'function') { - response.rawHeaders[i + 1] = value(req, response, responseBody) + response.rawHeaders[i + 1] = value(req, response, body) } }) response.headers = common.headersArrayToObject(response.rawHeaders) + } + + function continueWithResponseBody(rawBody) { + prepareResponseHeaders(rawBody) + const bodyAsStream = convertBodyToStream(rawBody) + bodyAsStream.pause() - respondUsingInterceptor({ - responseBody, - responseBuffers, + // IncomingMessage extends Readable so we can't simply pipe. + bodyAsStream.on('data', function (chunk) { + response.push(chunk) }) - } + bodyAsStream.on('end', function () { + // https://nodejs.org/dist/latest-v10.x/docs/api/http.html#http_message_complete + response.complete = true + response.push(null) + + interceptor.scope.emit('replied', req, interceptor) + }) + bodyAsStream.on('error', function (err) { + response.emit('error', err) + }) + + const { delayBodyInMs, delayConnectionInMs, delaySocketInMs } = interceptor - function respondUsingInterceptor({ responseBody, responseBuffers }) { function respond() { if (req.aborted) { return @@ -296,55 +307,17 @@ function playbackInterceptor({ logger('emitting response') req.emit('response', response) - if (common.isStream(responseBody)) { - logger('resuming response stream') - responseBody.resume() - } else { - responseBuffers = responseBuffers || [] - if (typeof responseBody !== 'undefined') { - logger('adding body to buffer list') - responseBuffers.push(responseBody) - } - - // Stream the response chunks one at a time. - common.setImmediate(function emitChunk() { - const chunk = responseBuffers.shift() - - if (chunk) { - logger('emitting response chunk') - response.push(chunk) - common.setImmediate(emitChunk) - } else { - logger('ending response stream') - response.push(null) - // https://nodejs.org/dist/latest-v10.x/docs/api/http.html#http_message_complete - response.complete = true - interceptor.scope.emit('replied', req, interceptor) - } - }) - } - } - - if (interceptor.socketDelayInMs && interceptor.socketDelayInMs > 0) { - socket.applyDelay(interceptor.socketDelayInMs) + common.setTimeout(() => bodyAsStream.resume(), delayBodyInMs) } - if ( - interceptor.delayConnectionInMs && - interceptor.delayConnectionInMs > 0 - ) { - socket.applyDelay(interceptor.delayConnectionInMs) - common.setTimeout(respond, interceptor.delayConnectionInMs) - } else { - respond() - } + socket.applyDelay(delaySocketInMs + delayConnectionInMs) + common.setTimeout(respond, delayConnectionInMs) } - // If there are no delays configured on the Interceptor, calling `start` will - // take the request all the way to the 'response' event during a single microtask - // execution. This setImmediate stalls the playback to ensure the correct events - // are emitted first ('socket', 'finish') and any aborts in the in the queue - // or called during a 'finish' listener can be called. + // Calling `start` immediately could take the request all the way to the connection delay + // during a single microtask execution. This setImmediate stalls the playback to ensure the + // correct events are emitted first ('socket', 'finish') and any aborts in the in the queue or + // called during a 'finish' listener can be called. common.setImmediate(() => { if (!req.aborted) { start() diff --git a/tests/test_abort.js b/tests/test_abort.js index 8a19932a0..784448646 100644 --- a/tests/test_abort.js +++ b/tests/test_abort.js @@ -154,6 +154,38 @@ describe('`ClientRequest.abort()`', () => { // The Interceptor is considered consumed just prior to the `response` event on the request, // all tests below assert the Scope is done. + it('Emits the expected event sequence when aborted after a delay from the `finish` event', done => { + // use the delay functionality to create a window where the abort is called during the artificial connection wait. + const scope = nock('http://example.test').get('/').delay(100).reply() + + const req = http.request('http://example.test') + const emitSpy = sinon.spy(req, 'emit') + + req.on('finish', () => { + setTimeout(() => { + req.abort() + }, 10) + }) + req.on('error', err => { + expect(err.message).to.equal('socket hang up') + expect(err.code).to.equal('ECONNRESET') + }) + req.end() + + setTimeout(() => { + const events = emitSpy.args.map(i => i[0]) + expect(events).to.deep.equal([ + 'socket', + 'finish', + 'abort', + 'error', + 'close', + ]) + scope.done() + done() + }, 200) + }) + it('Emits the expected event sequence when aborted inside a `response` event listener', done => { const scope = nock('http://example.test').get('/').reply() diff --git a/tests/test_content_encoding.js b/tests/test_content_encoding.js index 5867ec2f7..1ce80fd4a 100644 --- a/tests/test_content_encoding.js +++ b/tests/test_content_encoding.js @@ -1,6 +1,5 @@ 'use strict' -const assertRejects = require('assert-rejects') const { expect } = require('chai') const zlib = require('zlib') const nock = require('..') @@ -27,11 +26,11 @@ describe('Content Encoding', () => { scope.done() }) - it('Delaying the body is not available with content encoded responses', async () => { + it('Delaying the body works with content encoded responses', async () => { const message = 'Lorem ipsum dolor sit amet' const compressed = zlib.gzipSync(message) - nock('http://example.test') + const scope = nock('http://example.test') .get('/') .delay({ body: 100, @@ -40,9 +39,10 @@ describe('Content Encoding', () => { 'Content-Encoding': 'gzip', }) - await assertRejects( - got('http://example.test/'), - /Response delay of the body is currently not supported with content-encoded responses/ - ) + const { statusCode, body } = await got('http://example.test/') + + expect(statusCode).to.equal(200) + expect(body).to.equal(message) + scope.done() }) }) diff --git a/tests/test_delay.js b/tests/test_delay.js index 5155eb2e2..c11c017a4 100644 --- a/tests/test_delay.js +++ b/tests/test_delay.js @@ -107,10 +107,8 @@ describe('`delayBody()`', () => { it('should delay the clock between the `response` event and the first `data` event', done => { nock('http://example.test').get('/').delayBody(200).reply(201, 'OK') - // It would be preferable to get this start time inside the response callback, however, the delay starts just before - // the response event is emitted and the amount of time it takes to enter the callback varies wildly in the CI. - const start = process.hrtime() http.get('http://example.test', res => { + const start = process.hrtime() res.once('data', () => { checkDuration(start, 200) done() diff --git a/tests/test_events.js b/tests/test_events.js index 6ee3380d9..74cc9d411 100644 --- a/tests/test_events.js +++ b/tests/test_events.js @@ -2,6 +2,7 @@ const { expect } = require('chai') const http = require('http') +const path = require('path') const sinon = require('sinon') const nock = require('..') @@ -38,7 +39,7 @@ it('emits request and request body', async () => { scope.on('request', function (req, interceptor, body) { onRequest() expect(req.path).to.equal('/please') - expect(interceptor.interceptionCounter).to.equal(0) + expect(interceptor.interceptionCounter).to.equal(1) expect(body).to.deep.equal(data) expect(onReplied).to.not.have.been.called() }) @@ -62,6 +63,25 @@ it('emits request and request body', async () => { expect(onReplied).to.have.been.calledOnce() }) +it('emits request and replied events when response body is a stream', async () => { + const textFilePath = path.resolve(__dirname, './assets/reply_file_1.txt') + const scope = nock('http://example.test') + .get('/') + .replyWithFile(200, textFilePath) + + const onRequest = sinon.spy() + const onReplied = sinon.spy() + + scope.on('request', onRequest) + scope.on('replied', onReplied) + + await got('http://example.test') + + scope.done() + expect(onRequest).to.have.been.calledOnce() + expect(onReplied).to.have.been.calledOnce() +}) + it('emits no match when no match and no mock', done => { nock.emitter.once('no match', () => { done() diff --git a/tests/test_reply_headers.js b/tests/test_reply_headers.js index 59d283255..34f252987 100644 --- a/tests/test_reply_headers.js +++ b/tests/test_reply_headers.js @@ -235,8 +235,7 @@ describe('`reply()` headers', () => { myHeaderFnCalled() expect(req).to.be.an.instanceof(OverriddenClientRequest) expect(res).to.be.an.instanceof(IncomingMessage) - expect(body).to.be.an.instanceof(Buffer) - expect(Buffer.from('boo!').equals(body)).to.be.true() + expect(body).to.equal('boo!') return 'gotcha' }, }) From ec75f645595773f5f04eae44984fc1e1a9288112 Mon Sep 17 00:00:00 2001 From: Paul Melnikow Date: Sat, 2 May 2020 00:25:19 -0400 Subject: [PATCH 07/13] refactor: Chai assertions in test_define (#1985) --- tests/test_define.js | 134 ++++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 72 deletions(-) diff --git a/tests/test_define.js b/tests/test_define.js index ea32bb15f..5b4242cf6 100644 --- a/tests/test_define.js +++ b/tests/test_define.js @@ -1,15 +1,17 @@ 'use strict' const http = require('http') +const { expect } = require('chai') const assertRejects = require('assert-rejects') const { test } = require('tap') const nock = require('..') const got = require('./got_client') +require('./setup') require('./cleanup_after_each')() test('define() is backward compatible', async t => { - t.ok( + expect( nock.define([ { scope: 'http://example.test', @@ -21,39 +23,35 @@ test('define() is backward compatible', async t => { reply: '500', }, ]) - ) + ).to.be.ok() await assertRejects( got('http://example.test:12345/'), ({ response: { statusCode } }) => { - t.is(statusCode, 500) + expect(statusCode).to.equal(500) return true } ) }) test('define() throws when reply is not a numeric string', t => { - t.throws( - () => - nock.define([ - { - scope: 'http://example.test:1451', - method: 'GET', - path: '/', - reply: 'frodo', - }, - ]), - { - message: '`reply`, when present, must be a numeric string', - } - ) + expect(() => + nock.define([ + { + scope: 'http://example.test:1451', + method: 'GET', + path: '/', + reply: 'frodo', + }, + ]) + ).to.throw('`reply`, when present, must be a numeric string') t.end() }) test('define() applies default status code when none is specified', async t => { const body = '�' - t.equal( + expect( nock.define([ { scope: 'http://example.test', @@ -62,19 +60,18 @@ test('define() applies default status code when none is specified', async t => { body, response: '�', }, - ]).length, - 1 - ) + ]) + ).to.have.lengthOf(1) const { statusCode } = await got.post('http://example.test/', { body }) - t.equal(statusCode, 200) + expect(statusCode).to.equal(200) }) test('define() works when scope and port are both specified', async t => { const body = 'Hello, world!' - t.ok( + expect( nock.define([ { scope: 'http://example.test:1451', @@ -85,52 +82,45 @@ test('define() works when scope and port are both specified', async t => { response: '�', }, ]) - ) + ).to.be.ok() const { statusCode } = await got.post('http://example.test:1451/', { body }) - t.equal(statusCode, 200) + expect(statusCode).to.equal(200) t.end() }) test('define() throws the expected error when scope and port conflict', t => { - t.throws( - () => - nock.define([ - { - scope: 'http://example.test:8080', - port: 5000, - method: 'POST', - path: '/', - body: 'Hello, world!', - response: '�', - }, - ]), - { - message: - 'Mismatched port numbers in scope and port properties of nock definition.', - } + expect(() => + nock.define([ + { + scope: 'http://example.test:8080', + port: 5000, + method: 'POST', + path: '/', + body: 'Hello, world!', + response: '�', + }, + ]) + ).to.throw( + 'Mismatched port numbers in scope and port properties of nock definition.' ) t.end() }) test('define() throws the expected error when method is missing', t => { - t.throws( - () => - nock.define([ - { - scope: 'http://example.test', - path: '/', - body: 'Hello, world!', - response: 'yo', - }, - ]), - { - message: 'Method is required', - } - ) + expect(() => + nock.define([ + { + scope: 'http://example.test', + path: '/', + body: 'Hello, world!', + response: 'yo', + }, + ]) + ).to.throw('Method is required') t.end() }) @@ -139,7 +129,7 @@ test('define() works with non-JSON responses', async t => { const exampleBody = '�' const exampleResponseBody = 'hey: �' - t.ok( + expect( nock.define([ { scope: 'http://example.test', @@ -150,16 +140,16 @@ test('define() works with non-JSON responses', async t => { response: exampleResponseBody, }, ]) - ) + ).to.be.ok() const { statusCode, body } = await got.post('http://example.test/', { body: exampleBody, responseType: 'buffer', }) - t.equal(statusCode, 200) - t.type(body, Buffer) - t.equal(body.toString(), exampleResponseBody) + expect(statusCode).to.equal(200) + expect(body).to.be.an.instanceOf(Buffer) + expect(body.toString()).to.equal(exampleResponseBody) }) // TODO: There seems to be a bug here. When testing via `got` with @@ -170,7 +160,7 @@ test('define() works with binary buffers', t => { const exampleBody = '8001' const exampleResponse = '8001' - t.ok( + expect( nock.define([ { scope: 'http://example.test', @@ -181,7 +171,7 @@ test('define() works with binary buffers', t => { response: exampleResponse, }, ]) - ) + ).to.be.ok() const req = http.request( { @@ -190,7 +180,7 @@ test('define() works with binary buffers', t => { path: '/', }, res => { - t.equal(res.statusCode, 200) + expect(res.statusCode).to.equal(200) const dataChunks = [] @@ -200,7 +190,7 @@ test('define() works with binary buffers', t => { res.once('end', () => { const response = Buffer.concat(dataChunks) - t.equal(response.toString('hex'), exampleResponse, 'responses match') + expect(response.toString('hex')).to.equal(exampleResponse) t.end() }) } @@ -208,7 +198,7 @@ test('define() works with binary buffers', t => { req.on('error', () => { // This should never happen. - t.fail('Error should never occur.') + expect.fail() t.end() }) @@ -224,7 +214,7 @@ test('define() uses reqheaders', t => { authorization: authHeader, } - t.ok( + expect( nock.define([ { scope: 'http://example.test', @@ -234,7 +224,7 @@ test('define() uses reqheaders', t => { reqheaders, }, ]) - ) + ).to.be.ok() // Make a request which should match the mock that was configured above. // This does not hit the network. @@ -246,10 +236,10 @@ test('define() uses reqheaders', t => { auth, }, res => { - t.equal(res.statusCode, 200) + expect(res.statusCode).to.equal(200) res.once('end', () => { - t.equivalent(res.req.getHeaders(), reqheaders) + expect(res.req.getHeaders(), reqheaders) t.end() }) // Streams start in 'paused' mode and must be started. @@ -261,7 +251,7 @@ test('define() uses reqheaders', t => { }) test('define() uses badheaders', t => { - t.ok( + expect( nock.define([ { scope: 'http://example.test', @@ -280,7 +270,7 @@ test('define() uses badheaders', t => { }, }, ]) - ) + ).to.be.ok() const req = http.request( { @@ -292,7 +282,7 @@ test('define() uses badheaders', t => { }, }, res => { - t.equal(res.statusCode, 200) + expect(res.statusCode).to.equal(200) res.once('end', () => { t.end() From 6a5a3cfcfad4f53fb9e47dd7939f7ff42c1f077f Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Sat, 2 May 2020 07:57:12 -0600 Subject: [PATCH 08/13] feat: remove socketDelay (#1974) From a user's perspective, having both `delayConnection` and `socketDelay` is confusing and unnecessarily. The differences between the two are subtle, and in my experience, users need to be fairly well versed in both the Node HTTP module and the internals of Nock in order to accurately determine which to use. On the surface there seems to be two use cases: - A user wants to test how their code handles a `timeout` event. Whether their client is expected to abort the request, or some other action is taken in response to the event. Either way, the user doesn't care if wall-clock time passes, and would prefer the timeout be simulated so their test suite can get on with it. - A user wants to force wall-clock time to pass before a `response` event. This is usually to test some timeout feature that is not based on the `timeout` event, or to give other code time to complete a task first. Based on those two use cases, it seems obvious that there should be two different delay functions, like we seem to have now. However, there are two subtle aspects that blur the line. - When a socket emits a `timeout` event, which the request propagates, the request does not end. This is true in Node and in Nock today. Clients my choose to abort a request upon a timeout event, they may not. - In Nock today, when the socket is "applying" the artificial timeout, to determine if it should emit a `timeout`, it doesn't just use the value passed to `socketDelay`. It uses the sum of the values passed to `delayConnection` and `socketDelay`. This covers the flow of a user wanting to ensure their code sees a `timeout` event even if no action is taken. Therefore, there is no reason to have two different options for users. The value passed to `delayConnection` can trigger a `timeout` event right away and if the code chooses not to act on the event, then it will wait in real time until `response` is triggered. In fact, when I began working on this, I went into the `test_socket_delay.js` file and replaced all `socketDelay` with `delayConnection`. All the tests passed. Other minor tweaks included in this change: - The delay methods on the Interceptor are now just setters instead of additive. This was undocumented, unintuitive behavior. - Fixed a bug from #1973, where `replayWithError` would cause Interceptors to be consumed twice. BREAKING CHANGE: - `socketDelay` has been removed. Use `delayConnection` instead. - `delay`, `delayConnection`, and `delayBody` are now setters instead of additive. example: ```js nock('http://example.com') .get('/') .delay(1) .delay({ head: 2, body: 3 }) .delayConnection(4) .delayBody(5) .delayBody(6) .reply() ``` Previously, the connection would have been delayed by 7 and the body delayed by 14. Now, the connection will be delayed by 4 and the body delayed by 6. --- README.md | 73 ++++++++----------- lib/intercept.js | 2 + lib/interceptor.js | 24 +------ lib/playback_interceptor.js | 10 +-- lib/socket.js | 22 +++--- tests/setup.js | 1 + tests/test_delay.js | 64 +++++++++++++++++ tests/test_nock_lifecycle.js | 19 +++++ tests/test_socket.js | 32 +++++++++ tests/test_socket_delay.js | 132 ----------------------------------- types/index.d.ts | 1 - types/tests.ts | 8 --- 12 files changed, 166 insertions(+), 222 deletions(-) create mode 100644 tests/test_socket.js delete mode 100644 tests/test_socket_delay.js diff --git a/README.md b/README.md index 046b02ec9..146950718 100644 --- a/README.md +++ b/README.md @@ -43,10 +43,7 @@ For instance, if a module performs HTTP requests to a CouchDB server or makes HT - [Support for HTTP and HTTPS](#support-for-http-and-https) - [Non-standard ports](#non-standard-ports) - [Repeat response n times](#repeat-response-n-times) - - [Delay the response body](#delay-the-response-body) - [Delay the response](#delay-the-response) - - [Delay the connection](#delay-the-connection) - - [Socket timeout](#socket-timeout) - [Chaining](#chaining) - [Scope filtering](#scope-filtering) - [Conditional scope filtering](#conditional-scope-filtering) @@ -660,22 +657,9 @@ nock('http://zombo.com').get('/').thrice().reply(200, 'Ok') To repeat this response for as long as nock is active, use [.persist()](#persist). -### Delay the response body - -You are able to specify the number of milliseconds that the response body should be delayed. Response header will be replied immediately. -`delayBody(1000)` is equivalent to `delay({body: 1000})`. - -```js -nock('http://my.server.com') - .get('/') - .delayBody(2000) // 2 seconds - .reply(200, '') -``` - -NOTE: the [`'response'`](http://nodejs.org/api/http.html#http_event_response) event will occur immediately, but the [IncomingMessage](http://nodejs.org/api/http.html#http_http_incomingmessage) will not emit its `'end'` event until after the delay. - ### Delay the response +Nock can simulate response latency to allow you to test timeouts, race conditions, an other timing related scenarios. You are able to specify the number of milliseconds that your reply should be delayed. ```js @@ -685,53 +669,54 @@ nock('http://my.server.com') .reply(200, '') ``` -`delay()` could also be used as +`delay(1000)` is an alias for `delayConnection(1000).delayBody(0)` +`delay({ head: 1000, body: 2000 })` is an alias for `delayConnection(1000).delayBody(2000)` +Both of which are covered in detail below. -``` -delay({ - head: headDelayInMs, - body: bodyDelayInMs -}) -``` +#### Delay the connection + +You are able to specify the number of milliseconds that your connection should be idle before it starts to receive the response. -for example +To simulate a socket timeout, provide a larger value than the timeout setting on the request. ```js nock('http://my.server.com') .get('/') - .delay({ - head: 2000, // header will be delayed for 2 seconds, i.e. the whole response will be delayed for 2 seconds. - body: 3000, // body will be delayed for another 3 seconds after header is sent out. - }) + .delayConnection(2000) // 2 seconds .reply(200, '') + +req = http.request('http://my.server.com', { timeout: 1000 }) ``` -### Delay the connection +Nock emits timeout events almost immediately by comparing the requested connection delay to the timeout parameter passed to `http.request()` or `http.ClientRequest#setTimeout()`. +This allows you to test timeouts without using fake timers or slowing down your tests. +If the client chooses to _not_ take an action (e.g. abort the request), the request and response will continue on as normal, after real clock time has passed. + +##### Technical Details + +Following the `'finish'` event being emitted by `ClientRequest`, Nock will wait for the next event loop iteration before checking if the request has been aborted. +At this point, any connection delay value is compared against any request timeout setting and a [`'timeout'`](https://nodejs.org/api/http.html#http_event_timeout) is emitted when appropriate from the socket and the request objects. +A Node timeout timer is then registered with any connection delay value to delay real time before checking again if the request has been aborted and the [`'response'`](http://nodejs.org/api/http.html#http_event_response) is emitted by the request. -`delayConnection(1000)` is equivalent to `delay({ head: 1000 })`. +A similar method, `.socketDelay()` was removed in version 13. It was thought that having two methods so subtlety similar was confusing. +The discussion can be found at https://github.com/nock/nock/pull/1974. -### Socket timeout +#### Delay the response body -You are able to specify the number of milliseconds that your connection should be idle, to simulate a socket timeout. +You are able to specify the number of milliseconds that the response body should be delayed. +This is the time between the headers being received and the body starting to be received. ```js nock('http://my.server.com') .get('/') - .socketDelay(2000) // 2 seconds + .delayBody(2000) // 2 seconds .reply(200, '') ``` -To test a request like the following: - -```js -req = http.request('http://my.server.com', res => { - ... -}) -req.setTimeout(1000, () => { req.abort() }) -req.end() -``` +##### Technical Details -NOTE: the timeout will be fired immediately, and will not leave the simulated connection idle for the specified period of time. +Following the [`'response'`](http://nodejs.org/api/http.html#http_event_response) being emitted by `ClientRequest`, +Nock will register a timeout timer with the body delay value to delay real time before the [IncomingMessage](http://nodejs.org/api/http.html#http_http_incomingmessage) emits its first `'data'` or the `'end'` event. ### Chaining diff --git a/lib/intercept.js b/lib/intercept.js index c07fd57c5..a95eb514c 100644 --- a/lib/intercept.js +++ b/lib/intercept.js @@ -281,6 +281,8 @@ function overrideClientRequest() { debug('using', interceptors.length, 'interceptors') // Use filtered interceptors to intercept requests. + // TODO: this shouldn't be a class anymore + // the overrider explicitly overrides methods and attrs on the request so the `assign` below should be removed. const overrider = new InterceptedRequestRouter({ req: this, options, diff --git a/lib/interceptor.js b/lib/interceptor.js index 180b16a1e..0a76bba42 100644 --- a/lib/interceptor.js +++ b/lib/interceptor.js @@ -73,7 +73,6 @@ module.exports = class Interceptor { this.delayBodyInMs = 0 this.delayConnectionInMs = 0 - this.delaySocketInMs = 0 this.optional = false @@ -599,7 +598,7 @@ module.exports = class Interceptor { * @return {Interceptor} - the current interceptor for chaining */ delayBody(ms) { - this.delayBodyInMs += ms + this.delayBodyInMs = ms return this } @@ -610,26 +609,7 @@ module.exports = class Interceptor { * @return {Interceptor} - the current interceptor for chaining */ delayConnection(ms) { - this.delayConnectionInMs += ms - return this - } - - /** - * @private - * @returns {number} - */ - getTotalDelay() { - return this.delayBodyInMs + this.delayConnectionInMs - } - - /** - * Make the socket idle for a certain number of ms (simulated). - * - * @param {integer} ms - Number of milliseconds to wait - * @return {Interceptor} - the current interceptor for chaining - */ - socketDelay(ms) { - this.delaySocketInMs = ms + this.delayConnectionInMs = ms return this } } diff --git a/lib/playback_interceptor.js b/lib/playback_interceptor.js index 46161f3a9..fda1e7052 100644 --- a/lib/playback_interceptor.js +++ b/lib/playback_interceptor.js @@ -137,15 +137,15 @@ function playbackInterceptor({ interceptor.scope.emit('request', req, interceptor, requestBodyString) if (typeof interceptor.errorMessage !== 'undefined') { - interceptor.markConsumed() - let error if (typeof interceptor.errorMessage === 'object') { error = interceptor.errorMessage } else { error = new Error(interceptor.errorMessage) } - common.setTimeout(() => emitError(error), interceptor.getTotalDelay()) + + const delay = interceptor.delayBodyInMs + interceptor.delayConnectionInMs + common.setTimeout(emitError, delay, error) return } @@ -291,7 +291,7 @@ function playbackInterceptor({ response.emit('error', err) }) - const { delayBodyInMs, delayConnectionInMs, delaySocketInMs } = interceptor + const { delayBodyInMs, delayConnectionInMs } = interceptor function respond() { if (req.aborted) { @@ -310,7 +310,7 @@ function playbackInterceptor({ common.setTimeout(() => bodyAsStream.resume(), delayBodyInMs) } - socket.applyDelay(delaySocketInMs + delayConnectionInMs) + socket.applyDelay(delayConnectionInMs) common.setTimeout(respond, delayConnectionInMs) } diff --git a/lib/socket.js b/lib/socket.js index 6fd4b73e3..16808796c 100644 --- a/lib/socket.js +++ b/lib/socket.js @@ -20,12 +20,8 @@ module.exports = class Socket extends EventEmitter { this.destroyed = false this.connecting = true - // totalDelay that has already been applied to the current - // request/connection, timeout error will be generated if - // it is timed-out. - this.totalDelayMs = 0 - // Maximum allowed delay. Null means unlimited. - this.timeoutMs = null + // Maximum allowed delay. 0 means unlimited. + this.timeout = 0 const ipv6 = options.family === 6 this.remoteFamily = ipv6 ? 'IPv6' : 'IPv4' @@ -48,16 +44,22 @@ module.exports = class Socket extends EventEmitter { } setTimeout(timeoutMs, fn) { - this.timeoutMs = timeoutMs + this.timeout = timeoutMs if (fn) { this.once('timeout', fn) } + return this } + /** + * Artificial delay that will trip socket timeouts when appropriate. + * + * Doesn't actually wait for time to pass. + * Timeout events don't necessarily end the request. + * While many clients choose to abort the request upon a timeout, Node itself does not. + */ applyDelay(delayMs) { - this.totalDelayMs += delayMs - - if (this.timeoutMs && this.totalDelayMs > this.timeoutMs) { + if (this.timeout && delayMs > this.timeout) { debug('socket timeout') this.emit('timeout') } diff --git a/tests/setup.js b/tests/setup.js index ade96fc66..bfbac1666 100644 --- a/tests/setup.js +++ b/tests/setup.js @@ -16,6 +16,7 @@ chai.use(sinonChai) afterEach(function () { nock.restore() + nock.abortPendingRequests() nock.cleanAll() nock.enableNetConnect() nock.emitter.removeAllListeners() diff --git a/tests/test_delay.js b/tests/test_delay.js index c11c017a4..5357eccbb 100644 --- a/tests/test_delay.js +++ b/tests/test_delay.js @@ -312,4 +312,68 @@ describe('`delayConnection()`', () => { 100 ) }) + + it('emits a timeout - with setTimeout', done => { + nock('http://example.test').get('/').delayConnection(10000).reply(200, 'OK') + + const onEnd = sinon.spy() + + const req = http.request('http://example.test', res => { + res.once('end', onEnd) + }) + + req.setTimeout(5000, () => { + expect(onEnd).not.to.have.been.called() + done() + }) + + req.end() + }) + + it('emits a timeout - with options.timeout', done => { + nock('http://example.test').get('/').delayConnection(10000).reply(200, 'OK') + + const onEnd = sinon.spy() + + const req = http.request('http://example.test', { timeout: 5000 }, res => { + res.once('end', onEnd) + }) + + req.on('timeout', function () { + expect(onEnd).not.to.have.been.called() + done() + }) + + req.end() + }) + + it('does not emit a timeout when timeout > delayConnection', done => { + const responseText = 'okeydoke!' + const scope = nock('http://example.test') + .get('/') + .delayConnection(300) + .reply(200, responseText) + + const req = http.request('http://example.test', res => { + res.setEncoding('utf8') + + let body = '' + + res.on('data', chunk => { + body += chunk + }) + + res.once('end', () => { + expect(body).to.equal(responseText) + scope.done() + done() + }) + }) + + req.setTimeout(60000, () => { + expect.fail('socket timed out unexpectedly') + }) + + req.end() + }) }) diff --git a/tests/test_nock_lifecycle.js b/tests/test_nock_lifecycle.js index d5be2108b..0ef6873e2 100644 --- a/tests/test_nock_lifecycle.js +++ b/tests/test_nock_lifecycle.js @@ -106,6 +106,25 @@ describe('Nock lifecycle functions', () => { return true }) }) + + it('should be safe to call in the middle of a request', done => { + // This covers a race-condition where cleanAll() is called while a request + // is in mid-flight. The request itself should continue to process normally. + // Notably, `cleanAll` is being called before the Interceptor is marked as + // consumed and removed from the global map. Having this test wait until the + // response event means we verify it didn't throw an error when attempting + // to remove an Interceptor that doesn't exist in the global map `allInterceptors`. + nock('http://example.test').get('/').reply() + + const req = http.request('http://example.test', () => { + done() + }) + req.once('socket', () => { + nock.cleanAll() + }) + + req.end() + }) }) describe('`isDone()`', () => { diff --git a/tests/test_socket.js b/tests/test_socket.js new file mode 100644 index 000000000..bcc3ee8a0 --- /dev/null +++ b/tests/test_socket.js @@ -0,0 +1,32 @@ +'use strict' + +const http = require('http') +const nock = require('..') + +require('./setup') + +describe('`Socket#setTimeout()`', () => { + it('adds callback as a one-time listener for parity with a real socket', done => { + nock('http://example.test').get('/').delayConnection(100).reply() + + const onTimeout = () => { + done() + } + + http.get('http://example.test').on('socket', socket => { + socket.setTimeout(50, onTimeout) + }) + }) + + it('can be called without a callback', done => { + nock('http://example.test').get('/').delayConnection(100).reply() + + http.get('http://example.test').on('socket', socket => { + socket.setTimeout(50) + + socket.on('timeout', () => { + done() + }) + }) + }) +}) diff --git a/tests/test_socket_delay.js b/tests/test_socket_delay.js deleted file mode 100644 index ab9f87468..000000000 --- a/tests/test_socket_delay.js +++ /dev/null @@ -1,132 +0,0 @@ -'use strict' - -const http = require('http') -const { expect } = require('chai') -const sinon = require('sinon') -const nock = require('..') - -require('./setup') - -describe('`socketDelay()`', () => { - it('socketDelay', done => { - nock('http://example.test') - .get('/') - .socketDelay(200) - .reply(200, '') - - const req = http.get('http://example.test') - - const onTimeout = sinon.spy() - - req.on('socket', socket => { - if (!socket.connecting) { - req.setTimeout(100, onTimeout) - return - } - - socket.on('connect', () => { - req.setTimeout(100, onTimeout) - }) - }) - - req.on('response', () => { - expect(onTimeout).to.have.been.calledOnce() - done() - }) - }) - - it('emits a timeout - with setTimeout', done => { - nock('http://example.test').get('/').socketDelay(10000).reply(200, 'OK') - - const onEnd = sinon.spy() - - const req = http.request('http://example.test', res => { - res.setEncoding('utf8') - res.once('end', onEnd) - }) - - req.setTimeout(5000, () => { - expect(onEnd).not.to.have.been.called() - done() - }) - - req.end() - }) - - it('emits a timeout - with options.timeout', done => { - nock('http://example.test').get('/').socketDelay(10000).reply(200, 'OK') - - const onEnd = sinon.spy() - - const req = http.request('http://example.test', { timeout: 5000 }, res => { - res.setEncoding('utf8') - res.once('end', onEnd) - }) - - req.on('timeout', function () { - expect(onEnd).not.to.have.been.called() - done() - }) - - req.end() - }) - - it('does not emit a timeout when timeout > socketDelay', done => { - const responseText = 'okeydoke!' - const scope = nock('http://example.test') - .get('/') - .socketDelay(10000) - .reply(200, responseText) - - const req = http.request('http://example.test', res => { - res.setEncoding('utf8') - - let body = '' - - res.on('data', chunk => { - body += chunk - }) - - res.once('end', () => { - expect(body).to.equal(responseText) - scope.done() - done() - }) - }) - - req.setTimeout(60000, () => { - expect.fail('socket timed out unexpectedly') - }) - - req.end() - }) -}) - -describe('`Socket#setTimeout()`', () => { - it('adds callback as a one-time listener for parity with a real socket', done => { - nock('http://example.test') - .get('/') - .socketDelay(100) - .reply(200, '') - - const onTimeout = () => { - done() - } - - http.get('http://example.test').on('socket', socket => { - socket.setTimeout(50, onTimeout) - }) - }) - - it('can be called without a callback', done => { - nock('http://example.test').get('/').socketDelay(100).reply() - - http.get('http://example.test').on('socket', socket => { - socket.setTimeout(50) - - socket.on('timeout', () => { - done() - }) - }) - }) -}) diff --git a/types/index.d.ts b/types/index.d.ts index 3632cf4bc..9a81a905a 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -203,7 +203,6 @@ declare namespace nock { delay(opts: number | { head?: number; body?: number }): this delayBody(timeMs: number): this delayConnection(timeMs: number): this - socketDelay(timeMs: number): this } interface Options { diff --git a/types/tests.ts b/types/tests.ts index 5b98fbb48..cfc870137 100644 --- a/types/tests.ts +++ b/types/tests.ts @@ -130,7 +130,6 @@ inst = inst.delay(2000) inst = inst.delay({ head: 1000, body: 1000 }) inst = inst.delayBody(2000) inst = inst.delayConnection(2000) -inst = inst.socketDelay(2000) scope.done() // $ExpectType void scope.isDone() // $ExpectType boolean @@ -502,13 +501,6 @@ nock('http://example.test') }) .reply(200, '') -// Delay the connection -nock('http://example.test') - .get('/') - .socketDelay(2000) // 2 seconds - .delayConnection(1000) - .reply(200, '') - // Chaining scope = nock('http://example.test') .get('/users/1') From 3fc8d85cebc9c4f3c1f13009efb220b1179c9296 Mon Sep 17 00:00:00 2001 From: Paul Melnikow Date: Sat, 2 May 2020 14:34:58 -0400 Subject: [PATCH 09/13] refactor: Chai assertions in test_back (#1986) --- README.md | 4 + tests/test_back.js | 191 ++++++++++++++++++++++--------------------- tests/test_back_2.js | 34 ++++---- 3 files changed, 120 insertions(+), 109 deletions(-) diff --git a/README.md b/README.md index 146950718..9222231d0 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,10 @@ For instance, if a module performs HTTP requests to a CouchDB server or makes HT - [Non-standard ports](#non-standard-ports) - [Repeat response n times](#repeat-response-n-times) - [Delay the response](#delay-the-response) + - [Delay the connection](#delay-the-connection) + - [Technical Details](#technical-details) + - [Delay the response body](#delay-the-response-body) + - [Technical Details](#technical-details-1) - [Chaining](#chaining) - [Scope filtering](#scope-filtering) - [Conditional scope filtering](#conditional-scope-filtering) diff --git a/tests/test_back.js b/tests/test_back.js index 3a2388afa..9ea87e1f5 100644 --- a/tests/test_back.js +++ b/tests/test_back.js @@ -2,11 +2,15 @@ const http = require('http') const fs = require('fs') +const { expect } = require('chai') const path = require('path') +const sinon = require('sinon') const { beforeEach, test } = require('tap') const proxyquire = require('proxyquire').preserveCache() const nock = require('..') +const { back: nockBack } = nock +require('./setup') require('./cleanup_after_each')() beforeEach(done => { @@ -15,11 +19,8 @@ beforeEach(done => { done() }) -const nockBack = nock.back -const exists = fs.existsSync - function testNock(t) { - let dataCalled = false + const onData = sinon.spy() const scope = nock('http://www.example.test') .get('/') @@ -33,16 +34,16 @@ function testNock(t) { port: 80, }, res => { - t.equal(res.statusCode, 200) + expect(res.statusCode).to.equal(200) res.once('end', () => { - t.ok(dataCalled) + expect(onData).to.have.been.called() scope.done() t.end() }) res.on('data', data => { - dataCalled = true - t.ok(data instanceof Buffer, 'data should be a buffer') - t.equal(data.toString(), 'Hello World!', 'response should match') + onData() + expect(data).to.be.an.instanceOf(Buffer) + expect(data.toString()).to.equal('Hello World!') }) } ) @@ -53,7 +54,7 @@ function nockBackWithFixture(t, scopesLoaded) { const scopesLength = scopesLoaded ? 1 : 0 nockBack('good_request.json', function (done) { - t.equal(this.scopes.length, scopesLength) + expect(this.scopes).to.have.length(scopesLength) http.get('http://www.example.test/', () => { this.assertScopesFinished() done() @@ -68,11 +69,12 @@ function nockBackWithFixture(t, scopesLoaded) { // comment. function nockBackWithFixtureLocalhost(t) { nockBack('goodRequestLocalhost.json', function (done) { - t.equal(this.scopes.length, 0) + const onRequest = sinon.spy() - const server = http.createServer((request, response) => { - t.pass('server received a request') + expect(this.scopes).to.be.empty() + const server = http.createServer((request, response) => { + onRequest() response.writeHead(200) response.end() }) @@ -87,7 +89,8 @@ function nockBackWithFixtureLocalhost(t) { port: server.address().port, }, response => { - t.is(200, response.statusCode) + expect(onRequest).to.have.been.calledOnce() + expect(response.statusCode).to.equal(200) this.assertScopesFinished() done() t.end() @@ -103,45 +106,44 @@ function nockBackWithFixtureLocalhost(t) { test('nockBack throws an exception when fixtures is not set', t => { nockBack.fixtures = undefined - t.throws(nockBack, { message: 'Back requires nock.back.fixtures to be set' }) + expect(nockBack).to.throw('Back requires nock.back.fixtures to be set') t.end() }) test('nockBack throws an exception when fixtureName is not a string', t => { - t.throws(() => nockBack(), { - message: 'Parameter fixtureName must be a string', - }) + expect(nockBack).to.throw('Parameter fixtureName must be a string') t.end() }) test('nockBack returns a promise when neither options nor nockbackFn are specified', t => { - nockBack('test-promise-fixture.json').then(params => { - t.type(params.nockDone, 'function') - t.type(params.context, 'object') + nockBack('test-promise-fixture.json').then(({ nockDone, context }) => { + expect(nockDone).to.be.a('function') + expect(context).to.be.an('object') t.end() }) }) test('nockBack throws an exception when a hook is not a function', t => { nockBack.setMode('dryrun') - t.throws( - () => nockBack('good_request.json', { before: 'not-a-function-innit' }), - { message: 'processing hooks must be a function' } - ) + expect(() => + nockBack('good_request.json', { before: 'not-a-function-innit' }) + ).to.throw('processing hooks must be a function') t.end() }) test('nockBack.setMode throws an exception on unknown mode', t => { - t.throws(() => nockBack.setMode('bogus'), { message: 'Unknown mode: bogus' }) + expect(() => nockBack.setMode('bogus')).to.throw('Unknown mode: bogus') t.end() }) test('nockBack returns a promise when nockbackFn is not specified', t => { - nockBack('test-promise-fixture.json', { test: 'options' }).then(params => { - t.type(params.nockDone, 'function') - t.type(params.context, 'object') - t.end() - }) + nockBack('test-promise-fixture.json', { test: 'options' }).then( + ({ nockDone, context }) => { + expect(nockDone).to.be.a('function') + expect(context).to.be.an('object') + t.end() + } + ) }) test('with wild, normal nocks work', t => testNock(t)) @@ -167,10 +169,10 @@ test('nockBack dryrun tests', nw => { }) nw.test('goes to internet even when no nockBacks are running', t => { - t.plan(2) + const onRequest = sinon.spy() const server = http.createServer((request, response) => { - t.pass('server received a request') + onRequest() response.writeHead(200) response.end() @@ -184,7 +186,8 @@ test('nockBack dryrun tests', nw => { port: server.address().port, }, response => { - t.is(200, response.statusCode) + expect(response.statusCode).to.equal(200) + expect(onRequest).to.have.been.calledOnce() server.close(t.end) } @@ -200,19 +203,17 @@ test('nockBack dryrun tests', nw => { nw.test('uses recorded fixtures', t => nockBackWithFixture(t, true)) nw.test("goes to internet, doesn't record new fixtures", t => { - t.plan(5) - - let dataCalled = false + const onData = sinon.spy() + const onRequest = sinon.spy() const fixture = 'someDryrunFixture.json' const fixtureLoc = `${nockBack.fixtures}/${fixture}` - t.false(exists(fixtureLoc)) + expect(fs.existsSync(fixtureLoc)).to.be.false() - nockBack(fixture, done => { + nockBack(fixture, nockDone => { const server = http.createServer((request, response) => { - t.pass('server received a request') - + onRequest() response.writeHead(200) response.write('server served a response') response.end() @@ -226,15 +227,14 @@ test('nockBack dryrun tests', nw => { port: server.address().port, }, response => { - t.is(200, response.statusCode) + expect(response.statusCode).to.equal(200) - response.on('data', data => { - dataCalled = true - }) + response.on('data', onData) response.on('end', () => { - t.ok(dataCalled) - t.false(exists(fixtureLoc)) + expect(onRequest).to.have.been.calledOnce() + expect(onData).to.have.been.called() + expect(fs.existsSync(fixtureLoc)).to.be.false() server.close(t.end) }) @@ -256,17 +256,16 @@ test('nockBack record tests', nw => { }) nw.test('it records when configured correctly', t => { - t.plan(4) + const onRequest = sinon.spy() const fixture = 'someFixture.txt' const fixtureLoc = `${nockBack.fixtures}/${fixture}` - t.false(exists(fixtureLoc)) + expect(fs.existsSync(fixtureLoc)).to.be.false() - nockBack(fixture, done => { + nockBack(fixture, nockDone => { const server = http.createServer((request, response) => { - t.pass('server received a request') - + onRequest() response.writeHead(200) response.write('server served a response') response.end() @@ -280,10 +279,11 @@ test('nockBack record tests', nw => { port: server.address().port, }, response => { - done() + nockDone() - t.is(200, response.statusCode) - t.true(exists(fixtureLoc)) + expect(onRequest).to.have.been.calledOnce() + expect(response.statusCode).to.equal(200) + expect(fs.existsSync(fixtureLoc)).to.be.true() fs.unlinkSync(fixtureLoc) @@ -300,17 +300,16 @@ test('nockBack record tests', nw => { // Adding this test because there was an issue when not calling // nock.activate() after calling nock.restore(). nw.test('it can record twice', t => { - t.plan(4) + const onRequest = sinon.spy() const fixture = 'someFixture2.txt' const fixtureLoc = `${nockBack.fixtures}/${fixture}` - t.false(exists(fixtureLoc)) + expect(fs.existsSync(fixtureLoc)).to.be.false() - nockBack(fixture, function (done) { + nockBack(fixture, function (nockDone) { const server = http.createServer((request, response) => { - t.pass('server received a request') - + onRequest() response.writeHead(200) response.write('server served a response') response.end() @@ -324,10 +323,11 @@ test('nockBack record tests', nw => { port: server.address().port, }, response => { - done() + nockDone() - t.is(200, response.statusCode) - t.true(exists(fixtureLoc)) + expect(onRequest).to.have.been.calledOnce() + expect(response.statusCode).to.equal(200) + expect(fs.existsSync(fixtureLoc)).to.be.true() fs.unlinkSync(fixtureLoc) @@ -342,47 +342,45 @@ test('nockBack record tests', nw => { }) nw.test("it shouldn't allow outside calls", t => { - nockBack('wrong_uri.json', function (done) { + nockBack('wrong_uri.json', nockDone => { http - .get('http://other.example.test', res => - t.fail('Should not come here!') - ) + .get('http://other.example.test', res => expect.fail()) .on('error', err => { - t.equal( - err.message, + expect(err.message).to.equal( 'Nock: Disallowed net connect for "other.example.test:80/"' ) - done() + nockDone() t.end() }) }) }) nw.test('it loads your recorded tests', t => { - nockBack('good_request.json', function (done) { - t.true(this.scopes.length > 0) + nockBack('good_request.json', function (nockDone) { + expect(this.scopes).to.have.lengthOf.at.least(1) http.get('http://www.example.test/', () => { this.assertScopesFinished() - done() + nockDone() t.end() }) }) }) nw.test('it can filter after recording', t => { + const onRequest = sinon.spy() + const fixture = 'filteredFixture.json' const fixtureLoc = `${nockBack.fixtures}/${fixture}` - t.false(exists(fixtureLoc)) + expect(fs.existsSync(fixtureLoc)).to.be.false() // You would do some filtering here, but for this test we'll just return // an empty array. const afterRecord = scopes => [] - nockBack(fixture, { afterRecord }, function (done) { + nockBack(fixture, { afterRecord }, function (nockDone) { const server = http.createServer((request, response) => { - t.pass('server received a request') - + onRequest() response.writeHead(200) response.write('server served a response') response.end() @@ -396,11 +394,12 @@ test('nockBack record tests', nw => { port: server.address().port, }, response => { - done() + nockDone() - t.is(200, response.statusCode) - t.true(exists(fixtureLoc)) - t.equal(this.scopes.length, 0) + expect(onRequest).to.have.been.calledOnce() + expect(response.statusCode).to.equal(200) + expect(fs.existsSync(fixtureLoc)).to.be.true() + expect(this.scopes).to.be.empty() fs.unlinkSync(fixtureLoc) server.close(t.end) @@ -417,11 +416,11 @@ test('nockBack record tests', nw => { const fixture = 'filteredFixture.json' const fixtureLoc = `${nockBack.fixtures}/${fixture}` - t.false(exists(fixtureLoc)) + expect(fs.existsSync(fixtureLoc)).to.be.false() const afterRecord = scopes => 'string-response' - nockBack(fixture, { afterRecord }, function (done) { + nockBack(fixture, { afterRecord }, function (nockDone) { const server = http.createServer((request, response) => { t.pass('server received a request') @@ -438,11 +437,13 @@ test('nockBack record tests', nw => { port: server.address().port, }, response => { - done() + nockDone() - t.is(200, response.statusCode) - t.true(exists(fixtureLoc)) - t.is(fs.readFileSync(fixtureLoc, 'utf8'), 'string-response') + expect(response.statusCode).to.equal(200) + expect(fs.existsSync(fixtureLoc)).to.be.true() + expect(fs.readFileSync(fixtureLoc, 'utf8')).to.equal( + 'string-response' + ) fs.unlinkSync(fixtureLoc) server.close(t.end) @@ -476,8 +477,7 @@ test('nockBack lockdown tests', nw => { ) req.on('error', err => { - t.equal( - err.message.trim(), + expect(err.message.trim()).to.equal( 'Nock: Disallowed net connect for "other.example.test:80/"' ) t.end() @@ -493,10 +493,11 @@ test('assertScopesFinished throws exception when Back still has pending scopes', nockBack.setMode('record') const fixtureName = 'good_request.json' const fixturePath = path.join(nockBack.fixtures, fixtureName) - nockBack(fixtureName, function (done) { - const expected = `["GET http://www.example.test:80/"] was not used, consider removing ${fixturePath} to rerecord fixture` - t.throws(() => this.assertScopesFinished(), { message: expected }) - done() + nockBack(fixtureName, function (nockDone) { + expect(() => this.assertScopesFinished()).to.throw( + `["GET http://www.example.test:80/"] was not used, consider removing ${fixturePath} to rerecord fixture` + ) + nockDone() t.end() }) }) @@ -506,7 +507,7 @@ test('nockBack dryrun throws the expected exception when fs is not available', t nockBackWithoutFs.setMode('dryrun') nockBackWithoutFs.fixtures = `${__dirname}/fixtures` - t.throws(() => nockBackWithoutFs('good_request.json'), { message: 'no fs' }) + expect(() => nockBackWithoutFs('good_request.json')).to.throw('no fs') t.end() }) @@ -516,6 +517,6 @@ test('nockBack record mode throws the expected exception when fs is not availabl nockBackWithoutFs.setMode('record') nockBackWithoutFs.fixtures = `${__dirname}/fixtures` - t.throws(() => nockBackWithoutFs('good_request.json'), { message: 'no fs' }) + expect(() => nockBackWithoutFs('good_request.json')).to.throw('no fs') t.end() }) diff --git a/tests/test_back_2.js b/tests/test_back_2.js index cfec69664..f221e6be5 100644 --- a/tests/test_back_2.js +++ b/tests/test_back_2.js @@ -2,12 +2,14 @@ const http = require('http') const fs = require('fs') +const sinon = require('sinon') +const { expect } = require('chai') const { beforeEach, afterEach, test } = require('tap') const rimraf = require('rimraf') const nock = require('..') +const { back: nockBack } = nock -const nockBack = nock.back - +require('./setup') require('./cleanup_after_each')() const fixture = `${__dirname}/fixtures/recording_test.json` @@ -26,12 +28,11 @@ afterEach(done => { }) test('recording', t => { - t.plan(5) + const onRequest = sinon.spy() nockBack('recording_test.json', function (nockDone) { const server = http.createServer((request, response) => { - t.pass('server received a request') - + onRequest() response.writeHead(301) response.write('server served a response') response.end() @@ -50,15 +51,19 @@ test('recording', t => { response.once('end', () => { nockDone() + expect(onRequest).to.have.been.calledOnce() + const fixtureContent = JSON.parse( fs.readFileSync(fixture, { encoding: 'utf8' }) ) - t.equal(fixtureContent.length, 1) + expect(fixtureContent).to.have.length(1) const [firstFixture] = fixtureContent - t.equal(firstFixture.method, 'GET') - t.equal(firstFixture.path, '/') - t.equal(firstFixture.status, 301) + expect(firstFixture).to.include({ + method: 'GET', + path: '/', + status: 301, + }) server.close(t.end) }) @@ -74,15 +79,14 @@ test('recording', t => { }) test('passes custom options to recorder', t => { - t.plan(3) + const onRequest = sinon.spy() nockBack( 'recording_test.json', { recorder: { enable_reqheaders_recording: true } }, function (nockDone) { const server = http.createServer((request, response) => { - t.pass('server received a request') - + onRequest() response.writeHead(200) response.write('server served a response') response.end() @@ -100,12 +104,14 @@ test('passes custom options to recorder', t => { response.once('end', () => { nockDone() + expect(onRequest).to.have.been.calledOnce() + const fixtureContent = JSON.parse( fs.readFileSync(fixture, { encoding: 'utf8' }) ) - t.equal(fixtureContent.length, 1) - t.ok(fixtureContent[0].reqheaders) + expect(fixtureContent).to.have.length(1) + expect(fixtureContent[0].reqheaders).to.be.ok() server.close(t.end) }) From 8c75f4593ec48877d77b4b63e08c88b1bcbe03ce Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Sun, 3 May 2020 10:35:06 -0600 Subject: [PATCH 10/13] ci: Add Node 14 to test matrix (#1988) 14 is not LTS yet, but it is an active version as of 2020-04-21. https://nodejs.org/en/about/releases/ --- .github/workflows/continuous-integration.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/continuous-integration.yaml b/.github/workflows/continuous-integration.yaml index 610c678ab..c734af45e 100644 --- a/.github/workflows/continuous-integration.yaml +++ b/.github/workflows/continuous-integration.yaml @@ -161,5 +161,5 @@ jobs: strategy: fail-fast: false matrix: - node-version: [10, 12, 13] + node-version: [10, 12, 13, 14] os: [macos-latest, ubuntu-latest, windows-latest] From 46e004cf8b617bc37f05e93d738937de05b9849e Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Thu, 14 May 2020 08:35:03 -0600 Subject: [PATCH 11/13] fix(ClientRequest): Use native `abort` and `destroy` (#2000) - Stop monkey patching `abort` on the overridden `ClientRequest`. - Stop prematurely associating the socket with the request. - Prefer `destroy` instead of emitting an error when appropriate. --- lib/intercepted_request_router.js | 138 +++++++++++++----------------- lib/playback_interceptor.js | 14 +-- lib/socket.js | 5 ++ tests/test_request_overrider.js | 2 + 4 files changed, 69 insertions(+), 90 deletions(-) diff --git a/lib/intercepted_request_router.js b/lib/intercepted_request_router.js index fe8faebae..0d0ede514 100644 --- a/lib/intercepted_request_router.js +++ b/lib/intercepted_request_router.js @@ -13,6 +13,20 @@ const globalEmitter = require('./global_emitter') const Socket = require('./socket') const { playbackInterceptor } = require('./playback_interceptor') +function socketOnClose(req) { + debug('socket close') + + if (!req.res && !req.socket._hadError) { + // If we don't have a response then we know that the socket + // ended prematurely and we need to emit an error on the request. + req.socket._hadError = true + const err = new Error('socket hang up') + err.code = 'ECONNRESET' + req.emit('error', err) + } + req.emit('close') +} + /** * Given a group of interceptors, appropriately route an outgoing request. * Identify which interceptor ought to respond, if any, then delegate to @@ -48,10 +62,15 @@ class InterceptedRequestRouter { this.readyToStartPlaybackOnSocketEvent = false this.attachToReq() + + // Emit a fake socket event on the next tick to mimic what would happen on a real request. + // Some clients listen for a 'socket' event to be emitted before calling end(), + // which causes Nock to hang. + process.nextTick(() => this.connectSocket()) } attachToReq() { - const { req, socket, options } = this + const { req, options } = this for (const [name, val] of Object.entries(options.headers)) { req.setHeader(name.toLowerCase(), val) @@ -68,18 +87,9 @@ class InterceptedRequestRouter { req.path = options.path req.method = options.method - // ClientRequest.connection is an alias for ClientRequest.socket - // https://nodejs.org/api/http.html#http_request_socket - // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_client.js#L640-L641 - // The same Socket is shared between the request and response to mimic native behavior. - req.socket = req.connection = socket - - propagate(['close', 'error', 'timeout'], req.socket, req) - req.write = (...args) => this.handleWrite(...args) req.end = (...args) => this.handleEnd(...args) req.flushHeaders = (...args) => this.handleFlushHeaders(...args) - req.abort = (...args) => this.handleAbort(...args) // https://github.com/nock/nock/issues/256 if (options.headers.expect === '100-continue') { @@ -88,49 +98,51 @@ class InterceptedRequestRouter { req.emit('continue') }) } + } - // Emit a fake socket event on the next tick to mimic what would happen on a real request. - // Some clients listen for a 'socket' event to be emitted before calling end(), - // which causes nock to hang. - process.nextTick(() => { - if (req.aborted) { - return - } + connectSocket() { + const { req, socket } = this - socket.connecting = false - req.emit('socket', socket) + // Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled. + if (req.destroyed || req.aborted) { + return + } - // https://nodejs.org/api/net.html#net_event_connect - socket.emit('connect') + // ClientRequest.connection is an alias for ClientRequest.socket + // https://nodejs.org/api/http.html#http_request_socket + // https://github.com/nodejs/node/blob/b0f75818f39ed4e6bd80eb7c4010c1daf5823ef7/lib/_http_client.js#L640-L641 + // The same Socket is shared between the request and response to mimic native behavior. + req.socket = req.connection = socket - // https://nodejs.org/api/tls.html#tls_event_secureconnect - if (socket.authorized) { - socket.emit('secureConnect') - } + propagate(['error', 'timeout'], socket, req) + socket.on('close', () => socketOnClose(req)) - if (this.readyToStartPlaybackOnSocketEvent) { - this.maybeStartPlayback() - } - }) - } + socket.connecting = false + req.emit('socket', socket) - emitError(error) { - const { req } = this - process.nextTick(() => { - req.emit('error', error) - }) + // https://nodejs.org/api/net.html#net_event_connect + socket.emit('connect') + + // https://nodejs.org/api/tls.html#tls_event_secureconnect + if (socket.authorized) { + socket.emit('secureConnect') + } + + if (this.readyToStartPlaybackOnSocketEvent) { + this.maybeStartPlayback() + } } // from docs: When write function is called with empty string or buffer, it does nothing and waits for more input. // However, actually implementation checks the state of finished and aborted before checking if the first arg is empty. handleWrite(buffer, encoding, callback) { - debug('write', arguments) + debug('request write') const { req } = this if (req.finished) { const err = new Error('write after end') err.code = 'ERR_STREAM_WRITE_AFTER_END' - this.emitError(err) + process.nextTick(() => req.emit('error', err)) // It seems odd to return `true` here, not sure why you'd want to have // the stream potentially written to more, but it's what Node does. @@ -138,7 +150,7 @@ class InterceptedRequestRouter { return true } - if (req.aborted) { + if (req.socket && req.socket.destroyed) { return false } @@ -167,7 +179,7 @@ class InterceptedRequestRouter { } handleEnd(chunk, encoding, callback) { - debug('req.end') + debug('request end') const { req } = this // handle the different overloaded arg signatures @@ -191,42 +203,10 @@ class InterceptedRequestRouter { } handleFlushHeaders() { - debug('req.flushHeaders') + debug('request flushHeaders') this.maybeStartPlayback() } - handleAbort() { - debug('req.abort') - const { req, socket } = this - - if (req.aborted) { - return - } - - // Historically, `aborted` was undefined or a timestamp. - // Node changed this behavior in v11 to always be a bool. - req.aborted = true - req.destroyed = true - - // the order of these next three steps is important to match order of events Node would emit. - process.nextTick(() => req.emit('abort')) - - if (!socket.connecting) { - if (!req.res) { - // If we don't have a response then we know that the socket - // ended prematurely and we need to emit an error on the request. - // Node doesn't actually emit this during the `abort` method. - // Instead it listens to the socket's `end` and `close` events, however, - // Nock's socket doesn't have all the complexities and events to - // replicated that directly. This is an easy way to achieve the same behavior. - const connResetError = new Error('socket hang up') - connResetError.code = 'ECONNRESET' - this.emitError(connResetError) - } - socket.destroy() - } - } - /** * Set request headers of the given request. This is needed both during the * routing phase, in case header filters were specified, and during the @@ -268,7 +248,8 @@ class InterceptedRequestRouter { return } - if (!req.aborted && !playbackStarted) { + // Until Node 14 is the minimum, we need to look at both flags to see if the request has been cancelled. + if (!req.destroyed && !req.aborted && !playbackStarted) { this.startPlayback() } } @@ -345,14 +326,11 @@ class InterceptedRequestRouter { // We send the raw buffer as we received it, not as we interpreted it. newReq.end(requestBodyBuffer) } else { - const err = new Error( - `Nock: No match for request ${common.stringifyRequest( - options, - requestBodyString - )}` - ) + const reqStr = common.stringifyRequest(options, requestBodyString) + const err = new Error(`Nock: No match for request ${reqStr}`) + err.code = 'ERR_NOCK_NO_MATCH' err.statusCode = err.status = 404 - this.emitError(err) + req.destroy(err) } } } diff --git a/lib/playback_interceptor.js b/lib/playback_interceptor.js index fda1e7052..56fc0bdc5 100644 --- a/lib/playback_interceptor.js +++ b/lib/playback_interceptor.js @@ -123,12 +123,6 @@ function playbackInterceptor({ }) { const { logger } = interceptor.scope - function emitError(error) { - process.nextTick(() => { - req.emit('error', error) - }) - } - function start() { interceptor.markConsumed() interceptor.req = req @@ -145,7 +139,7 @@ function playbackInterceptor({ } const delay = interceptor.delayBodyInMs + interceptor.delayConnectionInMs - common.setTimeout(emitError, delay, error) + common.setTimeout(() => req.destroy(error), delay) return } @@ -170,7 +164,7 @@ function playbackInterceptor({ // wrapping in `Promise.resolve` makes it into a promise either way. Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody)) .then(continueWithResponseBody) - .catch(emitError) + .catch(err => req.destroy(err)) return } @@ -184,7 +178,7 @@ function playbackInterceptor({ Promise.resolve(fn.call(interceptor, options.path, parsedRequestBody)) .then(continueWithFullResponse) - .catch(emitError) + .catch(err => req.destroy(err)) return } @@ -234,7 +228,7 @@ function playbackInterceptor({ try { responseBody = parseFullReplyResult(response, fullReplyResult) } catch (err) { - emitError(err) + req.destroy(err) return } diff --git a/lib/socket.js b/lib/socket.js index 16808796c..2777ae28d 100644 --- a/lib/socket.js +++ b/lib/socket.js @@ -20,6 +20,9 @@ module.exports = class Socket extends EventEmitter { this.destroyed = false this.connecting = true + // Undocumented flag used by ClientRequest to ensure errors aren't double-fired + this._hadError = false + // Maximum allowed delay. 0 means unlimited. this.timeout = 0 @@ -83,11 +86,13 @@ module.exports = class Socket extends EventEmitter { return this } + debug('socket destroy') this.destroyed = true this.readable = this.writable = false process.nextTick(() => { if (err) { + this._hadError = true this.emit('error', err) } this.emit('close') diff --git a/tests/test_request_overrider.js b/tests/test_request_overrider.js index 5565d62a6..9def18583 100644 --- a/tests/test_request_overrider.js +++ b/tests/test_request_overrider.js @@ -554,6 +554,7 @@ describe('Request Overrider', () => { nock('http://example.test').get('/').reply(200, 'hey') const req = http.get('http://example.test') + req.on('error', () => {}) // listen for error so it doesn't bubble req.once('socket', socket => { socket.destroy() done() @@ -564,6 +565,7 @@ describe('Request Overrider', () => { nock('http://example.test').get('/').reply(200, 'hey') const req = http.get('http://example.test') + req.on('error', () => {}) // listen for error so it doesn't bubble req.once('socket', socket => { const closeSpy = sinon.spy() socket.on('close', closeSpy) From 75507727cf09a0b7bf0aa7ebdf3621952921b82e Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Thu, 28 May 2020 06:15:38 -0600 Subject: [PATCH 12/13] chore(docs): Add migration guides. (#2006) --- CHANGELOG.md | 5 +- migration_guides/migrating_to_10.md | 10 ++ migration_guides/migrating_to_11.md | 212 ++++++++++++++++++++++++++++ migration_guides/migrating_to_12.md | 33 +++++ migration_guides/migrating_to_13.md | 68 +++++++++ 5 files changed, 327 insertions(+), 1 deletion(-) create mode 100644 migration_guides/migrating_to_10.md create mode 100644 migration_guides/migrating_to_11.md create mode 100644 migration_guides/migrating_to_12.md create mode 100644 migration_guides/migrating_to_13.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 02f73c04c..b37be7d8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,6 @@ # Changelog -Nock’s changelog can be found directly in the [GitHub release notes](https://github.com/nock/nock/releases). These are automatically created by [semantic-release](https://github.com/semantic-release/semantic-release) based on their [commit message conventions](https://semantic-release.gitbook.io/semantic-release#commit-message-format). +Nock’s changelog can be found directly in the [GitHub release notes](https://github.com/nock/nock/releases). +These are automatically created by [semantic-release](https://github.com/semantic-release/semantic-release) based on their [commit message conventions](https://semantic-release.gitbook.io/semantic-release#commit-message-format). + +Migration guides are available for major versions in the [migration guides directory](https://github.com/nock/nock/tree/master/migration_guides). diff --git a/migration_guides/migrating_to_10.md b/migration_guides/migrating_to_10.md new file mode 100644 index 000000000..f5491bbd1 --- /dev/null +++ b/migration_guides/migrating_to_10.md @@ -0,0 +1,10 @@ +## Upgrading from Nock 9 to Nock 10 + +[Release Tag](https://github.com/nock/nock/releases/tag/v10.0.0) + +### Breaking changes + +Support for Node < 6 was dropped. + +To upgrade Nock, ensure your version of Node is also updated. +At the time of release, Node 6.x, 8.x, and 9.x were supported. diff --git a/migration_guides/migrating_to_11.md b/migration_guides/migrating_to_11.md new file mode 100644 index 000000000..d111f857d --- /dev/null +++ b/migration_guides/migrating_to_11.md @@ -0,0 +1,212 @@ +## Upgrading from Nock 10 to Nock 11 + +[Release Tag](https://github.com/nock/nock/releases/tag/v11.3.2) + +### Bug fixes and internal improvements + +Nock 11 includes many under-the-hood improvements, including a fully offline +test suite and 100% test coverage. The codebase was also converted to ES6 +syntax and formatted with Prettier. Leaning on the test coverage, some +substantial refactors have begun. + +Many bug fixes are included. See the detailed changelog below or the +[compare view][compare] for details. + +### Fabulous new features for developers + +1. The library ships with TypeScript definitions. (Added in v11.3) +1. Add support for the `http.request` signatures added in Node 10.9 +1. Scopes can be filtered using the system environment or any external factor + using e.g. `.conditionally(() => true)` +1. In-flight modifications to headers are preserved in mock requests. +1. Recorded mocks can be stringified using custom code in the `afterRecord()` + post-processing hook. When `afterRecord()` returns a string, the + recorder will no longer attempt to re-stringify it. (Added in v11.3) +1. Reply functions passed to `.reply()` can now be async/promise-returning. +1. Specifying reply headers, either via `.reply()` or `.defaultReplyHeaders()`, + can now be done consistently using an object, Map, or flat array. + +### Breaking changes + +For many developers no code changes will be needed. However, there are several +minor changes to the API, and it's possible that you will need to update your +code for Nock to keep working properly. It's unlikely that your tests will +falsely pass; what's more probable is that your tests will fail until the +necessary changes are made. + +1. Nock 11 requires Node 8 or later. Nock supports and tests all the "current" + and "maintenance" versions of Node. As of release, that's Node 8, 10, and 12. + +1. In Nock 10, when `reply()` was invoked with a function, the return values were + handled ambiguously depending on their types. + + Consider the following example: + + ```js + const scope = nock('http://example.com') + .get('/') + .reply(200, () => [500, 'hello world']) + ``` + + In Nock 10, the 200 was ignored, the 500 was interpreted as the status + code, and the body would contain `'hello world'`. This caused problems + when the goal was to return a numeric array, so in Nock 11, the 200 is + properly interpreted as the status code, and `[500, 'hello world']` as the + body. + + These are the correct calls for Nock 11: + + ```js + const scope = nock('http://example.com').get('/').reply(500, 'hello world') + + const scope = nock('http://example.com') + .get('/') + .reply(500, () => 'hello world') + ``` + + The `.reply()` method can be called with explicit arguments: + + ```js + .reply() // `statusCode` defaults to `200`. + .reply(statusCode) // `responseBody` defaults to `''`. + .reply(statusCode, responseBody) // `headers` defaults to `{}`. + .reply(statusCode, responseBody, headers) + ``` + + It can be called with a status code and a function that returns an array: + + ```js + .reply(statusCode, (path, requestBody) => responseBody) + .reply(statusCode, (path, requestBody) => responseBody, headers) + ``` + + Alternatively the status code can be included in the array: + + ```js + .reply((path, requestBody) => [statusCode]) + .reply((path, requestBody) => [statusCode, responseBody]) + .reply((path, requestBody) => [statusCode, responseBody, headers]) + .reply((path, requestBody) => [statusCode, responseBody], headers) + ``` + + `.reply()` can also be called with an `async` or promise-returning function. The + signatures are identical, e.g. + + ```js + .reply(async (path, requestBody) => [statusCode, responseBody]) + .reply(statusCode, async (path, requestBody) => responseBody) + ``` + + Finally, an error-first callback can be used, e.g.: + + ```js + .reply((path, requestBody, cb) => cb(undefined, [statusCode, responseBody])) + .reply(statusCode, (path, requestBody, cb) => cb(undefined, responseBody)) + ``` + +1. In Nock 10, errors in user-provided reply functions were caught by Nock, and + generated HTTP responses with status codes of 500. In Nock 11 these errors + are not caught, and instead are re-emitted through the request, like any + other error that occurs during request processing. + + Consider the following example: + + ```js + const scope = nock('http://example.com') + .post('/echo') + .reply(201, (uri, requestBody, cb) => { + fs.readFile('cat-poems.txt', cb) // Error-first callback + }) + ``` + + When `fs.readFile()` errors in Nock 10, a 500 error was emitted. To get the + same effect in Nock 11, the example would need to be rewritten to: + + ```js + const scope = nock('http://example.com') + .post('/echo') + .reply((uri, requestBody, cb) => { + fs.readFile('cat-poems.txt', (err, contents) => { + if (err) { + cb([500, err.stack]) + } else { + cb([201, contents]) + } + }) + }) + ``` + +1. When `.reply()` is invoked with something other than a whole number status + code or a function, Nock 11 raises a new error **Invalid ... value for status code**. + +1. Callback functions provided to the `.query` method now receive the result of [`querystring.parse`](https://nodejs.org/api/querystring.html#querystring_querystring_parse_str_sep_eq_options) instead of [`qs.parse`](https://github.com/ljharb/qs#parsing-objects). + + In particular, `querystring.parse` does not interpret keys with JSON + path notation: + + ```js + querystring.parse('foo[bar]=baz') // { "foo[bar]": 'baz' } + qs.parse('foo[bar]=baz') // { foo: { bar: 'baz' } } + ``` + +1. In Nock 10, duplicate field names provided to the `.query()` method were + silently ignored. We decided this was probably hiding unintentionally bugs + and causing frustration with users. In Nock 11, attempts to provide query + params more than once will throw a new error + **Query parameters have aleady been defined**. This could happen by calling + `.query()` twice, or by calling `.query()` after specifying a literal query + string via the path. + + ```js + nock('http://example.com') + .get('/path') + .query({ foo: 'bar' }) + .query({ baz: 'qux' }) // <-- will throw + .reply() + + nock('http://example.com') + .get('/path?foo=bar') + .query({ baz: 'qux' }) // <-- will throw + .reply() + ``` + +1. Paths in Nock have always required a leading slash. e.g. + + ```js + const scope = nock('http://example.com').get('/path').reply() + ``` + + In Nock 10, if the leading slash was missing the mock would never match. In + Nock 11, this raises an error. + +1. The `reqheaders` parameter should be provided as a plain object, e.g. + `nock('http://example.com', { reqheaders: { X-Foo: 'bar' }})`. When the + headers are specified incorrectly as e.g. `{ reqheaders: 1 }`, Nock 10 would + behave in unpredictable ways. In Nock 11, a new error + **Headers must be provided as an object** is thrown. + + ```js + nock('http://example.com', { reqheaders: 1 }).get('/').reply() + ``` + +1. In Nock 10, the `ClientRequest` instance wrapped the native `on` method + and aliased `once` to it. In Nock 11, this been removed and `request.once` + will correctly call registered listeners...once. + +1. In Nock 10, when the method was not specified in a call to `nock.define()`, + the method would default to `GET`. In Nock 11, this raises an error. + +1. In very old versions of nock, recordings may include a response status + code encoded as a string in the `reply` field. In Nock 10 these strings could + be non-numeric. In Nock 11 this raises an error. + +### Updates to the mock surface + +1. For parity with a real response, a mock request correctly supports all + the overrides to `request.end()`, including `request.end(cb)` in Node 12. +1. For parity with a real response, errors from the `.destroy()` method + are propagated correctly. (Added in v11.3) +1. For parity with a real response, the `.complete` property is set when + ending the response. +1. For parity with a real Socket, the mock Socket has an `unref()` function + (which does nothing). diff --git a/migration_guides/migrating_to_12.md b/migration_guides/migrating_to_12.md new file mode 100644 index 000000000..ee5f3f52d --- /dev/null +++ b/migration_guides/migrating_to_12.md @@ -0,0 +1,33 @@ +## Upgrading from Nock 11 to Nock 12 + +[Release Tag](https://github.com/nock/nock/releases/tag/v12.0.0) + +### Breaking changes + +1. Support for Node < 10 was dropped. + To upgrade Nock, ensure your version of Node is also updated. + At the time of release, Node 10.x, 12.x, and 13.x were supported. + +1. [`cleanAll()`](https://github.com/nock/nock#cleanall) no longer returns the global `nock` instance ([#1872](https://github.com/nock/nock/pull/1872)). + + ```js + // before + nock.cleanAll().restore() // Uncaught TypeError: Cannot read property 'restore' of undefined + + // upgraded + nock.cleanAll() + nock.restore() + ``` + +1. Support was dropped for the String constructor ([#1873](https://github.com/nock/nock/pull/1873)). + Only string primitive are supported. All strings passed to Nock for options should not use `new String` syntax. + [MDN web docs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#String_primitives_and_String_objects). + +### New features for developers + +1. [`enableNetConnect()`](https://github.com/nock/nock#enabling-requests) now accepts a function. + ```js + nock.enableNetConnect( + host => host.includes('amazon.com') || host.includes('github.com') + ) + ``` diff --git a/migration_guides/migrating_to_13.md b/migration_guides/migrating_to_13.md new file mode 100644 index 000000000..bb4454bf4 --- /dev/null +++ b/migration_guides/migrating_to_13.md @@ -0,0 +1,68 @@ +## Upgrading from Nock 12 to Nock 13 + +[Release Tag](https://github.com/nock/nock/releases/tag/v13.0.0) + +### Breaking changes + +1. `Scope.log` has been removed. Use the `debug` library when [debugging](https://github.com/nock/nock#debugging) failed matches. + +1. `socketDelay` has been removed. Use [`delayConnection`](https://github.com/nock/nock#delay-the-connection) instead. + +1. `delay`, `delayConnection`, and `delayBody` are now setters instead of additive. + + ```js + nock('http://example.com') + .get('/') + .delay(1) + .delay({ head: 2, body: 3 }) + .delayConnection(4) + .delayBody(5) + .delayBody(6) + .reply() + ``` + + Previously, the connection would have been delayed by 7 and the body delayed by 14. + Now, the connection will be delayed by 4 and the body delayed by 6. + +1. [When recording](https://github.com/nock/nock#recording), skipping body matching using `*` is no longer supported by `nock.define`. + Set the definition body to `undefined` instead. + + ```js + nock.define([ + { + scope: 'http://example.test', + method: 'POST', + path: '/', + body: '*', // remove this line or set to undefined + response: 'matched', + }, + ]) + ``` + +1. `ClientRequest.abort()` has been updated to align with Node's native behavior. + + - Nock use to always emit a 'socket hang up' error. Now it only emits the error if `abort` is called between the 'socket' and 'response' events. + - The emitted 'abort' event now happens on `nextTick`. + - The socket is now only destroyed if the 'socket' event has fired, and now emits a 'close' event on `nextTick` that propagates though the request object. + - `request.aborted` attribute is set to `true` instead of a timestamp. [Changed in Node v11.0](https://github.com/nodejs/node/pull/20230). + - Calling `write`, `end`, or `flushHeaders` on an aborted request no longer emits an error. + However, writing to a request that is already finished (ended) will emit a 'write after end' error. + +1. Playback of a mocked responses will now never happen until the 'socket' event is emitted. + The 'socket' event is still artificially set to emit on `nextTick` when a ClientRequest is created. + This means in the following code the Scope will never be done because at least one tick needs + to happen before any matched Interceptor is considered consumed. + ```js + const scope = nock(...).get('/').reply() + const req = http.get(...) + scope.done() + ``` + The correct way to verify such an action is to call [`scope.done`](https://github.com/nock/nock#expectations) after waiting for a 'response', 'timeout', or 'socket' event on the request. + For example: + ```js + const scope = nock(...).get('/').reply() + const req = http.get(...) + req.on('response', () => { + scope.done() + }) + ``` From a7b0c13c70b3b5641d4a6d1e708d483ebcdeaee2 Mon Sep 17 00:00:00 2001 From: "Matt R. Wilson" Date: Thu, 28 May 2020 10:40:29 -0600 Subject: [PATCH 13/13] Merge next into beta (#2007) --- CONTRIBUTORS.md | 2 -- README.md | 10 +++++----- package-lock.json | 6 +++--- package.json | 2 +- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 471368a25..862231d4d 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -315,7 +315,6 @@ Gio d'Amelio @giodamelio https://github.com/giodamelio Girish Ramakrishnan @gramakri https://github.com/gramakri Golo Roden @goloroden https://github.com/goloroden Goran Gajic @gorangajic https://github.com/gorangajic -Greenkeeper @greenkeeperio-bot https://github.com/greenkeeperio-bot Greg Leppert @leppert https://github.com/leppert Gregor Martynus @gr2m https://github.com/gr2m Gregory Cowan @KrekkieD https://github.com/KrekkieD @@ -903,7 +902,6 @@ narendra @reddynr https://github.com/reddynr @galenus https://github.com/galenus @getlittletech https://github.com/getlittletech @gmatroskin https://github.com/gmatroskin -@greenkeeper null @handane123 https://github.com/handane123 @hellboy81 https://github.com/hellboy81 @hermano360 https://github.com/hermano360 diff --git a/README.md b/README.md index 9222231d0..6b4ed040e 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ [![npm](https://img.shields.io/npm/v/nock.svg)][npmjs] [![Build Status](https://travis-ci.org/nock/nock.svg)][build] ![Coverage Status](http://img.shields.io/badge/coverage-100%25-brightgreen.svg) -[![Greenkeeper](https://badges.greenkeeper.io/nock/nock.svg)](https://greenkeeper.io/) +![Dependabot Status](https://api.dependabot.com/badges/status?host=github&repo=nock/nock) [![Backers on Open Collective](https://opencollective.com/nock/backers/badge.svg)](#backers) [![Sponsors on Open Collective](https://opencollective.com/nock/sponsors/badge.svg)](#sponsors) @@ -1388,7 +1388,7 @@ nockBack('zomboFixture.json', nockDone => { If your tests are using promises then use `nockBack` like this: -``` +```js return nockBack('promisedFixture.json') .then(({ nockDone, context }) => { // do your tests returning a promise and chain it with @@ -1461,14 +1461,14 @@ The same is true for `.replyWithError()`. Adding `{ retry: 0 }` to the `got` invocations will disable retrying, e.g.: -``` -await got("http://example.test/", { retry: 0 }) +```js +await got('http://example.test/', { retry: 0 }) ``` If you need to do this in all your tests, you can create a module `got_client.js` which exports a custom got instance: -``` +```js const got = require('got') module.exports = got.extend({ retry: 0 }) diff --git a/package-lock.json b/package-lock.json index 1efc2d94d..8020da30a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9127,9 +9127,9 @@ "dev": true }, "prettier": { - "version": "2.0.2", - "resolved": "https://registry.npmjs.org/prettier/-/prettier-2.0.2.tgz", - "integrity": "sha512-5xJQIPT8BraI7ZnaDwSbu5zLrB6vvi8hVV58yHQ+QK64qrY40dULy0HSRlQ2/2IdzeBpjhDkqdcFBnFeDEMVdg==", + "version": "2.0.5", + "resolved": "https://registry.npmjs.org/prettier/-/prettier-2.0.5.tgz", + "integrity": "sha512-7PtVymN48hGcO4fGjybyBSIWDsLU4H4XlvOHfq91pz9kkGlonzwTfYkaIEwiRg/dAJF9YlbsduBAgtYLi+8cFg==", "dev": true }, "process-nextick-args": { diff --git a/package.json b/package.json index 00de7a5a9..a9b172daf 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "mocha": "^7.0.1", "npm-run-all": "^4.1.5", "nyc": "^15.0.0", - "prettier": "2.0.2", + "prettier": "2.0.5", "proxyquire": "^2.1.0", "request": "^2.83.0", "rimraf": "^3.0.0",