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

Jump to bottom

stream: add a test case for the underlying cause. #18575

Closed
wants to merge 1 commit into from
Closed

stream: add a test case for the underlying cause. #18575

wants to merge 1 commit into from

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Feb 5, 2018

The original test case hides the underlying cause by using PassThrough. This change adds a test case for the underlying cause. This makes it clearer and easier to be understood.

Refs: #18372

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, stream

stream: add a test case for the underlying cause.
9f78934 
The original test case hides the underlying cause by using
`PassThrough`. This change adds a test case for the underlying cause.
This makes it clearer and easier to be understood.

Refs: nodejs#18372
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 5, 2018
@lpinca lpinca added the stream Issues and PRs related to the stream subsystem. label Feb 6, 2018
@mafintosh
Copy link
Member

Confirmed the new test case indeed reproduces the bug that was fixed in #18372. Good job @MoonBall.

LGTM

mcollina
mcollina approved these changes Feb 8, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

mcollina commented Feb 8, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/13035/

@mcollina
Copy link
Member

mcollina commented Feb 8, 2018

CI is ok landing.

@mcollina
Copy link
Member

mcollina commented Feb 8, 2018

Landed as 86c659b

@mcollina mcollina closed this Feb 8, 2018
mcollina pushed a commit that referenced this pull request Feb 8, 2018
@mcollina
stream: add a test case for the underlying cause.
86c659b 
The original test case hides the underlying cause by using
`PassThrough`. This change adds a test case for the underlying cause.
This makes it clearer and easier to be understood.

Refs: #18372

PR-URL: #18575
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins
Copy link
Member

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@mcollina
Copy link
Member

This should not be backported.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MayaLekova
stream: add a test case for the underlying cause.
7513eec 
The original test case hides the underlying cause by using
`PassThrough`. This change adds a test case for the underlying cause.
This makes it clearer and easier to be understood.

Refs: nodejs#18372

PR-URL: nodejs#18575
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MoonBall @mafintosh @mcollina @MylesBorins @lpinca @nodejs-github-bot