Promise.prototype.finally: add tests #1156

ljharb · 2017-07-26T06:46:52Z

(The first two commits can and should be cherry-picked in separately; i'll rebase this PR ASAP if that happens)

Promise.prototype.finally reached stage 3 today at TC39; here's my first crack at the tests.

See tc39/proposal-promise-finally#18, https://tc39.github.io/proposal-promise-finally/, and https://github.com/es-shims/Promise.prototype.finally/blob/master/test/tests.js for more info.

littledan

These tests look good--it's nice to see edge cases tested.

One interaction that I'm wondering about is the interaction with the HostPromiseRejectionTracker hook. Seems like the Promise there will be the result of finally, rather than the Promise used as input to it.

It'd be nice to have a test for this behavior, as I imagine some implementers might intuit to go the other way. That could be through host hooks, or if that's too tough, in a web-platform-test.

benjamingr · 2017-07-26T13:27:19Z

One interaction that I'm wondering about is the interaction with the HostPromiseRejectionTracker hook. Seems like the Promise there will be the result of finally, rather than the Promise used as input to it.

That's a good point, the promise caught should be the result of the finally and not the result of the original promise. More specifically:

let p = Promise.reject(new Error());
let p2 = p.finally(() => {});

The unhandled rejection should be p2 and not p.

littledan · 2017-07-26T14:02:40Z

@benjamingr To be clear, I believe the semantics you're saying are the right ones are what the specification implies. I was just wondering about tests for this property. Thanks for spelling it out better.

ljharb · 2017-07-26T16:10:52Z

If there are existing tests for this hook, then I should be able to utilize them to test it here - if not, I'd need some guidance.

benjamingr · 2017-07-26T17:17:40Z

@ljharb Node tracks them with process.on("unhandledRejection", (e, p) => {}) and the browser with window.addEventListener("unhandledrejection", e => {... }).

If you'd like help I can send you links to the PRs adding the hooks which should shed light on how one might test them.

ljharb · 2017-07-26T17:48:37Z

@benjamingr i'm aware of how a specific implementation tests them; but how would test262 - which is supposed to be host-agnostic - test those? It's run on more engines than browsers and node, and the spec permits unhandled rejection tracking, but does not mandate it.

ljharb · 2017-07-26T19:35:26Z

I don't see any mention of HostPromiseRejectionTracker in the entire codebase; it seems like testing this isn't yet a solved problem.

littledan · 2017-07-26T19:39:36Z

Test262 would have to add a host hook to test this. I don't think it has a hook like this currently. If test262 maintainers don't like the idea of adding such a hook, another method of testing would be to add something to web-platform-tests.

leobalter

I don't have the proper time to do a complete review but I left some comments. Some are about whitespace or conventions.

Something I miss a lot reviewing the files is the info tag with at least some minimal spec part as a reference for the tests.

leobalter · 2017-07-26T22:48:47Z

test/built-ins/Promise/prototype/finally/invokes-then-with-non-function.js

+author: Jordan Harband
+description: Promise.prototype.finally invokes `then` method
+esid: sec-promise.prototype.finally
+features: [Promise, Promise.prototype.finally]


Promise.prototype.finally is sufficient.

Would nobody want to run tests against an engine without Promise?

leobalter · 2017-07-26T22:50:34Z

features.txt

@@ -64,12 +68,19 @@ default-arg
 default-parameters
 destructuring-binding
 Float64Array
+Function.prototype.call


I believe we should not add these features in this PR.

We can't assure this flag is enough here to capture every test using the feature. The same goes for any other new features added that are not Promise.prototype.finally.

leobalter · 2017-07-26T22:53:06Z

test/built-ins/Promise/prototype/finally/length.js

+
+verifyNotEnumerable(Promise.prototype.finally, 'length');
+verifyNotWritable(Promise.prototype.finally, 'length');
+verifyConfigurable(Promise.prototype.finally, 'length');


propertyHelper as a new function that we should use instead:

verifyProperty(Promise.prototype.finally, "length", { value: 1, enumerable: false, configurable: true, writable: false, });

The order of the properties are not relevant in this function.

leobalter · 2017-07-26T22:53:56Z

test/built-ins/Promise/prototype/finally/name.js

+description: Promise.prototype.finally `name` property
+esid: sec-promise.prototype.finally
+info: >
+    ES6 Section 17:


We are over ES6 :)

ES should be fine

leobalter · 2017-07-26T22:55:45Z

features.txt

@@ -7,6 +7,10 @@
 #
 # https://github.com/tc39/process-document

+# Promise.prototype.finally
+# https://github.com/tc39/proposal-promise-finally
+Promise.prototype.finally


Perfect, thanks!

leobalter · 2017-07-26T22:59:54Z

test/built-ins/Promise/prototype/finally/this-value-then-poisoned.js

+author: Jordan Harband
+description: >
+  Promise.prototype.finally called with a `this` value whose `then` property is
+  an accessor property that returns an abrupt completion


thanks for the clarification here.

leobalter · 2017-07-26T23:01:46Z

test/built-ins/Promise/prototype/finally/this-value-then-poisoned.js

+});
+
+assert.throws(Test262Error, function() {
+  Promise.prototype.finally.call(poisonedThen);


this would like calling poisonedThen.finally(), right? We can add a new assert.throws in this file.

leobalter · 2017-07-26T23:01:56Z

test/built-ins/Promise/prototype/finally/this-value-then-throws.js

+
+assert.throws(Test262Error, function() {
+  Promise.prototype.finally.call(thrower);
+});


this would like calling thrower.finally(), right? We can add a new assert.throws in this file.

leobalter · 2017-07-26T23:04:56Z

test/built-ins/Promise/prototype/finally/this-value-then-not-callable.js

+
+p.then = undefined;
+assert.throws(TypeError, function() {
+  Promise.prototype.finally.call(p);


we could poison finally to make sure TypeError happens at the expected time:

Correct me if I'm not using the API properly:

p.finally(function() { throw new Test262Error('this should not run'); });

ah, interesting. sure, i can do that (no need to poison the finally method, but i can certainly pass a poisoned onFinally)

leobalter · 2017-07-26T23:07:17Z

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

+
+assert.throws(TypeError, function() {
+  Promise.prototype.finally.call(undefined);
+}, 'undefined');


here you can poison a promise's then:

const p = new Promise(function() {}) p.then = function() { throw new Test262Error('poison'); }; assert.throws(TypeError, function() { p.finally.call(undefined); }, 'undefined');

I'm not sure what value this adds; it we use .call then the function has no more link to p, so p.then couldn't be reached even if i wanted it to.

leobalter · 2017-07-26T23:11:21Z

Test262 would have to add a host hook to test this. I don't think it has a hook like this currently. If test262 maintainers don't like the idea of adding such a hook, another method of testing would be to add something to web-platform-tests.

I would need more time to review this, maybe @rwaldron is interested.

ljharb · 2017-07-27T04:35:17Z

Thanks, updated! I haven't added "info" since I'm not sure where to link to prior to actually merging with the spec.

gsathya · 2017-07-27T04:35:29Z

Quick question: do these tests pass when running V8 with --harmony-promise-finally flag?

FYI, there's a lot of V8 tests as well if you want to import them: https://cs.chromium.org/chromium/src/v8/test/mjsunit/harmony/promise-prototype-finally.js

ljharb · 2017-07-27T04:38:32Z

@gsathya running test262 tests isn't easy; i can definitely give that a shot :-)

in general tho, wouldn't it make sense to never write v8-only tests, when something could go in test262? :-p

gsathya · 2017-07-27T04:49:00Z

@gsathya running test262 tests isn't easy; i can definitely give that a shot :-)

Sounds good. Thanks! Node 8 should have this feature behind the flag, no need to build d8.

in general tho, wouldn't it make sense to never write v8-only tests, when something could go in test262? :-p

Until quite recently it wasn't possible to write test262 tests as part of a v8 patch -- we had to do out of band test262 code review dance and then roll that change into v8. @littledan has done some great work in making this possible now, so going forward we'll be writing more test262 tests.

ljharb · 2017-07-27T04:49:45Z

Tests pass with both my promise.prototype.finally npm module, but I can't get test262-harness to run with the --harmony-promise-finally flag on node 8 latest (altho the flag works by itself) :-/

ljharb · 2017-07-27T04:54:37Z

However, when I run promise.prototype.finally's tests against node --harmony-promise-finally, there is one of my own module's tests that fails in v8 - specifically around observable then calls with a Promise subclass.

Needless to say we'll have to make sure both implementations as well as the spec and the test262 tests are all in line :-)

rwaldron · 2017-07-27T15:54:24Z

tests pass with both my promise.prototype.finally npm module, but I can't get test262-harness to run with the --harmony-promise-finally flag on node 8 latest (altho the flag works by itself) :-/

I'll dig into this.

rwaldron · 2017-07-27T15:55:32Z

@ljharb the flag is --harmony_promise_finally (according to d8 built from source)

rwaldron · 2017-07-27T17:05:59Z

I wouldn't bother using Node.js to test anything until the Node.js patch to eshost has landed. Also, I am experiencing the same problem with --harmony_promise_finally and it's because the order of arguments is wrong:

(inside ConsoleAgent.js, which is in eshost)

  createChildProcess(args) {
    args = args || [];
    args = args.concat(this.args);
    return cp.spawn(this.hostPath, args);
  }

Which results in the args coming out as (for example):

[ '/var/folders/xz/k2lxgs3s43180_tjwn1v_kwc0000gn/T/117627-60511-189e99u.f98kg.js',
  '--harmony_promise_finally' ]

The flag needs to be before the file:

rwaldron in ~/codez/javascript
$ node -v
v8.2.1
rwaldron in ~/codez/javascript
$ cat promise.prototype.finally.js
console.log(Promise.prototype.finally);
$ node --harmony_promise_finally promise.prototype.finally.js
[Function: finally]
rwaldron in ~/codez/javascript
$ node promise.prototype.finally.js --harmony_promise_finally
undefined

So I will make a PR to eshost that re-arranges the args for NodeAgent.

Moving on to my local run results:

d8

$ test262-harness --hostType=d8 --hostPath=`which d8` --hostArgs='--harmony_promise_finally' test/built-ins/Promise/prototype/finally/**/*.js
Ran 32 tests
32 passed
0 failed

ljharb · 2017-07-27T17:25:09Z

@rwaldron ah! thanks. with node --harmony-promise-finally it works in the repl, perhaps node normalizes dashes and the v8 api doesn't?

rwaldron · 2017-07-27T17:26:31Z

Using my patch, I was able to confirm these tests on Node.js 8.2.1

Nice work @ljharb!

We can do the eshost dependent tests in a follow up PR.

ljharb · 2017-07-27T17:28:06Z

figured it out; it's a bug in eshost via test262-harness. When patched, node --harmony_promise_finally passes this PR's tests!

(edit: lol, github didn't update so i missed you figuring this out half an hour ago)

rwaldron · 2017-07-27T17:32:46Z

perhaps node normalizes dashes and the v8 api doesn't?

Ah, that's probably very likely. There's no reason why NodeAgent couldn't/shouldn't try to do that as well.

rwaldron · 2017-07-27T17:36:58Z

@ljharb

Ah, that's probably very likely.

I was wrong. Either dash or underscore work just fine, the issue was always just the order of the args. Patch to eshost is coming.

gskachkov · 2017-08-01T19:55:38Z

test/built-ins/Promise/prototype/finally/rejected-observable-then-calls.js

+
+no.catch(function (e) {
+  assert.sameValue(e, noReason);
+  throw e;


I would add cases with thow e, within fulfillment and rejection handlers i.e.:

Promise.resolve(1) .then(function () { throw new Error('');}) .finally(/* Check if finally is reached */); Promise.reject(1) .then(function () {}, function () { throw new Error('');}) .finally(/* Check if finally is reached */)

It seems that we need test that we raise TypeError when we use finally with new i.e.

const p = new Promise((resolve)=>{}); assert.throws(TypeError, function() { const f = new p.finally(); }, 'no constructor');

Hmm - I can add that test, but p.finally() returns a Promise, so I have no idea why it'd ever work with new, and .catch() lacks the same test. I'm pretty sure we don't test every returned object value to ensure it doesn't work with new - would checking that typeof wasn't "function" suffice?

Ohh, I see. You're right.
So it will be enough to check if finally returns Promise. Not sure that I can find such test, or I missed something.
If there is no such test we can use Symbol.toStringTag:

const p = Promise.resolve(1); assert.sameValue(p.finally()[Symbol.toStringTag], 'Promise');

I'd think chaining a .then or .catch on the end would be sufficient, and I think some of the tests already do that.

rwaldron · 2017-08-02T19:53:45Z

The patch to eshost is in final stages now, so I don't consider this "blocked" anymore (not that it ever was, I just hoped to get that fixed first... you know how it is).

rwaldron · 2017-08-11T16:24:28Z

eshost and test262-harness are both updated now and I ran these tests locally against d8:

$ test262-harness --hostType d8 --hostPath `which d8` --hostArgs="--harmony_promise_finally" test/built-ins/Promise/prototype/finally/*.js
Ran 32 tests
32 passed
0 failed

Success!

gsathya · 2017-08-11T22:55:54Z

test/built-ins/Promise/prototype/finally/resolution-value-no-override.js

+  assert.sameValue(arguments.length, 0, 'onFinally receives zero args');
+  return {};
+}).then(function (x) {
+  assert.sameValue(x, obj, 'onFinally can not override the resolution value');


If the onFinally and the onFulfilled callbacks are skipped, the tests would still pass right?

(I see other tests also that don't verify if the onFulfilled/onRejected/onFinally has actually run.)

yes, that's true. i can make a followup PR that adds counters to ensure they're called; that's a good idea

gsathya · 2017-08-11T23:01:38Z

We are missing tests that call the SpeciesConstructor. The following test fails in V8:

// Flags: --allow-natives-syntax --harmony-promise-finally

var thenCalled = 0;

class FooPromise extends Promise {
  then(onFullfilled, onRejected) {
    thenCalled++;
    return super.then(onFullfilled, onRejected);
  }
}

var x = new FooPromise(r => r()).finally(() => {});
%RunMicrotasks();

// (1) Promise.prototype.finally
// 1. 7. Return ? Invoke(promise, "then", thenFinally, catchFinally).
// (2) ThenFinally function
// 1.1. 8. Return ? Invoke(promise, "then", valueThunk>
// (3) Promise Resolve Functions
// 25.4.1.3.2. 8. Let then be Get(resolution, "then").

print(thenCalled == 3);

ljharb · 2017-08-11T23:40:23Z

Thanks, I'll write a followup PR soon that strengthens the tests!

leobalter · 2017-08-12T00:12:59Z

+1 for follow up PRs.

…

On Fri, Aug 11, 2017 at 7:40 PM Jordan Harband ***@***.***> wrote: Thanks, I'll write a followup PR this weekend that strengthens the tests! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASYkV6LQzTLgjQccsBNcuGZt1tITMOdks5sXOZngaJpZM4Ojf3A> .

ljharb · 2018-01-31T07:10:10Z

#1398 is merged; but more followup is still ideal.

ljharb added 3 commits July 25, 2017 23:05

gitignore node_modules

a37ce90

Only apps should have lockfiles

4433c42

Promise.prototype.finally: add tests

3528601

ljharb mentioned this pull request Jul 26, 2017

Path to Stage 4! tc39/proposal-promise-finally#18

Closed

25 tasks

littledan reviewed Jul 26, 2017

View reviewed changes

ljharb force-pushed the finally branch from ed366ef to 740b27b Compare July 26, 2017 19:36

ljharb force-pushed the finally branch from 740b27b to b60143a Compare July 26, 2017 19:49

leobalter suggested changes Jul 26, 2017

View reviewed changes

ljharb force-pushed the finally branch from b60143a to 3528601 Compare July 27, 2017 04:34

rwaldron mentioned this pull request Jul 27, 2017

Host-specific hook for HostPromiseRejectionTracker tc39/eshost#29

Open

ljharb mentioned this pull request Jul 27, 2017

ConsoleAgent passes the path first, which doesn't work with node tc39/eshost#31

Closed

gskachkov reviewed Aug 1, 2017

View reviewed changes

leobalter mentioned this pull request Aug 3, 2017

Promise.prototype.finally #866

Closed

1 task

leobalter approved these changes Aug 11, 2017

View reviewed changes

leobalter merged commit e467c83 into tc39:master Aug 11, 2017

ljharb deleted the finally branch August 11, 2017 17:35

gsathya reviewed Aug 11, 2017

View reviewed changes

Promise.prototype.finally: add tests #1156

Promise.prototype.finally: add tests #1156

Conversation

ljharb commented Jul 26, 2017 • edited Loading

littledan left a comment

Choose a reason for hiding this comment

benjamingr commented Jul 26, 2017

littledan commented Jul 26, 2017

ljharb commented Jul 26, 2017

benjamingr commented Jul 26, 2017

ljharb commented Jul 26, 2017

ljharb commented Jul 26, 2017

littledan commented Jul 26, 2017

leobalter 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Jul 26, 2017

ljharb commented Jul 27, 2017

gsathya commented Jul 27, 2017

ljharb commented Jul 27, 2017

gsathya commented Jul 27, 2017

ljharb commented Jul 27, 2017

ljharb commented Jul 27, 2017 • edited Loading

rwaldron commented Jul 27, 2017

rwaldron commented Jul 27, 2017

rwaldron commented Jul 27, 2017

d8

ljharb commented Jul 27, 2017

rwaldron commented Jul 27, 2017

ljharb commented Jul 27, 2017 • edited Loading

rwaldron commented Jul 27, 2017

rwaldron commented Jul 27, 2017

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

Choose a reason for hiding this comment

rwaldron commented Aug 2, 2017

rwaldron commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsathya commented Aug 11, 2017

ljharb commented Aug 11, 2017 • edited Loading

leobalter commented Aug 12, 2017 via email

ljharb commented Jan 31, 2018

ljharb commented Jul 26, 2017 •

edited

Loading

ljharb commented Jul 27, 2017 •

edited

Loading

ljharb commented Jul 27, 2017 •

edited

Loading

ljharb commented Aug 11, 2017 •

edited

Loading