test: add few stream utils to common

[lundibundi]

• test: add collectStream and collectChildStreams utilities

  collectStream allows to collect arbitrary readable that supports async
  iteration (which should be almost all) with Promise and callback API support.

  collectChildStreams expands on collectStream for child processes as
  it's quite common to need the resulting output of the child and collects
  their stdout, stderr if avaialble and optionally waits for another
  promise (i.e once(child, 'exit') can be used).

• test: refactor few tests with collectStream/collectChildStreams utils

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

/cc @nodejs/testing

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

To maintain alphabetical order in the README, collectStream should be moved after collectChildStream.

[lundibundi]

ping @Trott @jasnell @nodejs/streams @nodejs/testing feedback addressed, PTAL.

[nodejs-github-bot]

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

[Trott]

Optional suggestion: Since these utilities are only used by a few tests, maybe put them in their own common/streams.js module instead of the main common module that has to be imported by (nearly) all our tests?

[lundibundi]

@Trott Sounds good, done.

[nodejs-github-bot]

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

[vweevers]

Is there a precedent for combining promises with assert in the tests? It seems to me that (without --unhandled-rejections=strict) the following pattern results in a unhandled promise rejection:

const assert = require('assert')

async function commonUtil () {
  return 'nope'
}

commonUtil().then(function (res) {
  assert.strictEqual(res, 'foo')
})

[lundibundi]

@vweevers our test's policy is to crash on unhandledRejection (see

node/test/common/index.js

Line 620 in 8429295

process.on('unhandledRejection', crashOnUnhandledRejection);

).
And the common/index.js is included in every test so it should be fine.

[BridgeAR]

I personally would rather keep the code as it is. Adding more Node.js specific helper functions is always a mental overhead for new collaborators and the benefit seems small to me. Currently it's straight forward to understand what the code does while it's somewhat less clear with the helper function.

[lundibundi]

@BridgeAR while I agree that mental overhead is an important consideration but in this particular use case every time I write a test related to Streams or any type of IO basically I look for such utility considering how widespread and used this pattern is in our tests. Also, this helper is imo easy to understand even from the name alone while adding benefits of properly handling data/errors in the async use cases which may be hard to get right (callback/events -> async bridges).

[addaleax]

So … I don’t know if we should move forward with this, but how would people feel about moving something alone these lines to stream.Readable? That would make it part of our public API, and given the popularity of modules like bl, it might make sense to do so?

[jasnell]

/cc @mcollina @rvagg (who can comment from a bl point of view)

[rvagg]

I don't really have much to say except that I always think increasing Node's API surface area should be considered a bad thing without a strong case otherwise; performance being one of them (fs.mkdir({recursive}) being a good example when mkdirp is available in npm). "lots of people want this" and "X module is really popular" are not good reasons.

Don't add something that's easy to do in userland, and is already done in userland, unless you want to be maintaining that standard library forever. Python anyone? How about Java? How long are you prepared for CI to take. How many tests do you want in your way when you want to change something in the future and how many people are you prepared to scream at you when you break their favourite thing? How many weird edge cases are you comfortable with users finding and relying on that aren't covered by the test suite?

I'm sad that we never wrote this down clearly somewhere at a time when we all roughly agreed this was how Node's standard library was governed.

[mcollina]

I’m not convinced to move this to the stream module. They do not follow any of the good reasons to add them to core as their implementation is covered by our docs.

I’m not really convinced to have them as test utilities, as they would likely make things harder to debug when a stream bug arises.