fix: Error w/ cause stack
#50
Conversation
|
It seems like an existing test case was already failing on |
test/test.js
Outdated
| it('should return Error message with 1 level cause trace', function (done) { | ||
| var err = new Error('foo', { cause: new Error('bar') }) | ||
|
|
||
| var expectedError = `Error: foo |
There was a problem hiding this comment.
I dont think we want tests which couple to the entire stack trace of mocha and this library. Similarly, we dont want the test to couple to the full html. Both of these mean even small unrelated changes can make this one brittle.
test/test.js
Outdated
| // The pathRegex is meant to find/replace the `/Users/.../finalhandler` parts of the expectedError | ||
| // The pathRegex is also intended to work when ran on other computers and from different root paths. | ||
| // Warning: this may or may not work for your local environment | ||
| var pathRegex = /\/Users(?:\/[^/]+){3}/g; |
There was a problem hiding this comment.
As with the above comment, this is much too brittle. I would prefer we just use the most simple string contains style check here to ensure the cause error is included. If you like though, you can also see above some tests which dont have so much hard coded assumptions in them.
There was a problem hiding this comment.
Agreed, what is the best way to go about testing this? I would prefer something a little more explicit in terms of checking the exact output vs just doing a string match against parts of the output.
My reasoning against the string match is that is leaves a lot of assumptions and doesn't necessarily validate that we get the full error output that we want in this case.
I actually had another possibly way to "mask" user/env specific paths via the process.cwd() and find/replace it in the final output. What do you think?
There was a problem hiding this comment.
Let me know if this is better? 39909fb
* fix: ignore status message for HTTP/2 * test: fix * test: fix node@9 * refactor: tests * test: fix * test: fix syntax
This is a replacement for pillarjs#49. I just pulled out the specific change related to this issue and added it here. Closes: expressjs/express#5630 test: Error w/ cause case fix: recursively find Error.cause stacks Remove the error.stack logic as it is redundant test: check for both 1 level & 2 level Error.cause refactor: use native util.format() API for Error printing fix: put back original lines of code test: update tests to be less brittle
wesleytodd
force-pushed
the
fix/error-cause-stack
branch
from
39909fb
to
afcdabb
Compare
|
I have been seeing this error with |
|
Oh, weird, it is more than just that. This is breaking because of something in this pr. I think it is the underlying error I mentioned, but then causing tests to fail in a bad way because of it. I think we are going to need to fix these. If you get a chance to look at them let me know. If not, then I might have to publish 2.0 before we can merge this. Ideally though we can land this as a minor. |
wesleytodd
force-pushed
the
2.x
branch
2 times, most recently
from
2afa91a
to
c45583d
Compare
I'm fine with either way. I can wait for 2.0. I'm not sure what the issues might be related to this PR. It is basically a 1 line change (not accounting for the changes you added). |
To replace: #49
The
causestack in the Error class was not included in the response text.This uses the native
util.format()API to log out the Error stack, which includes thecause. This is essentially the same asconsole.[error|log]()for anError.This PR closes expressjs/express#5630
This PR closes #41