Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add lifecycle function to clear pending timeouts #1748

Merged
merged 5 commits into from Oct 24, 2019
Merged

feat: Add lifecycle function to clear pending timeouts #1748

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19b4065
fix:#1118 fixes the bug where there is a midflight request with a del…
Oct 17, 2019
e0b6ff8
prettier
Oct 17, 2019
ef7972b
fix:#1748 updated codebase to use common[timers] so we can capture ti…
Oct 19, 2019
c6524c4
Merge branch 'master' into clearAll-1118
TLPcoder Oct 23, 2019
9eb850c
Merge branch 'clearAll-1118' of https://github.com/TLPcoder/nock into…
Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/common.js
View file
Open in desktop
Expand Up @@ -593,6 +593,22 @@ function deepEqual(expected, actual) {
return expected === actual
}

const timeouts = []

function addTimeout(id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would clean up the calling code if, instead of taking a timeout ID, we had a setTimeout function that wrapped the native call and saved the id. eg:

function setTimeout(callback, delay, ...args) {
  const id = timers.setTimeout(callback, delay, ...args)
  timeouts.push(id)
  return id
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if that is the approach you would like to take, i would also like to make the suggestion of adding our own timers module so we are able to follow the same pattern for all timers and not just setTimeout, but also setInterval(maybe used in the future) and setImmediate. As This will provide the opportunity to cover similar uses cases moving forward with less code changes. Dont think doing the same for nextTick is needed as there is no clear method.

timeouts.push(id)
}

function removeAllTimeouts() {
if (_.isEmpty(timeouts)) {
return
}

clearTimeout(timeouts.shift())

return removeAllTimeouts()
}
TLPcoder marked this conversation as resolved.
Show resolved Hide resolved

exports.normalizeClientRequestArgs = normalizeClientRequestArgs
exports.normalizeRequestOptions = normalizeRequestOptions
exports.normalizeOrigin = normalizeOrigin
Expand All @@ -615,3 +631,5 @@ exports.matchStringOrRegexp = matchStringOrRegexp
exports.formatQueryValue = formatQueryValue
exports.isStream = isStream
exports.dataEqual = dataEqual
exports.addTimeout = addTimeout
exports.removeAllTimeouts = removeAllTimeouts
20 changes: 11 additions & 9 deletions lib/delayed_body.js
View file
Open in desktop
Expand Up @@ -34,15 +34,17 @@ module.exports = class DelayedBody extends Transform {

// TODO: This would be more readable if the stream case were moved into
// the `if` statement above.
setTimeout(function() {
if (common.isStream(body) && !ended) {
body.once('end', function() {
self.end(data)
})
} else {
self.end(data || body)
}
}, ms)
common.addTimeout(
setTimeout(function() {
if (common.isStream(body) && !ended) {
body.once('end', function() {
self.end(data)
})
} else {
self.end(data || body)
}
}, ms)
)
}

_transform(chunk, encoding, cb) {
Expand Down
1 change: 1 addition & 0 deletions lib/intercept.js
View file
Open in desktop
Expand Up @@ -124,6 +124,7 @@ function remove(interceptor) {
}

function removeAll() {
common.removeAllTimeouts()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for adding this functionality. 👍Thank you for picking this up! 💯

Though I'm not sure I think it belongs in the call to clearAll(). That function's job is to remove interceptors from the list. In contrast, what's happening here is that we're cancelling playback of an interceptor that has already matched and is in process.

I'd suggest exporting this as a new function, maybe calling it nock.abortRequests() or nock.abortPendingRequests(). The test name in common seems fine, "timeouts" being the implementation.

In RFC-001 I advocated creating a new lifecycle method to reset nock to its initial state.

I am not sure whether aborting current requests should be added to that reset. There is a bit of discussion in #1118 about Mocha leaving the process running until the pending request flushes, though I think I could use a bit more clarification about the use case and why aborting the requests is what's desirable. (It's not the behavior of real HTTP to abort pending requests when tests finish.) The reason Mocha doesn't quit the process anymore is to not paper over sloppy cleanup. I'm happy to let people force automatic cleanup if they want it, though others may prefer to clean things up on their own. Mocha's choice isn't everyone's cup of tea but it's arguably a good default.

That reset function doesn't exist yet; as of now you need to run nock.restore(); nock.cleanAll(); nock.enableNetConnect(); nock.activate() and when we add nock.reset() I'd suggest we stick with those four calls to start. And that developers call nock.abortPendingRequests() followed by nock.reset() if they're experiencing this problem. We can gather feedback on the use cases over time before deciding to include that reset function by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit was created to close the original bug which complained about cleanAll not removing pending request. However, I agree with you that cleanAll should stay as is and a new method should be exposed I am partial to"abortPendingRequests". I dont see a current issue raised for nock.reset() be happy to work on that too. Sounds like it could be nocked out quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant clarify why this behavior is desirable, I personally am confused why someone needs it as well. I was just looking to contribute to your project because i was able to take the mocking approach and apply to one of my own projects. My way of saying thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @TLPcoder I am really happy to have your help on the library. Nock is several years old – definitely a legacy project – and we've done a good job of getting it in shape to work on. @mastermatt has also done a lot of great work to clarify the ambiguous parts of the API. At the same time it still has a huge API surface, mostly because of the inclusionist way it was maintained, which was to merge many PRs that handled everyone's various niche use cases. A goal is to pare that back where possible, and I do want to be careful and thoughtful about expanding the API surface. Re the new lifecycle API, what I'd suggest is that we start by reviving #1441. Basically that PR got really big, and should be broken out into four separate RFCs. (The analysis of lifecycle methods RFC-001 came from the preamble that preceded the four proposals, via #1474.) The comments on the proposals also need to be incorporated.

In part the goal of the RFCs is to generate comments; and in part it's to have a thoughtful analysis and discussion that folks can read to understand the rationale as they migrate to the new APIs.

Would love to have your help on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think guys did a good job refactoring the code base and moving everything over to ES6.

Be happy to help where I can.

Object.keys(allInterceptors).forEach(function(key) {
allInterceptors[key].scopes.forEach(function(interceptor) {
interceptor.scope.keyedInterceptors = {}
Expand Down
6 changes: 4 additions & 2 deletions lib/playback_interceptor.js
View file
Open in desktop
Expand Up @@ -106,7 +106,9 @@ function playbackInterceptor({
} else {
error = new Error(interceptor.errorMessage)
}
timers.setTimeout(() => emitError(error), interceptor.getTotalDelay())
common.addTimeout(
timers.setTimeout(() => emitError(error), interceptor.getTotalDelay())
)
return
}

Expand Down Expand Up @@ -338,7 +340,7 @@ function playbackInterceptor({
interceptor.delayConnectionInMs > 0
) {
socket.applyDelay(interceptor.delayConnectionInMs)
setTimeout(respond, interceptor.delayConnectionInMs)
common.addTimeout(setTimeout(respond, interceptor.delayConnectionInMs))
} else {
respond()
}
Expand Down
13 changes: 13 additions & 0 deletions tests/test_common.js
View file
Open in desktop
@@ -1,6 +1,7 @@
'use strict'

const http = require('http')
const sinon = require('sinon')
const { test } = require('tap')
const common = require('../lib/common')
const matchBody = require('../lib/match_body')
Expand Down Expand Up @@ -486,3 +487,15 @@ test('normalizeClientRequestArgs with a single callback', async t => {
t.deepEqual(options, {})
t.is(callback, cb)
})

test('addTimeout / removeAllTimeouts', t => {
const timeoutSpy = sinon.spy()

setTimeout(() => {
t.equal(timeoutSpy.called, false)
t.end()
}, 1)

common.addTimeout(setTimeout(timeoutSpy, 0))
common.removeAllTimeouts()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nock's strategy is to test as much as possible either through the public API or, when that is not possible, through the mock surface. Would it be possible to do that, maybe modeling the test after one of the tests in test_delay and adding it to test_nock_lifecycle?

(This has been discussed in a bunch of PRs but isn't really documented anywhere else; will open a PR to add it.)

})
18 changes: 18 additions & 0 deletions tests/test_delay.js
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
const fs = require('fs')
const path = require('path')
const http = require('http')
const sinon = require('sinon')
const stream = require('stream')
const assertRejects = require('assert-rejects')
const mikealRequest = require('request')
Expand Down Expand Up @@ -491,3 +492,20 @@ test('mikeal/request with delayConnection and request.timeout', t => {
}
)
})

test('remove midflight delay when cleanAll is called', t => {
const reqSpy = sinon.spy()

nock('http://example.test')
.get('/')
.delayConnection(100)
.reply(200, 'OK')

http.get('http://example.test', reqSpy)

setTimeout(() => {
t.equal(reqSpy.called, false)
t.end()
}, 200)
process.nextTick(nock.cleanAll)
})