test: add tests for stream3 buffering using cork

[alexjeffburke]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Description of change

Add tests of stream3 the buffering behaviour of cork() followed by uncork()/end(). Opening a PR for review per request of the streams-wg.

cc @nodejs/readable-stream

[calvinmetcalf]

@nodejs/streams

[mcollina]

Thanks!!! You might want to check this runs on node v4, just in case.

I think we should start the habit of explaining the tests in plain English first, because otherwise they imply a lot of knowledge around the stream state machine and they might scare newcomers. Most of it is already explained in the comments, it's probably just a matter of putting a paragraph to describe it at the beginning.

Use const instead of var for the non-changing variables. I will also argue that we should stick to use function () {} instead of the fat arrows when we attach methods to prototypes.

How about you add a test also for _writev?

[alexjeffburke]

Thanks for the feedback @mcollina - so I've just pushed an update with the following changes:

• set _write using a full function (having looked kinda like it too)
• used const for includes and expectedChunks
• tightened up a couple of the comments

Regarding _writev, it appeared to me test-stream-writev.js does some of that but I don't think its the most approachable code. What I'd be tempted to do in that case is separate out a test for all the different encodings from the buffering behaviour using it.

Finally, happy to add a paragraph but the question would be where and using /* */ or multiple // ? Could you perhaps point me to another file where this is done, keen to follow not set any precedents for now :)

[calvinmetcalf]

What I'd be tempted to do in that case is separate out a test for all the different encodings from the buffering behaviour using it.

that would be amazing

as for comment style I believe they tend to be // but I don't believe there are hard and fast rules

[jasnell]

Buffer.from(expectedChunks[i])

[jasnell]

Left a couple nits pointing where Buffer.from() can be used instead of new Buffer(). That would obviously only apply to master/v6/v5 tho so if this test also needs to run in v4, feel free to ignore!

[alexjeffburke]

@jasnell appreciate you taking a look!

I'm happy to do that, but per discussions of @nodejs/streams it may be preferable to allow these to run against all versions of node that implement streams3. What could be worth an explicit decision is if that should be evident within 'in-tree' files or if any necessary glue be added in ports to readable-stream repo.

[jasnell]

Works for me... as I said, feel free to ignore ;-) That's actually the main reason why I didn't go through all of the existing tests and change them to use Buffer.from (or the other ones).

Come to think of it, as a best practice it might be a good idea to explicitly indicate (using code comments) in the tests themselves which node.js versions the test needs to be able to run on.

[Trott]

Come to think of it, as a best practice it might be a good idea to explicitly indicate (using code comments) in the tests themselves which node.js versions the test needs to be able to run on.

Yes, please. My dream is to eventually have that information as metadata in test comments so that an automated process can confirm that changes to tests don't suddenly make them pass in versions where they are supposed to fail. But that is way beyond the scope of this PR. :-)

[mcollina]

👍 on writing a comment stating on which node version this belongs to.

[alexjeffburke]

I'll look at adding those descriptive paragraphs - as for versioning, any objection to the following:

// node version target: 0.12

[alexjeffburke]

Just updated these two tests to address comments:

• added a couple of lines explaining the test
• included some info about the node version targeted

I also renamed consumeChunks() -> writeChunks() to better its behaviour.

[mcollina]

Apart for the two Buffer.from nits, LGTM.

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

[calvinmetcalf]

fyi I've got a shim working so I can run the readable streams (including the Buffer.from) against old node

[alexjeffburke]

@mcollina@calvinmetcalf re Buffer.from() - I understand the objection, but that's why I previously asked which version of node in-tree tests should target hence the exchange with @jasnell above :)

It's easy enough to conditionally check for the Buffer method etc, but it's still unclear whether the in-tree tests should target the lowest version of node (thus compat glue be added in test) or if it is ok for this to be handled in the readable-streams repo.

[jasnell]

Given the requirement that readable-stream has to support older versions of Node I think not going with the Buffer.from() here is appropriate.

[calvinmetcalf]

sorry, to clarify this is handled in readable-stream already

[alexjeffburke]

Seems like I can leave the files as is with 'new Buffer()'. Given previous LGTMs, is there anything else I should do before these can be landed?

[jasnell]

LGTM if @nodejs/streams is good with it.

[calvinmetcalf]

yup it's good, merged

[alexjeffburke]

Awesome, thanks! Very glad this was useful.

[MylesBorins]

@nodejs/streams should this be backported to v4?

[mcollina]

@thealphanerd Ideally yes, if it is not passing ping us.