Streams: add test for _readableState.readingMore | #8685 by chmln · Pull Request #9868 · nodejs/node

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 <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

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 mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

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

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

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

		});


		readable.push("abc");

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!

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

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