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

Adding promise support to runnables (and thus tests). #329

Closed
wants to merge 1 commit into from

Conversation

@domenic
Copy link
Contributor

@domenic domenic commented Mar 16, 2012

  • 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.

Would really appreciate merging this! I tried to stick as close as possible to the existing style. We <3 Mocha but also <3 promises.

* 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.
@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

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

@tj tj closed this Mar 16, 2012
@domenic
Copy link
Contributor Author

@domenic domenic commented Mar 16, 2012

Well, all promise libs in widespread usage (Q, when, Windows 8 WinJS, jQuery, Dojo, ...) follow CommonJS Promises/A ("then-ables").

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);
});
@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

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

@domenic
Copy link
Contributor Author

@domenic domenic commented Mar 16, 2012

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).

@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

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

@pbouzakis
Copy link

@pbouzakis pbouzakis commented Mar 16, 2012

+1

I use promises all the time and it has definitely helped keep our large codebase clean and readable.

@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

also for example lots of APIs support .end(callback) but I'm not adding support for returning those either, which are effectively the same thing as promises

@domenic
Copy link
Contributor Author

@domenic domenic commented Mar 16, 2012

You already have added support for those, since they follow the style of conflating success and error callbacks, so you can just pass in done.

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.

@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

and that's an artifact of it being difficult to work with, that's not really something I want in mocha

@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

maybe we can come up with a way to make done() extensible, so people can have adhoc plugins for stuff like this

@domenic
Copy link
Contributor Author

@domenic domenic commented Mar 16, 2012

I'd be up for that; I was just going to write a package that overrode mocha.Runnable.prototype.run, which of course would be a bitch to maintain in the face of upstream changes since that method is so long.

@kriskowal
Copy link

@kriskowal kriskowal commented Mar 16, 2012

+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 then as a way to freely-trade promises between various systems, a great tool for agreeing to disagree.

@kriskowal
Copy link

@kriskowal kriskowal commented Mar 16, 2012

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.

@tj
Copy link
Contributor

@tj tj commented Mar 16, 2012

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 :)

@eldargab
Copy link

@eldargab eldargab commented Mar 17, 2012

@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

@kriskowal
Copy link

@kriskowal kriskowal commented Mar 18, 2012

@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.

@domenic
Copy link
Contributor Author

@domenic domenic commented Mar 18, 2012

It also obviates the need for changing your test's signature to function ("foo", done) { from simply function ("foo") {, fitting with promises' general pattern of letting results flow through the system without needing to make sure everyone passes their callback responsibilities around correctly.

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.

@tj
Copy link
Contributor

@tj tj commented Mar 18, 2012

returning in general as an API this sort of thing kinda sucks since you're then limited as far as assertions in callbacks go etc

@domenic
Copy link
Contributor Author

@domenic domenic commented Mar 18, 2012

Well but it's not used in a callback system; the assertions would be transformations on the promises themselves.

@novemberborn
Copy link

@novemberborn novemberborn commented Mar 19, 2012

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.

@nonplus
Copy link

@nonplus nonplus commented Mar 25, 2012

+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.

http://busterjs.org/docs/overview/

@tomjack
Copy link

@tomjack tomjack commented May 7, 2012

+1, currently using domenic's fork

@domenic
Copy link
Contributor Author

@domenic domenic commented Jun 14, 2012

Rebased my fork's branch on top of master for anyone using it.

@jmreidy
Copy link

@jmreidy jmreidy commented Jul 1, 2012

+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.)

@domenic
Copy link
Contributor Author

@domenic domenic commented Jul 1, 2012

Thanks @jmreidy, rebased again.

@domenic
Copy link
Contributor Author

@domenic domenic commented Jul 8, 2012

Announcing Mocha as Promised, for a more long-term solution. Let me know how it goes for you guys.

@mjackson
Copy link
Contributor

@mjackson mjackson commented Oct 23, 2012

+1, would love to see support for this in Mocha!

@tj
Copy link
Contributor

@tj tj commented Oct 23, 2012

cool @domenic! should add those to the wiki

@thanpolas
Copy link

@thanpolas thanpolas commented Feb 8, 2014

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 .then(null, done); statement at the tail of your original promise:

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
});
@00dani
Copy link

@00dani 00dani commented Feb 8, 2014

@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);
});
@thanpolas
Copy link

@thanpolas thanpolas commented Feb 8, 2014

@00Davo that's even better 👍

@giggio
Copy link
Contributor

@giggio giggio commented Mar 5, 2014

+1
Or at least make Mocha more extensible to allow it to be plugged in without a hack.
This is the very reason I abandoned Jasmine: they got stuck in the past. I hope it doesn't happen to Mocha.

@patkujawa-wf
Copy link

@patkujawa-wf patkujawa-wf commented Mar 6, 2014

+1

@Bartvds
Copy link

@Bartvds Bartvds commented Mar 7, 2014

@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 mocha2? I'd hate to schism the community but if this line is going stale then it might be our best option.

@tj
Copy link
Contributor

@tj tj commented Mar 7, 2014

@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

@Bartvds
Copy link

@Bartvds Bartvds commented Mar 7, 2014

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.

@giggio
Copy link
Contributor

@giggio giggio commented Mar 9, 2014

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?
What exactly needs to be done? Just saying it is up to the community does not cut it. What exactly are you waiting for so this is done, @visionmedia?

@xaka
Copy link

@xaka xaka commented Mar 13, 2014

+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?

travisjeffery added a commit that referenced this pull request Mar 13, 2014
travisjeffery added a commit that referenced this pull request Mar 13, 2014
if (!this.pending) this.fn.call(ctx);
this.duration = new Date - start;
fn();
if (!this.pending) {

This comment has been minimized.

@tj

tj Mar 13, 2014
Contributor

this part would be a lot nicer if we did something like toCallback(result) below, lots of nested conditionals are really confusing to read

This comment has been minimized.

@domenic

domenic Mar 13, 2014
Author Contributor

Hmm will give it a shot at factoring it out.

This comment has been minimized.

@travisjeffery

travisjeffery Mar 13, 2014
Contributor

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);
  }
@xaka
Copy link

@xaka xaka commented Mar 13, 2014

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?

domenic added a commit to domenic/mocha that referenced this pull request Mar 14, 2014
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.
travisjeffery added a commit that referenced this pull request Mar 14, 2014
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.
@travisjeffery
Copy link
Contributor

@travisjeffery travisjeffery commented Mar 14, 2014

1.18.0 is out now with promise support.

@benjamingr
Copy link

@benjamingr benjamingr commented Mar 14, 2014

Awesome! Great work - very good news.

@Bartvds
Copy link

@Bartvds Bartvds commented Mar 14, 2014

Wow, this is excellent! Thanks for making it happen.

@bajtos
Copy link

@bajtos bajtos commented Mar 14, 2014

Awesome! ✌️

cscott added a commit to cscott/prfun that referenced this pull request Mar 31, 2014
Mocha now supports tests returning promises directly, so let's do that!
mochajs/mocha#329 (comment)
@xogeny
Copy link

@xogeny xogeny commented May 6, 2014

Very cool. Thanks!

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

Successfully merging this pull request may close these issues.

None yet