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

test: tests for _readableStream.awaitDrain #8914

Closed
wants to merge 1 commit into from

Conversation

shmuga
Copy link
Contributor

@shmuga shmuga commented Oct 3, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 3, 2016
@shmuga
Copy link
Contributor Author

shmuga commented Oct 3, 2016

#8684

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Oct 3, 2016
reader.push(buffer);

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

cb();
}, 3)
});


Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// 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();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@shmuga shmuga Oct 3, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks.

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

Choose a reason for hiding this comment

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

maybe there are typos 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.

yep 😊 , fixed now

@mcollina
Copy link
Member

mcollina commented Oct 3, 2016

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

@shmuga
Copy link
Contributor Author

shmuga commented Oct 3, 2016

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

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

mcollina commented Dec 6, 2016

@shmuga any update on this?

@mcollina
Copy link
Member

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

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jan 12, 2017
@italoacasas
Copy link
Contributor

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

@mcollina
Copy link
Member

@italoacasas, go ahead. LGTM

@mcollina
Copy link
Member

@shmuga
Copy link
Contributor Author

shmuga commented Jan 17, 2017

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
Copy link
Member

@shmuga you said you could add another test here.

that ARM failure is transient, do not worry.

@shmuga
Copy link
Contributor Author

shmuga commented Jan 17, 2017

@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.

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.

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

@shmuga
Copy link
Contributor Author

shmuga commented Jan 17, 2017

@mcollina hope I haven't missed anything now 😄

@mcollina
Copy link
Member

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 if CI is green

@mcollina
Copy link
Member

Failures are unrelated, merging.

@mcollina
Copy link
Member

Landed as 21a077a.

@mcollina mcollina closed this Jan 18, 2017
mcollina pushed a commit that referenced this pull request Jan 18, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Fixes: #8684
PR-URL: #8914
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants