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 tests for Promise.withResolvers proposal #3867

Merged
merged 11 commits into from
Sep 8, 2023

Conversation

peetklecha
Copy link
Contributor

This shouldn't be merged yet as Promise.withResolvers is still only Stage 2.

@peetklecha peetklecha marked this pull request as ready for review July 12, 2023 09:02
@peetklecha peetklecha requested a review from a team as a code owner July 12, 2023 09:02
@peetklecha
Copy link
Contributor Author

Now Stage 3.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
features.txt Outdated Show resolved Hide resolved
peetklecha and others added 5 commits July 18, 2023 10:18
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Ms2ger <Ms2ger@gmail.com>
Co-authored-by: Linus Groh <mail@linusgroh.de>
@ptomato
Copy link
Contributor

ptomato commented Aug 9, 2023

Thanks for contributing this! Would you mind opening a tracking issue? An example is #3756. You don't have to write the whole testing plan, someone else can help with that, but we should use one to verify that we're covering everything in the proposal.

I haven't looked too closely at this PR yet so I might be missing something, but it seems like there are some cases particularly with subclassing Promise that aren't yet tested.

We also need to add the standard boilerplate tests for built-in APIs. For an example, see length.js, name.js, prop-desc.js, not-a-constructor.js, builtin.js in https://github.com/tc39/test262/tree/main/test/built-ins/Array/fromAsync.

We don't have to block reviewing and merging this PR on complete coverage, the other tests can come in following PRs, but we should keep track in the tracking issue of what's been merged.

@syg
Copy link
Contributor

syg commented Sep 8, 2023

Could we have this merged soon for shipping in Chrome?

@peetklecha
Copy link
Contributor Author

sorry for the delay with this.

  • i've now addressed all the comments i've recieved
  • there's a failing test from circle/ci which wasn't there before and which i don't understand, any help with it would be appreciated
  • per @ptomato 's request i've created an issue to track full test coverage of Promise.withResolvers: Tests for Promise.withResolvers #3910

@ptomato ptomato merged commit 1db9a49 into tc39:main Sep 8, 2023
1 check passed
@ptomato
Copy link
Contributor

ptomato commented Sep 8, 2023

Thanks, @peetklecha. The failing CI job is #3891, we really should remove the job until that's fixed. I'll merge this now, and add some suggestions to the tracking issue you opened.

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

Successfully merging this pull request may close these issues.

7 participants