Streams: add test for _readableState.readingMore | #8685#9868

chmln · 2016-12-01T07:09:12Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

stream

Description of change

Test cases for stream.Readable._readableState.
reading and readingMore properties are tested on initialization, under data, readable, and end events.

italoacasas · 2016-12-01T13:56:21Z

cc: @mcollina

cjihrig · 2016-12-01T14:39:41Z

test/parallel/test-stream-readable-readingMore.js

@@ -0,0 +1,31 @@
+'use strict'
+const Readable = require('stream').Readable,


Please require ../common.js, as done in other test files.

cjihrig · 2016-12-01T14:40:20Z

test/parallel/test-stream-readable-readingMore.js

@@ -0,0 +1,31 @@
+'use strict'
+const Readable = require('stream').Readable,
+ assert = require('assert');


Please use a new const statement on each line (drop the comma separated declarations).

cjihrig · 2016-12-01T14:41:03Z

test/parallel/test-stream-readable-readingMore.js

+assert.strictEqual(readable._readableState.readingMore, false);
+
+readable.on('data', data =>{
+ // still in a flowing state, try to read more


Please capitalize and punctuate comments.

cjihrig · 2016-12-01T14:41:45Z

test/parallel/test-stream-readable-readingMore.js

+// false initially
+assert.strictEqual(readable._readableState.readingMore, false);
+
+readable.on('data', data =>{


Since there are assertions inside of this callback, can you wrap it in common.mustCall().

cjihrig · 2016-12-01T14:42:02Z

test/parallel/test-stream-readable-readingMore.js

+ assert.strictEqual(readable._readableState.readingMore, true);
+});
+
+readable.on('end', () =>{


Same comment about common.mustCall() here.

cjihrig · 2016-12-01T14:42:17Z

test/parallel/test-stream-readable-readingMore.js

+});
+
+
+readable.push("abc");


Please use single quotes for strings.

@cjihrig Thank you for the guidance 👍

mcollina · 2016-12-01T20:39:20Z

We need to have an equivalent for readable as well, would you mind adding it, or renaming the test file to test/parallel/test-stream-readable-readingMore-ondata.js?

chmln · 2016-12-01T20:41:18Z

We need to have an equivalent for readable as well, would you mind adding it,

Absolutely.

…ble-readingMore

mcollina · 2016-12-07T08:00:11Z

@chmln keep it coming :). The current test looks good, I'll wait for the one on the 'readable' event too!

…ble-readingMore

mcollina · 2016-12-09T10:02:48Z

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

mcollina

LGTM

italoacasas · 2016-12-11T23:27:57Z

Landed 8621ccc

Thanks for the contribution.

PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

Streams: add test for _readableState.readingMore |#8685
19432ad

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016

mscdex added the stream Issues and PRs related to the stream subsystem. label Dec 1, 2016

readingMore is always false on nextTick(); simplify
370674a

cjihrig requested changes Dec 1, 2016
View reviewed changes

common.mustCall's, follow punctuation and syntax rules
9747c04

chmln added 4 commits December 3, 2016 15:10

Merge branch 'master' of git://github.com/nodejs/node into test-reada…
395368e
…ble-readingMore

Merge branch 'master' of git://github.com/nodejs/node into test-reada…
1968956
…ble-readingMore

Incorporate tests for state.reading
71ebbbc

Add missing reading check on nextTick()
1e6428e

chmln added 2 commits December 9, 2016 01:10

Merge branch 'master' of git://github.com/nodejs/node into test-reada…
a655f4c
…ble-readingMore

Add reading and readingMore tests for 'readable' event
466c083

nodejs-github-bot added lts-watch-v6.x labels Dec 9, 2016

indent with two spaces
2973949

nodejs-github-bot added lts-watch-v6.x labels Dec 9, 2016

mcollina approved these changes Dec 9, 2016
View reviewed changes

cjihrig approved these changes Dec 9, 2016
View reviewed changes

italoacasas approved these changes Dec 11, 2016
View reviewed changes

italoacasas closed this Dec 11, 2016

italoacasas pushed a commit that referenced this pull request Dec 11, 2016
test: stream readableState readingMore state
8621ccc
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
test: stream readableState readingMore state
6780c0e
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

italoacasas mentioned this pull request Dec 15, 2016
v7.3.0 proposal #10277
Merged

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
test: stream readableState readingMore state
8f4c29b
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
test: stream readableState readingMore state
16a74f4
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
test: stream readableState readingMore state
8be5e04
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
test: stream readableState readingMore state
23677ba
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

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 readableState readingMore state
25d6eed
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
test: stream readableState readingMore state
544920f
PR-URL: #9868 Refs: #8685 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Italo A. Casas <[email protected]>

		@@ -0,0 +1,31 @@
		'use strict'
		const Readable = require('stream').Readable,

		});


		readable.push("abc");

Uh oh!

Streams: add test for _readableState.readingMore | #8685#9868

Streams: add test for _readableState.readingMore | #8685 #9868

Uh oh!

Conversation

chmln 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!

italoacasas commented Dec 1, 2016

Uh oh!

cjihrigDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

chmlnDec 1, 2016

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 1, 2016

Uh oh!

chmln commented Dec 1, 2016

Uh oh!

mcollina commented Dec 7, 2016

Uh oh!

mcollina 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!

7 participants

chmln commented Dec 1, 2016•
edited
Loading