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.

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

stream, test: add tests for _readableStream.awaitDrain #8684

Closed
mcollina opened this issue Sep 21, 2016 · 6 comments
Closed

stream, test: add tests for _readableStream.awaitDrain #8684

mcollina opened this issue Sep 21, 2016 · 6 comments
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.

Comments

@mcollina
Copy link
Member

mcollina commented Sep 21, 2016

Part of #8644.

The _readableState that needs to be tested is:

stream._readableState.awaitDrain

// the number of writers that are awaiting a drain event in .pipe()s
this.awaitDrain = 0;
.

See also #8683

cc @Fishrock123 @nodejs/streams

@mcollina mcollina added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. good first issue Issues that are suitable for first-time contributors. labels Sep 21, 2016
@shmuga
Copy link
Contributor

shmuga commented Oct 3, 2016

I have made a proposal how this state variable can be tested in PR #8914 .
As we already have some tests for awaitDrain - I think it's better to add some asserts to them for covering real awairDrain value.
Also tests now cover reseting awaitDrain counter.
But I don't understand what this test is doing and can't rewrite it correctly. Can you someone help me with that?

@mcollina
Copy link
Member Author

mcollina commented Oct 3, 2016

@shmuga yes, good job!

The test you mentioned is increasing the awaitDrain mechanism manually, by calling resume: https://github.com/nodejs/node/blob/master/test/parallel/test-stream-pipe-await-drain-manual-resume.js#L29-L34.
You need to check that awaitDrain gets increased.

On that test, you should basically check that awaitDrain goes to zero as describe in: https://github.com/nodejs/node/blob/master/test/parallel/test-stream-pipe-await-drain-manual-resume.js#L37-L40

@shmuga
Copy link
Contributor

shmuga commented Oct 3, 2016

@mcollina great. will add some more tests for manual-resume. But I think this comment

// First pause, resume manually. The next write() to writable will still
// return false, because chunks are still being buffered, so it will increase
// the awaitDrain counter again.

is a bit strange at the place "increase again".
As we have only one writer here - the counter can't be increased more than one time.
So probably I will change that comment.

@mcollina
Copy link
Member Author

mcollina commented Oct 3, 2016

@shmuga resume interacts with awaitDrain: https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L741

See also https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L556.

When was that test added? Who did it? It might be worth getting the original PR here, and discussing with the author.

@shmuga
Copy link
Contributor

shmuga commented Oct 4, 2016

@mcollina I found nice description of this test in commit e2e615e

I will just follow those instructions and add cover tests for awaitDrain state variable.

@mcollina
Copy link
Member Author

Closed by 21a077a

mcollina pushed a commit that referenced this issue Jan 18, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 18, 2017
Fixes: nodejs#8684
PR-URL: nodejs#8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 23, 2017
Fixes: nodejs#8684
PR-URL: nodejs#8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 25, 2017
Fixes: nodejs#8684
PR-URL: nodejs#8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 27, 2017
Fixes: nodejs#8684
PR-URL: nodejs#8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Fixes: #8684
PR-URL: #8914
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
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

2 participants