Wrapping RequestError obscures its source
#795
Comments
|
What Node.js version is this from? Are you sure this is caused by wrapping the error and not by catching and then emitting the error asynchronously? Note that Node.js 12 has support for async stack traces, so the error stacks should ideally be much better there. Could you try? |
|
Hi! Sorry it's taken me a moment to get back to this. I'm testing now in Node 10, and it's definitely the wrapping and not the async emitting that's causing it. If I comment out the wrapping and With wrapping: Without: The stack trace of the wrapped errors in Node 12 looks the same as in Node 10: |
|
@paulmelnikow Can you provide some code example? |
|
Sure! Here are two fairly small examples that show two different ways errors can be emitted through Nock. The first one trigger's nock's "no match for request" error, which is triggered when the application makes an unmocked request to a mocked hostname. It's one of the most common errors in Nock. The second one simulates a programmer error in a nock consumer. const nock = require('.')
const got = require('got').extend({ retry: 0 })
async function noMatch() {
nock('http://example.com')
.get('/')
.reply()
await got('http://example.com/this-route-is-not-mocked')
}
async function programmerError() {
nock('http://example.com')
.get('/')
.reply(() => {
throw Error('This is a programmer error in test.js')
})
await got('http://example.com/')
}
;(async () => {
try {
await noMatch()
} catch (e) {
console.error(e)
}
nock.cleanAll()
try {
await programmerError()
} catch (e) {
console.error(e)
}
process.exit(0)
})()I tried to add it in RunKit but I'm not sure how to request the beta release of nock. This was tested on the beta branch of nock though it works equally well with the latest beta release 11.0.0-beta.23. There are a few places in to what I'd like to see: |
|
Thanks for the quick response! I'll solve this issue tomorrow because it's late here. |
|
Amazing! Let me know if there's anything I can help clarify :) |
const oldStack = error.stack;
error = new RequestError(error, options);
console.log('OLD STACKTRACE', oldStack, '\n', 'NEW STACKTRACE', error.stack);gives me: IOW |
|
Node solves this via: https://github.com/nodejs/node/blob/cc6f99de449beb0993db3647de4ef979bead804d/lib/internal/errors.js#L870-L878 Possible alternatives:
I think we should do the same as Node. |
|
Another solution would be to |
szmarczak
changed the title
RequestError obscures its source
How does the error stack trace look when you do that? |
|
It looks like this: It'll be more readable without the first line of the NockError because it's already contained in the RequestError: |
|
The last one looks good 👍 |
|
Thanks so much for fixing this! It’s going to make debugging nock much easier. When you release another beta I’d be glad to take it for a spin. |
|
No problem! If there are any enhancements left to be done, just open an issue, we're happy to help :) |
What would you like to discuss?
When I’m working on and with nock, I often see stack traces that look like this:
Or sometimes:
The underlying stack trace is this:
However when got wraps the error in a RequestError, the original stack trace is no longer available.
To be fair, part of what makes this confusing is that the test runner’s output (which has nothing to do with nock or got) and looks like this:
For a long time it was unclear that the error and had anything to do with Nock. Nock doesn’t appear in the stack trace despite being the layer that threw the error. And the test runner's output doesn't make it quite so clear to me that “Nock: No match for request” was the message for the error that was being emitted by got.
There is not much that is documented about RequestError, though I imagine the benefit of wrapping errors in RequestError is to associate with the thrown error the additional bits information about the request. In this case, however, the wrapping did not provide a benefit. Instead, it made it harder to understand what was happening.
Some possible ideas for making this easier for developers to understand:
error.stackand attach it to the new RequestError. This could be done in GotError’s constructor. I experimented with this locally and it seems to work. (Though it is slightly disorienting to see Nock’s stack trace attached to something called RequestError, which is not from nock.)optionsto the existing error object.I’d be really curious to hear your thoughts on (1) and (2)! Regardless of what's decided here, I think (3) could be worth pursuing, as it will make it easier both to test nock and also to use it.
I can’t find a discussion on this specific topic, though there was some discussion about async stack traces in #279 and #447.
Thanks so much for all your work on this amazing library!
Checklist
The text was updated successfully, but these errors were encountered: