#1836 fixes expected Node behaviour for http.get #1853
Conversation
|
@paulmelnikow, do you have any time to review this PR soon? Thanks, Kenneth |
|
Is there any repo manager that can please review this for merging? This issue is bedrock for a hierarchy of issues blocking progress for me at work right now. |
|
I am on the same boat, because newrelic depends on this. |
|
Sorry, I’ve been really swamped. I’ll try to take a look at this today. |
tests/test_common.js
Outdated
| @@ -512,3 +512,30 @@ test('testing timers are deleted correctly', t => { | |||
| t.end() | |||
| }) | |||
| }) | |||
|
|
|||
| test('correct node behavior', t => { | |||
There was a problem hiding this comment.
Could this test title capture the stated goal?
This is from the original PR:
The test makes sure http.request is not called when using http.get.
… on a mocked request? Is that it?
Nock has accumulated so much unusual code over the years to work around other libraries quirks, and it’s important that we can document why things are done the way they are. Same goes for a test. You’ll find a lot of tests like “filteringScope works” but it’s not as helpful in the future as saying the behavior that is wanted.
There was a problem hiding this comment.
Having dug into this a little more, I think this is not the right test. I think what we want to test is that the overridden / original http.request() is invoked only once by http.get(), instead of twice. That's the stated bug of #1836.
I'm going to dig in a bit and see if I can express that in a new test.
There was a problem hiding this comment.
I might have expressed myself a bit poorly in #1836. The main problem is that the native node behavior is not calling req.request when req.get is called. Nock does that in common.js. When using nock together with another library like newrelic that also overwrites these functions, you get a wrong call.
There was a problem hiding this comment.
This test checks that you don't call req.request when req.get is called, which is the native node behavior. There is not really anything else to test here in my opinion. I'll rename the test to what you suggested.
There was a problem hiding this comment.
Hey, thanks, would appreciate your help to clarify at #1836 (comment).
|
I realize this has a high ecosystem impact and I'd like to get this merged and released in the next day or so. I need some clarification on what the bug's behavior is so I can write a regression test that expresses it clearly. If folks following this thread understand what's happening in New Relic, I'd appreciate your comments in #1836. |
….request Thanks @kennethaasan for finding this fix! Fix #1836 Closes #1853
|
Closing this PR in favor of #1868 |
|
🎉 This issue has been resolved in version 11.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #1836
@mastermatt @paulmelnikow, sorry but I had to make a new PR because I fucked up git when doing a rebase. Would be great if you could release this ASAP so that we can fix newrelic/node-newrelic#316