Added compatibility with node-http-proxy header altering #1484
Conversation
|
Coveralls is breaking because there's a line that looks for empty headers, but headers are never empty now because nock always sets I'm not sure if having Might wait to see your comments (or if you think this is a good idea at all) before I proceed further :). |
lib/request_overrider.js
Outdated
| const reqHeaders = req.getHeaders ? req.getHeaders() : req._headers | ||
| if (Object.keys(reqHeaders).length > 0) { | ||
| options.headers = req.getHeaders ? req.getHeaders() : req._headers | ||
| } |
There was a problem hiding this comment.
Can this be replaced with options.headers = req.getHeaders()? The req.getHeaders truthiness check is unnecessary in the Node versions we support. That would also fix the coverage issue you mentioned.
There was a problem hiding this comment.
Yep sounds simpler - the only problem is that is causes some of the other tests that are expecting headers: undefined to fail because they now get headers: {}, but that's easy enough to change in the test if you're happy with it :).
tests/test_intercept.js
Outdated
| @@ -1711,3 +1712,43 @@ test('teardown', t => { | |||
| t.deepEqual(leaks, [], 'No leaks') | |||
| t.end() | |||
| }) | |||
|
|
|||
| test('works when headers removed by http-proxy', t => { | |||
There was a problem hiding this comment.
We're in the process of changing the way we test nock–client integration issues. These issues are important, though it's unwieldy to include large-bracket tests in this repo because it doesn't tell nock developers what the problem is. In addition, the internals of client libraries can change. While this is a decent regression test for the integration, it doesn't prevent against regression of the specific behaviors we want to ensure.
The idea is to build nock–client integration tests in separate repositories, which test the latest development version of nock against. We'd really welcome help getting that started! See a discussion in #1427 which goes into more detail about it.
What we need here is a unit test, probably using the http module, which exercises the issue you're seeing, in a way that a future developer working on nock can understand. Is it possible to express the bug that way?
In general we want to do the standard thing, not implement workarounds for libraries, so writing the test using http should help make it clear whether what we're doing here is sufficiently standard. (It sounds like it is, though TBH it's we're all still getting up to speed with the Nock codebase.)
There was a problem hiding this comment.
Fair enough - I think I can probably pull out what http-proxy is doing into the test itself and remove the dependency :).
|
Hey @AlexGilleran, thought I would follow up on this! Would you have time to take a look at writing a more isolated test? |
|
Oops yep sorry, I'll get back on it :) |
# Conflicts: # package-lock.json # package.json
|
@paulmelnikow - simplified the code change and rewrote the tests to only depend on |
|
@AlexGilleran Could you remove your changes from package-lock.json and merge the beta branch into your branch again? There is a conflict because some other tests were also added to the end of test_intercept.js. |
|
@mastermatt done! Thing is that because There's also a few formatting changes even after I manually prettier-ified everything using the version in package.json - I can manually revert if needed. |
There was a problem hiding this comment.
@AlexGilleran Thanks, this looks great.
I expected those header checks to go away, so I think we're good to go here.
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks so much for the great project, really really helpful for testing node microservices.
This PR makes it possible to test node-http-proxy using nock. Node-http-proxy is essentially a reverse proxy for node - you can get incoming http calls, modify them and redirect them to another url.
One of its features is being able to modify headers programmatically with code like this:
If you try to make it proxy to a nock mock, however, you'll find that the headers it recieves are always the unmodified ones (i.e. "X-Special-Proxy-Header" won't be present) because of how nock copies headers out of the original
http.requestcall intooptions.This adds a bit of code to copy the headers out of the
reqat the last moment in case they've changed, similar to howreq.pathis copied back intooptions.pathfor superagent compatibility.