feat: Support http.request signatures added in Node 10.9+ by mastermatt · Pull Request #1588 · nock/nock

mastermatt · 2019-06-17T02:44:38Z

Fixes #1227
Supersedes #1428

Beginning with v10.9.0, the url parameter can be passed along with a
separate options object to http[s].request, http[s].get, or to the
constructor of ClientRequest. docs

The logic that does the juggling was mostly copied from Node's source.

There was a bit of other cleanup that was committed separately to aid with review.

paulmelnikow · 2019-06-17T09:35:30Z

This is great! I can give it a read tomorrow.

mastermatt · 2019-06-18T19:35:29Z

fyi @nijynot and @gkalpak

Instead of passing it all around the overrider. This mimics how the native implementation does it.

Fixes nock#1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source.

mastermatt · 2019-07-06T20:35:43Z

@paulmelnikow were you able to read through this pr?

paulmelnikow · 2019-07-06T20:39:14Z

Ah no, I think I lost track of it. Sorry! I'll aim to take a look tonight or tomorrow!

paulmelnikow

Thanks so much for this work! I understand most of it, I think, though I left a few questions.

I'll take another pass through thinking about test coverage though this is looking really good.

Sorry it's taken me so long to read this!

lib/common.js

lib/intercept.js

paulmelnikow · 2019-07-08T17:01:09Z

lib/intercept.js

+
+    // The option per the docs is `protocol`. Its unclear if this line is meant to override that and is misspelled or if
+    // the intend is to explicitly keep track of which module was called using a separate name.
+    // Either way, `proto` is used as the source of truth from here on out.


Not sure what we ought to do here but really appreciate this comment!

paulmelnikow · 2019-07-08T17:02:03Z

lib/intercept.js

-        res.on('response', callback)
-      }
-      return req
+      return new http.ClientRequest(options, callback)


paulmelnikow · 2019-07-08T17:03:18Z

lib/intercept.js

      Object.assign(this, overrider)
+
+      if (callback) {
+        this.once('response', callback)


lib/intercept.js

lib/recorder.js

Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>

paulmelnikow · 2019-07-08T18:07:35Z

tests/test_common.js

  t.done()
 })
+
+test('normalizeClientRequestArgs throws for invalid URL', async t => {


@gr2m has expressed a preference for testing our helper functions through real use cases in the public API wherever possible, and it seems to pay off well. It also has the advantage of surfacing code that is no longer needed.

Would you be up for expressing these tests in terms of calls to http?

Yes please 👍

paulmelnikow · 2019-07-08T18:07:54Z

tests/test_intercept.js

+      scope.done()
+      t.done()
+    })
+  })


tests/test_intercept.js

gr2m

Could you update the docs to mention the new request signature?

paulmelnikow · 2019-07-08T18:24:59Z

Could you update the docs to mention the new request signature?

This PR doesn't change the documented API, rather it makes the mock surface compatible with Node 10.9+ (#1227).

I don't think we have documentation of the supported mock surface, beyond the tests. We could add some, though I don't think it needs to be in this PR.

gr2m · 2019-07-08T18:36:41Z

Okay, thank you for the clarification

mastermatt · 2019-07-08T22:29:29Z

I believe I've addressed all the comments.
@gr2m @paulmelnikow could I get another pass from you?

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

tests/test_common.js

paulmelnikow

Feel free to merge when ready!

…-signature

nockbot · 2019-07-09T05:39:07Z

🎉 This PR is included in version 11.0.0-beta.22 🎉

The release is available on:

Your semantic-release bot 📦🚀

nockbot · 2019-08-12T23:56:25Z

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.

kennethaasan · 2019-12-18T21:30:43Z

lib/common.js

-      )
+    // https://nodejs.org/api/http.html#http_http_get_options_callback
+    module.get = function(input, options, callback) {
+      const req = module.request(input, options, callback)


Why is this not calling newRequest instead? This breaks the Node API as well

This is done to mimic native Node behavior in case other libs are monkey-patching the function.
Can you provide more details on "This breaks the Node API as well"? Preferably in a new issue.

I can make a new issue. But writing the details here as well.

New Relic (https://github.com/newrelic/node-newrelic) is overwriting http.get and http.request like you do. But when http.get is calling http.request like here, a call to http.get will call http.request twice when using nock in a New Relic test.

To prove that this is not native Node behaviour (at least on Node 12):

const http = require('http'); function request() { console.log('http.request', arguments); } http.request = request; // our request function is not called http.get('http://google.com', () => {});

Ref nock#1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in nock#1588. Also ref nock#1607

* Allow three arg form of `request` and `ClientRequest`. Fixes nock#1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.

mastermatt added the feature label Jun 17, 2019

mastermatt requested a review from paulmelnikow June 17, 2019 02:44

mastermatt force-pushed the support-new-request-signature branch from 826d859 to 3efe155 Compare June 17, 2019 03:09

mastermatt force-pushed the support-new-request-signature branch from aa4d82c to 9a7ec1b Compare June 18, 2019 16:22

mastermatt added 4 commits June 22, 2019 21:37

Remove unnecessary overrider in intercept.

f598816

Consolidate var declarations in intercept.

3c93c3d

Register ClientRequest callback on response event.

e2e59e4

Instead of passing it all around the overrider. This mimics how the native implementation does it.

mastermatt force-pushed the support-new-request-signature branch from 9a7ec1b to fd54a81 Compare June 23, 2019 03:39

weekly-digest bot mentioned this pull request Jun 23, 2019

Weekly Digest (16 June, 2019 - 23 June, 2019) #1594

Closed

weekly-digest bot mentioned this pull request Jun 30, 2019

Weekly Digest (23 June, 2019 - 30 June, 2019) #1600

Closed

weekly-digest bot mentioned this pull request Jul 7, 2019

Weekly Digest (30 June, 2019 - 7 July, 2019) #1604

Closed

Merge branch 'beta' into support-new-request-signature

a03480b

paulmelnikow mentioned this pull request Jul 7, 2019

fix(intercept): Add support for node 10.x ClientRequest #1428

Closed

paulmelnikow reviewed Jul 8, 2019

View reviewed changes

mastermatt and others added 2 commits July 8, 2019 11:34

Prefer Object.keys over lodash.isEmpty.

9dac372

Apply suggestions from code review

759783a

Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>

paulmelnikow reviewed Jul 8, 2019

View reviewed changes

gr2m reviewed Jul 8, 2019

View reviewed changes

paulmelnikow mentioned this pull request Jul 8, 2019

Document the mock surface #1606

Open

paulmelnikow changed the title ~~feat: Support new request signature~~ feat: Support http.request signatures added in Node 10.9+ Jul 8, 2019

This was referenced Jul 8, 2019

11.x release plan #1516

Closed

Help wanted to reach ~100% code coverage #1404

Closed

Add coverage ignores to recorder, and emit text report #1608

Merged

mastermatt mentioned this pull request Jul 8, 2019

refactor: simplify some cases of common.formatQueryValue #1598

Merged

Update tests per PR feedback.

e8e90b5

paulmelnikow added a commit that referenced this pull request Jul 8, 2019

Add coverage ignores to recorder, and emit text report (#1608)

2bca61b

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

paulmelnikow reviewed Jul 8, 2019

View reviewed changes

tests/test_common.js Outdated Show resolved Hide resolved

tests/test_common.js Outdated Show resolved Hide resolved

paulmelnikow approved these changes Jul 8, 2019

View reviewed changes

gr2m approved these changes Jul 8, 2019

View reviewed changes

mastermatt added 2 commits July 8, 2019 23:22

Better use of .basicAuth for test.

fdd0688

Merge remote-tracking branch 'upstream/beta' into support-new-request…

17f203c

…-signature

mastermatt merged commit e3e6a65 into nock:beta Jul 9, 2019

mastermatt deleted the support-new-request-signature branch July 9, 2019 05:32

nockbot added the released label Jul 9, 2019

weekly-digest bot mentioned this pull request Jul 14, 2019

Weekly Digest (7 July, 2019 - 14 July, 2019) #1618

Closed

gr2m pushed a commit that referenced this pull request Sep 4, 2019

Add coverage ignores to recorder, and emit text report (#1608)

50791bc

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

gr2m pushed a commit that referenced this pull request Sep 4, 2019

Add coverage ignores to recorder, and emit text report (#1608)

ba15ec0

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

nockbot mentioned this pull request Sep 4, 2019

The automated release is failing 🚨 #1704

Closed

gr2m pushed a commit that referenced this pull request Sep 5, 2019

Add coverage ignores to recorder, and emit text report (#1608)

4fd70a6

Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607

kennethaasan reviewed Dec 18, 2019

View reviewed changes

This was referenced Dec 18, 2019

Support http/https APIs for node >= 10 newrelic/node-newrelic#316

Closed

Breaking expected Node behaviour for http.get #1836

Closed

Uh oh!

Conversation

mastermatt commented Jun 17, 2019

Uh oh!

paulmelnikow commented Jun 17, 2019

Uh oh!

mastermatt commented Jun 18, 2019

Uh oh!

mastermatt commented Jul 6, 2019

Uh oh!

paulmelnikow commented Jul 6, 2019

Uh oh!

paulmelnikow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gr2m left a comment

Choose a reason for hiding this comment

Uh oh!

paulmelnikow commented Jul 8, 2019

Uh oh!

gr2m commented Jul 8, 2019

Uh oh!

mastermatt commented Jul 8, 2019

Uh oh!

Uh oh!

Uh oh!

paulmelnikow left a comment

Choose a reason for hiding this comment

Uh oh!

nockbot commented Jul 9, 2019

Uh oh!

nockbot commented Aug 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants