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

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

Merged
merged 6 commits into from Apr 17, 2019

Conversation

@leobalter
Copy link
Member

commented Apr 5, 2019

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 added the coverage label Apr 5, 2019

@leobalter leobalter self-assigned this Apr 5, 2019

@leobalter leobalter force-pushed the leobalter:2112-allsettled branch 3 times, most recently from 8ae055d to 481708c Apr 11, 2019

@leobalter leobalter force-pushed the leobalter:2112-allsettled branch from 481708c to 4bd5852 Apr 16, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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 Apr 16, 2019

@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

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

@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@leobalter

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

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

@gsathya

This comment has been minimized.

Copy link
Member

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 Apr 17, 2019

@leobalter leobalter removed the needs review label Apr 17, 2019

@leobalter leobalter merged commit 7e7b9e1 into tc39:master Apr 17, 2019

1 of 7 checks passed

ci/circleci: ChakraCore: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: JSC: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: SpiderMonkey: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: V8 --harmony: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: V8: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: XS: New or modified tests execution Your tests failed on CircleCI
Details
ci/circleci: Project lint, generation tests and build Your tests passed on CircleCI!
Details

@leobalter leobalter deleted the leobalter:2112-allsettled branch Apr 17, 2019

@mathiasbynens mathiasbynens referenced this pull request May 2, 2019

Closed

Advance to stage 4 #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.