stream, test: Increase the coverage of _readableState and _writableState properties

[mcollina]

• Version: all
• Platform: all
• Subsystem: stream

_readableStream and _writableStream in streams are widely used in userland, but the coverage for the state machine is insufficient, as only few properties of those are checked by unit tests.

I propose we add some more tests for the internal properties and state. This is probably a meta-issue, in the sense that we might send multiple PRs to increase the coverage.

Edit: The tests should possibly use only the stream module, rather than fs or net.

$ grep _readableState *
test-https-truncate.js:    assert.equal(res._readableState.length, 0);
test-stream-duplex.js:assert(stream._readableState.objectMode);
test-stream-push-order.js:  assert.deepStrictEqual(s._readableState.buffer.join(','), '1,2,3,4,5,6');
test-stream-readable-event.js:    assert(!r._readableState.reading);
test-stream-readable-event.js:    assert(r._readableState.reading);
test-stream-readable-event.js:    assert(!r._readableState.reading);
test-stream-readable-flow-recursion.js:  assert.equal(stream._readableState.highWaterMark, 8192);
test-stream-readable-flow-recursion.js:  assert.equal(stream._readableState.length, 0);
test-stream-transform-split-objectmode.js:assert(parser._readableState.objectMode);
test-stream-transform-split-objectmode.js:assert(parser._readableState.highWaterMark === 16);
test-stream-transform-split-objectmode.js:assert(!serializer._readableState.objectMode);
test-stream-transform-split-objectmode.js:assert(serializer._readableState.highWaterMark === (16 * 1024));
test-stream2-large-read-stall.js:var rs = r._readableState;
test-stream2-push.js:  console.error('data', stream._readableState.length);
test-stream2-read-sync-stack.js:  if (!(r._readableState.length % 256))
test-stream2-read-sync-stack.js:    console.error('readable', r._readableState.length);
test-stream2-transform.js:  t.equal(tx._readableState.length, 10);
test-stream2-unpipe-leak.js:console.error(src._readableState);
test-stream2-unpipe-leak.js:  src._readableState.buffer.length = 0;
test-stream2-unpipe-leak.js:  console.error(src._readableState);
test-stream2-unpipe-leak.js:  assert(src._readableState.length >= src._readableState.highWaterMark);
test-stream3-pause-then-read.js:      assert(!r._readableState.flowing);
$ grep _writableState *
test-http-pipeline-regr-3508.js:  if (size <= req.socket._writableState.highWaterMark) {
test-net-reconnect.js:    console.error('CLIENT connected', client._writableState);
test-net-reconnect.js:    console.error('CLIENT: calling end', client._writableState);
test-stream-big-packet.js:var big = Buffer.alloc(s1._writableState.highWaterMark + 1, 'x');
test-stream-duplex.js:assert(stream._writableState.objectMode);
test-stream-transform-split-objectmode.js:assert(!parser._writableState.objectMode);
test-stream-transform-split-objectmode.js:assert(parser._writableState.highWaterMark === (16 * 1024));
test-stream-transform-split-objectmode.js:assert(serializer._writableState.objectMode);
test-stream-transform-split-objectmode.js:assert(serializer._writableState.highWaterMark === 16);
test-stream2-transform.js:  t.same(tx._writableState.getBuffer().map(function(c) {
test-stream2-writable.js:      assert(tw._writableState.length >= 50);
test-zlib-flush-drain.js:const ws = deflater._writableState;

cc @nodejs/streams

[Fishrock123]

Maybe we should make a GitHub Project for "Streams: _readableState and _writableState"?

[mcollina]

@Fishrock123 if we want to try that, I'm happy to. I do not have much time to do the tests, but I am happy to divide this into single issues.

[sstern6]

hey @mcollina and @Fishrock123!!

I am fairly new to the code base but would like to help out on this if you guys would like.

@mcollina have you separated out the issues as you mentioned?

[Fishrock123]

I've made a meta-project at https://github.com/nodejs/node/projects/2

@mcollina if you could make issues and then add them to the "issues" section, that could be a good start!

Edit: I still think we should do some groupings though, or should this issue be good enough to sum them up as an issue description?

[mcollina]

@Fishrock123 I added some. Probably it needs some more. I tried to split them up in manageable chunks of work that could be achieved independently.

Let me know if I should clarify the text of the issues, or if they are easy to pick up.

[ALJCepeda]

Could this issue be in conflict with #445? It seemed like _writableState and _readableState should not be used and instead new API should be introduced where these properties were deemed necessary

[mcollina]

Currently there are very little tests on streams internal state machine. Currently we just test the public API.

Anything that changes the behavior of the linked properties is likely to be semver-major, so a decent test suite is in order to catch that situation.

IMHO unit tests are a basic step forward for any refactoring of the streams code.

[sstern6]

Thanks for setting up the issues @mcollina. Will try and tackle one of them and let you know if I have any questions.

[mcollina]

Closing this, as we have good coverage of that now, thanks to everyone involved in this effort!!!