test: include value to assert message by hwaisiu · Pull Request #15970 · nodejs/node

hwaisiu · 2017-10-06T17:52:53Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

BridgeAR · 2017-10-07T17:46:32Z

test/parallel/test-zlib-from-concatenated-gzip.js

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

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

hwaisiu · 2017-10-08T01:01:41Z

Hi Ruben Thanks for the comment I have modified the code and checked in. This is my first time contributing to Node. Please let me know if I missed any steps. ThanksHazel Sent from my Samsung Galaxy smartphone. -------- Original message --------From: Ruben Bridgewater <notifications@github.com> Date: 10/7/17 10:47 AM (GMT-08:00) To: nodejs/node <node@noreply.github.com> Cc: hwaisiu <hazelsiu@gmail.com>, Author <author@noreply.github.com> Subject: Re: [nodejs/node] test: include value to assert message (#15970) @BridgeAR commented on this pull request. In test/parallel/test-zlib-from-concatenated-gzip.js:

@@ -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)); The more important part is actually what result.toString() looks like. Please change it to something like `${result.toString()} should equal ${abc + def}` — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/nodejs/node","title":"nodejs/node","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/nodejs/node"}},"updates":{"snippets":[{"icon":"PERSON","message":"@BridgeAR commented on #15970"}],"action":{"name":"View Pull Request","url":"#15970 (review)"}}}

joyeecheung · 2017-10-08T05:36:02Z

@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

jasnell

LGTM with @BridgeAR's suggestion

…ting First member

test: because of length limit of message

hwaisiu · 2017-10-11T04:00:23Z

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

hwaisiu · 2017-10-11T04:28:39Z

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

Thanks

joyeecheung · 2017-10-12T13:59:02Z

test/parallel/test-zlib-from-concatenated-gzip.js

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


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

BridgeAR · 2017-10-18T19:18:09Z

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

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2017-10-19T04:36:54Z

Landed in ef96b05

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

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs/node#15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017

mscdex added the zlib Issues and PRs related to the zlib 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

BridgeAR reviewed Oct 7, 2017

View reviewed changes

jasnell reviewed Oct 10, 2017

View reviewed changes

hwaisiu added 3 commits October 10, 2017 20:18

test: include value to assert message

f33c963

Use default message for assert.strictEqual() except the one for asser…

a24ecaf

…ting First member

test: use default message for assert.strictEqual()

fee0e72

test: because of length limit of message

hwaisiu force-pushed the node-dev branch from ae58dc9 to fee0e72 Compare October 11, 2017 04:26

jasnell approved these changes Oct 12, 2017

View reviewed changes

joyeecheung reviewed Oct 12, 2017

View reviewed changes

BridgeAR approved these changes Oct 18, 2017

View reviewed changes

BridgeAR self-assigned this Oct 18, 2017

BridgeAR pushed a commit that referenced this pull request Oct 19, 2017

test: use default message for assert.strictEqual

ef96b05

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR closed this Oct 19, 2017

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017

test: use default message for assert.strictEqual

f4a5aba

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

test: use default message for assert.strictEqual

fe19f14

PR-URL: nodejs/node#15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins pushed a commit that referenced this pull request Nov 16, 2017

test: use default message for assert.strictEqual

2a19aff

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

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

test: use default message for assert.strictEqual

b74a405

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

test: use default message for assert.strictEqual

339bdca

PR-URL: #15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

test: use default message for assert.strictEqual

219fbf5

PR-URL: nodejs/node#15970 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: include value to assert message#15970

test: include value to assert message#15970
hwaisiu wants to merge 3 commits intonodejs:masterfrom
hwaisiu:node-dev

hwaisiu commented Oct 6, 2017 •

edited by BridgeAR

Loading

Uh oh!

BridgeAR Oct 7, 2017

Uh oh!

hwaisiu commented Oct 8, 2017 via email

Uh oh!

joyeecheung commented Oct 8, 2017 •

edited

Loading

Uh oh!

jasnell left a comment

Uh oh!

hwaisiu commented Oct 11, 2017

Uh oh!

hwaisiu commented Oct 11, 2017

Uh oh!

joyeecheung Oct 12, 2017

Uh oh!

BridgeAR commented Oct 18, 2017

Uh oh!

BridgeAR commented Oct 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

hwaisiu commented Oct 6, 2017 • edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

hwaisiu commented Oct 8, 2017 via email

Uh oh!

joyeecheung commented Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

hwaisiu commented Oct 11, 2017

Uh oh!

hwaisiu commented Oct 11, 2017

Uh oh!

joyeecheung Oct 12, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 18, 2017

Uh oh!

BridgeAR commented Oct 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hwaisiu commented Oct 6, 2017 •

edited by BridgeAR

Loading

joyeecheung commented Oct 8, 2017 •

edited

Loading