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 stream writable state #21292

Closed
wants to merge 3 commits into from

Conversation

deleteman
Copy link
Contributor

@deleteman deleteman commented Jun 12, 2018

First contribution here, this one cleans up one of the test cases to improve the error message thrown when the test fails.
It used to be in the form of:

AssertionError [ERR_ASSERTION]: two chunks to write

Now, with this change, error message is in the form of:


AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
+ expected - actual

- 2
+ 3
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Removes the string error from the assertion call to
improve the error message shown on screen when the
test fails.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 12, 2018
@@ -7,7 +7,8 @@ const stream = require('stream');
const writable = new stream.Writable();

writable._writev = common.mustCall((chunks, cb) => {
assert.strictEqual(chunks.length, 2, 'two chunks to write');
//error: two chunks to write
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 the comment can be removed. Maybe it's just me but I don't find it useful.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. This will also fail our linter due to the lack of a space after //.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello both, thanks for the comments, I've added a new commit to this PR to remove the comment.

Let me know what you think.

Cheers

Removing a code comment with the error message, due to
feedback from reviewers

reviewers: Ipinca, apapirovski
@addaleax
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 18, 2018
targos pushed a commit to targos/node that referenced this pull request Jun 24, 2018
Removes the string error from the assertion call to improve the error
message shown on screen when the test fails.

PR-URL: nodejs#21292
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos
Copy link
Member

targos commented Jun 24, 2018

Landed in fd520e7.

Thank you @deleteman for your first contribution to Node.js!

@targos targos closed this Jun 24, 2018
targos pushed a commit that referenced this pull request Jun 24, 2018
Removes the string error from the assertion call to improve the error
message shown on screen when the test fails.

PR-URL: #21292
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants