Test stream writable state #21292
Closed
Test stream writable state #21292
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removes the string error from the assertion call to improve the error message shown on screen when the test fails.
lpinca
approved these changes
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 | |||
There was a problem hiding this comment.
I think the comment can be removed. Maybe it's just me but I don't find it useful.
There was a problem hiding this comment.
I agree. This will also fail our linter due to the lack of a space after //.
There was a problem hiding this comment.
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
trivikr
approved these changes
Jun 13, 2018
jasnell
approved these changes
Jun 14, 2018
BridgeAR
approved these changes
Jun 18, 2018
BridgeAR
added
the
author ready
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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
|
Landed in fd520e7. Thank you @deleteman for your first contribution to Node.js! |
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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 writeNow, with this change, error message is in the form of:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes