FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: improve assert message by tnguyen14 · Pull Request #15909 · nodejs/node · GitHub

/ node Public

test: improve assert message - #15909

Closed
tnguyen14 wants to merge 1 commit into
nodejs:masterfrom
tnguyen14:improve-test-message
Closed

test: improve assert message#15909
tnguyen14 wants to merge 1 commit into
nodejs:masterfrom
tnguyen14:improve-test-message

Conversation

Copy link
Copy Markdown
Contributor

First contribution for Node.js Interactive

Checklist
Affected core subsystem(s)

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
mscdex added the cluster Issues and PRs related to the cluster subsystem. label Oct 6, 2017
Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

Trott commented Oct 8, 2017

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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.

gibfahn commented Oct 9, 2017

Copy link
Copy Markdown
Member

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.

Trott commented Oct 10, 2017

Copy link
Copy Markdown
Member

I agree with @gibfahn. I think the error message you show when the message argument has been removed entirely is actually quite useful.

tnguyen14 force-pushed the improve-test-message branch from a37ea43 to e9618b7 Compare October 10, 2017 22:14

Copy link
Copy Markdown
Contributor Author

Updated the assertion to remove the message.

Trott commented Oct 11, 2017

Copy link
Copy Markdown
Member

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 12, 2017
PR-URL: nodejs#15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Trott commented Oct 12, 2017

Copy link
Copy Markdown
Member

Landed in b6a87db.
Thanks for the contribution! 🎉

Trott closed this Oct 12, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL