-
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
repl: always show all error properties #20801
Conversation
Currently errors only show the error stack. This changes it to inspect the full error instead of just using the stack.
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. Need to determine if this is semver-major because of the change to console output.
I personally think this is a semver-patch. @nodejs/tsc PTAL |
const timeout = setTimeout(() => { | ||
reject( | ||
new Error(`Repl did not reply as expected for:\n\n${inspect(expected)}`) | ||
); |
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.
Is expected
always an event name (a string?).
If it is I think padding with two newlines is a bit much. It could just go right after the :
.
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.
No, expected is the expected output, so either a string or an array with many strings.
}, 500); | ||
ee.once('data', common.mustCall((...args) => { | ||
clearTimeout(timeout); | ||
resolve(...args); |
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.
With promises I believe once resolve
is called a reject
will have no effect so it could go without clearing since the timeout has no other side effects.
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.
But it would keep the test alive after being done if I am not mistaken.
Update: Closed by mistake. |
Currently errors only show the error stack. This changes it to
inspect the full error instead of just using the stack.
Refs: #20253
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes