-
Notifications
You must be signed in to change notification settings - Fork 468
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
Comments
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 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 :(.
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.
This is probably worth a helper
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... |
If I recall correctly, the main reason I did that was so I could call After that, though, I found it convenient to be able to write both In any case, I will go over the README again and try to clarify the naming of the sequence points.
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 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 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
In the console runner, test timeouts are currently handled in JavaScript, but they could be moved up into the python code. In browser runners, Then we can test that sequencing works as expected:
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. |
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
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. |
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 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). |
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. |
Was there any movement regarding 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. |
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 :
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. |
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
Also note that promises queue their async tasks NOT on the next event loop tick (as Why that matters is that For example, testing sequencing like this would not work with 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? |
Well, since the standard doesn't speak to |
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. |
I'm trying to understand the intent of the test above. Thanks so much for bringing up a concrete example!! The second setTimeout call:
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? |
Actually, the onRejected callback should never be called in that test. It's a bit of a strange one. |
... 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). |
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. |
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. |
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 :) |
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 I don't think it's a good idea to have the tests use the various environment specific hacks for microtasks, like node's |
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! |
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 |
Yup, and I say we can test it like this:
|
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:
|
Reviewing the current ES6 draft, they're not tasks or microtasks anymore but rather Jobs. |
fine. s/task/job |
@getify we can ensure 1 but not 2 (see below for implementation)
(note: I originally set a to [1] but then edited this because it didn't rule out synchrounous ES6 doesn't say anything about next event loop ticks, I think. checking spec now. |
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); |
Yes. BTW are you stuck in an airport right now? |
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.
A: time = t0, begin the test 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 |
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 |
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. |
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. |
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:
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();
}); |
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 |
@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. |
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+ :( |
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. |
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).
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. |
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(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 But then I built a promise polyfill that used a naive I moved my polyfill to a unified/centralized |
I think it's nailed down. Quoting me from before:
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. |
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:
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. |
@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:
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 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. |
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? |
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. |
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? |
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. |
I think we're decided that converting A+ is not a good approach at this point. Closing this. |
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 toresolve
orreject
will happen after all currently queued Promise jobs.I would be very grateful for feedback about my approach.
/cc @domenic @getify @anba and @niksurya
The text was updated successfully, but these errors were encountered: