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: stream readable resumeScheduled state #10299

Closed
wants to merge 1 commit into from
Closed

test: stream readable resumeScheduled state #10299

wants to merge 1 commit into from

Conversation

italoacasas
Copy link
Contributor

@italoacasas italoacasas commented Dec 16, 2016

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)

test

Description of change

Adding test for the resumeScheduled state in stream.Readable

Ref: #8683
CI: https://ci.nodejs.org/job/node-test-pull-request/5446/console

cc: @mcollina

@italoacasas italoacasas added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Dec 16, 2016
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM up to the linter errors CI is showing

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.

Can you please add checks also for on('data') and pipe()? Both of those trigger a resume().

// pipe() test case
const r = new Readable({ read() {} });
// event 'data' test case
const r2 = new Readable({ read() {} });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you separate these using block scopes, as done in a lot of other tests. Then, you can reuse the same variable names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@italoacasas
Copy link
Contributor Author

ping @cjihrig @mcollina

const { Readable, Writable } = require('stream');

{
// First test, expect the correct behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

I would drop the "First test," "Second test," etc. from the comments. They will likely become out of date as people change the file.

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

@italoacasas
Copy link
Contributor Author

Landed 6137983

cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 20, 2016
PR-URL: nodejs#10299
Ref: nodejs#8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 20, 2016
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
PR-URL: #10299
Ref: #8683
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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.

6 participants