add initial set of tests for Promise #100

smikes · 2014-10-23T22:30:44Z

For review, here are the first few tests for Promise

These tests are based on the promises-es6 project, but have been converted from node/mocha to Test262 conventions.

I would be grateful for review and comments by the usual suspects (@anba, @bterlson, @getify, @domenic)

These tests mostly pass on current node v0.11.14. "Mostly" because:

THIS WAS A MISTAKE ON MY PART, FIXED BELOW

v8 does not pass undefined as 'this' even though the spec requires this (regardless of strict/sloppy mode)

FAIL test/suite/es6/ch25/25.4/25.4.2/25.4.2.1/S25.4.2.1_A3.1_T1.js
     Promise.onFulfilled gets undefined as 'this'
     Exp: no error
     Got: 'this' must be undefined in onResolved (regardles of strict status), got [object global]

FAIL test/suite/es6/ch25/25.4/25.4.2/25.4.2.1/S25.4.2.1_A3.2_T1.js
     Promise.onRejected gets undefined as 'this'
     Exp: no error
     Got: 'this' must be undefined in onRejected (regardles of strict status), got [object global]

FAIL test/suite/es6/ch25/25.4/25.4.3/25.4.3.1/S25.4.3.1_A5.1_T1.js
     Promise executor gets undefined as 'this'
     Exp: no error
     Got: 'this' must be undefined in executor (regardless of strict status), got [object global]

v8 does not seem to handle foreign thenables correctly, or else I wrote the test wrong

FAIL test/suite/es6/ch25/25.4/25.4.4/25.4.4.5/S25.4.4.5_A3.1_T1.js
     Promise.resolve passes through an unsettled promise w/ same Constructor
     Exp: no error
     Got: Test262 Error: Test did not run to completion ($DONE not called)

FAIL test/suite/es6/ch25/25.4/25.4.4/25.4.4.5/S25.4.4.5_A3.1_T1.js
     Promise.resolve passes through an unsettled promise w/ same Constructor (Strict Mode)
     Exp: no error
     Got: Test262 Error: Test did not run to completion ($DONE not called)

domenic · 2014-10-23T22:35:23Z

v8 does not pass undefined as 'this' even though the spec requires this (regardless of strict/sloppy mode)

This was not the intent of the spec... I might have written it wrong though.

v8 does not seem to handle foreign thenables correctly, or else I wrote the test wrong

See https://code.google.com/p/v8/issues/detail?id=3641 and related links ... there's something very weird going on in V8 with regard to foreign thenables.

smikes · 2014-10-23T22:46:44Z

not the intent of the spec..

Well, it looked very clear to me, but I certainly may have read it wrong! Here's the relevant section: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-initializepromise

25.4.3.1.1 InitializePromise ( promise, executor )

Let completion be the result of calling the [[Call]] internal method of executor with undefined as thisArgument and (resolvingFunctions.[[Resolve]], resolvingFunctions.[[Reject]]) as argumentsList.

smikes · 2014-10-23T22:48:59Z

And for the ReactionJobs: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promisereactionjob

25.4.2.1 PromiseReactionJob ( reaction, argument )

Else, let handlerResult be the result of calling the [[Call]] internal method of handler passing undefined as thisArgument and (argument) as argumentsList.

domenic · 2014-10-23T22:51:14Z

I was under the impression that calling [[Call]] with undefined as thisArgument got translated into the global in sloppy mode. Digging for spec text now...

smikes · 2014-10-23T22:54:56Z

You're right ; 9.2.2 ; 12 b i 1

Else,
If thisMode is strict, then let thisValue be thisArgument.
Else
if thisArgument is null or undefined, then
Let thisValue be calleeRealm.[[globalThis]].

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ecmascript-function-objects-call-thisargument-argumentslist

smikes · 2014-10-23T23:02:22Z

Squashed that fix in, plus some minor other fixes. Only the thenables test fails now (node 0.11.14, both strict and nonstrict modes)

smikes · 2014-10-24T14:06:59Z

I have made a google doc spreadsheet listing testable assertions that I read into the Promise spec. (The link is here : https://docs.google.com/spreadsheets/d/1rdXv8ggpUmFkZpCrrlasukw0tKCymmuBg8Ar0yWzGK0/edit?usp=sharing )

So far I have covered about half of the things I can identify. (130 lines in assertions sheet, 78 tests so far, but some are duplicates)

domenic · 2014-10-24T14:38:29Z

test/suite/es6/ch25/25.4/25.4.2/25.4.2.1/S25.4.2.1_A1.1_T1.js

+var p = Promise.resolve(obj).then(/*Identity, Thrower*/)
+    .then(function (arg) {
+	if (arg !== obj) {
+	    $ERROR("Expected resolved promise to be obj, actually " + arg);


Expected promise fulfillment value to be obj

(These changes need to happen throughout the tests)

domenic · 2014-10-24T14:53:22Z

Ran out of steam halfway through, but hope that helps as an initial set of review...

smikes · 2014-10-24T14:56:21Z

Thanks. I will spend some time on those changes later today.

On Fri, Oct 24, 2014 at 8:53 AM, Domenic Denicola
notifications@github.com wrote:

Ran out of steam halfway through, but hope that helps as an initial set of review...

Reply to this email directly or view it on GitHub:
#100 (comment)

smikes · 2014-10-24T23:40:36Z

Incorporated numerous changes, many fixing sloppy language use (resolved, fulfilled, rejected, oh my!).

Also a number of changes to code. Using Symbol.iterator to define iterators; removed redundant tests (where new Error() was used); add test of throw from IteratorValue(), etc.

Overall it should be less unpleasant to review these changes. Will review again later (tomorrow?) when I am fresh to look for incoherency/word salad in info/description fields.

For reasons I am too lazy to troubleshoot now, nvmw will not let me install node 0.11.14 on Windows, so I am testing with 0.11.13. Slightly different set of failures:

Z:\Code\github\test262>node --harmony ..\test262-harness\bin\run.js test/suite/es6/ch25/**/*.js

FAIL test/suite/es6/ch25/25.4/25.4.4/25.4.4.4/S25.4.4.4_A3.1_T1.js
     Promise.reject throws TypeError for bad 'this'
     Exp: error matching /TypeError/
     Got: no error

FAIL test/suite/es6/ch25/25.4/25.4.4/25.4.4.5/S25.4.4.5_A3.1_T1.js
     Promise.resolve delegates to foreign thenable
     Exp: no error
     Got: Test262 Error: Test did not run to completion ($DONE not called)

FAIL test/suite/es6/ch25/25.4/25.4.5/25.4.5.3/S25.4.5.3_A4.2_T1.js
     Promise.prototype.then treats non-callable arg1, arg2 as undefined
     Exp: no error
     Got: TypeError: number is not a function

FAIL test/suite/es6/ch25/25.4/25.4.5/25.4.5.3/S25.4.5.3_A4.2_T2.js
     Promise.prototype.then treats non-callable arg1, arg2 as undefined
     Exp: no error
     Got: Expected resolution object to be passed through, got TypeError: number
 is not a function

Ran 80 tests
76 passed
4 failed

Added a test of Promise.reject.call(ZeroArgumentConstructor, undefined). My understanding is that this should throw, but it doesn't.
Foreign thenable again.
It looks like node 0.11.13 has a version of v8 that does not implement the correct PerformPromiseThen for non-callable onFulfilled and onRejected. I will check again, later, with the Mac.

anba · 2014-10-25T14:31:11Z

test/suite/es6/ch25/25.4/25.4.4/25.4.4.1/S25.4.4.1_A3.1_T3.js

+Promise.all(iterThrows).then(function () {
+    $ERROR('Promise unexpectedly fulfilled: Promise.all(iterThrows) should throw TypeError');
+},function (err) {
+    if (!(err instanceof TypeError)) {


The abrupt completion returned in 25.4.4.1 Promise.all, step 5 is not changed to a TypeError, so the expected result is the Error object thrown in the getter function above.

Yes. I will dumb down the test a bit. I was trying for too much, trying to test the inner logic of GetIterator. It's enough to check that if GetIterator gets an abrupt completion, the Promise is rejected

bterlson · 2014-10-25T19:14:42Z

I really like the spreadsheet of coverage.

Possible to add es6id attribute to the tests? Adding this would allow us to generate the table later,

bterlson · 2014-10-25T19:17:28Z

test/suite/es6/ch25/25.4/25.4.2/25.4.2.1/S25.4.2.1_A3.2_T1.js

+var expectedThis,
+    obj = {};
+
+(function () { expectedThis = this; }());


Could prob use fnGlobalObject here too?

bterlson · 2014-10-25T19:36:05Z

Sequence verification should ensure that the callbacks are called on subsequent turns (I think you can just add a push at the very end of the test and ensure that comes before any of the handlers). Good case especially for zero-length argument to Promise.all..

bterlson · 2014-10-25T19:39:24Z

test/suite/es6/ch25/25.4/25.4.4/25.4.4.3/S25.4.4.3_A7.3_T1.js

+description: Promise.race([p1, p2]) settles when first settles
+---*/
+
+var resolveP1, rejectP2,


Would it make sense to add a "deferred" promise helper that constructs a promise and returns an object with promise, resolve, and reject? Might simplify some of the test setup.

If I was going to write 100 more tests like the above, then probably yes. At this point though, I don't think it would add a lot to the tests that are there, and I know when I use a helper like that (as I messed around with a bit in #56) I wind up doing slopping thinking / sloppy naming of objects.

smikes · 2014-10-25T19:43:33Z

Sequence verification should ensure that the callbacks are called on subsequent turns (I think you can just add a push at the very end of the test and ensure that comes before any of the handlers).

That's a good idea. I will revisit the sequence tests and add that.

We still can't guarantee that they're actually on different main event loop turns, from my reading of the various notes linked previously (here or in #56), we can still observe that they happen after straight-through execution of the main test body.

Re: es6id -- this would be in the context of the new naming scheme -- Promise.all.returns_a_promise.js etc? I will think about it. It's hard because each Promise test covers a substantial part of the Promise spec -- pretty much every test exercises NewPromiseCapability for example.

In any case, the tests currently are mostly named/put into directories by the part of the spec that inspired the test, not necessarily by the part of the spec that each test is meant to cover.

bterlson · 2014-10-25T19:47:52Z

Re: es6id -- this would be in the context of the new naming scheme -- Promise.all.returns_a_promise.js etc? I will think about it. It's hard because each Promise test covers a substantial part of the Promise spec -- pretty much every test exercises NewPromiseCapability for example.

No, I just mean add es6id: to the frontmatter. It doesn't have to be exhaustive, but it will help in the future when we want to automatically produce an awesome spreadsheet of coverage like you created.

smikes · 2014-10-25T19:52:04Z

Another thing -- on the topic of coverage -- I ran the promises-unwrapping test suite under Istanbul and found the following stats:

File	% Stmts	% Branches	% Funcs	% Lines
testable-implementation.js	93.07	80.56	100	93.07

So now I am all fired up to figure out how to write an adapter that allows me to run test262 tests against the istanbul-instrumented version of testable-implementation.js so I can see what parts of the reference implementation I have covered.

I have a mocha-test262-adapter that works (for test262 files only; you can't yet mix mocha tests with test262 tests), but I haven't checked whether it can run under istanbul. If that works then the only ("only" - heh) remaining thing is to write a pure-JS wrapper of Promise that delegates to the testable-implementation and calls all ObjectConstruct and ArrayToIterable etc.

smikes · 2014-10-25T20:00:10Z

Apparently I also have:

o tabs/spaces problems
o a fair amount of copy-pasted text

Working on those too.

@anba

add more tests of Promise.all additional Promise test add tests of Promise.prototype add some tests for Promise.race additional Promise tests add Promise.reject and Promise.resolve tests correct test description rename badly-named files use current license and minor style cleanup correct understanding of undefined as thisArgument incorporate line notes Is this enough to make a constructor in ES6? more accurate use of resolved,fulfilled etc. remove some redundant tests add new tests remove "constant array" unclear language better description Update S25.4.2.1_A3.2_T1.js address dangling () changes per @anba line notes - if GetIterator is abrupt, the Promise.race / Promise.all should reject - if Promise.race is called with nonconforming constructor as 'this', TypeError should be thrown (cannot reject if exeption is thrown from NewPromiseCapability; no promise exists yet...) correct description of "this" testing in callbacks fix whitespace, formatting remove tab add "next-turn" checking to sequencers

smikes · 2014-11-10T23:41:07Z

Fixed tab/space issues. Added sequence point at end of test body to verify promise functions are called after test body completes.

More review welcome.

bterlson · 2014-11-12T02:15:40Z

Do you explicitly want more review or can I pull these in? LGTM 👍

smikes · 2014-11-12T02:57:14Z

I'm cool with these going in.

On Tue, Nov 11, 2014 at 7:15 PM, Brian Terlson notifications@github.com
wrote:

Do you explicitly want more review or can I pull these in? LGTM [image:
👍]

—
Reply to this email directly or view it on GitHub
#100 (comment).

add initial set of tests for Promise

bterlson · 2014-11-12T19:29:16Z

Alright, I'm pulling them in then! Thanks!

smikes force-pushed the promise-tests branch from 4655a17 to 488e8a5 Compare October 23, 2014 23:01

domenic reviewed Oct 24, 2014
View reviewed changes

bterlson added the coverage label Oct 24, 2014

smikes force-pushed the promise-tests branch 2 times, most recently from eb49178 to 7b6fd5c Compare October 24, 2014 23:31

smikes force-pushed the promise-tests branch from ace387b to e6c8bd1 Compare October 24, 2014 23:35

smikes force-pushed the promise-tests branch from 0887929 to 6d2c533 Compare October 25, 2014 01:49

anba reviewed Oct 25, 2014
View reviewed changes

smikes force-pushed the promise-tests branch from 6d2c533 to a73c2f0 Compare October 25, 2014 18:11

bterlson reviewed Oct 25, 2014
View reviewed changes

smikes force-pushed the promise-tests branch from a73c2f0 to 0267e43 Compare October 25, 2014 19:46

smikes force-pushed the promise-tests branch from 954601a to 8bf0045 Compare November 10, 2014 23:23

smikes force-pushed the promise-tests branch from 8bf0045 to 26787ab Compare November 10, 2014 23:38

split "expectedThis" cases into strict/nonstrict branches

f544773

bterlson added a commit that referenced this pull request Nov 12, 2014

Merge pull request #100 from smikes/promise-tests

0caf4ee

add initial set of tests for Promise

bterlson merged commit 0caf4ee into tc39:master Nov 12, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add initial set of tests for Promise #100

add initial set of tests for Promise #100

smikes commented Oct 23, 2014

domenic commented Oct 23, 2014

smikes commented Oct 23, 2014

smikes commented Oct 23, 2014

domenic commented Oct 23, 2014

smikes commented Oct 23, 2014

smikes commented Oct 23, 2014

smikes commented Oct 24, 2014

domenic Oct 24, 2014

domenic Oct 24, 2014

domenic commented Oct 24, 2014

smikes commented Oct 24, 2014

Ran out of steam halfway through, but hope that helps as an initial set of review...

smikes commented Oct 24, 2014

anba Oct 25, 2014

smikes Oct 25, 2014

bterlson commented Oct 25, 2014

bterlson Oct 25, 2014

bterlson commented Oct 25, 2014

bterlson Oct 25, 2014

smikes Oct 25, 2014

smikes commented Oct 25, 2014

bterlson commented Oct 25, 2014

smikes commented Oct 25, 2014

smikes commented Oct 25, 2014

smikes commented Nov 10, 2014

bterlson commented Nov 12, 2014

smikes commented Nov 12, 2014

bterlson commented Nov 12, 2014

add initial set of tests for Promise #100

add initial set of tests for Promise #100

Conversation

smikes commented Oct 23, 2014

domenic commented Oct 23, 2014

smikes commented Oct 23, 2014

smikes commented Oct 23, 2014

domenic commented Oct 23, 2014

smikes commented Oct 23, 2014

smikes commented Oct 23, 2014

smikes commented Oct 24, 2014

domenic Oct 24, 2014

Choose a reason for hiding this comment

domenic Oct 24, 2014

Choose a reason for hiding this comment

domenic commented Oct 24, 2014

smikes commented Oct 24, 2014

Ran out of steam halfway through, but hope that helps as an initial set of review...

smikes commented Oct 24, 2014

anba Oct 25, 2014

Choose a reason for hiding this comment

smikes Oct 25, 2014

Choose a reason for hiding this comment

bterlson commented Oct 25, 2014

bterlson Oct 25, 2014

Choose a reason for hiding this comment

bterlson commented Oct 25, 2014

bterlson Oct 25, 2014

Choose a reason for hiding this comment

smikes Oct 25, 2014

Choose a reason for hiding this comment

smikes commented Oct 25, 2014

bterlson commented Oct 25, 2014

smikes commented Oct 25, 2014

smikes commented Oct 25, 2014

smikes commented Nov 10, 2014

bterlson commented Nov 12, 2014

smikes commented Nov 12, 2014

bterlson commented Nov 12, 2014