Promise.prototype.finally: add missing tests #1398

ljharb · 2018-01-25T06:22:10Z

Per:

Closes #866.

cc @gsathya, do you think these tests are correct? v8 fails 12 of them.

Per: - tc39#866 (comment) - tc39#866 (comment) Closes tc39#866.

gsathya · 2018-01-25T06:31:33Z

test/built-ins/Promise/prototype/finally/subclass-species-constructor-reject-count.js

+var p = Promise.reject().finally(() => FooPromise.reject());
+
+assert.sameValue(p instanceof Promise, true);
+assert.sameValue(p instanceof FooPromise, true);


This should be false because .then() in Step 7 of Promise.prototype.finally is called on the receiver of finally which is Promise.

gsathya · 2018-01-25T06:35:57Z

test/built-ins/Promise/prototype/finally/subclass-reject-count.js

+}
+
+FooPromise.reject().finally(() => {}).then($DONE).catch(() => {
+  assert.sameValue(6, count);


This should be 7 -- the first 6 promises are created same as the below test and +1 for the extra .catch()

aha, thanks

gsathya · 2018-01-25T06:38:40Z

test/built-ins/Promise/prototype/finally/subclass-species-constructor-resolve-count.js

+var p = Promise.resolve().finally(() => FooPromise.resolve());
+
+assert.sameValue(p instanceof Promise, true);
+assert.sameValue(p instanceof FooPromise, true);


Should be false, same as above test

ljharb · 2018-01-25T06:42:54Z

Thanks! With your feedback, the failures drop to 8 in latest node :-)

gsathya · 2018-01-25T06:52:44Z

test/built-ins/Promise/prototype/finally/this-value-non-promise.js

+var oldThen = Promise.prototype.then;
+Promise.prototype.then = () => { called = true; };
+Promise.prototype.finally.call(p);
+assertTrue(called);


This should be assert.sameValue(called, true);

gsathya · 2018-01-25T06:55:33Z

the failures drop to 8 in latest node :-)

Can you list the failing tests? All the 5 new tests pass for me locally:

➜  v8 (31cde0ee36) ✗ tools/run-tests.py --gn test262/built-ins/Promise/prototype/finally/this-value-non-promise
>>> Latest GN build found: x64.optdebug
Build found: /Users/gunasekaran/code/v8/out.gn/x64.optdebug
>>> Running tests for x64.debug
>>> Running with test processors
>>> Running 1 base tests
[00:00|% 100|+   2|-   0]: Done
>>> 2 tests ran
➜  v8 (31cde0ee36) ✗ tools/run-tests.py --gn test262/built-ins/Promise/prototype/finally/subclass-species-constructor-reject-count
>>> Latest GN build found: x64.optdebug
Build found: /Users/gunasekaran/code/v8/out.gn/x64.optdebug
>>> Running tests for x64.debug
>>> Running with test processors
>>> Running 1 base tests
[00:00|% 100|+   2|-   0]: Done
>>> 2 tests ran
➜  v8 (31cde0ee36) ✗ tools/run-tests.py --gn test262/built-ins/Promise/prototype/finally/subclass-species-constructor-resolve-count
>>> Latest GN build found: x64.optdebug
Build found: /Users/gunasekaran/code/v8/out.gn/x64.optdebug
>>> Running tests for x64.debug
>>> Running with test processors
>>> Running 1 base tests
[00:00|% 100|+   2|-   0]: Done
>>> 2 tests ran
➜  v8 (31cde0ee36) ✗ tools/run-tests.py --gn test262/built-ins/Promise/prototype/finally/subclass-resolve-count
>>> Latest GN build found: x64.optdebug
Build found: /Users/gunasekaran/code/v8/out.gn/x64.optdebug
>>> Running tests for x64.debug
>>> Running with test processors
>>> Running 1 base tests
[00:00|% 100|+   2|-   0]: Done
>>> 2 tests ran
➜  v8 (31cde0ee36) ✗ tools/run-tests.py --gn test262/built-ins/Promise/prototype/finally/subclass-reject-count
>>> Latest GN build found: x64.optdebug
Build found: /Users/gunasekaran/code/v8/out.gn/x64.optdebug
>>> Running tests for x64.debug
>>> Running with test processors
>>> Running 1 base tests
[00:00|% 100|+   2|-   0]: Done
>>> 2 tests ran

ljharb · 2018-01-25T07:17:36Z

Sure, with the latest update:

$ node --version
v9.4.0
$ npx test262-harness --hostType=node --hostPath=`which node` --hostArgs="--harmony-promise-finally" test/built-ins/Promise/prototype/finally/*
FAIL test/built-ins/Promise/prototype/finally/species-constructor.js
  Test did not run to completion

FAIL test/built-ins/Promise/prototype/finally/species-constructor.js
  Test did not run to completion

FAIL test/built-ins/Promise/prototype/finally/subclass-reject-count.js
  Test did not run to completion

FAIL test/built-ins/Promise/prototype/finally/subclass-reject-count.js
  Test did not run to completion

FAIL test/built-ins/Promise/prototype/finally/subclass-resolve-count.js
  Test did not run to completion

FAIL test/built-ins/Promise/prototype/finally/subclass-resolve-count.js
  Test did not run to completion

FAIL test/built-ins/Promise/prototype/finally/this-value-non-promise.js
  Expected no error, got TypeError: Method Promise.prototype.finally called on incompatible receiver [object Object]

FAIL test/built-ins/Promise/prototype/finally/this-value-non-promise.js
  Expected no error, got TypeError: Method Promise.prototype.finally called on incompatible receiver [object Object]

Ran 46 tests
38 passed
8 failed

gsathya · 2018-01-25T07:27:26Z

FAIL test/built-ins/Promise/prototype/finally/this-value-non-promise.js
Expected no error, got TypeError: Method Promise.prototype.finally called on incompatible receiver [object Object]

FAIL test/built-ins/Promise/prototype/finally/this-value-non-promise.js
Expected no error, got TypeError: Method Promise.prototype.finally called on incompatible receiver [object Object]

I fixed this recently in V8 (~month ago) so may be node v9 doesn't have the fix.

I'm not sure about the rest. They all run to completion (and pass the test) in V8 for me locally. In fact, I have these tests checked in to V8 already here:
https://cs.chromium.org/chromium/src/v8/test/mjsunit/harmony/promise-prototype-finally.js?l=545

I wonder if it's something to do with the harness.

gsathya

Sorry I missed this in the earlier review

gsathya · 2018-01-25T07:35:49Z

test/built-ins/Promise/prototype/finally/species-constructor.js

@@ -22,4 +22,4 @@ new FooPromise(r => r())
  .then(() => {
    assert.sameValue(count, 6, "6 new promises were created");
    $DONE();
-});
+  }, $DONE);


This should be $ERROR because we shouldn't be catching anything

gsathya · 2018-01-25T07:41:24Z

test/built-ins/Promise/prototype/finally/subclass-reject-count.js

+  }
+}
+
+FooPromise.reject().finally(() => {}).then($DONE).catch(() => {


The then callback should take $ERROR as resolving would pass the test silently

gsathya · 2018-01-25T07:41:49Z

test/built-ins/Promise/prototype/finally/subclass-resolve-count.js

+FooPromise.resolve().finally(() => {}).then(() => {
+  assert.sameValue(6, count);
+  $DONE();
+}, $DONE);


Same as above, the catch callback should be $ERROR

gsathya · 2018-01-25T18:38:44Z

Thanks @ljharb 💯

rwaldron · 2018-01-25T19:03:25Z

@ljharb these are awesome. Thanks @gsathya for working through the review!

Promise.prototype.finally: add missing tests

1547494

Per: - tc39#866 (comment) - tc39#866 (comment) Closes tc39#866.

gsathya reviewed Jan 25, 2018

View reviewed changes

ljharb force-pushed the finally_redux branch from 841506c to 0c1109e Compare January 25, 2018 06:42

gsathya reviewed Jan 25, 2018

View reviewed changes

ljharb force-pushed the finally_redux branch from 0c1109e to 230475b Compare January 25, 2018 07:16

ljharb force-pushed the finally_redux branch from 230475b to 122b0d8 Compare January 25, 2018 07:27

gsathya reviewed Jan 25, 2018

View reviewed changes

ljharb force-pushed the finally_redux branch from 122b0d8 to 1547494 Compare January 25, 2018 07:51

gsathya approved these changes Jan 25, 2018

View reviewed changes

rwaldron merged commit 7c5b5bf into tc39:master Jan 25, 2018

ljharb deleted the finally_redux branch January 25, 2018 19:17

ljharb mentioned this pull request Jan 31, 2018

Promise.prototype.finally: add tests #1156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promise.prototype.finally: add missing tests #1398

Promise.prototype.finally: add missing tests #1398

ljharb commented Jan 25, 2018

gsathya Jan 25, 2018

gsathya Jan 25, 2018

ljharb Jan 25, 2018

gsathya Jan 25, 2018

ljharb commented Jan 25, 2018

gsathya Jan 25, 2018

gsathya commented Jan 25, 2018

ljharb commented Jan 25, 2018

gsathya commented Jan 25, 2018

gsathya left a comment

gsathya Jan 25, 2018

gsathya Jan 25, 2018

gsathya Jan 25, 2018

gsathya commented Jan 25, 2018

rwaldron commented Jan 25, 2018

Promise.prototype.finally: add missing tests #1398

Promise.prototype.finally: add missing tests #1398

Conversation

ljharb commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jan 25, 2018

Choose a reason for hiding this comment

gsathya commented Jan 25, 2018

ljharb commented Jan 25, 2018

gsathya commented Jan 25, 2018

gsathya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsathya commented Jan 25, 2018

rwaldron commented Jan 25, 2018