Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1536013002:
[promise] make Promise.resolve match spec (Closed)
|
Created:
5 years ago by caitp (gmail) Modified:
4 years, 11 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[promise] make Promise.resolve match spec
Fixes a number of test262 tests, including
- built-ins/Promise/resolve/resolve-from-promise-capability.js
- built-ins/Promise/resolve/context-non-object-with-promise.js
- built-ins/Promise/executor-function-length.js
BUG=v8:4633
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
Committed: https://crrev.com/4f9471152ce6c83e8c669946c670f3b612838acf
Cr-Commit-Position: refs/heads/master@{#33094}
Patch Set 1 : #Patch Set 2 : Update test expectations #
Total comments: 3
Patch Set 3 : Fixups #Patch Set 4 : Fixups #Patch Set 5 : Re-add mjsunit/es6/promises.js fixups #
Messages
Total messages: 31 (13 generated)
Description was changed from ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js BUG=v8:4633 LOG=N R=littledan@chromium.org ========== to ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + cbruni@chromium.org
some other Promise fixups
Patchset #1 (id:1) has been deleted
LGTM. I presume you have to update the tests?
On 2015/12/22 15:18:23, cbruni wrote: > LGTM. I presume you have to update the tests? Need to try this again now that the test262 roll landed
On 2015/12/22 15:34:16, caitp wrote: > On 2015/12/22 15:18:23, cbruni wrote: > > LGTM. I presume you have to update the tests? > > Need to try this again now that the test262 roll landed built-ins/Promise/executor-function-length seems to pass in this CL, but is marked to fail in test262.status --- however this change shouldn't change its pass/fail status. So, did someone forget to update test262.status previously? Should I make that one change here, or just leave it? The mjsunit failures are probably invalid tests to be removed, which is scary and probably means not landing this just yet, as the change could possibly break embedder code
Description was changed from ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org ========== to ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js - built-ins/Promise/executor-function-length.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org ==========
On 2015/12/22 15:57:43, caitp wrote: > On 2015/12/22 15:34:16, caitp wrote: > > On 2015/12/22 15:18:23, cbruni wrote: > > > LGTM. I presume you have to update the tests? > > > > Need to try this again now that the test262 roll landed > > built-ins/Promise/executor-function-length seems to pass in this CL, but is > marked to fail in test262.status --- however this change shouldn't change its > pass/fail status. > Nevermind, I see why this change fixed that test. > So, did someone forget to update test262.status previously? Should I make that > one change here, or just leave it? > > The mjsunit failures are probably invalid tests to be removed, which is scary > and probably means not landing this just yet, as the change could possibly break > embedder code
Test expectations are updated https://codereview.chromium.org/1536013002/diff/40001/test/mjsunit/es6/promis... File test/mjsunit/es6/promises.js (right): https://codereview.chromium.org/1536013002/diff/40001/test/mjsunit/es6/promis... test/mjsunit/es6/promises.js:1060: assertTrue(log === "nx24nnx21nnx[object Promise]nnx23nnnx25nnx26n", This result matches the current behaviour of JavaScriptCore, and passes related Test262 bits, so I assume this is correct.
lgtm
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1536013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1536013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_linux_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/12555)
https://codereview.chromium.org/1536013002/diff/40001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1536013002/diff/40001/src/js/promise.js#newco... src/js/promise.js:363: var promiseCapability = NewPromiseCapability(this); Technically, the assertions added in NewPromiseCapability in a separate CL are important here, and observable in test262 tests fixed in that other CL --- so I don't think we can take the shortcut as done in the deleted code :(
lgtm https://codereview.chromium.org/1536013002/diff/40001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1536013002/diff/40001/src/js/promise.js#newco... src/js/promise.js:363: var promiseCapability = NewPromiseCapability(this); On 2016/01/04 at 14:52:11, caitp wrote: > Technically, the assertions added in NewPromiseCapability in a separate CL are important here, and observable in test262 tests fixed in that other CL --- so I don't think we can take the shortcut as done in the deleted code :( Yep, I agree. Still looks good to me. The trybot just failed because it needs a rebase.
On 2016/01/04 18:04:29, Dan Ehrenberg wrote: > lgtm > > https://codereview.chromium.org/1536013002/diff/40001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/1536013002/diff/40001/src/js/promise.js#newco... > src/js/promise.js:363: var promiseCapability = NewPromiseCapability(this); > On 2016/01/04 at 14:52:11, caitp wrote: > > Technically, the assertions added in NewPromiseCapability in a separate CL are > important here, and observable in test262 tests fixed in that other CL --- so I > don't think we can take the shortcut as done in the deleted code :( > > Yep, I agree. Still looks good to me. The trybot just failed because it needs a > rebase. Thanks, here goes
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/1536013002/#ps80001 (title: "Fixups")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1536013002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1536013002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
On 2016/01/04 18:20:03, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) Test fixups added from a different machine slipped out of the patch --- fixed
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/1536013002/#ps100001 (title: "Re-add mjsunit/es6/promises.js fixups")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1536013002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1536013002/100001
Message was sent while issue was closed.
Description was changed from ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js - built-ins/Promise/executor-function-length.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org ========== to ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js - built-ins/Promise/executor-function-length.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js - built-ins/Promise/executor-function-length.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org ========== to ========== [promise] make Promise.resolve match spec Fixes a number of test262 tests, including - built-ins/Promise/resolve/resolve-from-promise-capability.js - built-ins/Promise/resolve/context-non-object-with-promise.js - built-ins/Promise/executor-function-length.js BUG=v8:4633 LOG=N R=littledan@chromium.org, cbruni@chromium.org Committed: https://crrev.com/4f9471152ce6c83e8c669946c670f3b612838acf Cr-Commit-Position: refs/heads/master@{#33094} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4f9471152ce6c83e8c669946c670f3b612838acf Cr-Commit-Position: refs/heads/master@{#33094} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
