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: include value to assert message #15970

Closed
wants to merge 3 commits into from
Closed

Conversation

hwaisiu
Copy link

@hwaisiu hwaisiu commented Oct 6, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the zlib Issues and PRs related to the zlib subsystem. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
@@ -20,13 +23,13 @@ assert.strictEqual(zlib.gunzipSync(data).toString(), 'abcdef');
zlib.gunzip(data, common.mustCall((err, result) => {
assert.ifError(err);
assert.strictEqual(result.toString(), 'abcdef',
'result should match original string');
'result should match original string: ' + (abc + def));
Copy link
Member

Choose a reason for hiding this comment

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

The more important part is actually what result.toString() looks like. Please change it to something like

`${result.toString()} should equal ${abc + def}`

@hwaisiu
Copy link
Author

hwaisiu commented Oct 8, 2017 via email

@joyeecheung
Copy link
Member

joyeecheung commented Oct 8, 2017

@hwaisiu Hi, have you pushed your changes to your branch (hwaisiu:node-dev)? The workflow should be something like:

$ git checkout node-dev # or other branch name that you have in your local repo that points to this PR
# make changes
$ git commit --amend # You can leave the commit message as-is in the editor
$ git push --force hwaisiu node-dev # assuming "hwaisiu" is pointing to your forked repository

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with @BridgeAR's suggestion

@hwaisiu
Copy link
Author

hwaisiu commented Oct 11, 2017

I got a 403 error after I entered:
$ git push --force

@hwaisiu
Copy link
Author

hwaisiu commented Oct 11, 2017

I think I have figured out how to push the changes.
Could you please review my updated changes?

Thanks

assert.strictEqual(result.toString(), 'abc',
'result should match contents of first "member"');
assert.strictEqual(result.toString(), abc,
`First "member": ${result.toString()} === ${abc}`);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is actually more readable if the assertion message is removed.

@BridgeAR BridgeAR self-assigned this Oct 18, 2017
@BridgeAR
Copy link
Member

Just a note for a potential next PR - the commit messages do not follow the guidelines.

BridgeAR pushed a commit that referenced this pull request Oct 19, 2017
PR-URL: #15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

Landed in ef96b05

Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 !

@BridgeAR BridgeAR closed this Oct 19, 2017
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
PR-URL: #15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
PR-URL: #15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#15970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants