assert: improve AssertionError in case of "Errors" by BridgeAR · Pull Request #15025 · nodejs/node

BridgeAR · 2017-08-25T02:42:46Z

Showing the stack trace in a error message obfuscates the actual
message and should not be visible therefore.

I think this is actually part of the assert subsystem even though the AssertionError is placed in the internal/errors.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

assert

Showing the stack trace in a error message obfuscates the actual message and should not be visible therefore.

BridgeAR · 2017-08-25T03:59:30Z

CI https://ci.nodejs.org/job/node-test-pull-request/9834/

jasnell

This is likely a semver-major change

BridgeAR · 2017-08-25T19:36:22Z

@jasnell I personally would consider it as a bugfix as I highly doubt this was really anticipated and ever wanted behavior.

jasnell · 2017-08-25T19:37:25Z

I would tend to agree. Let's see if others in @nodejs/ctc do also :)

Trott · 2017-08-25T20:50:27Z

I agree it's a bugfix, but a CITGM run for good measure wouldn't be a terrible idea.

jasnell · 2017-08-25T22:55:45Z

When I'm actually able to get to CI I'll give it a run.

thefourtheye · 2017-08-26T00:40:30Z

+ if (message) {
+ super(message);
 } else {
+ if (actual && actual.stack && actual instanceof Error)


Isn't actual instanceof Error itself enough?

It is but it has a performance implication (it might not be important because it is only about errors but it is something that I always like to consider).

Hmmm.. actually, I believe .stack is a getter isn't it? It will have a performance impact also.

@jasnell util.inspect will look at the stack as well. The first call to the stack is heavy and it does not matter if it is done here or later on (well it is actually not anymore as we only pass a string through instead of the error but the performance for the error will stay the same as before and the average case wont have any negative hit).

So yes, it would be faster for Error objects (the instanceof check is added on top of the stack access) but slower for the average case.

Yeah. Hmm. It's likely fine. Doing it the same way internal/util isError does would likely be ok also but I'm good with it :)

BridgeAR · 2017-08-29T03:44:13Z

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/959/

BridgeAR · 2017-08-30T20:23:27Z

Ping @nodejs/ctc

BridgeAR · 2017-09-02T06:36:16Z

Ping @nodejs/tsc PTAL

BridgeAR · 2017-09-03T23:06:58Z

It would also be nice to get a comment about

@BridgeAR: I personally would consider it as a bugfix as I highly doubt this was really anticipated and ever wanted behavior.

@jasnell: I would tend to agree. Let's see if others in @nodejs/ctc do also :)

BridgeAR · 2017-09-13T21:34:17Z

PTAL @nodejs/tsc (semver-patch / semver-major)

targos · 2017-09-13T21:44:22Z

+1 for bug fix / semver-patch

joyeecheung · 2017-09-14T10:14:22Z

+1 for semver-patch

jasnell · 2017-09-14T18:25:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10082/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/983/

BridgeAR · 2017-09-15T21:53:01Z

Thanks a lot! Landed in 2e8217c

Showing the stack trace in a error message obfuscates the actual message and should not be visible therefore. PR-URL: #15025 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Showing the stack trace in a error message obfuscates the actual message and should not be visible therefore. PR-URL: nodejs/node#15025 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

jasnell · 2017-09-20T21:23:28Z

This does not land cleanly on v8.x, a backport would be needed.

BridgeAR · 2017-09-20T23:41:52Z

This should land cleanly as soon as #14167 got in. I open a backport for that in a few minutes.

Showing the stack trace in a error message obfuscates the actual message and should not be visible therefore. PR-URL: #15025 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

jasnell · 2017-09-21T14:56:09Z

landed in v8.x-staging. Thank you!

Showing the stack trace in a error message obfuscates the actual message and should not be visible therefore. PR-URL: nodejs/node#15025 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins · 2017-10-17T18:15:26Z

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

assert: improve AssertionError in case of "Errors"
4602f90
Showing the stack trace in a error message obfuscates the actual message and should not be visible therefore.

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 25, 2017

BridgeAR added the assert Issues and PRs related to the assert subsystem. label Aug 25, 2017

jasnell approved these changes Aug 25, 2017
View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 25, 2017

thefourtheye reviewed Aug 26, 2017
View reviewed changes

Trott added the tsc-review label Sep 2, 2017

joyeecheung approved these changes Sep 3, 2017
View reviewed changes

Trott removed the tsc-review label Sep 4, 2017

jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 14, 2017

BridgeAR closed this Sep 15, 2017

jasnell added the backport-requested-v8.x label Sep 20, 2017

jasnell added backported-to-v8.x and removed backport-requested-v8.x labels Sep 21, 2017

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

apapirovski mentioned this pull request Dec 9, 2017
assert: improve error check #17574
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

BridgeAR commented Aug 25, 2017

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR commented Aug 25, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 25, 2017

Uh oh!

jasnell commented Aug 25, 2017

Uh oh!

Trott commented Aug 25, 2017

Uh oh!

jasnell commented Aug 25, 2017

Uh oh!

thefourtheyeAug 26, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeARAug 26, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnellAug 26, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeARAug 26, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeARAug 26, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellAug 26, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 29, 2017

Uh oh!

BridgeAR commented Aug 30, 2017

Uh oh!

BridgeAR commented Sep 2, 2017

Uh oh!

BridgeAR commented Sep 3, 2017

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

targos commented Sep 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Sep 14, 2017

Uh oh!

jasnell commented Sep 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Sep 15, 2017

Uh oh!

jasnell commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

jasnell commented Sep 21, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

BridgeARAug 26, 2017•
edited
Loading

BridgeARAug 26, 2017•
edited
Loading

targos commented Sep 13, 2017•
edited
Loading

jasnell commented Sep 14, 2017•
edited
Loading