Skip to content

Port tests from Promise.all to Promise.allSettled #2124

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
Merged
merged 6 commits into from
Apr 17, 2019
Merged

Port tests from Promise.all to Promise.allSettled #2124

merged 6 commits into from
Apr 17, 2019

Conversation

leobalter
Copy link
Member

@leobalter leobalter commented Apr 5, 2019 •
edited
Loading

Most of the spec tests are pretty similar, so I've been porting the tests over. This still requires modding things here and there.

@leobalter leobalter self-assigned this Apr 5, 2019
@leobalter leobalter mentioned this pull request Apr 5, 2019
Closed
@leobalter leobalter force-pushed the 2112-allsettled branch 3 times, most recently from 8ae055d to 481708c Compare April 12, 2019 22:36
@leobalter leobalter changed the title [WIP] Port tests from Promise.all to Promise.allSettled Port tests from Promise.all to Promise.allSettled Apr 16, 2019
@leobalter
Copy link
Member Author

I only need to finish reviewing my own tests and it's good for a review

@leobalter
Copy link
Member Author

This is now all set!

v8 with the --harmony flag is pretty close to have full compliance, nice job! cc @gsathya @mathiasbynens

@leobalter leobalter requested a review from rwaldron April 16, 2019 23:37
@mathiasbynens
Copy link
Member

@gsathya Could you take a look at the failing tests please?

@mathiasbynens
Copy link
Member

cc @jasonwilliams @rpamely

@mathiasbynens
Copy link
Member

Ref. tc39/proposal-promise-allSettled#11.
Ref. #2112.

@leobalter
Copy link
Member Author

@mathiasbynens @gsathya I think we should limit the resolve lookup to only happen if the iterable has at least one item to iterate with. This requires some fix in the spec text. WDYT?

Current status (shown in the tests) will always get the resolve once even if iterable is empty.

@leobalter
Copy link
Member Author

The failing test checking the built-in function name is also happening in the tests for Promise.all

@gsathya
Copy link
Member

gsathya commented Apr 17, 2019

@gsathya Could you take a look at the failing tests please?

One is the unimplemented optimization (tc39/proposal-promise-allSettled#40) and the other is a known web compat issue with function name.

The failing test checking the built-in function name is also happening in the tests for Promise.all

Yeah, that's a known inconsistency. More background here:
https://bugs.chromium.org/p/v8/issues/detail?id=4709
tc39/ecma262#1049

@leobalter leobalter requested a review from rwaldron April 17, 2019 17:48
@leobalter leobalter merged commit 7e7b9e1 into tc39:master Apr 17, 2019
@leobalter leobalter deleted the 2112-allsettled branch April 17, 2019 18:18
@mathiasbynens mathiasbynens mentioned this pull request May 2, 2019
Closed
@jasonwilliams jasonwilliams mentioned this pull request Jun 12, 2019
Merged
@chicoxyzzy chicoxyzzy mentioned this pull request Nov 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwaldron rwaldron rwaldron approved these changes

Assignees

@leobalter leobalter

Labels
coverage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leobalter @mathiasbynens @gsathya @rwaldron