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

Implements Promise.allSettled() #1090

Merged
merged 3 commits into from
Jan 27, 2022
Merged

Conversation

grob
Copy link

@grob grob commented Nov 21, 2021

Resolves #1061

Reg. tests: i tried re-generating the 262 test properties file, but it marks lots of allSettled tests as to-skip with {unsupported: [async]}, so i just removed them from the list of skipped ones. Hope this is correct.

@p-bakker
Copy link
Collaborator

Great, thnx for contributing.

As for test262 properties and the lines with '{unsupported: ...}' behind them: that means that that testcase depends on features Rhino doesn't support yet. Such lines should remain in the .properties file, as otherwise the test suite fails

@grob
Copy link
Author

grob commented Nov 21, 2021

thanks for your fast reply! the main reason i just removed the tests from the skip list is because they all pass, so just to clarify – should i now:

  1. just remove those allSettled/… lines without {unsupported: [async]}, or
  2. re-generate the whole test262.properties file?

i'm a bit afraid to do the latter because i don't want to affect the whole tests within this pull request.

@p-bakker
Copy link
Collaborator

If you regenerate the properties file, the difference with the current .properties file should only be the tests that have started passing due to your feature implementation,.

There's also little risk in running the regenerate, as you can check the result (and the delta with the previous version) before committing it

If the tests marked with 'unsupported: ...' pass, something is wrong, as Rhino doesn't support async

@grob
Copy link
Author

grob commented Nov 21, 2021

thanks, i re-generated test262.properties. but – sorry to bother you, being a long time happy user of rhino, but a newbie contributor, i'm probably asking stupid questions – if i remove all allSettled tests from the skiplist, why do they pass?

nevertheless, thanks for you help! i humbly hope this pull request makes it into 1.7.14 release ;-)

@p-bakker
Copy link
Collaborator

Have to look into why they would pass or what else is going on.

1.7.14 RC1 has already been released and we're only fixing regressions or major bugs for the final release, so this PR would go into 1.7.15

@p-bakker
Copy link
Collaborator

Wasn't thinking straight yesterday: because the tests marked with 'unsupported: ...'rely on features unsupported by Rhino, they are skipped when running the tests (because they would always fail).

Hence, when you removed them from the .properties file and ran the testsuite, you didn't see them fail, because they were skipped anyway: the inclusion of those tests in the .properties file and their marking with `{unsupported: ...}' is just for us to see how close we are to passing the entire Test262 test suite. The actual annotation skipping of the tests is not done based on the content of the .properties file, but is based on annotations inside the testsuite itself.

Hope that explains it

@gbrail
Copy link
Collaborator

gbrail commented Nov 29, 2021

This is a great candidate for after we finish the 1.7.14 release process. Thanks!

@gbrail
Copy link
Collaborator

gbrail commented Jan 27, 2022

This is great -- thanks!

@gbrail gbrail merged commit aa57d5a into mozilla:master Jan 27, 2022
@grob
Copy link
Author

grob commented Jan 27, 2022

It was a pleasure :-) Thanks for merging!

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.

Support ES2020 Promise.allSettled
3 participants