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 #10241

Closed
wants to merge 1 commit into from
Closed

stream, test: add test for _readableState.needReadable #10241

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

stream, test

Description of change

Part of #10230 & #8644, increase coverage of the internal state machine of streams.

@nodejs-github-bot nodejs-github-bot added lts-watch-v4.x test Issues and PRs related to the tests. labels Dec 12, 2016
@joyeecheung joyeecheung changed the title stream, test: Add test for _readableState.needReadable.needReadable stream, test: Add test for _readableState.needReadable Dec 12, 2016
@joyeecheung joyeecheung mentioned this pull request Dec 12, 2016
Closed
@joyeecheung joyeecheung changed the title stream, test: Add test for _readableState.needReadable stream, test: add test for _readableState.needReadable Dec 12, 2016
@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Dec 12, 2016
mscdex
mscdex reviewed Dec 12, 2016
View reviewed changes
test/parallel/test-stream-readable-needReadable.js
// When the readable event fires, needReadable is reset.
assert.strictEqual(readable._readableState.needReadable, false);
readable.read();
}), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the , 1 here is a typo and should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for pointing this out. That's meant for common.mustCall but I accidentally put it inside the wrong parentheses. The default is 1 anyway so I agree with you, this can be removed.

@joyeecheung
Copy link
Member Author

I have removed the misplaced argument to common.mustCall and reworded the comments.

mcollina
mcollina approved these changes Dec 13, 2016
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

@mscdex
Copy link
Contributor

mscdex commented Dec 13, 2016

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

@mcollina
Copy link
Member

ARM failure unrelated, @mscdex you merge or should I merge?

@joyeecheung joyeecheung mentioned this pull request Dec 13, 2016
Closed
3 tasks
@joyeecheung
Copy link
Member Author

I discovered that the slowProducer example is not covered accurately(pushing is done synchronously). Can I improve this a little bit more?

@mcollina
Copy link
Member

@joyeecheung Sure go ahead.

@joyeecheung
Copy link
Member Author

Updates pushed, please take a look @mcollina .

@italoacasas italoacasas mentioned this pull request Dec 17, 2016
Merged
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
@joyeecheung @cjihrig
test: stream readable needReadable state
8806342 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@joyeecheung joyeecheung deleted the test-need-readable branch January 2, 2017 05:28
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@joyeecheung @MylesBorins
test: stream readable needReadable state
083ff5c 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@joyeecheung @MylesBorins
test: stream readable needReadable state
ba36d55 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@joyeecheung @MylesBorins
test: stream readable needReadable state
2c7a59a 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@joyeecheung @MylesBorins
test: stream readable needReadable state
76b034e 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@joyeecheung @MylesBorins
test: stream readable needReadable state
a24a35f 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@joyeecheung @MylesBorins
test: stream readable needReadable state
135a7c9 
PR-URL: #10241
Ref: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@snyk-bot snyk-bot mentioned this pull request Apr 24, 2023
Open
@aliscco aliscco mentioned this pull request Apr 24, 2023
Open
This was referenced Apr 24, 2023
Open
Open
@aliscco aliscco mentioned this pull request Apr 24, 2023
Open
This was referenced Apr 25, 2023
Open
Open
Open
@aliscco aliscco mentioned this pull request Apr 26, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Apr 26, 2023
Open
@aliscco aliscco mentioned this pull request Apr 28, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Apr 28, 2023
Open
@aliscco aliscco mentioned this pull request Apr 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@italoacasas italoacasas italoacasas approved these changes

@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.
6 participants
@joyeecheung @mscdex @mcollina @italoacasas @MylesBorins @nodejs-github-bot