FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: stream readable readableListening internal state by italoacasas · Pull Request #9864 · nodejs/node · GitHub

/ node Public

test: stream readable readableListening internal state - #9864

Closed
italoacasas wants to merge 1 commit into
nodejs:masterfrom
italoacasas:test/needReadable
ConversationCommits (1)ChecksFiles changed
Closed

test: stream readable readableListening internal state#9864
italoacasas wants to merge 1 commit into
nodejs:masterfrom
italoacasas:test/needReadable

Conversation

italoacasas commented Dec 1, 2016 •
edited
Loading

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

stream

Description of change

Adding test for the readableListening state in stream.Readable

Issue related: #8683

cc: @mcollina

italoacasas added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Dec 1, 2016
nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Typo: common

cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure what this is supposed to say. Also, there is an extra space before the.

italoacasas Dec 1, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

me neither...

cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

successfully?

cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No need for deepEqual(). strictEqual() is good for comparing primitives.

cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This can fit on the previous line I think.

cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Instead of having r, r2, etc., you can place the isolated test cases inside a block scope and reuse the same variable names.

cjihrig reviewed Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-stream-readableListening-state.js Outdated

cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

asd?

cjihrig requested changes Dec 1, 2016
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The comments in general need some grammar and spell checking.

mcollina requested changes Dec 1, 2016
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some nits on the comments.

Comment thread test/parallel/test-stream-readableListening-state.js Outdated

mcollina Dec 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you remove this?

Comment thread test/parallel/test-stream-readableListening-state.js Outdated

mcollina Dec 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you remove this comment?

Comment thread test/parallel/test-stream-readableListening-state.js Outdated

mcollina Dec 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you condense this into a one-liner?

Comment thread test/parallel/test-stream-readableListening-state.js Outdated

mcollina Dec 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can you condense this into a oneliner?

italoacasas commented Dec 1, 2016

Copy link
Copy Markdown
Author

nits resolved

mcollina commented Dec 1, 2016

Copy link
Copy Markdown
Member

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

mcollina commented Dec 1, 2016

Copy link
Copy Markdown
Member

The linter seems failing.

italoacasas commented Dec 1, 2016

Copy link
Copy Markdown
Author

@mcollina I cannot find in the CI where is failing, can you see it ?

mcollina commented Dec 1, 2016

Copy link
Copy Markdown
Member

Is make lint passing?

italoacasas commented Dec 1, 2016

Copy link
Copy Markdown
Author

I re-run the CI, is on queue.

italoacasas commented Dec 1, 2016

Copy link
Copy Markdown
Author

Really funny this error Mandatory module "common" must be loaded

If i call const common = require('../common.js) I get the error

I need to call common without the .js

italoacasas commented Dec 9, 2016

Copy link
Copy Markdown
Author

The CI failure looks not related with the PR, can someone confirm ?

Trott commented Dec 9, 2016

Copy link
Copy Markdown
Member

CI failure is unrelated.

mcollina approved these changes Dec 9, 2016
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

italoacasas commented Dec 11, 2016

Copy link
Copy Markdown
Author

Landed 8dbf1af

Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
test: stream readableListening internal state
c792e2a 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas mentioned this pull request Dec 15, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
test: stream readableListening internal state
3bd7ab1 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
test: stream readableListening internal state
f355e20 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
test: stream readableListening internal state
89bd0d3 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
test: stream readableListening internal state
0cc8d69 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
test: stream readableListening internal state
a75883e 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
test: stream readableListening internal state
f12bab6 
PR-URL: #9864
Refs: #8683
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
cjihrig cjihrig 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

Back | FazBrowse Home | New Git URL