Skip to content
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

test: improve assert message #15909

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@tnguyen14
Copy link
Contributor

commented Oct 6, 2017

First contribution for Node.js Interactive

Checklist
Affected core subsystem(s)

test

@mscdex mscdex added the cluster label Oct 6, 2017

@Trott Trott added the code-and-learn label Oct 6, 2017

@Trott

This comment has been minimized.

Copy link
Member

commented Oct 8, 2017

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?

@tnguyen14

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

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

@tnguyen14 tnguyen14 force-pushed the tnguyen14:improve-test-message branch from a37ea43 to e9618b7 Oct 10, 2017

@tnguyen14

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

Updated the assertion to remove the message.

@Trott

Trott approved these changes Oct 11, 2017

@Trott

This comment has been minimized.

@mhdawson
Copy link
Member

left a comment

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Oct 12, 2017

test: improve assert message
PR-URL: nodejs#15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

Landed in b6a87db.
Thanks for the contribution! 🎉

@Trott Trott closed this Oct 12, 2017

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 15, 2017

test: improve assert message
PR-URL: nodejs/node#15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

targos added a commit that referenced this pull request Oct 18, 2017

test: improve assert message
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

targos added a commit that referenced this pull request Oct 18, 2017

test: improve assert message
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 15, 2017

test: improve assert message
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@MylesBorins MylesBorins referenced this pull request Nov 21, 2017

Merged

v6.12.1 proposal #17180

MylesBorins added a commit that referenced this pull request Nov 21, 2017

test: improve assert message
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 28, 2017

test: improve assert message
PR-URL: #15909
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.