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.

Sign up for GitHub

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

Jump to bottom

Async-from-sync test the iterator closes when .throw() is undefined #3976

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Async-from-sync test the iterator closes when .throw() is undefined #3976

merged 4 commits into from
Jan 23, 2024

Conversation

ioannad
Copy link
Contributor

@ioannad ioannad commented Dec 28, 2023 •
edited
Loading

This adds tests for the normative changes in tc39/ecma262#2600 for the new steps in %AsyncFromSyncIteratorPrototype%.throw, also modifying two existing tests.

These tests have been tested with this PR of engine262, which implements the normative changes: engine262/engine262#230

Partly fixes #3387

@ioannad ioannad requested a review from a team as a code owner December 28, 2023 10:13
@ioannad ioannad mentioned this pull request Dec 28, 2023
Open
ljharb
ljharb previously requested changes Dec 28, 2023
View reviewed changes
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-null.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-get-return-undefined.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-get-return-undefined.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-get-return-undefined.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-poisoned-return.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-return-not-object.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-return-object.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-return-object.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined-return-object.js Outdated Show resolved Hide resolved
test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-undefined.js Outdated Show resolved Hide resolved
@ioannad
Copy link
Contributor Author

ioannad commented Dec 29, 2023

Thank you @nicolo-ribaudo and @ljharb for the review and suggestions!

@ioannad ioannad mentioned this pull request Dec 29, 2023
Closed
@bakkot
Copy link
Contributor

bakkot commented Jan 9, 2024

@Ms2ger Re: the label: the PR to the specification has had approval for ~2 years and is only waiting on these tests to land (i.e. it's conceptually "stage 3"). If you're just waiting on the specification, and these are otherwise ready to land, they should be landed, I think.

We can land the spec PR first if necessary, but I don't want to do that until the tests are actually ready to land.

@ptomato ptomato added has consensus This has committee consensus and removed awaiting specification labels Jan 13, 2024
ptomato
ptomato approved these changes Jan 13, 2024
View reviewed changes
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I did not check these line by line against the normative change. So it would be nice-to-have if someone more familiar with that change would take a look. Even without that, I think the use of engine262 gives sufficient confidence and I would be fine with landing it as is.

test/built-ins/AsyncFromSyncIteratorPrototype/throw/throw-null.js Outdated Show resolved Hide resolved
ptomato
ptomato approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

👍

ioannad and others added 4 commits January 23, 2024 13:07
@ioannad @ptomato
Adjust existing tests to match new spec for async-from-sync iterators
de3cccd 
Testing normative changes of ecma626 PR 2600
@ioannad @ptomato
Test that async-from-sync iterator closes when throw is undefined.
5f9a5b9 
These test paths from newly added call to IteratorClose in step 7.c of
 %AsyncFromSyncIteratorPrototype%.throw
as per normative changes of ecma626 PR 2600
@ioannad @ljharb @ptomato
Apply suggestions from code review
1f4a2b7 
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ioannad @ptomato
Refactoring tests to use the Async Helpers.
216e3c3 
Addresses review comments
- https://github.com/tc39/test262/pull/3976/files#r1451071569
- https://github.com/tc39/test262/pull/3976/files#r1451075479
@ptomato ptomato force-pushed the async-from-sync-iterator-close-tests-throw-undefined branch from 72f2345 to 216e3c3 Compare January 23, 2024 21:07
@ptomato ptomato dismissed ljharb’s stale review January 23, 2024 21:07

Comments have been addressed

@ptomato
Copy link
Contributor

ptomato commented Jan 23, 2024

All comments have been addressed and no new ones have shown up since last week, let's let this train leave the station! 😄

@ptomato ptomato merged commit 2c58671 into tc39:main Jan 23, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo left review comments

@ptomato ptomato ptomato approved these changes

@ljharb ljharb ljharb left review comments

Assignees
No one assigned
Labels
has consensus This has committee consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tests for Async-from-Sync Iterator changes
6 participants
@ioannad @bakkot @ptomato @ljharb @nicolo-ribaudo @Ms2ger