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

test: add tests for end event of stream.Duplex #21325

Closed
wants to merge 3 commits into from
Closed

test: add tests for end event of stream.Duplex #21325

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0354af
test: add tests for end event of stream.Duplex
Jun 13, 2018
9ab67e5
test: use `stream.push(null) and `assert.strictEqual()`
Jun 17, 2018
c850c14
test: add a new line beefore tests in test-stream-duplex-end.js
Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions test/parallel/test-stream-duplex-end.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const Duplex = require('stream').Duplex;
{
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can please add a new line before this?

const stream = new Duplex();
assert(stream.allowHalfOpen);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use assert.strictEqual() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Fixed it. Thank you for your comment.

stream.on('finish', common.mustNotCall());
assert.strictEqual(stream.emit('end'), false);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it and the codes you indicated emit 'end'. Thank you for your comments.

}

{
const stream = new Duplex({
allowHalfOpen: false
});
assert.strictEqual(stream.allowHalfOpen, false);
stream.on('finish', common.mustCall());
assert(stream.emit('end'));
}

{
const stream = new Duplex({
allowHalfOpen: false
});
assert.strictEqual(stream.allowHalfOpen, false);
stream._writableState.ended = true;
stream.on('finish', common.mustNotCall());
assert(stream.emit('end'));
}