test: tests for _readableStream.awaitDrain #8914

shmuga · 2016-10-03T21:15:45Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

shmuga · 2016-10-03T21:16:20Z

#8684

mscdex · 2016-10-03T22:04:09Z

test/parallel/test-stream-pipe-await-drain.js

 reader.push(buffer);
+


Unnecessary whitespace?

mscdex · 2016-10-03T22:04:38Z

test/parallel/test-stream-pipe-await-drain-push-while-write.js

    cb();
  }, 3)
 });

+


Unnecessary whitespace?

removed. maybe it's better to add lint rule and not to check this each time?

mcollina · 2016-10-03T22:20:24Z

test/parallel/test-stream-pipe-await-drain.js


 // This is very similar to test-stream-pipe-cleanup-pause.js.

 const reader = new stream.Readable();
 const writer1 = new stream.Writable();
 const writer2 = new stream.Writable();
+const writer3 = new stream.Writable();


I don't think we should add a new writer here. Why this change?

I thought it's better to trace if the counter is not stuck on value 1 if we have multiple writers in pipeline. For example, if someone will breake the behavior of this counter and it will work as a flag, he will get error in this test.

Perfect, thanks.

mcollina · 2016-10-03T22:28:22Z

test/parallel/test-stream-pipe-await-drain.js

+writer3._write = common.mustCall(function(chunk, encoding, cb) {
+  assert.strictEqual(
+    reader._readableState.awaitDrain, 2,
+    'writer2 should make awaitDrain = 1 after first buffer push'


maybe there are typos here

yep 😊 , fixed now

mcollina · 2016-10-03T22:38:37Z

@shmuga are you adding the other test mentioned in #8684?

shmuga · 2016-10-03T22:40:55Z

@mcollina yep, I'll do that. I made a comment in issue.

mcollina · 2016-12-06T08:14:07Z

@shmuga any update on this?

mcollina · 2017-01-12T16:13:41Z

@shmuga do you still plan to move this forward, or might someone else take over?

italoacasas · 2017-01-16T22:05:23Z

@mcollina can we merge this as is, and open another issue with the other test?

mcollina · 2017-01-17T06:22:49Z

@italoacasas, go ahead. LGTM

mcollina · 2017-01-17T07:54:32Z

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

shmuga · 2017-01-17T09:06:56Z

Sorry guys, just found that I have no notifications for github on email.
I found that test/arm failed but can't find any debug information. How can I handle that?

mcollina · 2017-01-17T09:11:15Z

@shmuga you said you could add another test here.

that ARM failure is transient, do not worry.

shmuga · 2017-01-17T09:57:11Z

@mcollina I've looked through the original commit message mentioned here and through comments in test and decided to cover only the logic that .resume() should reset awaitDrain counter.

mcollina

The logic is good. Can you shorten up the assertion messages? we tend to put them in the present tense, without 'should'.

shmuga · 2017-01-17T13:41:25Z

@mcollina hope I haven't missed anything now 😄

mcollina · 2017-01-17T14:45:06Z

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

mcollina

LGTM if CI is green

mcollina · 2017-01-18T13:03:39Z

Failures are unrelated, merging.

mcollina · 2017-01-18T13:12:23Z

Landed as 21a077a.

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fixes: nodejs#8684 PR-URL: nodejs#8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 3, 2016

shmuga mentioned this pull request Oct 3, 2016

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

Closed

mscdex added the stream Issues and PRs related to the stream subsystem. label Oct 3, 2016

mscdex reviewed Oct 3, 2016

View reviewed changes

mcollina reviewed Oct 3, 2016

View reviewed changes

shmuga force-pushed the feature/8684 branch from 328ea12 to f20dc8c Compare October 3, 2016 22:26

mcollina reviewed Oct 3, 2016

View reviewed changes

shmuga force-pushed the feature/8684 branch from f20dc8c to f7d66ca Compare October 3, 2016 22:33

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell added the stalled Issues and PRs that are stalled. label Jan 12, 2017

mcollina approved these changes Jan 17, 2017

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Jan 17, 2017

mcollina requested changes Jan 17, 2017

View reviewed changes

shmuga force-pushed the feature/8684 branch from b31a5dd to c921fb7 Compare January 17, 2017 10:45

nodejs-github-bot added lts-watch-v6.x labels Jan 17, 2017

shmuga force-pushed the feature/8684 branch from c921fb7 to ac44b99 Compare January 17, 2017 12:19

nodejs-github-bot added lts-watch-v6.x labels Jan 17, 2017

mcollina approved these changes Jan 17, 2017

View reviewed changes

mcollina closed this Jan 18, 2017

mcollina pushed a commit that referenced this pull request Jan 18, 2017

test: tests for _readableStream.awaitDrain

21a077a

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017

test: tests for _readableStream.awaitDrain

e736db0

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 pull request Jan 23, 2017

test: tests for _readableStream.awaitDrain

ce443a6

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 pull request Jan 25, 2017

test: tests for _readableStream.awaitDrain

c55d83e

Fixes: nodejs#8684 PR-URL: nodejs#8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017

test: tests for _readableStream.awaitDrain

96babb2

Fixes: nodejs#8684 PR-URL: nodejs#8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins pushed a commit that referenced this pull request Mar 8, 2017

test: tests for _readableStream.awaitDrain

385e3fe

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added land-on-v4.x labels Mar 8, 2017

MylesBorins pushed a commit that referenced this pull request Mar 8, 2017

test: tests for _readableStream.awaitDrain

3eecc92

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins removed lts-watch-v4.x labels Mar 8, 2017

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017

test: tests for _readableStream.awaitDrain

80e2ff9

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017

test: tests for _readableStream.awaitDrain

89e9da6

Fixes: #8684 PR-URL: #8914 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: tests for _readableStream.awaitDrain #8914

test: tests for _readableStream.awaitDrain #8914

shmuga commented Oct 3, 2016

shmuga commented Oct 3, 2016 •

edited

Loading

mscdex Oct 3, 2016

shmuga Oct 3, 2016

mscdex Oct 3, 2016

shmuga Oct 3, 2016

mcollina Oct 3, 2016

shmuga Oct 3, 2016 •

edited

Loading

mcollina Oct 3, 2016

mcollina Oct 3, 2016

shmuga Oct 3, 2016

mcollina commented Oct 3, 2016

shmuga commented Oct 3, 2016

mcollina commented Dec 6, 2016

mcollina commented Jan 12, 2017

italoacasas commented Jan 16, 2017

mcollina commented Jan 17, 2017

mcollina commented Jan 17, 2017

shmuga commented Jan 17, 2017

mcollina commented Jan 17, 2017

shmuga commented Jan 17, 2017

mcollina left a comment

shmuga commented Jan 17, 2017

mcollina commented Jan 17, 2017

mcollina left a comment

mcollina commented Jan 18, 2017

mcollina commented Jan 18, 2017

test: tests for _readableStream.awaitDrain #8914

test: tests for _readableStream.awaitDrain #8914

Conversation

shmuga commented Oct 3, 2016

Checklist

Affected core subsystem(s)

Description of change

shmuga commented Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmuga Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Oct 3, 2016

shmuga commented Oct 3, 2016

mcollina commented Dec 6, 2016

mcollina commented Jan 12, 2017

italoacasas commented Jan 16, 2017

mcollina commented Jan 17, 2017

mcollina commented Jan 17, 2017

shmuga commented Jan 17, 2017

mcollina commented Jan 17, 2017

shmuga commented Jan 17, 2017

mcollina left a comment

Choose a reason for hiding this comment

shmuga commented Jan 17, 2017

mcollina commented Jan 17, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jan 18, 2017

mcollina commented Jan 18, 2017

shmuga commented Oct 3, 2016 •

edited

Loading

shmuga Oct 3, 2016 •

edited

Loading