stream: initial port of test262 tests by benjamingr · Pull Request #41775 · nodejs/node · GitHub

This adds some test262 tests from tc39/test262#2818 to Node's implementation of iterator-helpers + a few fixes.

I want to do this in batches since it will probably require a bunch of changes (this one just adds the ones for asIndexedPairs and drop as they make sense).

cc @nodejs/streams @ljharb @rwaldron (sorry for the no context ping Rick! thought I'd give you a heads up we're going to use all of these we can unless there is something else I should be aware of!)

Review requested:

@nodejs/streams

Why copy paste the tests instead of running test262 directly?

@ljharb well - few reasons:

Node typically doesn't implement TC39 features so I'm not aware of infrastructure to do this. I don't expect this to be a common occurrence as anything that can come from the language directly (and v8) should.
We're not actually implementing the spec de-jure here since the spec speaks of AsyncIterators and Iterators and we're adding this methods on Readable streams.
A bunch of them aren't really relevant (e.g. stuff that isn't relevant to streams because of backpressure like the timing of exceptions when you subclass an AsyncIterator with a custom get).

Though this is mostly just my intuition and I recently started - it's possible there is a way to run test262 tests in Node I'm not aware of and to somehow tell it to use a different object with construction methods (like Readable.from instead of AsyncIterator.from) and exclude specific tests but that seemed like a lot more work than just porting them and checking every few months what changed and porting that.

@benjamingr those are good points. i think it'd be useful if test262 allowed you to point the tests at an arbitrary object - but the only way to do it now would probably be to use vm and hack together a fake global, so you could exercise the tests. Copypasting seems fine then ¯\_(ツ)_/¯

Moving to draft to not trigger many github cis

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

@mcollina @ronag any chance this can get a review? It's just adding tests + some esoteric behavior changes (like the .length and .name properties of the methods) to match the spec

@nodejs/streams ping

This arguably should be added to LICENSE instead with a comment here indicating that's where the relevant license info is located. But not blocking.

Landed in 662fb5f

PR-URL: #41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

@benjamingr when pulling in for the v16 for a patch release, this breaks the build. I am not sure if this is dependent on a semver minor change, or needs a backport - do you mind taking a look? Thank you.

@danielleadams it depends on a previous semver-major patch (adding these methods).

@benjamingr got it. thanks!

PR-URL: #41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs/node#41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

benjamingr commented Jan 30, 2022

View reviewed changes

Comment thread lib/stream.js Outdated

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

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

This comment has been minimized.

Sign in to view

Mesteery reviewed Jan 30, 2022

View reviewed changes

Comment thread lib/stream.js Outdated

Comment thread lib/stream.js Outdated

aduh95 reviewed Jan 30, 2022

View reviewed changes

Comment thread lib/stream.js Outdated

Comment thread lib/stream.js Outdated

Comment thread lib/internal/streams/operators.js Outdated

Comment thread lib/internal/streams/operators.js Outdated

Comment thread lib/internal/streams/operators.js Outdated

benjamingr marked this pull request as draft January 30, 2022 18:22

benjamingr marked this pull request as ready for review January 30, 2022 19:48

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

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

This comment has been minimized.

Sign in to view

github-actions Bot mentioned this pull request Jan 31, 2022

CI Reliability 2022-01-31 nodejs/reliability#188

Open

28 tasks

This was referenced Feb 1, 2022

CI Reliability 2022-02-01 nodejs/reliability#189

Open

CI Reliability 2022-02-02 nodejs/reliability#190

Open

github-actions Bot mentioned this pull request Feb 3, 2022

CI Reliability 2022-02-03 nodejs/reliability#191

Open

41 tasks

Linkgoron approved these changes Feb 3, 2022

View reviewed changes

github-actions Bot mentioned this pull request Feb 4, 2022

CI Reliability 2022-02-04 nodejs/reliability#192

Open

38 tasks

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022

jasnell reviewed Feb 5, 2022

View reviewed changes

jasnell approved these changes Feb 5, 2022

View reviewed changes

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 5, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 5, 2022

nodejs-github-bot merged commit 662fb5f into nodejs:master Feb 5, 2022

This was referenced Feb 6, 2022

CI Reliability 2022-02-06 nodejs/reliability#194

Open

CI Reliability 2022-02-07 nodejs/reliability#195

Open

CI Reliability 2022-02-08 nodejs/reliability#196

Open

ruyadorno pushed a commit that referenced this pull request Feb 8, 2022

stream: initial port of test262 tests

f8d2955

PR-URL: #41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ruyadorno mentioned this pull request Feb 8, 2022

v17.5.0 release proposal #41897

Merged

github-actions Bot mentioned this pull request Feb 9, 2022

CI Reliability 2022-02-09 nodejs/reliability#197

Open

24 tasks

benjamingr mentioned this pull request Feb 11, 2022

Suddenly started to get this error Automattic/mongoose#11379

Closed

danielleadams added backport-blocked-v16.x and removed backport-blocked-v16.x labels Feb 28, 2022

aduh95 added dont-land-on-v12.x and removed backport-requested-v16.x labels Mar 1, 2022

juanarbol mentioned this pull request May 28, 2022

stream: port more test262 tests #41974

Merged

targos removed the dont-land-on-v16.x label Jul 12, 2022

targos pushed a commit that referenced this pull request Jul 12, 2022

stream: initial port of test262 tests

02b81e4

PR-URL: #41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Jul 31, 2022

stream: initial port of test262 tests

3ef9f8d

PR-URL: #41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022

stream: initial port of test262 tests

4186b8b

PR-URL: nodejs/node#41775 Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

+              // These tests are manually ported from the draft PR for the test262 test suite
+              // Authored by Rick Waldron in https://github.com/tc39/test262/pull/2818/files
+              // test262 license:

stream: initial port of test262 tests by benjamingr · Pull Request #41775 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: initial port of test262 tests - #41775

stream: initial port of test262 tests#41775nodejs-github-bot merged 1 commit intonodejs:masternodejs/node:masterfrom benjamingr:add-iterator-helper-test262-testsbenjamingr/io.js:add-iterator-helper-test262-testsCopy head branch name to clipboard

Conversation

benjamingr commented Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 30, 2022

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

ljharb commented Jan 30, 2022

Uh oh!

benjamingr commented Jan 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ljharb commented Jan 30, 2022

Uh oh!

benjamingr commented Jan 30, 2022

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 30, 2022

Uh oh!

benjamingr commented Jan 31, 2022

Uh oh!

benjamingr commented Feb 2, 2022

Uh oh!

jasnell Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 5, 2022

Uh oh!

danielleadams commented Feb 28, 2022

Uh oh!

benjamingr commented Mar 1, 2022

Uh oh!

danielleadams commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

stream: initial port of test262 tests#41775
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
benjamingr:add-iterator-helper-test262-tests

benjamingr commented Jan 30, 2022 •
edited
Loading