Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: port more test262 tests #41974

Merged
merged 1 commit into from
Feb 23, 2022

Conversation

benjamingr
Copy link
Member

Add some test262 tests for every, add some length checks.

Turns out properties need to be writable after all according to the
test262 tests.

cc @nodejs/streams @aduh95

@benjamingr benjamingr added the stream Issues and PRs related to the stream subsystem. label Feb 14, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 14, 2022
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2022
@nodejs-github-bot
Copy link
Collaborator

@@ -186,15 +186,15 @@ function asIndexedPairs(options = undefined) {
}.call(this);
}

async function some(fn, options) {
async function some(fn, options = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to create a throw away object.

Suggested change
async function some(fn, options = {}) {
async function some(fn, options = undefined) {

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2022
@nodejs-github-bot
Copy link
Collaborator

Add some test262 tests for `every`, add some `length` checks.

Turns out properties need to be writable after all according to the
test262 tests.
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

Commit Queue failed
- Loading data for nodejs/node/pull/41974
✔  Done loading data for nodejs/node/pull/41974
----------------------------------- PR info ------------------------------------
Title      stream: port more test262 tests (#41974)
Author     Benjamin Gruenbaum  (@benjamingr)
Branch     benjamingr:add-more-test262-tests -> nodejs:master
Labels     stream, needs-ci
Commits    1
 - stream: port more test262 tests
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/41974
Reviewed-By: James M Snell 
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41974
Reviewed-By: James M Snell 
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - stream: port more test262 tests
   ℹ  This PR was created on Mon, 14 Feb 2022 20:47:25 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41974#pullrequestreview-887813994
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/41974#pullrequestreview-887815061
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41974#pullrequestreview-887906150
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-23T18:10:07Z: https://ci.nodejs.org/job/node-test-pull-request/42747/
- Querying data for job/node-test-pull-request/42747/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1889467631

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 23, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2022
@nodejs-github-bot nodejs-github-bot merged commit fd3997b into nodejs:master Feb 23, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in fd3997b

sxa pushed a commit to sxa/node that referenced this pull request Mar 7, 2022
Add some test262 tests for `every`, add some `length` checks.

Turns out properties need to be writable after all according to the
test262 tests.

PR-URL: nodejs#41974
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@sxa sxa mentioned this pull request Mar 8, 2022
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Add some test262 tests for `every`, add some `length` checks.

Turns out properties need to be writable after all according to the
test262 tests.

PR-URL: nodejs#41974
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@juanarbol
Copy link
Member

This PR can not be landed in V16.x due to a dependency with #41775

targos pushed a commit that referenced this pull request Jul 12, 2022
Add some test262 tests for `every`, add some `length` checks.

Turns out properties need to be writable after all according to the
test262 tests.

PR-URL: #41974
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Add some test262 tests for `every`, add some `length` checks.

Turns out properties need to be writable after all according to the
test262 tests.

PR-URL: #41974
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Add some test262 tests for `every`, add some `length` checks.

Turns out properties need to be writable after all according to the
test262 tests.

PR-URL: nodejs/node#41974
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[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.

8 participants