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

italoacasas · 2016-12-01T03:44:24Z

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

cjihrig · 2016-12-01T03:57:21Z

test/parallel/test-stream-readableListening-state.js

Typo: common

cjihrig · 2016-12-01T04:00:14Z

test/parallel/test-stream-readableListening-state.js

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

me neither...

cjihrig · 2016-12-01T04:00:48Z

test/parallel/test-stream-readableListening-state.js

successfully?

cjihrig · 2016-12-01T04:01:42Z

test/parallel/test-stream-readableListening-state.js

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

cjihrig · 2016-12-01T04:02:52Z

test/parallel/test-stream-readableListening-state.js

This can fit on the previous line I think.

cjihrig · 2016-12-01T04:04:22Z

test/parallel/test-stream-readableListening-state.js

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

cjihrig · 2016-12-01T04:04:29Z

test/parallel/test-stream-readableListening-state.js

cjihrig

The comments in general need some grammar and spell checking.

mcollina

Some nits on the comments.

mcollina · 2016-12-01T16:21:47Z

test/parallel/test-stream-readableListening-state.js

can you remove this?

mcollina · 2016-12-01T16:22:42Z

test/parallel/test-stream-readableListening-state.js

can you remove this comment?

mcollina · 2016-12-01T16:23:17Z

test/parallel/test-stream-readableListening-state.js

can you condense this into a one-liner?

mcollina · 2016-12-01T16:23:33Z

test/parallel/test-stream-readableListening-state.js

can you condense this into a oneliner?

italoacasas · 2016-12-01T19:58:18Z

nits resolved

mcollina · 2016-12-01T20:32:38Z

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

mcollina · 2016-12-01T20:43:57Z

The linter seems failing.

italoacasas · 2016-12-01T20:53:59Z

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

mcollina · 2016-12-01T20:57:30Z

Is make lint passing?

italoacasas · 2016-12-01T21:05:08Z

I re-run the CI, is on queue.

italoacasas · 2016-12-01T21:13:44Z

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 · 2016-12-09T01:38:46Z

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

Trott · 2016-12-09T03:17:43Z

CI failure is unrelated.

mcollina

LGTM

italoacasas · 2016-12-11T20:51:31Z

Landed 8dbf1af

PR-URL: #9864 Refs: #8683 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

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

Copy link

Contributor

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

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

Typo: common

italoacasas reacted with thumbs up emoji

cjihrig reviewed Dec 1, 2016

View reviewed changes

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

Copy link

Contributor

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

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

successfully?

italoacasas reacted with thumbs up emoji

cjihrig reviewed Dec 1, 2016

View reviewed changes

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

Copy link

Contributor

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

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

This can fit on the previous line I think.

cjihrig reviewed Dec 1, 2016

View reviewed changes

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

Copy link

Contributor

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

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

asd?

italoacasas reacted with thumbs up emoji

cjihrig requested changes Dec 1, 2016

View reviewed changes

mcollina requested changes Dec 1, 2016

View reviewed changes

test: stream readable readableListening internal state

980b775

mcollina approved these changes Dec 9, 2016

View reviewed changes

cjihrig approved these changes Dec 9, 2016

View reviewed changes

italoacasas closed this Dec 11, 2016

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

v7.3.0 proposal #10277

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 added land-on-v4.x labels Jan 16, 2017

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

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

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>

Uh oh!

Conversation

italoacasas commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

italoacasas commented Dec 1, 2016

Uh oh!

mcollina commented Dec 1, 2016

Uh oh!

mcollina commented Dec 1, 2016

Uh oh!

italoacasas commented Dec 1, 2016

Uh oh!

mcollina commented Dec 1, 2016

Uh oh!

italoacasas commented Dec 1, 2016

Uh oh!

italoacasas commented Dec 1, 2016

Uh oh!

italoacasas commented Dec 9, 2016

Uh oh!

Trott commented Dec 9, 2016

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

italoacasas commented Dec 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

italoacasas commented Dec 1, 2016 •

edited

Loading