test: add stream map tests #41642

benjamingr · 2022-01-22T08:26:46Z

Adds more tests to cover more cases of map namely that it works on infinite streams and the error handling behavior. Most other methods also need these sort of tests.

cc @nodejs/streams @aduh95

nodejs-github-bot · 2022-01-22T08:46:26Z

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

mcollina

lgtm

aduh95 · 2022-01-22T09:56:31Z

test/parallel/test-stream-map.js

+  const stream = Readable.from([1, 2, 3, 4, 5]).map(async (x) => {
+    if (x === 3) {
+      stream.emit('error', new Error('boom'));
+    }
+    return x + x;
+  });
+  assert.rejects(
+    stream.map((x) => x + x).toArray(),
+    /boom/,


We could check the error by reference here and below.

Suggested change

const stream = Readable.from([1, 2, 3, 4, 5]).map(async (x) => {

if (x === 3) {

stream.emit('error', new Error('boom'));

}

return x + x;

});

assert.rejects(

stream.map((x) => x + x).toArray(),

/boom/,

const error = new Error();

const stream = Readable.from([1, 2, 3, 4, 5]).map(async (x) => {

if (x === 3) {

stream.emit('error', error);

}

return x + x;

});

assert.rejects(

stream.map((x) => x + x).toArray(),

error,

I'm not sure that's actually a guarantee we necessarily want to make in this case (that is - that if we emit we don't wrap the error in any way) - the guarantee currently tested is much smaller (that we include the message).

If you feel strongly it's a guarantee we should make - I'll change it (we should also probably update the docs)

If the current implementation does reject with the same error object, and in the future we decide to wrap this object in some other object, wouldn't you agree this would be a semver-major change? That's why I would say testing for it would make sense.

The proposal says we should Await at each step, so I would think that implies the same rejected object is returned. Maybe we should make another test that verifies that when using a non-Error objects, everything still behaves. Non-blocking suggestion.

wouldn't you agree this would be a semver-major change?

Not on an experimental API - once it graduates 100% absolutely.

The proposal says we should Await at each step, so I would think that implies the same rejected object is returned.

Yes but note that in this case the user is explicitly emiting the error event rather than throwing an error - I agree that for throwing we 100% should align.

Non-blocking suggestion.

Please keep making suggestions discussing these things is important!

aduh95 · 2022-01-22T09:59:48Z

test/parallel/test-stream-map.js

+  (async () => {
+    assert.deepStrictEqual(await stream.toArray(), [2, 4, 6, 8, 10]);
+  })().then(common.mustCall());


It might be sound to consider using a .mjs file for future tests like this so we can use TLA and skip all those async functions + then(mustCall).

I was wondering if that was allowed but couldn’t find non-loader related .mjs tests. If it is I think it’s a good idea

@aduh95 so can you confirm it's allowed :)? I'll port the file to .mjs I don't mind the tests running sequentially that'd actually make things easier to debug and the performance won't change since it's all microticks anyway.

Nothing forbids it, so it's allowed (and there's no reason to avoid it since Node.js 10 is EOL and all maintained versions of Node.js support ESM). For example see test/parallel/test-child-process-fork-url.mjs and test/parallel/test-fs-cp.mjs which have little to do with loaders and have been written this way because it was more convenient.

Ack I'll move all of these to mjs and simplify them as soon as the two PRs by outside contributors land to not create conflicts for them.

nodejs-github-bot · 2022-01-23T21:14:32Z

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

nodejs-github-bot · 2022-01-24T08:42:49Z

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

nodejs-github-bot · 2022-01-24T10:42:34Z

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

aduh95 · 2022-01-24T11:16:27Z

@benjamingr we should probably fix this flaky test (failing on node-test-binary-arm-12+):

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

4 !== 2

    at Immediate._onImmediate (/home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-stream-map.js:68:12)
    at processImmediate (node:internal/timers:466:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 4,
  expected: 2,
  operator: 'strictEqual'
}

Node.js v18.0.0-pre

benjamingr · 2022-01-24T11:23:49Z

@aduh95 hmm, any idea why it's failing?

cc @ronag

aduh95 · 2022-01-24T11:37:30Z

setImmediate is scheduled after 100ms? Would replacing setImmediate with queueMicrotask save us here? Or use a very big number for the setTimeout value?

benjamingr · 2022-01-24T11:52:16Z

@aduh95

setImmediate is scheduled after 100ms? Would replacing setImmediate with queueMicrotask save us here? Or use a very big number for the setTimeout value?

The failing test doesn't seem to use timers at all -

{
  // Map works on non-objectMode streams
  const stream = new Readable({
    read() {
      this.push(Uint8Array.from([1]));
      this.push(Uint8Array.from([2]));
      this.push(null);
    }
  }).map(async ([x]) => {
    return x + x;
  }).map((x) => x + x);
  const result = [4, 8];
  (async () => {
    for await (const item of stream) {
      assert.strictEqual(item, result.shift());
    }
  })().then(common.mustCall());
}

That's the one that's flakey on arm if I understand correctly right?

aduh95 · 2022-01-24T12:14:39Z

It's this one:

node/test/parallel/test-stream-map.js

Lines 49 to 69 in b97b81d

    
           // Concurrency + AbortSignal 
        
           const ac = new AbortController(); 
        
           let calls = 0; 
        
           const stream = Readable.from([1, 2, 3, 4, 5]).map(async (_, { signal }) => { 
        
             calls++; 
        
             await setTimeout(100, { signal }); 
        
           }, { signal: ac.signal, concurrency: 2 }); 
        
           // pump 
        
           assert.rejects(async () => { 
        
             for await (const item of stream) { 
        
               // nope 
        
               console.log(item); 
        
             } 
        
           }, { 
        
             name: 'AbortError', 
        
           }).then(common.mustCall()); 
        
           setImmediate(() => { 
        
             ac.abort(); 
        
             assert.strictEqual(calls, 2); 
        
           });

The test is flaky on master, I've copied the stack trace from another PR hence the non-matching line numbers, sorry for the confusion.

benjamingr · 2022-01-24T12:38:57Z

Ah phew that's much much better and a way less severe bug. I'll push a fix.

Add more tests to check and enforce the behavior of the map method. Co-Authored-By: Antoine du Hamel <[email protected]>

benjamingr · 2022-01-24T12:52:49Z

I'm giving you Co-Authored-By on the commit @aduh95 for the feedback and help. If you prefer otherwise let me know.

nodejs-github-bot · 2022-01-24T13:12:22Z

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

nodejs-github-bot · 2022-01-24T15:01:39Z

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

aduh95

Let's land this!

nodejs-github-bot · 2022-01-24T17:59:46Z

Commit Queue failed

- Loading data for nodejs/node/pull/41642 ✔ Done loading data for nodejs/node/pull/41642 ----------------------------------- PR info ------------------------------------ Title test: add stream map tests (#41642) Author Benjamin Gruenbaum (@benjamingr) Branch benjamingr:stream-map-improve-coverage -> nodejs:master Labels test, author ready, needs-ci Commits 1 - test: add stream map tests Committers 1 - Benjamin Gruenbaum PR-URL: https://github.com/nodejs/node/pull/41642 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41642 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 22 Jan 2022 08:26:46 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41642#pullrequestreview-860186327 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/41642#pullrequestreview-860186417 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/41642#pullrequestreview-861240334 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-01-24T15:01:39Z: https://ci.nodejs.org/job/node-test-pull-request/42121/ - Querying data for job/node-test-pull-request/42121/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 41642 From https://github.com/nodejs/node * branch refs/pull/41642/merge -> FETCH_HEAD ✔ Fetched commits as 3ee8c3e45e5f..a5d10870149e -------------------------------------------------------------------------------- [master e621b51bb8] test: add stream map tests Author: Benjamin Gruenbaum Date: Sat Jan 22 10:25:18 2022 +0200 1 file changed, 105 insertions(+), 16 deletions(-) ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- test: add stream map tests

Add more tests to check and enforce the behavior of the map method.

Co-Authored-By: Antoine du Hamel [email protected]

PR-URL: #41642
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Antoine du Hamel [email protected]

[master 943a5f0805] test: add stream map tests
Author: Benjamin Gruenbaum [email protected]
Date: Sat Jan 22 10:25:18 2022 +0200
1 file changed, 105 insertions(+), 16 deletions(-)
✖ 943a5f08059dfefd2d2b65eb8610ce479d04bb18
✖ 3:0 Co-authored-by must be a trailer co-authored-by-is-trailer
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 5:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/1741370378

Add more tests to check and enforce the behavior of the map method. Co-Authored-By: Antoine du Hamel <[email protected]> PR-URL: #41642 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

benjamingr · 2022-01-24T18:16:35Z

Landed in Landed in ce41395 🎉

Add more tests to check and enforce the behavior of the map method. Co-Authored-By: Antoine du Hamel <[email protected]> PR-URL: nodejs#41642 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Add more tests to check and enforce the behavior of the map method. Co-Authored-By: Antoine du Hamel <[email protected]> PR-URL: #41642 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

danielleadams · 2022-04-11T07:35:27Z

@benjamingr do you mind backporting these to v16.x? They break when backporting as is.

benjamingr · 2022-04-11T09:31:07Z

@danielleadams I don't think these need backporting they test a feature that doesn't exist on v16.x - let me add the labels

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 22, 2022

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2022

mcollina approved these changes Jan 22, 2022

View reviewed changes

ronag approved these changes Jan 22, 2022

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 22, 2022

aduh95 reviewed Jan 22, 2022

View reviewed changes

github-actions bot mentioned this pull request Jan 23, 2022

CI Reliability 2022-01-23 nodejs/reliability#180

Open

13 tasks

github-actions bot mentioned this pull request Jan 24, 2022

CI Reliability 2022-01-24 nodejs/reliability#181

Open

22 tasks

test: add stream map tests

a5d1087

Add more tests to check and enforce the behavior of the map method. Co-Authored-By: Antoine du Hamel <[email protected]>

benjamingr force-pushed the stream-map-improve-coverage branch from 4851448 to a5d1087 Compare January 24, 2022 12:53

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2022

aduh95 approved these changes Jan 24, 2022

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2022

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 24, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 24, 2022

benjamingr closed this Jan 24, 2022

ruyadorno mentioned this pull request Feb 8, 2022

v17.5.0 release proposal #41897

Merged

aduh95 mentioned this pull request Feb 12, 2022

stream: add more filter tests #41936

Closed

benjamingr added dont-land-on-v12.x labels Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add stream map tests #41642

test: add stream map tests #41642

benjamingr commented Jan 22, 2022

nodejs-github-bot commented Jan 22, 2022

mcollina left a comment

aduh95 Jan 22, 2022

benjamingr Jan 24, 2022

aduh95 Jan 24, 2022

benjamingr Jan 24, 2022

aduh95 Jan 22, 2022

benjamingr Jan 22, 2022

benjamingr Jan 23, 2022

aduh95 Jan 24, 2022

benjamingr Jan 24, 2022

nodejs-github-bot commented Jan 23, 2022

nodejs-github-bot commented Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

aduh95 commented Jan 24, 2022 •

edited

Loading

benjamingr commented Jan 24, 2022

aduh95 commented Jan 24, 2022

benjamingr commented Jan 24, 2022

aduh95 commented Jan 24, 2022

benjamingr commented Jan 24, 2022

benjamingr commented Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

aduh95 left a comment

nodejs-github-bot commented Jan 24, 2022

PR-URL: #41642
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Antoine du Hamel [email protected]

benjamingr commented Jan 24, 2022

danielleadams commented Apr 11, 2022

benjamingr commented Apr 11, 2022

test: add stream map tests #41642

test: add stream map tests #41642

Conversation

benjamingr commented Jan 22, 2022

nodejs-github-bot commented Jan 22, 2022

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 23, 2022

nodejs-github-bot commented Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

aduh95 commented Jan 24, 2022 • edited Loading

benjamingr commented Jan 24, 2022

aduh95 commented Jan 24, 2022

benjamingr commented Jan 24, 2022

aduh95 commented Jan 24, 2022

benjamingr commented Jan 24, 2022

benjamingr commented Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

nodejs-github-bot commented Jan 24, 2022

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 24, 2022

PR-URL: #41642 Reviewed-By: Matteo Collina [email protected] Reviewed-By: Robert Nagy [email protected] Reviewed-By: Antoine du Hamel [email protected]

benjamingr commented Jan 24, 2022

danielleadams commented Apr 11, 2022

benjamingr commented Apr 11, 2022

aduh95 commented Jan 24, 2022 •

edited

Loading

PR-URL: #41642
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Antoine du Hamel [email protected]