Adding promise support to runnables (and thus tests). #329
Conversation
* If a runnable returns a duck-typed promise, i.e. an object with a `then` method, it gets treated as an async test, with success if the promise is fulfilled and failure if it is rejected (with the rejection reason as the error). * Includes tests both of new functionality and some to show that introducing this doesn't break any old functionality.
|
sorry I dont want to abstract any of that stuff, just a callback is fine, .then() is not the only way to write promises etc so it would just be a mess and wouldn't support all the promise-style libs out there anyway |
|
Well, all promise libs in widespread usage (Q, when, Windows 8 WinJS, jQuery, Dojo, ...) follow CommonJS Promises/A (" A callback forces me to write the following boilerplate every time: promise.then(
function () {
(2 + 2).should.equal(4);
},
function (err) {
throw new Error("Promise was rejected when it should not have been.");
}
).then(done, done);instead of, with this (tiny!) change, return promise.then(function () {
(2 + 2).should.equal(4);
}); |
|
personally I dont really even get the point of promise libs, your API can defer things without it being a "promise", pretty much every node lib does this, but they dont use .then() etc |
|
I understand that you're not a fan, but enough people are that I was hoping you could accommodate them in your feature-rich library that makes asynchronous testing simple and fun. My goal is not to convince you to start using promises, but instead to make Mocha a good choice for those of us that already do (like, say, every jQuery, Dojo, or Windows 8 developer). |
|
i hate jquery's xhr api haha. we can let people vote here but -1 from me, ultimately mocha is about being simple and crud-free, adding support for adhoc things like this takes away some of the elegance IMO |
|
+1 I use promises all the time and it has definitely helped keep our large codebase clean and readable. |
|
also for example lots of APIs support |
|
You already have added support for those, since they follow the style of conflating success and error callbacks, so you can just pass in Promises are harder to work with without additional support from the testing framework, since they separate fulfilled and rejected cases in order to achieve exception-style error bubbling. Such APIs are not really the same thing as promises in any way, since they do not support the various guarantees of a promise, or functionality such as chaining and bubbling. |
|
and that's an artifact of it being difficult to work with, that's not really something I want in mocha |
|
maybe we can come up with a way to make done() extensible, so people can have adhoc plugins for stuff like this |
|
I'd be up for that; I was just going to write a package that overrode |
|
+1, but you probably already know that For what it’s worth, @visionmedia, there have been enough bad implementations of promises, including Node’s original Emitter promises and jQuery’s multi-promises, that skepticism toward the concept is well-deserved. There has been enormous momentum toward supporting |
|
Also, @visionmedia, for what it’s worth, promises are easy to work with. It’s just slightly inconvenient to have to switch between callbacks and promises, and that slight inconvenience goes both ways. That slight inconvenience becomes tedium without support at low levels. So we’re shopping around for a friendly test library. |
|
sure, that's understandable, I just prefer callbacks because they make less assumptions, and of course since every node lib in existence supports them. I'll think about it :) |
|
@domenic and other promises funs. What about just extending your favorite promise libraries? Something like: promise.step(function () {
(2 + 2).should.equal(4);
}).cb(done);definitely -1 from me, though it's not my concern |
|
@eldargab With any CommonJS/A compliant promise library; including Dojo, jQuery, and Q; you can do just that: promise
.then(done, done);This assumes that the promise fulfills to undefined, which is likely most of the time. Our desire is to be able to do this instead, for the same effect: return promise;It’s not much, but it’s nice when using promises because returning the last promise is the common idiom. |
|
It also obviates the need for changing your test's signature to Finally, and this is the biggest one---perhaps I should have brought it up before---support for this will allow patterns like return promise.should.be.rejected;
return promise.should.be.rejected(TypeError);
return promise.should.eventually.equal(5);instead of promise.should.be.rejected(done, done)
promise.should.be.rejected(TypeError, done, done);
promise.should.eventually.equal(5, done, done);which I think we can agree is a really nice win for readability. |
|
|
|
Well but it's not used in a callback system; the assertions would be transformations on the promises themselves. |
|
I use Mocha in my promise library and had to resort to making a test case factory that handles returned promise values. Relevant wrapper at https://github.com/promised-io/core/blob/master/test/test-case/index.js#L81. |
|
+1 FWIW, Buster supports returning thenables from test cases and it looks like they'll be coming to Jasmine as well. I'm using @domenic's fork until this makes it into Mocha. |
|
+1, currently using domenic's fork |
|
Rebased my fork's branch on top of master for anyone using it. |
|
+1. (Also, for anyone else, @domenic's fork is currently not supporting node 0.8.x, you may need to update your package.json file appropriately.) |
|
Thanks @jmreidy, rebased again. |
|
Announcing Mocha as Promised, for a more long-term solution. Let me know how it goes for you guys. |
|
+1, would love to see support for this in Mocha! |
|
cool @domenic! should add those to the wiki |
|
Improving on the above example, when you have assertions, in order to see the thrown error properly vs the test timing out, you have to add one more test('Create a record', function(done) {
model.create(fixture.one).then(function(data) {
assert.equal(data.name, fixture.one.name, 'Name should be the same');
done();
}, done).then(null, done); // right here
}); |
|
@thanpolas But why mix promise async and callback async like that? You can just let the success or failure bubble up, like this: test('Create a record', function(done) {
model.create(fixture.one).then(function(data) {
assert.equal(data.name, fixture.one.name, 'Name should be the same');
}).then(done, done);
}); |
|
@00Davo that's even better 👍 |
|
+1 |
|
+1 |
|
@visionmedia @travisjeffery Could you advise your users how many more +1's we need to get real promise support in mocha, if at all? The spec for ES6 promises is out there, let's not ignore it. Holding out until generators is so awkward as that would not work for old browsers. There are plenty of suggestions and options here in the thread to get this working for all environment that can currently run mocha. If it is just a matter of your attention having moved on to other projects then it would be great to assign a maintainer with publishing rights to decide on these things, as there are still a lot of mocha users who'd like to move on with the times (but not be limited by generator support, as our clients' IE9 user-base is still paying many bills). If this cannot be realised in this project even with so many request and no real technical roadblock, then would it be a better idea to leverage the MIT licence, fork and start a |
|
@Bartvds I already said "it is a community project so it's kinda up to you guys" we could maybe use another maintainer or two, @travisjeffery has been pretty on top of things though! but if anyone else is interested I'd be open to that |
|
Cool, thanks for confirming that. From the +1's I get the feeling that the community would like to see this happen. But it is still hanging in limbo by lack of concrete decision (Yes/No/Wait). @travisjeffery When I read back I see you are positive about promise support: if you still like to see this happening and think we have enough community support then please hammer the consensus so we can move out of limbo and on to the implementation details. |
|
If it is up to the community and it seems there is a lot of support, why not add the option to have promises at once? |
|
+1. As it was pointed out earlier here, ES6 does support the promises already, so i see no reason to keep fighting against the reality. Just let it go. Please. The PR itself looks very simple and makes no harm. @travisjeffery? |
| if (!this.pending) this.fn.call(ctx); | ||
| this.duration = new Date - start; | ||
| fn(); | ||
| if (!this.pending) { |
There was a problem hiding this comment.
this part would be a lot nicer if we did something like toCallback(result) below, lots of nested conditionals are really confusing to read
There was a problem hiding this comment.
Hmm will give it a shot at factoring it out.
There was a problem hiding this comment.
something like?
function toCallback(result) {
if (result && typeof result.then === 'function') {
result.then(
function(){
done();
},
done
);
} else {
done();
}
}
// sync
try {
if (!this.pending) {
toCallback(this.fn.call(ctx));
} else {
done();
}
} catch (err) {
done(err);
}
|
Thank you guys! I'd love to use it right away and remove all those hack-ish packages. When do you think the new mocha version will be released with promises support? |
Per comments in mochajs#329 we can factor out the calling into its own function. I also reused `this.resetTimeout()` instead of manually setting it, and added the timeout for promise case as well.
Per comments in #329 we can factor out the calling into its own function. I also reused `this.resetTimeout()` instead of manually setting it, and added the timeout for promise case as well.
|
|
|
Awesome! Great work - very good news. |
|
Wow, this is excellent! Thanks for making it happen. |
|
Awesome! ✌️ |
Mocha now supports tests returning promises directly, so let's do that! mochajs/mocha#329 (comment)
|
Very cool. Thanks! |
thenmethod, it gets treated as an async test, with success if the promise is fulfilled and failure if it is rejected (with the rejection reason as the error).Would really appreciate merging this! I tried to stick as close as possible to the existing style. We <3 Mocha but also <3 promises.