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

Add coverage for Promise.allSettled proposal #2112

Closed
rpamely opened this issue Mar 27, 2019 · 7 comments
Closed

Add coverage for Promise.allSettled proposal #2112

rpamely opened this issue Mar 27, 2019 · 7 comments

Comments

@rpamely
Copy link

rpamely commented Mar 27, 2019

This issue is to track the progress of the test262 coverage of the stage 3 Promise.allSettled proposal. https://github.com/tc39/proposal-promise-allSettled

Suggestions for tests welcome, and I will update this text. The tests for Promise.all are a good place to start.

@mathiasbynens @jasonwilliams

@rpamely
Copy link
Author

rpamely commented Mar 27, 2019

@domenic previously posted when reviewing the spec:

Reviewing the spec text, I was reminded of the extra care we put in to making Promise.all behave reasonably in the face of weird promise subclasses and similar scenarios, e.g. with the [[AlreadyResolved]] internal slot. I think those are pretty exhaustively tested in existing test262, and this issue is about making sure the same is done for Promise.allSettled.

When writing test262 tests, please be sure to test all the same weird edge cases that can occur as with Promise.all---plus the extra ones introduced by the reject element function and its interactions.

@mathiasbynens
Copy link
Member

@anba wrote some tests while working on his SpiderMonkey implementation: https://phabricator.services.mozilla.com/D25209#change-qATBO8Ktuxxu

@astlaurent1
Copy link

I got myself up and running with one test: astlaurent1@ade44b4. I had copied over a test from the Promise.all directory that I thought would behave the same, but I found slightly different behavior

Note: I'm running against a webpack'd version of the shim: astlaurent1/Promise.allSettled@631acd6 (use the file in dist/ as the prelude)

Oh and --prelude is broken without this: bocoup/test262-stream#23

@leobalter
Copy link
Member

@astlaurent1 I'm porting the tests from Promise.all, I had my commits online but didn't open a PR while it's still a WIP. Now you can see it here: #2124

One thing to consider is that a test should not be adapted to a shim, and maybe the shim might need a fix as well.

@leobalter
Copy link
Member

Also, the plan is to review and port tests from SpiderMonkey to Test262, I just need to finish one thing to start another.

@astlaurent1
Copy link

@leobalter Very cool! Thank you for commenting. My efforts were still a good learning exercise (and uncovered a recent bug in test262-harness), so no wasted time there

Is there a communication channel where people discuss what they're working on? I'd like to contribute in general, and would prefer to know that I'm not duplicating efforts when I pick something up

One thing to consider is that a test should not be adapted to a shim, and maybe the shim might need a fix as well.

Oh, absolutely! I highlighted the behavior difference with this in mind, but did not communicate that explicitly.

@leobalter
Copy link
Member

Coverage is done.

Is there a communication channel where people discuss what they're working on?

@astlaurent1 Not much other than flagging on the issues/PRs here. There is a #test262 channel on Mozilla IRC but it's not very active.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants