Add tests to increase coverage of the Request Overrider. #1577
Conversation
Returning early from this function was moved to the top with 28f6b29, the original check was left behind.
Because of the way we override `http.request`, this callback is only defined when `http.ClientRequest` is called directly with a callback.
In the normal error case, that the value of response body is a string, but not hex encoded, the attempt to create a Buffer will return a Uint8Array with nothing in it. Looking at the [Node source for Buffer.from](https://github.com/nodejs/node/blob/2817fde844de39b0e8c17c352433ee6124b5ef09/lib/buffer.js#L196), the reason errors are thrown are: - The first arg isn't a value type, but we've already ensured it's a string. - The encoding arg is not valid, but we hard code 'hex'.
| http.get(reqOpts, res => { | ||
| res.on('error', err => { | ||
| t.is(err, 'oh no!') | ||
| t.done() |
| .reply(201, (path, requestBody, callback) => { | ||
| callback(null, 'one') | ||
| callback(null, 'two') | ||
| callback(null, 'three') |
There was a problem hiding this comment.
Should we add a t.ok() here with a t.plan() to make sure we get all the way through this function before the test finishes?
| const scope = nock('http://example.test') | ||
| .get('/') | ||
| .reply((path, requestBody, callback) => { | ||
| callback(Error('boom')) |
| t.match( | ||
| err, | ||
| Error( | ||
| 'Response delay of the body is currently not supported with content-encoded responses.' |
| // TODO-coverage: Add a test of this error case. | ||
| debug( | ||
| 'exception during Buffer construction from hex data:', | ||
| responseBody, |
There was a problem hiding this comment.
I broke out each change into separate commits with descriptions in the messages for the non-trivial ones.
de1d60a
In the normal error case, that the value of response body is a string, but
not hex encoded, the attempt to create a Buffer will return a Uint8Array
with nothing in it.
Looking at the Node source for Buffer.from, the reason errors are
thrown are:
- The first arg isn't a valid type, but we've already ensured it's a string.
- The encoding arg is not valid, but we hardcoded 'hex'.
Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>
|
@paulmelnikow I think this my first pr since becoming a maintainer, what's the protocol for determining when a PR can be merged? |
|
Generally we're looking for one 👍 review from another maintainer unless you feel extra discussion is warranted. It took me a while to get the hang of conventional commit messages; feel free to ping me or @gr2m if you need help determining what to use. For this I'd probably write refactor(request_overrider): Remove unreachable code and increase coverage |
|
🎉 This PR is included in version 11.0.0-beta.18 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This, along with another PR I have for headers, will take
lib/request_overrider.jsto 100% branch coverage.For #1404