-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assert: fix throws trace #18595
assert: fix throws trace #18595
Conversation
The current stack trace thrown in case `assert.throws(fn, object)` is used did not filter the stack trace. This fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a small nit
lib/assert.js
Outdated
return msg; | ||
return `${key}: expected ${inspect(expected[key])}, ` + | ||
`not ${inspect(actual[key])}`; | ||
function compareKey(actual, expected, key, msg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could we give this function a slightly more descriptive name since it only works for throws
? Like compareExceptionKey
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I only changed the function name since the CI and it was green before, so I do not rerun it now. |
The current stack trace thrown in case `assert.throws(fn, object)` is used did not filter the stack trace. This fixes it. PR-URL: nodejs#18595 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Landed in cccddc5 |
Should this be backported to |
The current stack trace thrown in case `assert.throws(fn, object)` is used did not filter the stack trace. This fixes it. PR-URL: nodejs#18595 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Backport opened in #19230 |
The current stack trace thrown in case `assert.throws(fn, object)` is used did not filter the stack trace. This fixes it. PR-URL: nodejs#18595 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Requested backport to 8.x in #19230 |
The current stack trace thrown in case `assert.throws(fn, object)` is used did not filter the stack trace. This fixes it. PR-URL: nodejs#18595 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
The current stack trace thrown in case
assert.throws(fn, object)
is used did not filter the stack trace. This fixes it.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
assert