Add tests for Promise.all|race resolve lookup #2131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks, I ran these against V8's implementation and they all pass! But, the following test262 tests need updating as well: |
|
Thanks! I'll take a look at these and any potentially matching for allSettled |
These are the failing allSettled ones -- |
leobalter
force-pushed
the
resolve-lookup
branch
2 times, most recently
from
2a0d69f to
2076005
Compare
|
tests are up, thanks for reporting the errors! I also caught some new false positives that are not prevented in the modified tests. |
|
CircleCI is giving a false positive for the tests execution due to an upgrade on Test262's version that requires another upgrade on Test262-harness that is planned to be updated soon. |
|
@gsathya would you take a look, please? I can tell from local runs the only tests I'm getting errors are the ones related to the function name. |
leobalter
force-pushed
the
resolve-lookup
branch
from
6765bc8 to
3b2ad0b
Compare
|
All tests pass on ToT v8 🔥 |
oleavr
pushed a commit
to frida/v8
that referenced
this pull request
Apr 26, 2019
In the PerformPromise{All, Race, AllSettled} operations, the resolve
property of the constructor is looked up only once.
In the implementation, for the fast path, where the constructor's
resolve property is untainted, the resolve function is set to undefined.
Since undefined can't be a valid value for the resolve function,
we can switch on it (in CallResolve) to directly call the PromiseResolve
builtin. If the resolve property is tainted, we do an observable property
lookup, save this value, and call this property later (in CallResolve).
I ran this CL against the test262 tests locally and they all pass:
tc39/test262#2131
Spec:
- tc39/ecma262#1506
- tc39/proposal-promise-allSettled#40
Bug: v8:9152
Change-Id: Icb36a90b5a244a67a729611c7b3315d2c29de6e9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1574705
Commit-Queue: Sathya Gunasekaran <[email protected]>
Reviewed-by: Jakob Gruber <[email protected]>
Cr-Commit-Position: refs/heads/master@{#60957}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref tc39/ecma262#1506
The proposed spec needs consensus, please don't merge this yet.