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.

Sign up for GitHub

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

Jump to bottom

test: improve test-stream2-objects.js. #9565

Closed
wants to merge 1 commit into from
Closed

test: improve test-stream2-objects.js. #9565

wants to merge 1 commit into from

Conversation

kt3k
Copy link
Contributor

@kt3k kt3k commented Nov 12, 2016

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

test, stream

Description of change

This commit improves the assertions of
test-stream2-objects.js.

This is a part of Code And Learn at NodeFest 2016 Challenge
nodejs/code-and-learn#58

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2016
@shigeki
Copy link
Contributor

shigeki commented Nov 12, 2016

CI is running on https://ci.nodejs.org/job/node-test-pull-request/4826/

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Nov 12, 2016
shigeki
shigeki approved these changes Nov 12, 2016
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

One CI error is in Centos7-64 due to missing g++ in the build environment. Others are fine.
LGTM.

shigeki
shigeki suggested changes Nov 12, 2016
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

Could you revise your commit message to describe your improvement of using strict assert checks?

@kt3k
Copy link
Contributor Author

kt3k commented Nov 12, 2016

Sure, of course!

@kt3k kt3k force-pushed the feature/improve-test-assertion-code-and-learn branch from 9ac5134 to d6819de Compare November 12, 2016 08:55
@kt3k
Copy link
Contributor Author

kt3k commented Nov 12, 2016

I added description of the change in commit message.

shigeki
shigeki approved these changes Nov 12, 2016
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM again.

cjihrig
cjihrig reviewed Nov 12, 2016
View reviewed changes
test/parallel/test-stream2-objects.js
@@ -232,7 +232,7 @@ test('high watermark push', function(t) {
r._read = function(n) {};
for (var i = 0; i < 6; i++) {
var bool = r.push(i);
assert.equal(bool, i === 5 ? false : true);
assert.strictEqual(bool, i === 5 ? false : true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be assert.strictEqual(bool, i !== 5);?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I'll change it.

@kt3k
test: improve test-stream2-objects.js
24514d7 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016
nodejs/code-and-learn#58
@kt3k kt3k force-pushed the feature/improve-test-assertion-code-and-learn branch from d6819de to 24514d7 Compare November 12, 2016 14:01
addaleax
addaleax approved these changes Nov 12, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

shigeki pushed a commit that referenced this pull request Nov 13, 2016
@kt3k @shigeki
test: improve test-stream2-objects.js
8ca322d 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@shigeki
Copy link
Contributor

shigeki commented Nov 13, 2016

Thanks. Landed in 8ca322d.
Congrats on your first contribution to Node.

@shigeki shigeki closed this Nov 13, 2016
@kt3k kt3k deleted the feature/improve-test-assertion-code-and-learn branch November 13, 2016 02:11
@kt3k
Copy link
Contributor Author

kt3k commented Nov 13, 2016

Thanks!

addaleax pushed a commit that referenced this pull request Nov 22, 2016
@kt3k @addaleax
test: improve test-stream2-objects.js
e7eb9cc 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
@kt3k @addaleax
test: improve test-stream2-objects.js
92f8073 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: nodejs#9565
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
@kt3k @MylesBorins
test: improve test-stream2-objects.js
eca12d4 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@kt3k @MylesBorins
test: improve test-stream2-objects.js
90027c3 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@kt3k @MylesBorins
test: improve test-stream2-objects.js
56677b6 
This commit improves the test cases in
test-stream2-objects.js by using assert.strictEqual
instead of assert.equal.

This is a part of Code And Learn at NodeFest 2016

Fixes: nodejs/code-and-learn#58
PR-URL: #9565
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This was referenced Dec 21, 2016
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@shigeki shigeki shigeki approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kt3k @shigeki @addaleax @lpinca @cjihrig @mscdex @MylesBorins @nodejs-github-bot