| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Hi, @tnguyen14! Welcome and thanks! This change doesn't address the issue that was requested in the Code + Learn task. It still masks that actual values. That is: The message is a string literal that does not provide the contents of the variables that are being compared. There are two possible ways to address this. One is to remove the message entirely and allow assert.strictEqual() to use its default message which provides the values. The other way is to change the string literal to a template string and use ${w} and ${cluster.worker} in the template literal to display the values. Can you update this to use one of those instead? |
Sorry, something went wrong.
|
Thanks for the quick response @Trott. I tried both of the solutions you mentioned. When removing the message completely, the default message is not entirely useful. It would show something like assert.js:45
throw new errors.AssertionError({
^
AssertionError [ERR_ASSERTION]: {} === Worker {
domain: null,
_events: {},
_eventsCount: 0,
_maxListeners: undefined,
exitedAfterDisconnect: true,
state: '
at Object.<anonymous> (/home/tri/dev/node/test/parallel/test-cluster-worker-destroy.js:54:12)
at Module._compile (module.js:600:30)
at Object.Module._extensions..js (module.js:611:10)
(I made w to {} to trigger the failure) Using ${w} and ${cluster.worker} doesn't work either, because it just prints out [object Object]. JSON.stringify doesn't work because the object contains circular reference. I discussed with @mhdawson briefly, and he suggested that it's best to put in an error message that makes the most sense for developers to debug the issue in the case of failure. Please let me know if you think it's best to drop the error message entirely. |
Sorry, something went wrong.
|
I'd be +1 on dropping the error message entirely. AssertionError [ERR_ASSERTION]: {} === Worker { isn't useful by itself, but when you look at the source you'll see assert.strictEqual(w, cluster.worker), which tells you that the assertion failed because w was the empty object not a Worker. I'm not sure printing the contents of the Worker object would be useful in this case. |
Sorry, something went wrong.
|
I agree with @gibfahn. I think the error message you show when the message argument has been removed entirely is actually quite useful. |
Sorry, something went wrong.
|
Updated the assertion to remove the message. |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
PR-URL: nodejs#15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
Landed in b6a87db. |
Sorry, something went wrong.
PR-URL: nodejs/node#15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #15909 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
| Back | FazBrowse Home | New Git URL |
First contribution for Node.js Interactive
Checklist
(https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines)
Affected core subsystem(s)
test