Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for Comments on Promises/A+ test translation #56

Closed
smikes opened this issue Jul 24, 2014 · 46 comments
Closed

Request for Comments on Promises/A+ test translation #56

smikes opened this issue Jul 24, 2014 · 46 comments

Comments

@smikes
Copy link
Contributor

smikes commented Jul 24, 2014

I have started work on translating the Promises/A+ tests into a format that works with Test262.

I have a branch for this work -- https://github.com/smikes/test262/tree/promises-aplus-tests-1/test/suite/promises-aplus -- where I created a harness file, a subdirectory test/suite/promises-aplus for the tests, and a README file documenting the approach that I'm taking.

The first thing I am working out is how to put operations into a particular order without using setTimeout; the Promises/A+ tests use this at times, e.g., to guarantee that a particular call to resolve or reject will happen after all currently queued Promise jobs.

I would be very grateful for feedback about my approach.

/cc @domenic @getify @anba and @niksurya

@domenic
Copy link
Member

domenic commented Jul 28, 2014

The first thing I am working out is how to put operations into a particular order without using setTimeout; the Promises/A+ tests use this at times, e.g., to guarantee that a particular call to resolve or reject will happen after all currently queued Promise jobs.

This is very interesting. Any setTimeouts you can remove from the Promises/A+ tests will definitely be accepted back into the original tests themselves.

However upon reading the readme it seems you are doing this by assuming the promise implementation works, which is bad.

Commenting on the readme:

The Promises/A+ test suite uses the setTimeout function in two ways: sometimes to delay an operation, and sometimes as a timeout. In Test262, setTimeout is not available. Delays must be established using Promises, and timeouts are handled by the test runner.

The test framework of the Promises/A+ tests has a timeout as well, so we could remove any uses of it as a timeout from the source tests.

As for the delay using Promises ... assuming a non-broken promise implementation in order to test promises seems quite unwise :(.

Thus in the Test262 harness, the return value of deferred() is also a thenable.

I don't see a reason for doing this. It hurts readability and is extremely bad practice in the real world; testing is different, certainly, but I don't think it's worth the gain.

You can even see the confusion in the subsequent section, where you refer to "sequence point promises" when you really mean "sequence point deferreds." In fact the confusion is present throughout the readme and converted tests.

function shouldNotReject(arg) {
       $ERROR("Unexpected: promise should not reject " + arg);
   }

This is probably worth a helper

Table of Tests By Section

Wow, this is an awesome breakdown :)


Overall I am a bit queasy about the fact that the tests assume a working promise implementation in order to test the promise implementation. I am not sure how to avoid this, but this is such a big flaw that to me it's unclear whether it's worth proceeding...

@smikes
Copy link
Contributor Author

smikes commented Jul 28, 2014

Thus in the Test262 harness, the return value of deferred() is also a thenable.

I don't see a reason for doing this. It hurts readability and is extremely bad practice in the real world; testing is different, certainly, but I don't think it's worth the gain.

You can even see the confusion in the subsequent section, where you refer to "sequence point promises" when you really mean "sequence point deferreds." In fact the confusion is present throughout the readme and converted tests.

If I recall correctly, the main reason I did that was so I could call Promise.all on an array of deferreds without having to map( d => d.promise ).

After that, though, I found it convenient to be able to write both a[0].then() and a[0].resolve(). The sequence points can be both thenables and deferreds, since the APIs do not conflict.

In any case, I will go over the README again and try to clarify the naming of the sequence points.

Overall I am a bit queasy about the fact that the tests assume a working promise implementation in order to test the promise implementation. I am not sure how to avoid this, but this is such a big flaw that to me it's unclear whether it's worth proceeding...

Yes, this is probably my primary concern, although I was not able to state it so succinctly.

I would note also that the async test runner, as written, tries to use setTimeout; but if setTimeout is not available it falls back to an async test based only on native Promise. That affects testing of v8/shell, which has Promise but not setTimeout. At present, browserless SpiderMonkey and JavaScriptCore have neither, so there is no async test support for them.

However, I think we have to go on, for a number of reasons.

A fundamental problem is that JavaScript has always had async features which were provided by the environment. Now ES6 introduces Promise and there is an asynchronous mechanism built into the language. But since Promise is the only way to express asynchrony in pure ES6, there is no way to test Promises except in terms of themselves.

This looks circular, so it's tempting to give up.

Luckily, we have a long tradition of solving circular problems by bootstrapping. Or alternatively, as in mathematical induction, we can define a base step and proceed from there.

For example, a minimally functional Promise implementation must pass this test, while a non-functional Promise will either throw a SyntaxError or will time out:

// function passed to "then" is called for a resolved promise
Promise.resolve().then(function () {
    $DONE();
})

In the console runner, test timeouts are currently handled in JavaScript, but they could be moved up into the python code. In browser runners, setTimeout is presumed to be available. In this way, we can verify basic Promise functionality in terms of an external async implementation.

Then we can test that sequencing works as expected:

// function passed to "then" is called in a later turn
var result = [];

function check() {
   assert.deepEqual([1,2,3], result);
}
var a = makeSequenceArray(2, $DONE, check);

a[0].promise.then(function () {
   a[1].resolve();
   result.push(2);
});

a[1].promise.then(function () {
   result.push(3);
});

a[0].resolve();
result.push(1);

Going on in this way, we can build up a test suite over the whole Promise implementation. This is my intention for the future of the promises-es6 tests (that's the separate test suite that started out as a test suite for getify/native-promise-only )

When that bootstrapped test suite is in place, the Promises/A+ tests are not just floating in air, relying on a valid Promise implementation to test Promises. Rather, they demonstrate that the ES6 Promise implementation also conforms to the earlier Promises/A+ standard and also ensure that a number of corner cases and hostile-implementation cases are addressed.

smikes added a commit to smikes/test262 that referenced this issue Jul 28, 2014
Incorporate feedback from tc39#56 (comment)

1. drop documentation of `thenable` status of deferreds
2. use 'sequence point' throughout instead of 'sequence-point promise' 
3. always use d.promise.then for deferreds

bump version number and date
@bterlson
Copy link
Member

Testing promises has made me believe we need a lower-level primitive to enqueue a job (either in a separate queue for such jobs or even any existing queue used in the spec). It's a real pain to test promises in hosts without setTimeout (v8shell isn't the only one either, there are numerous). I agree that we are left with no other option than to test promises with promises, but the good part is that the timer in the harness doesn't require super in-depth promise conformance before it starts doing its job. Really the only thing it has to do is queue the then job at some later point in time.

I'll look at the rest of this shortly.

@smikes
Copy link
Contributor Author

smikes commented Jul 28, 2014

Testing promises has made me believe we need a lower-level primitive to enqueue a job

But then how will we test that? /joke

If this is going to come up at the TC39 meeting (tomorrow?) then the question becomes, should ES6 adopt an existing interface (e.g., HTML5 timers, which specifies setTimeout, clearTimeout, setInterval and clearInterval); or are you going to propose an all-new interface for scheduling jobs?

I believe the Mozilla team is blocking on Promises in part because they have not decided how to write an event loop into the js standalone (https://bugzilla.mozilla.org/show_bug.cgi?id=911216 ; see also https://bugzilla.mozilla.org/show_bug.cgi?id=856410 and https://bugzilla.mozilla.org/show_bug.cgi?id=918806 ) modifications to the ES6 spec regarding async jobs will probably affect this (and of course the other 3 browsers).

@bterlson
Copy link
Member

I will probably mention the testing challenges of promises in my test262 update. While off-topic, I think the only thing to specify is selfishly ECMAScript focused - asap() or something that just takes a function to queue. setTimeout, etc. can be layered on this I suspect.

@smikes
Copy link
Contributor Author

smikes commented Aug 22, 2014

Was there any movement regarding asap or setTimeout at the TC39 meeting?

I would like to move forward with this work, but I don't want to proceed unless the methods I am using to test async behavior are trustable.

@smikes
Copy link
Contributor Author

smikes commented Oct 22, 2014

To convert the Promises/APlus tests to something that can run under Test262, I need to have a way of checking sequencing. Promises/APlus does this by introducting delays with setTimeout -- see this search.

My initial idea was to bootstrap this using Promises themselves (see discussion above).

A good motivating example is the test here, on lines 41-57 :

    specify("trying to fulfill then reject, delayed", function (done) {
        var d = deferred();
        var onFulfilledCalled = false;

        d.promise.then(function onFulfilled() {
            onFulfilledCalled = true;
        }, function onRejected() {
            assert.strictEqual(onFulfilledCalled, false);
            done();
        });

        setTimeout(function () {
            d.resolve(dummy);
            d.reject(dummy);
        }, 50);
        setTimeout(done, 100);
    });

Without a way to execute code in a subsequent tick, I cannot faithfully reproduce this test. If the ES6 spec only provides access to the microtask queue(s) using Promises, then the only way I can port this Promises/A+ test to a pure ES6 execution environment which does not provide setTimeout is to use Promises to enqueue later actions.

smikes added a commit to smikes/test262 that referenced this issue Oct 22, 2014
Incorporate feedback from tc39#56 (comment)

1. drop documentation of `thenable` status of deferreds
2. use 'sequence point' throughout instead of 'sequence-point promise'
3. always use d.promise.then for deferreds

bump version number and date
@getify
Copy link

getify commented Oct 22, 2014

Also note that promises queue their async tasks NOT on the next event loop tick (as setTimeout(..) does), but on the microtask queue, which I kinda envision as a queue of tasks that hangs off the end of the current event loop tick.

Why that matters is that setTimeout(..) isn't actually sufficient to test all sequencing issues, since it runs on the next event loop tick, long after any microtasks in the current tick are done, so you can't interject into that microtask queue to force tests of ordering.

For example, testing sequencing like this would not work with setTimeout(..):

var p = Promise.resolve(42);

p.then(function(){
   return Promise.resolve("foo");
})
.then(function(msg){
   console.log(msg);
});

// nope, won't be "in the middle"
setTimeout(function(){
   console.log("in the middle?");
},0);

p.then(function(msg){
   console.log(msg);
});
// 42
// foo
// in the middle?

@smikes
Copy link
Contributor Author

smikes commented Oct 22, 2014

Well, since the standard doesn't speak to setTimeout at all, I suppose the interleaving out setTimeout() tasks with Promise microtasks is formally undefined. I'd be curious to see whether all current implementations of Promise do what you just wrote, above. It would depend on whether Promise is implemented internally, or in terms of setImmediate, nextTick, or setTimeout(f, 0)

@smikes
Copy link
Contributor Author

smikes commented Oct 22, 2014

My question remains, is it possible to test microtask sequencing using only Promise() in a way that convinces @bterlson @domenic and @getify that it makes sense to go forward with porting Promise/A+ to a self-consistent standard? E.g, what primitive relations between microtasks need to be proved in order to use Promises as a substitute for setTimeout?

Alternatively (it having been 3 months), I will just go forward with this approach and see if I can scare out any bugs. Even if a standard of self-consistency is weaker than an absolute standard, egregious bugs will violate the self-consistency standard.

@bterlson
Copy link
Member

I'm trying to understand the intent of the test above. Thanks so much for bringing up a concrete example!!

The second setTimeout call:

setTimeout(done, 100);

appears to effectively be a timeout correct? Done should be called first by the onRejected callback if everything goes correctly. So, this is not needed and can (and should) be handled by the harness.

The earlier setTimeout call is more interesting, but I don't understand why the "delayed" scenario is interesting necessarily. AFAIK it should be semantically identical to the non-delayed scenario from a spec perspective? The semantically interesting piece is that resolve and reject are called in the same turn (or whatever we're calling a turn these days). Is this the case?

@smikes / @getify / @domenic

@domenic
Copy link
Member

domenic commented Oct 22, 2014

Actually, the onRejected callback should never be called in that test. It's a bit of a strange one.

@bterlson
Copy link
Member

... but there's an assertion there. I guess I don't see how the test won't pass if either callback is called now that you mention it (as long as if rejected is called, resolved is not).

@domenic
Copy link
Member

domenic commented Oct 22, 2014

Yeah that test is pretty strange and best viewed in context of the whole file. It's trying to test the smallest possible assertion about "When fulfilled, a promise: must not transition to any other state."

All of the tests in that file will try to fulfill, and then add a reject handler that will fail if the promise was first fulfilled. They then do a few other things to try to maybe trigger that reject handler (which in any good promise implementation will never trigger).

The success path in all cases is just to wait 100 ms and have no asserts get triggered.

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

Remember that my goal here is to provide a faithful port of the Aplus tests, even if we wouldn't necessarily write all of them the same way today -- with the overall goal being to provide face validity to ES6 Promise implementations, to show they also conform to Aplus.

@bterlson
Copy link
Member

I understand, but I want to understand the intention of the tests and how they would ideally be written. Once the ideal is known we can then consider a faithful transformation of the existing tests to the ideal or why a faithful transformation is not possible. I will take a closer look later this week so I can understand the context of the example better :)

@getify
Copy link

getify commented Oct 23, 2014

It's also important to note that the scope of the tests actually includes testing 100% of the ES6 Promises spec, not just the a+ spec tests (though that's the first step). So when I talk about sequencing, I mean in relation to the implications from the spec of there being a single microtask queue, and what that means for how two different promises would sequence themselves, etc.

Earlier in other threads, I argued that we shouldn't guarantee anything about inter-promise sequencing (since it's merely implied by the nature of a single queue, but not specifically called out in the inter-promise sense)... but I was outvoted in those threads, and told that in fact the implication of the queue semantics was more than enough to say that conforming implementations should all inter-promise sequence in a predictable way (regardless of whether that's a good idea for devs to rely on).

So, if we're going to say out of one side of our mouth, "sequencing should be guaranteed" (aka, required for strict conformance), then out of the other side of our mouth we should be consistent and actually provide tests for that.

I remain extremely wary of the fact that the test harness is not provided even a special backdoor for manually scheduling microtasks (nor any form of asynchrony for that matter). I understand why there isn't a general ES6 API for it, but I personally think the test suite should somehow be given that capability. Promises are the first to rely on these microtasks, but I'm sure it won't be the last. We should plan for needing to do general, low-level microtask testing.

I think to do that, the JS engine(s) the harness runs in would have to provide some sort of flag like --microtasks or whatever.

I don't think it's a good idea to have the tests use the various environment specific hacks for microtasks, like node's process.nextTick(..), or browsers' postMessage(..), etc. BUT, at the very least, that would provide a more reasonable set of test-backed guarantees than relying on setTimeout(..).

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

Hang on- there's two separate issues.

One is that promise micro tasks are guaranteed a specific ordering , per the reference implementation.

Second is that the implementation is allowed to have as many micro task queues as it likes and interleave them however it likes, as long as all promises go through just one queue in the correct order.

This is apparently a live issue right now on es6-discuss (which I do not follow) with respect to what happens to a promise's then() handler if the promise is resolved after the browser navigates away.

But anyway, I now believe that it would actually be incorrect to test promise ordering in terms of a different micro task queue since the spec doesn't constrain inter queue ordering!

@getify
Copy link

getify commented Oct 23, 2014

I don't know about separate queues or not... I suppose that's a hidden implementation detail we wouldn't be testing.

But we still can and should be testing stuff like this, no?

function output(msg){ console.log(msg); }

var p1 = Promise.resolve(42);
var p2 = Promise.resolve("foo");

p2.then(output);
p1.then(output);

// "foo" 42

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

Yup, and I say we can test it like this:

var a = [];
function output (m) { a.push(m) }

var p1 = Promise.resolve(42);
var p2 = Promise.resolve("foo");

p2.then(output);
p1.then(output);

Promise.resolve().then(function () {
   assert.deepEqual(["foo", 42], a);  // (use a test262 fn here instead of a node primitive)
}).then($DONE, $DONE);

@getify
Copy link

getify commented Oct 23, 2014

But that doesn't test that it was done with a microtask, only that it was done in first-come-first-served order. We have to introduce external (to the promise) asynchrony to make sure:

  1. neither output(..) is called synchronously
  2. neither output(..) is called "too late" (aka not on the microtask queue but on the next event loop tick)

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

Reviewing the current ES6 draft, they're not tasks or microtasks anymore but rather Jobs.

@getify
Copy link

getify commented Oct 23, 2014

fine. s/task/job

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

@getify we can ensure 1 but not 2 (see below for implementation)

var a = [];
function output (m) { a.push(m) }

var p1 = Promise.resolve(42);
var p2 = Promise.resolve("foo");

p2.then(output);
p1.then(output);

a.push(1);

Promise.resolve().then(function () {
   assert.deepEqual([1, "foo", 42], a);  // (use a test262 fn here instead of a node primitive)
}).then($DONE, $DONE);

(note: I originally set a to [1] but then edited this because it didn't rule out synchrounous .then())

ES6 doesn't say anything about next event loop ticks, I think. checking spec now.

@getify
Copy link

getify commented Oct 23, 2014

minor correction, to ensure assertion #1:

var a = [];
function output (m) { a.push(m) }

var p1 = Promise.resolve(42);
var p2 = Promise.resolve("foo");

p2.then(output);
p1.then(output);
a[0] = 1;

Promise.resolve().then(function () {
   assert.deepEqual([1, "foo", 42], a);  // (use a test262 fn here instead of a node primitive)
}).then($DONE, $DONE);

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

Yes. BTW are you stuck in an airport right now?

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

I found this: "we are oblivious to future turns at the language level" https://esdiscuss.org/topic/are-promises-and-microtasks-introduced-into-es6#content-1

Also " the event loop and shared state concurrency have haunted ECMA-262 like Banquo's ghost, forever." - B Eich

That suggests that we will not be able to identify code run in a later event loop turn, ever, provided we stay within ES6.

For what it's worth, I interpret the above test as specifying a number of sequence points with associated constraints. Annotated below.

   specify("trying to fulfill then reject, delayed", function (done) {
        // point A
        var d = deferred();
        var onFulfilledCalled = false;

        d.promise.then(function onFulfilled() {
            // point B
            onFulfilledCalled = true;
        }, function onRejected() {
            // point C
            assert.strictEqual(onFulfilledCalled, false);
            done();
        });

        // point D
        setTimeout(function () {
            // point E
            d.resolve(dummy);
            d.reject(dummy);
            // point F
        }, 50);
        setTimeout(done, 100);
    });
            // point G

A: time = t0, begin the test
D: time = t0 + epsilon, chronologically next, set up the setTimeout calls
E: time = t0 + 50, begin resolving
F: time = t0 + 50 + epsilon, all Jobs from resolve/reject are now enqueued
B: time = t0 + 50 + epsilon' , executing onFulfilled, set onFulfilledCalled to true
G: time = t0 + 100, execute done
C: not reached by a correct implementation

I claim that it is possible to port this test to ES6 Promise by using promises instead of the explicit 50ms time offsets. And that it is possible to create a framework using promises that provides the same sequencing (but not time characteristics) as the setTimeout calls above.

@getify
Copy link

getify commented Oct 23, 2014

Is the suggestion then that a browser would be allowed (aka, not required to avoid) scheduling these jobs as just events on the next event tick (rather than in a same-tick-queue), such that my earlier setTimeout.. sequencing in some browsers could be different than others?

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

I believe that is true, since the browser/host env can interrupt NextJob at step 4 and do whatever it wants between Jobs -- even load another module -- and process other code as long as the relative order within each JobQueue is preserved.

@domenic
Copy link
Member

domenic commented Oct 23, 2014

Yes, but only because browsers haven't specced integration with the ES event loop yet. The plan is for the HTML spec to mandate that PromiseJobs are executed using the HTML microtask queue. (Remember that the microtask and task queues are HTML constructs, not ES ones.)

So for example another environment like Node could decide that PromiseJobs are executing on Node's equivalent of the macrotask queue, or could decide to execute them by waiting a second and then executing the job, or whatever. That would of course be insane, but it would still be compliant with the ES spec---just not the (eventual) HTML spec.

@bterlson
Copy link
Member

My understanding is that as far as ECMA-262 is concerned, PromiseTasks need FIFO ordering with respect to themselves. Any other ordering, including among other non-promise tasks, is up to implementations.

I think @getify's latest test case fully tests the required semantics of promises for that scenario - ordering and asynchrony.

In terms of ES6 semantics, the A+ test under consideration:

  • is "delayed", presumably referring to the delayed resolution/rejection by 50ms. The relevant ECMA-262 semantics is that d.promise is created and then resolves in a subsequent turn.
  • resolves and rejects on the same turn
  • verifies that the promise onResolved callback is called and that the onRejected callback is not.

It also gives some coverage of setTimeout and promise interaction, but this falls outside the scope of ECMA-262. Maybe one day we can test the ordering of promises and setTimeout and all other common forms of asynchrony in a rigorous way...

In terms of validating the ES6 semantics, the toughest part is that the proper callbacks were called because you can only really do so after all promise tasks have been run, whether scheduled properly or erroneously. setTimeout is nice for this task because you can just do a setTimeout of 50ms and assume that all the promise stuff will shake out before then. It seems possible to test without setTimeout, but then you have to make assumptions about how buggy implementations might behave - for example, that erroneous promise rejection tasks are queued in FIFO order.

I think this approximates the original test with pure ES6 semantics, but I'm not entirely comfortable with it...

var d = deferred();
var resolved, rejected;
d.promise.then(
  function() {
    resolved = true;
    Promise.resolve().then(function() { // give time for onRejected to be called if it's going to
      if(rejected) $ERROR("onRejected called");
      else $DONE();
    }); 
  },
  function() { rejected = true }
)

Promise.resolve().then(function() {
  // assumes we've tested that this is indeed a subsequent turn from above
  r.resolve();
  r.reject();
});

@domenic
Copy link
Member

domenic commented Oct 23, 2014

In general I am not sure of the value of converting the Promises/A+ tests to test262 format. As discussed here, they are written assuming a more complex event loop setup than is purely present in ES. But furthermore, as far as I know all open-source browsers are already running them as part of their test suite. So what benefit does converting them to test262 actually bring to the world?

It seems more fruitful to write a set of tests from scratch that (a) covers the entire promise API, and not just .then + incidentally a few of the factory methods; (b) captures many of the edge cases of the .then behavior given in the Promises/A+ tests, but in a more straightforward way (see promises-aplus/promises-tests#48 and promises-aplus/promises-tests#56); and (c) only assumes the ordering invariants given by ES, and not those that apply in browsers. We can then leave the Promises/A+ test suite as extra test collateral, which applies in environments with a browser-like event loop (which, who are we kidding, is all of them).

@getify
Copy link

getify commented Oct 23, 2014

@domenic I don't believe the browsers are reliably running the A+ test suite, unless that's a very new occurrence. Do you have a link to info on that?

When I found those several promise bugs in Chrome and FF a couple of months ago, I was wondering how it was possible since the A+ test suite clearly would have showed them, and when I posed such a question, I was told the browsers were not (necessarily) running test suites against things like Promises.

@domenic
Copy link
Member

domenic commented Oct 23, 2014

The tests are checked in to V8, Safari, and Firefox, and running on every push. You can find them in the usual code search places.

From what I remember of the bugs you found, they were pretty subtle (often, indeed, having to do with microtask or inter-promise semantics) and not caught by Promises/A+ :(

@smikes
Copy link
Contributor Author

smikes commented Oct 23, 2014

Oh! Well if we don't need to port A+ into test262, then I'll go back to the other project of just moving promises-es6-tests into a test262 format. That would be much easier, and it's a smaller project too --because the tests there are not generated at runtime and their number doesn't grow combinatorially.

I am happy with the analysis work I did and the breakdown of Promises/A+ tests by section.

@getify
Copy link

getify commented Oct 23, 2014

... not caught by Promises/A+

Nah, I caught them specifically by running the A+ test suite in chrome/node against the shipped "stable" promise, when trying to build my own promise polyfill.

For reference/posterity sake...

Chrome:

Firefox:

This is the only bug I found/reported (was for FF) which was specifically related to the ES6 task scheduling (inter-promise) and thus wouldn't have been covered by A+:

In particular, the Chrome bugs were really bad because they all made it into the V8 that is going to (I believe, still) ship with node 0.12. They should have been caught earlier, but weren't, and when I asked why, the answer was always "we're not running the test suite for promises", followed by some excuse about the promise/ES6 promise spec changing "late" (for whatever that means).

The tests are checked in...

Glad to hear it. I don't routinely check the code bases of the browsers.

I know at the time I reported the above linked bugs, these tests were either not present at all, or not being run. A big part of this whole test-suite making push (with @smikes) was to solve that, because I was (again) told several times the browsers were only running 262 tests, and it seemed crazy to me that none of those tests were verifying browsers' implementations of promises.

If we are sure now that A+ is already being tested, and rigorously, and we're happy that it's testing everything it should, then I would agree that moving A+ into 262 is probably not worth it.


We definitely still need 262 tests for the promise API surface, as well as whatever level (if any) of inter-promise sequencing we're prepared to claim is guaranteed by the ES spec. This last point seems like a moving target, because every time I've raised/discussed the issue, I seem to get a different answer.

We need to nail down what if any sequencing is required, and we need tests for it. I have opinions on what those should be, which are now well and verbosely documented, but I'm not in any official position (TC39 or otherwise) to advance such discussions and come to any conclusions. I just keep nagging that it needs to happen.

Whatever that official conclusion is with stamp of approval, that should dictate what tests need to be made, not the other way around.

And then if we find out that we need other things from the test harness (asynchrony, job scheduling, etc) that we do not have, we have to decide if promise-testing-promise is really good enough (as @smikes claims) or not. I don't like it. It's like testing a test-suite with the same test-suite. But it may be our only option.

@getify
Copy link

getify commented Oct 23, 2014

On a side note, I am not convinced that it would genuinely be OK for browsers to use event loop tick(s) for promise job scheduling, at least not from the perspective of timers (aka setTimeout(..0)). I have run into several cases where "clock shift" (the actual reason I was given to explain) caused essentially this kind of thing to not be predictably ordered (when run in mass volume of course):

setTimeout(function(){ console.log("A"); },0);
setTimeout(function(){ console.log("B"); },0);

You can think of that example kinda like this:

var p = Promise.resolve(42);
p.then(function(){ console.log("A"); });
p.then(function(){ console.log("B"); });

Most people would assume A B ordering in the setTimeout(..0) example. I think that's a reasonable assumption. And it has always seemed to be true (at least to me).

But then I built a promise polyfill that used a naive setTimeout(..0)-based scheduling, and under heavy load I found that (in node -- possibly chrome) I would sometimes get B A. Again, the cause for this was explained to me as "clock shift" (which I admit I don't fully understand).

I moved my polyfill to a unified/centralized setTimeout(..0) queue to avoid those problems. But it illustrates that there is in fact some level of sequencing guarantee implied by the ES6 spec which applies to behavior of a single promise (to say nothing of inter-promise sequencing), and that guarantee may not be satisfiable entirely (naively) with event ticks.

@bterlson
Copy link
Member

We need to nail down what if any sequencing is required, and we need tests for it.

I think it's nailed down. Quoting me from before:

My understanding is that as far as ECMA-262 is concerned, PromiseTasks need FIFO ordering with respect to themselves. Any other ordering, including among other non-promise tasks, is up to implementations.

I of course agree that test262 needs coverage for promises. I also agree with Domenic in that converting A+ tests to test262 will necessarily remove some coverage since we will not be using APIs like setTimeout, but these are still important scenarios to test, and so the A+ suite should continue to exist and be run by implementers.

However, I don't necessarily agree that converting the A+ tests isn't useful. If we'd get good coverage from doing the conversion then even if the conversion is lossy it seems like a good thing to do. I'll leave it up to @smikes, though, whether the work to get useful coverage in test262 from converted A+ tests is worth the amount of coverage we'd get, beyond what's in other suites or what could be hand-written in an equal amount of time.

@getify
Copy link

getify commented Oct 23, 2014

Any other ordering, including among other non-promise tasks, is up to implementations.

You have said that, and I read it. But I am asserting that there's been mixed messages elsewhere, and I'm not positive that everyone actually now agrees.

FWIW, I used to say the same thing ("no inter-promise sequencing guaranteed"), and I was vehemently shot down in other threads. For one such example (there are others not handy to me to link to), @domenic said this directly contradicting the assertion:

getify: There's actually no standard (nor will there be, apparently) that talks about the semantics of ordering between two different promises, only within a single promise.

domenic: That's not correct. Promises/A+ does not give a standard for that, but ES6 does.
The second set of results are correct per spec, due to the nature of the microtask queue as FIFO.

source: getify/native-promise-only#34 (comment)

This kind of disagreement is exactly what I mean by officially nailing it down. It's not enough for just one person in one thread to say something, if two other people in two other threads say other things.

Perhaps the thinking used to be one way and now it's another? Perhaps there's still disagreement?

Either way, I don't consider it even remotely "nailed down", given the conflicting messages. Just to be clear though, this thread is NOT where this topic needs to get nailed down. There needs to be an official agreement (per TC39 notes, or es-discuss thread, or whatever) that's centralized that everyone can point back to from here on out. It's too important a topic not to.

There are indeed many threads which dance around and on top of the topic, including current es-discuss threads. But I haven't seen yet (maybe I missed it) the final, explicit, undeniable nail.

@domenic
Copy link
Member

domenic commented Oct 23, 2014

@getify a bit off topic but: setTimeout tasks, as well as all tasks are supposed to be ordered; I guess Node's reimplementation of setTimeout broke that invariant :(. It's specced as step 12 in https://html.spec.whatwg.org/multipage/webappapis.html#dom-windowtimers-settimeout

@getify regarding your latest post: I don't see the contradiction. There are three separate statements, all true:

  • Promises/A+ does not specify inter-promise ordering
  • ES6 does specify inter-promise ordering
  • ES6 does not specify ordering between promises and other tasks

All of these are already "nailed down" in the relevant specs.

@getify
Copy link

getify commented Oct 23, 2014

I don't see the contradiction....All of these are already "nailed down" in the relevant specs.

That's not an accurate characterization of what you said in that other thread, though.

  1. "Promises/A+ does not specify inter-promise ordering". Agreed, all around.
  2. "ES6 does specify inter-promise ordering". In the snippet I quoted myself and you above, I said "there's no inter-promise ordering" and you said "yes there is, it's in ES6". So which is it?
  3. "ES6 does not specify ordering between promises and other tasks". OK, fine. Agreed.

1 and 3 are settled. 2 is not settled, unless what you said in that other thread explicitly was either wrong then, or is wrong now.

@getify
Copy link

getify commented Oct 23, 2014

And btw, that's not the only place that I asserted "no inter-promise order guarantee" and someone in some pseudo-official standing shot me down. So, are all those wrong? Were they always wrong, or are they only recently wrong? Do they know they're wrong? Do they agree they are currently wrong?

@domenic
Copy link
Member

domenic commented Oct 23, 2014

"ES6 does specify inter-promise ordering". In the snippet I quoted myself and you above, I said "there's no inter-promise ordering" and you said "yes there is, it's in ES6". So which is it?

How are these in contradiction?

In the old thread, you said there's no inter-promise ordering, and I said yes there is in ES6.

In the new thread, I am saying that there is inter-promise ordering in ES6 in my point 2.

@getify
Copy link

getify commented Oct 23, 2014

Ahh, apologies. I read "does" as "doesn't". My bad.


OK, then the opposite contradiction exists, I believe.

I'm reading @bterlson to say "...FIFO...Any other ordering..." as "Promises are internally FIFO ordered, but any other ordering [[including inter-promise ordering]], including among other non-promise tasks..."

Am I reading him wrong?

@domenic
Copy link
Member

domenic commented Oct 23, 2014

Yeah, I think so. I read him as meaning all promise-related ordering is nailed down. That is, internal to the promise ecosystem, not to a particular promise. Which is what the spec says, after all.

@bterlson
Copy link
Member

I think we're decided that converting A+ is not a good approach at this point. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants