Skip to content

Conversation

benjamingr
Copy link
Member

@benjamingr benjamingr commented Jan 30, 2022

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!)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot 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 benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2022
@nodejs-github-bot

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2022

Why copy paste the tests instead of running test262 directly?

@benjamingr
Copy link
Member Author

@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.

@ljharb
Copy link
Member

ljharb commented Jan 30, 2022

@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 ¯\_(ツ)_/¯

@benjamingr
Copy link
Member Author

Moving to draft to not trigger many github cis

@benjamingr benjamingr marked this pull request as draft January 30, 2022 18:22
@benjamingr benjamingr marked this pull request as ready for review January 30, 2022 19:48
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@benjamingr
Copy link
Member Author

@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

@benjamingr
Copy link
Member Author

@nodejs/streams ping

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
// 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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 5, 2022
@nodejs-github-bot 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 nodejs-github-bot merged commit 662fb5f into nodejs:master Feb 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 662fb5f

@danielleadams
Copy link
Contributor

@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.

@benjamingr
Copy link
Member Author

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

@danielleadams
Copy link
Contributor

@benjamingr got it. thanks!

targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #41775
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #41775
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: James M Snell <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#41775
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants