-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: updated test-stream-pipe-unpipe-stream #10100
Conversation
@@ -25,6 +25,9 @@ source.unpipe(dest2); | |||
assert.strictEqual(source._readableState.pipes, dest1); | |||
assert.notStrictEqual(source._readableState.pipes, dest2); | |||
|
|||
dest2.on('unpipe', assert.fail); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use common.fail
instead of assert.fail
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it needs to include a message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't, but others probably do. In that case, you'd have to do something like:
() => { common.fail('your message here'); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR, not using message as it looks obvious in the code and most of the common.fail
aren't using message.
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream
be91f7f
to
23c1aa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but cc @nodejs/streams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merging now. |
Merged as acb44d0 |
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: #10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: #10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: nodejs#10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: nodejs#10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: #10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: #10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test readableStream.unpipe(dest) is no operation when dest is not a destination for readable stream. PR-URL: #10100 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
test readableStream.unpipe(dest) is no operation when dest is
not a destination for readable stream