feat: Add lifecycle function to clear pending timeouts #1748

TLPcoder · 2019-10-17T23:49:55Z

…ay which causes it to not be removed when cleanAll is called

mastermatt

Couple of suggestions.

lib/common.js

mastermatt · 2019-10-18T14:18:46Z

lib/common.js

@@ -593,6 +593,22 @@ function deepEqual(expected, actual) {
  return expected === actual
 }

+const timeouts = []
+
+function addTimeout(id) {


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 }

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.

paulmelnikow · 2019-10-18T23:05:48Z

tests/test_common.js

+  }, 1)
+
+  common.addTimeout(setTimeout(timeoutSpy, 0))
+  common.removeAllTimeouts()


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.)

paulmelnikow · 2019-10-18T23:21:33Z

lib/intercept.js

@@ -124,6 +124,7 @@ function remove(interceptor) {
 }

 function removeAll() {
+  common.removeAllTimeouts()


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.

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.

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!

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!

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

Be happy to help where I can.

…mer ids to later delete when abortPendingRequests is called. Updated clear timer from recursion to a while up. Update from only timeout to covering all three timers, updated docs with new lifecycle method abortPendingRequests, updated types with new lifecycle method

paulmelnikow · 2019-10-21T11:01:15Z

Any objections to adding nock.abortPendingRequests() to the API, as an alternative to putting this potentially surprising functionality in cleanAll()? @nock/maintainers

gr2m · 2019-10-21T18:07:40Z

No objections, sounds good to me

… clearAll-1118

TLPcoder · 2019-10-23T01:09:12Z

screen froze and i think i ended up trying to merge a branch my apologizes

paulmelnikow · 2019-10-23T08:44:08Z

Hey @mastermatt are you satisfied with the code changes?

mastermatt

Even updated the type defs ❤️

nockbot · 2019-10-24T09:08:47Z

🎉 This PR is included in version 11.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Trevor Pellegrini added 2 commits October 17, 2019 16:43

fix:#1118 fixes the bug where there is a midflight request with a del…

19b4065

…ay which causes it to not be removed when cleanAll is called

prettier

e0b6ff8

mastermatt requested changes Oct 18, 2019

View reviewed changes

paulmelnikow changed the title ~~Clear all 1118~~ feat: Add lifecycle API to clear pending timeouts Oct 18, 2019

paulmelnikow reviewed Oct 18, 2019

View reviewed changes

paulmelnikow changed the title ~~feat: Add lifecycle API to clear pending timeouts~~ feat: Add lifecycle function to clear pending timeouts Oct 18, 2019

paulmelnikow mentioned this pull request Oct 18, 2019

docs: Mention strategy of testing common utility code #1749

Merged

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #1757

Closed

TLPcoder and others added 2 commits October 22, 2019 17:42

Merge branch 'master' into clearAll-1118

c6524c4

Merge branch 'clearAll-1118' of https://github.com/TLPcoder/nock into…

9eb850c

… clearAll-1118

mastermatt approved these changes Oct 24, 2019

View reviewed changes

paulmelnikow merged commit 187db85 into nock:master Oct 24, 2019

nockbot added the released label Oct 24, 2019

paulmelnikow mentioned this pull request Oct 30, 2019

docs: Revise and accept RFC-001 #1753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add lifecycle function to clear pending timeouts #1748

feat: Add lifecycle function to clear pending timeouts #1748

TLPcoder commented Oct 17, 2019 •

edited by mastermatt

mastermatt left a comment

mastermatt Oct 18, 2019

TLPcoder Oct 18, 2019

paulmelnikow Oct 18, 2019

paulmelnikow Oct 18, 2019 •

edited

TLPcoder Oct 19, 2019

TLPcoder Oct 19, 2019

paulmelnikow Oct 19, 2019

TLPcoder Oct 21, 2019

paulmelnikow commented Oct 21, 2019

gr2m commented Oct 21, 2019

TLPcoder commented Oct 23, 2019

paulmelnikow commented Oct 23, 2019

mastermatt left a comment

nockbot commented Oct 24, 2019

feat: Add lifecycle function to clear pending timeouts #1748

feat: Add lifecycle function to clear pending timeouts #1748

Conversation

TLPcoder commented Oct 17, 2019 • edited by mastermatt

mastermatt left a comment

Choose a reason for hiding this comment

mastermatt Oct 18, 2019

Choose a reason for hiding this comment

TLPcoder Oct 18, 2019

Choose a reason for hiding this comment

paulmelnikow Oct 18, 2019

Choose a reason for hiding this comment

paulmelnikow Oct 18, 2019 • edited

Choose a reason for hiding this comment

TLPcoder Oct 19, 2019

Choose a reason for hiding this comment

TLPcoder Oct 19, 2019

Choose a reason for hiding this comment

paulmelnikow Oct 19, 2019

Choose a reason for hiding this comment

TLPcoder Oct 21, 2019

Choose a reason for hiding this comment

paulmelnikow commented Oct 21, 2019

gr2m commented Oct 21, 2019

TLPcoder commented Oct 23, 2019

paulmelnikow commented Oct 23, 2019

mastermatt left a comment

Choose a reason for hiding this comment

nockbot commented Oct 24, 2019

TLPcoder commented Oct 17, 2019 •

edited by mastermatt

paulmelnikow Oct 18, 2019 •

edited