Fail when test resolution method is overspecified

[jugglinmike]

Users may register Runnables as asynchronous in one of two ways:

• Via callback (by defining the body function to have an arity of one)
• Via promise (by returning a Promise object from the body function)

When both a callback function is specified and a Promise object is
returned, the Runnable's resolution condition is ambiguous.
Practically speaking, users are most likely to make this mistake as they
transition between asynchronous styles.

Currently, Mocha silently prefers the callback amd ignores the Promise
object. Update the implementation of the Runnable class to fail
immediately when the test resolution method is over-specified in this
way.

[boneskull]

I like this idea, but it's also breaking, so perhaps a warning would be a better idea. Anyone else have input?

[jbnicolai]

Looks good to me.

Yes, it's breaking, but only for already-incorrect tests. I think we're save to accept it as is :)

[boneskull]

@jbnicolai I mean, that's fine, but I'd be loathe to put this into a minor or patch release.

[jbnicolai]

@boneskull I agree.

[jugglinmike]

Do you guys have a sense of a timeline for Mocha version 2.0? I don't mean to rush, but it would be nice to have an idea about when this change might make it to master.

[boneskull]

@jugglinmike No idea.

[ilanbiala]

I'm guessing this isn't going into 2.0.0 anymore?

[boneskull]

Nope.

[boneskull]

merged via ac4b2e8

[RobertWHurst]

Please revert this change. It's not adding any value, it's just annoying. If I ask for a callback I want to use the callback. Any promise returned should be ignored.

Because of this change we will have to re-write a hundreds of test files. We are using mongoose and the before hook to insert fixture data into the database. Mongoose returns a promise, but we don't use it, nor do we expect mocha to use it either.

[RobertWHurst]

An issue reported by a fellow dev. It's from one of our GitHub repos. This is what we will have to do for all of our tests across all of our service repositories. We have a lot.

The only important breaking change seems to be in async hooks. The actual version does not allow a hook to return a promise and pass a cb simultaneously.

Most of our hooks are returning (unintentionally) a promise and also using the callback :

// mongoose methods return promises
beforeEach((cb) => connection.db.collection('users').remove({}, cb));

You can check in the spec files that now we use just one beforeEach chaining methods with .then():

beforeEach(() => {
    accountD.server.expressApp.request.user = {};

    return connection.db.collection('merchants').remove({})
      .then(connection.db.collection('accounts').remove({}))
      .then(connection.db.collection('accounts').insertOne(account1Fixture));
  });

I think is better to use this way because if we want to keep using multiple beforeEach hooks (as we did before) we need to intentionally add a "fake" return statement to avoid return a promise:

beforeEach((cb) => {
    connection.db.collection('merchants').remove({}, cb)
    return true;
  });
  beforeEach((cb) => {
    connection.db.collection('accounts').remove({}, cb)
    return true;
  });
  beforeEach((cb) => {
    connection.db.collection('accounts').insertOne(account1Fixture, cb)
    return true;
  });

[boneskull]

Please revert this change. It's not adding any value, it's just annoying. If I ask for a callback I want to use the callback. Any promise returned should be ignored.

There's no consensus whether it should be the callback or the Promise which should be ignored. Or that either should be ignored. Or when one of your beforeEach callbacks is actually "done". Different users may have different opinions; different libraries may have different opinions. Yours isn't the only way.

(I've also seen libraries omit the return value if a callback is passed. Evidently, Mongoose doesn't do this, but that's neither here nor there)

Mocha's pre-v3.0.0 behavior is undefined, which means that tests using both risk false positives and/or false negatives. It introduces ambiguity. Based on that alone, I would say this adds value.

The following:

beforeEach((cb) => {
    connection.db.collection('merchants').remove({}, cb)
    return true;
  });
  beforeEach((cb) => {
    connection.db.collection('accounts').remove({}, cb)
    return true;
  });
  beforeEach((cb) => {
    connection.db.collection('accounts').insertOne(account1Fixture, cb)
    return true;
  });

Is not necessary. There is no implicit return in a lambda unless the enclosing curly braces ({ }) are omitted. This would work fine:

beforeEach((cb) => {
    connection.db.collection('merchants').remove({}, cb);
  });
  beforeEach((cb) => {
    connection.db.collection('accounts').remove({}, cb);
  });
  beforeEach((cb) => {
    connection.db.collection('accounts').insertOne(account1Fixture, cb);
  });

It's even pretty trivial to do replace this (using a regexp):

beforeEach((cb) => connection.db.collection('users').remove({}, cb));

with this:

beforeEach((cb) => { connection.db.collection('users').remove({}, cb); } );

or this, to simplify things outright:

beforeEach(() => connection.db.collection('users').remove({}));

[RobertWHurst]

If I request a callback I expect mocha to use that callback. What kind of person requests a callback, but then expects a promise to be observed instead? The idea that someone would expect otherwise doesn't make any sense.

beforeEach((cb) => connection.db.collection('users').remove({}, cb));
            ^^

^^ That is not ambiguous. It's pretty clear what is expected by the author.

[boneskull]

It's ambiguous, because you return a Promise as well. I fail to see how that isn't ambiguous.

[RobertWHurst]

You can't see the cb in the arguments for beforeEach? That makes if extremely clear the author wants to use a callback.

[dasilvacontin]

If I request a callback I expect mocha to use that callback.

You could also say that if you return a Promise you'd expect it to be used for status resolution.

What kind of person requests a callback, but then expects a promise to be observed instead?

Maybe you re-write a hook/test, and you make it return a Promise instead, but forget that a callback was declared. Not very user-friendly to ignore the Promise.

^^ That is not ambiguous. It's pretty clear what is expected by the author.

(user's point of view) If you know that Promises can be returned in hooks/tests, it's not clean to use both. If you didn't know that Promises could be returned, now you do (thanks to the exception).

In your case:

beforeEach(() => connection.db.collection('users').remove({}))

[boneskull]

@RobertWHurst I'm not interested in arguing about this further. I apologize that the new behavior requires extra work on your part. Ultimately, this change was made to help test authors avoid extra debugging overhead.

[RobertWHurst]

You could also say that if you return a Promise you'd expect it to be used for status resolution.

I don't agree, this seems pretty distinct. You don't have to request a promise. You do have to request a cb. That is why it's not ambiguous.

Maybe you re-write a hook/test, and you make it return a Promise instead, but forget that a callback was declared. Not very user-friendly to ignore the Promise.

True, but I think most but I don't think most people expect Mocha to be bad programming training wheels. It's pretty hard to miss this kind of error.

(user's point of view) If you know that Promises can be returned in hooks/tests, it's not clean to use both. If you didn't know that Promises could be returned, now you do (thanks to the exception).

I think that sort thing belongs in documentation, not an exception. It kinda sounds like you guys are trying to push promises. Some people prefer error first callbacks. Mocha has always been flexible in that regard, and rightfully allowed developers to pick which best suited their use case. This seems like you guys are trying to replace that flexibility opinion. If I miss understand you intentions, then I'd argue that at the very least this is a bit to zealous with the training wheels, so to speak.

I understand you have the best of intentions in mind here, but I'm concerned that this won't go over very well with the community. I'm going to leave it at that. I think I've made my point. I don't think this is the last time you'll have this raised though. At the very least your going to be inundated with people who are getting this exception but don't know why. People who don't know that library X returns a promise, or like me, don't care for the error in the first place.

Thanks for engaging with me.

[Munter]

There is no intent to remove flexibility. done callbacks still work. Promises still work. Chose either, not both. The fact that you haven't run into ambiguous test results yet with the old behavior is purely down to luck. This error only makes the possible error condition explicit.

You shouldn't see this as "mocha 3 requires me to rewrite my tests". See it as "mocha 3 made me aware that my tests contained potential errors, which I am now aware of how to fix". You are far from the only one that is using arrow functions with implicit returns, and a lot of others have run into ambiguity in tests because of it. I hope we can all agree that ambiguity is the last thing you'd want from a testing library

[ndelvalle]

I think if there's a callback in the argument, its because the user is trying to use that instead of a promise regardless on what its returning.

[RobertWHurst]

The only way this could be ambiguous is if mocha tried to handle both the promise and callback. It doesn't do that. If a callback is requested, then mocha expects the callback to be used. It completely ignores the promise, as it should.

If a test author requests a callback and returns a promise, expecting that the promise is to be used isn't realistic. There would be no point to requesting the callback in the first place. This would be a programming error, and one that is easily caught. I don't expect my test framework to nanny me about how to write code. That's a task better suited to other modules.

[factoidforrest]

Ah this broke my entire test suite! Change it back! It should prefer the callback if one was specified, as it did before. That was definitely the correct behavior. A warning would be fine. But it shouldn't fail out. A breaking feature when none was necessary.

The offending code:

  // sync or promise-returning
323   try {
324     if (this.isPending()) {
… 
368       if (result && utils.isPromise(result)) {
369         return done(new Error('Resolution method is overspecified. Specify a callback *or* return a Promise; not both.'));
370       }
371 
372       done();
373     });
374   }
375 };

I think if there's a callback in the argument, its because the user is trying to use that instead of a promise regardless on what its returning.

^ 100% correct

[RobertWHurst]

/unsubscribe

Oh I like that. That thing you did with the /unsubscribe, super subtle 👌🏻

@dasilvacontin Thanks for your response, you raise some good points. I still think this error message is unnecessary and will become increasingly an issue with CoffeeScript users and those who use async functions with events. That said, I think I've made my points clearly as have you. Thanks for taking the time to engage with me and the others who raised concerns here. Best of luck with the project 🍻

[dasilvacontin]

@light24bulbs @RobertWHurst I haven't thought about tests written in CoffeeScript. It probably causes trouble with lots of them, and makes upgrading cumbersome..

Throwing an idea: what if we let you use both at the same time, and we wait for both to resolve? (callback called and Promise no longer pending). Test would pass if both are fulfilled. If any is rejected (either callback or Promise), we show status/trace of both, for ease of debugging, and to clarify that test is using both. Would this solve the problems you have?

Also, it would help if you show more examples of tests that now fail because of this. Thanks!

[factoidforrest]

Sure, here is some sample code of one of my before hooks. All it's doing is making some calls to BookShelfJS( an ORM ) models to set up some data. Notice the 'Program' object, which is the model.

    before (done) -> 
        Program.fetchAll().then (programs) ->
            program = programs.first()
            secondProgram = programs.models[1]
            thirdProgram = programs.models[2]
            User.where(email: '[email protected]').fetch().then (user) ->
                userId = user.get('id')
                Card.build {program_id: program.get('id'), balance: 2, user_id: user.get('id')}, (err, card) ->
                    #if err? return done(err)
                    testCard = card
                    done()

The issue here is that any blocking call to bookshelf returns a promise. It's standard practice to just ignore them when you don't want them. Also, it is standard practice to ignore implicit returns in coffeescript. See the second answer here. http://stackoverflow.com/questions/15469580/how-to-avoid-an-implicit-return-in-coffeescript-in-conditional-expressions

So here we have two totally idiomatic, normal behaviors that are causing mocha to throw. Which would be fine, except mocha intentionally made the choice to make this behavior illegal.

So I would strongly argue to process the callback instead of the promise, if one is given. Waiting for both is more complex and I doubt has much use. A user will not accidentally request a callback and then not call it, like they might accidentally return a promise.

Please at least scale this back to a warning.

[Munter]

A user will not accidentally request a callback and then not call it

I have helped a lot of people deal with issues because of exactly this thing you say will never happen. It happens. A lot

[dasilvacontin]

Thanks for the example @light24bulbs!

Waiting for both is more complex and I doubt has much use.

In the examples I've seen so far, it helps with the fact that your code wouldn't throw anymore, because the Promise being returned gets fulfilled.

Also, if you want to use the done callback instead of returning the Promise, you should have a .catch that calls done, otherwise you may be swallowing errors and your tests will hang; it's something a lot of people forget, and why paying attention to the Promise you return – even involuntarily – might be a good idea.

The issue here is that any blocking call to bookshelf returns a promise. It's standard practice to just ignore them when you don't want them. Also, it is standard practice to ignore implicit returns in coffeescript. See the second answer here. http://stackoverflow.com/questions/15469580/how-to-avoid-an-implicit-return-in-coffeescript-in-conditional-expressions.

I think when not needed is a vague description. I don't see how ignore implicit returns in coffeescript is a standard practice. As I see it, you can ignore them if they don't produce side-effects, because if they don't produce side-effects then why bother. But the problem is that in this case, there are side effects: you can't ignore the implicit return when the receiving end acts upon the returned value.

Same as when using _.forEach: you can't ignore the returned value, and since they accept any type of value they can't even ping you about your slip. You add a return at the end of the function and move along – it's one of the perks of using CoffeeScript, even if you are not the one who made the choice of using it.

I also want to point out that I haven't seen any example that wouldn't be simpler (or even better) using Promises instead of done:

    before () -> 
        Promise.all([
            Program.fetchAll(),
            User.where(email: '[email protected]').fetch()
        ]).then ([programs, user]) ->
            [firstProgram, secondProgram, thirdProgram] = programs.models
            program_id = firstProgram.get('id')
            user_id = user.get('id')
            Card.build {program_id, balance: 2, user_id}
        .then (card) ->
            testCard = card

Parallel fetches with early-exit and errors not being swallowed because we are returning the Promise.

Which makes me think that usually compromises are made when done is used instead of returning the Promise the user is working with.

NB: I get the point of, hey, JavaScript is freedom, I should just be able to write code however I want, and nobody should dictate how I write my code. If I don't want semicolons, then it should all work without using semicolons. If I want my return values to be ignored, or my done to be preferred over my returned Promise, then it should just work.

My question/doubt is, do we really do any good in allowing that? My thoughts are, I want to benefit the majority, and avoid people to waste time debugging, where a helpful exception could do the job. But I have no big problem I guess with people who want to do it their own way at the expense of possibly wasting some time debugging in the future – it's their choice. Yes, I'm saying that I'm kinda okay with having a --prefer-done-over-promises option. Just that it shouldn't be the default behaviour.

I have helped a lot of people deal with issues because of exactly this thing you say will never happen. It happens. A lot

Thanks for your input @Munter. I remember having to help some people because of the same thing. Do you recall the reason why they couldn't figure it out on their own? The error that we used to throw is Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.. Maybe they didn't understand what we meant, that they had defined a callback in the test function which was later not being called? Definitely the exception message could have been better.

[Munter]

Maybe they didn't understand what we meant, that they had defined a callback in the test function which was later not being called?

Sounds about right. It was non-obvious to them as a newcomer. I've also seen countless examples of people using a done callback for completely synchronous tests. Bad tutorials, copy/pasting, cargo culting and whatnot. In an ideal world everyone would read all the docs, but this isn't the case in reality, and I would hate exposing this abstraction leak to a new user by throwing them into hours of debugging and confusion when a simple error message immediately gives them something to google

[RobertWHurst]

I have more to say here, but I'm out for the evening so I'll just make a quick remark about async testing.

So here is a synchronous test for an async API. Because I'm using nock I could potentially use sinon or another stub/spy lib to capture the callback result without the need for calling done, but why? I don't see the problem with using done here. It's clearer and demonstrates how to use the API. When people use done, they are not necessarily testing code that is running across ticks. Sometimes they are just testing an async interface. An important distinction.

async example

it('fetches all of the widgets then calls back', (done) => {
  const fixtureWidgets = [widgetFixture1, widgetFixture2];
  const getWidgets     = nock('http://api.widgetco.com')
                           .get('/widget')
                           .reply(200, fixtureWidgets);

  const wigetFactory = new WidgetFactory();

  wigetFactory.collectWidgets((err, widgets) => {
    if (err) { return done(err); }
    assertContains(widgets. fixtureWidgets);
    done();
  });
});

sync example

it('fetches all of the widgets then calls back', () => {
  const fixtureWidgets = [widgetFixture1, widgetFixture2];
  const cbStub         = sinon.stub();
  const getWidgets     = nock('http://api.widgetco.com')
                           .get('/widget')
                           .reply(200, fixtureWidgets);

  const wigetFactory = new WidgetFactory();

  wigetFactory.collectWidgets(cbStub);

  sinon.assert.calledOnce(cbStub);
  sinon.assert.calledWith(cbStub, null, fixtureWidgets);
});

Personally, I find the former is much clearer, and also does not require a stubbing lib (or an ugly and confusing function the sets enclosed vars with side effects).

Back with more later. 🍻

[dasilvacontin]

@RobertWHurst If you don't care if collectWidgets executes the callback sync or async, the 1st test is better, because it allows both, and it will still work if collectWidgets switches between being sync/async.

The second test enforces that the callback is being called sync, which is sometimes desired and therefore asserted.

Not sure what the point you are rising is though – I personally like done. But if I'm using Promises already, and it makes the test simpler, then I'll avoid using done.

[dasilvacontin]

I hadn't realised about the discussion that went on at #2407.

@elado's example at #2407 (comment) is very interesting. Copying it over:

it('fires change event when calling blah.doSomethingThatFiresChange', async function (done) {
  const blah = await getBlah()
  blah.on('change', () => done())
  blah.doSomethingThatFiresChange()
})

Imagine blah.doSomethingThatFiresChange() raises an exception after having emitted the change event. If we ignore the async function's Promise, that exception gets swallowed and ignored, and the test will pass.

I keep seeing more reasons for using both done and the Promise, and every reason looks even more awesome that the last one. It also goes along with paying attention to anything the user 'requests', done, Promise, or both at the same time.

[RobertWHurst]

@dasilvacontin I agree.

It's probably not directly relevant but I was responding to @Munter who said:

I've also seen countless examples of people using a done callback for completely synchronous tests

I was just trying to illustrate an example of a test that uses the async interface but actually runs async. I retrospect though I think It might be an aside to the conversation as I'd imagine @Munter you meant when people write things like:

it('runs my cool fn', (done) => {
  coolFn();
  done();
});

[RobertWHurst]

On another note, I had an idea this morning which I thought might be interesting to float with you guys.

Imagine we go back to the behaviour from mocha 2 for a minute. I accidentally request done in my test (perhaps I pull a classic copy and paste error) and now I'm trying to figure out why my test is timing out even though I returned a promise. The original error message timeout of 2000ms exceeded isn't helping. What if the timeout error message was simply amended to say timeout of 2000ms exceeded. Expected done to be called but is never was.. This could also be followed with a warning when a promise is returned in addition to requesting done; A promise was returned but ignored because done was requested. For the case where a promise times out, the error could be timeout of 2000ms exceeded. Expected the returned promise to resolve but it never did.. These messages are rough, but would something like this not bring a similar benefit to the error we are currently discussing?

I'll make a PR later today so you guys can evaluate and play with it; see if it's any good.

[RobertWHurst]

Here is my PR => #2454

[dasilvacontin]

The original error message timeout of 2000ms exceeded isn't helping.

Jut fyi, using mocha 2.5.3, the error message for:

it('requests both a callback and a promise', (done) => {
  return Promise.resolve()
})

is:

Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

@RobertWHurst , your suggestions improve the old error message. I'll continue my feedback in the PR.

I still think that, when both Promise and done are requested, we should either raise an Error or consider both for the outcome (instead of ignoring the Promise). And if we go for the latter, not sure a flag for the old behaviour would be necessary, seems apparently it would make all examples I've seen so far work.

Edit: I'd like feedback regarding using both done and Promise when both are requested/given. I've seen reasons for, but no reasons against.

[dasilvacontin]

Example of using both done and Promise when both are requested/given, catching an Error that would be missed otherwise:

it('fires change event when calling blah.doSomethingThatFiresChange', async function (done) {
  const blah = await getBlah()
  blah.on('change', () => done())
  blah.doSomethingThatFiresChange()
})

  1) fires change event when calling blah.doSomethingThatFiresChange:

     Error: the `done` callback was successfully called, but the returned `Promise`
     was rejected with the following `Error`:

     TypeError: Cannot read property 'likes' of undefined
         at Blah.doSomethingThatFiresChange (lib/Blah.js:5:9)
         at Context.<anonymous> (test/Blah.spec.js:4:8)

     More info: https://github.com/mochajs/mocha/wiki/some-page

[RobertWHurst]

@dasilvacontin I'm still thinking about the implications of handling both the promise and the callback. Suppose my initial concern is what happens if a library returns a promise, but doesn't resolve it if given a callback? Hmm, might not actually be an issue, but I'm unsure so I'm left a bit uneasy about it. I'll do some thinking and look at some 3rd party libs and see if I can find such an example. I'll continue here in a bit.

[dasilvacontin]

@RobertWHurst Hmm, if a function uses either a node-style completion callback or a completion Promise, not settling/giving the same error for both feels like an implementation error in the function.