test: add tests for end event of stream.Duplex #21325
Conversation
Added tests to check the stream will automatically end the writable side when readable side ends when allowHalfOpen option is false.
|
And do they fail? Or perhaps increase coverage? I fail to see why you just added extra tests. |
|
@ryzokuken
There was no test cases that allowHalfOpen is false and I added tests. |
addaleax
added
the
author ready
| const Duplex = require('stream').Duplex; | ||
| { | ||
| const stream = new Duplex(); | ||
| assert(stream.allowHalfOpen); |
There was a problem hiding this comment.
Can you use assert.strictEqual() here.
There was a problem hiding this comment.
@cjihrig Fixed it. Thank you for your comment.
| const stream = new Duplex(); | ||
| assert(stream.allowHalfOpen); | ||
| stream.on('finish', common.mustNotCall()); | ||
| assert.strictEqual(stream.emit('end'), false); |
There was a problem hiding this comment.
I wonder if it's better to use stream.push(null) instead of emitting 'end' directly. We can still verify that there is no listener for 'end' event with stream.listenerCount('end') if wanted.
There was a problem hiding this comment.
@lpinca Thanks for your review. Would you please tell me why it is better to use stream.push(null) instead of stream.emit('end'). I tried to use stream.push(null) but test didn't emit 'end' and the function onend didn't run.
https://coverage.nodejs.org/coverage-7551de1fc2fbf24e/root/_stream_duplex.js.html#L100
There was a problem hiding this comment.
That's how the 'end' event is emitted in a real use case. readable.push(null) is used to signal that the stream has ended, you don't emit 'end' manually.
If you switch the stream in flowing mode with stream.resume() and then call stream.push(null) it should emit 'end'. You probably also have to add an implementation for _read().
const stream = new Duplex({ read() {} });
stream.resume();
stream.push(null);There was a problem hiding this comment.
I fixed it and the codes you indicated emit 'end'. Thank you for your comments.
| const common = require('../common'); | ||
| const assert = require('assert'); | ||
| const Duplex = require('stream').Duplex; | ||
| { |
There was a problem hiding this comment.
Nit: can please add a new line before this?
Added tests to check the stream will automatically end the writable side when readable side ends when allowHalfOpen option is false. PR-URL: #21325 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
|
Landed in 383b1b6 |
Added tests to check the stream will automatically end the writable side when readable side ends when allowHalfOpen option is false. PR-URL: #21325 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>
Added tests to check the stream will automatically end the writable side
when readable side ends when allowHalfOpen option is false.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes