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, test: Add test for _readableState.needReadable and _readableState.emittedReadable #10230

Closed
mcollina opened this issue Dec 11, 2016 · 8 comments
Closed

stream, test: Add test for _readableState.needReadable and _readableState.emittedReadable #10230

mcollina opened this issue Dec 11, 2016 · 8 comments
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.

Comments

@mcollina
Copy link
Member

mcollina commented Dec 11, 2016 •
edited
Loading

  • Version: all
  • Platform: all
  • Subsystem: stream

Part of #8644.

State variable definition: https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L72-L77

See also #10214

cc @addaleax @Fishrock123 @nodejs/streams
@mcollina mcollina added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Dec 11, 2016
@mcollina
Copy link
Member Author

This is really a good issue if someone wants to dig into how streams work. I'm happy to help.

@joyeecheung
Copy link
Member

Hi, I am interested in the internals of streams since folks in my company have bumped into some perf issues related to it. I would love to spend some time digging into it and learning how it works :)

@mcollina
Copy link
Member Author

@joyeecheung please go ahead and prep a PR!

@joyeecheung joyeecheung mentioned this issue Dec 12, 2016
Closed
3 tasks
@joyeecheung
Copy link
Member

joyeecheung commented Dec 12, 2016 •
edited
Loading

I have attempted a PR for needReadable(#10241), please take a look.

Since there is #10214 involved I might need a little bit more time to get my head around emittedReadable.

@mcollina
Copy link
Member Author

@joyeecheung I would like to have a test for emittedReadable before a fix for #10214 happen. I would like to track possible regression, so write a test for what it is doing now.

@joyeecheung
Copy link
Member

@mcollina OK, working on it :)

@joyeecheung joyeecheung mentioned this issue Dec 13, 2016
Closed
3 tasks
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 13, 2016
@joyeecheung
stream, test: test _readableState.emittedReadable
83e6738 
Part of nodejs#10230, increase coverage of the internal
state machine of streams.
@italoacasas
Copy link
Contributor

@mcollina this is duplicated ? #8683

@mcollina
Copy link
Member Author

mcollina commented Dec 13, 2016 •
edited
Loading

Yes but not entirely. We can close this in fact, my bad.

joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 13, 2016
@joyeecheung
stream, test: test _readableState.needReadable
6633c5c 
Part of nodejs#10230, increase coverage of the internal
state machine of streams.
mcollina pushed a commit that referenced this issue Dec 16, 2016
@joyeecheung @mcollina
stream, test: test _readableState.emittedReadable
8127501 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
italoacasas pushed a commit that referenced this issue Dec 17, 2016
@joyeecheung @italoacasas
stream, test: test _readableState.emittedReadable
28ffd59 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
cjihrig pushed a commit that referenced this issue Dec 20, 2016
@joyeecheung @cjihrig
stream, test: test _readableState.emittedReadable
8ddbd53 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 23, 2017
@joyeecheung @MylesBorins
stream, test: test _readableState.emittedReadable
f26213a 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 23, 2017
@joyeecheung @MylesBorins
stream, test: test _readableState.emittedReadable
9fa1e7d 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 24, 2017
@joyeecheung @MylesBorins
stream, test: test _readableState.emittedReadable
1f6b10d 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 24, 2017
@joyeecheung @MylesBorins
stream, test: test _readableState.emittedReadable
a023886 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 31, 2017
@joyeecheung @MylesBorins
stream, test: test _readableState.emittedReadable
9a84162 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 1, 2017
@joyeecheung @MylesBorins
stream, test: test _readableState.emittedReadable
271f578 
Part of #8683, increase coverage of the internal
state machine of streams.

PR-URL: #10249
See: #8683
See: #10230
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

No branches or pull requests
3 participants
@mcollina @joyeecheung @italoacasas