Error: Resolution method is overspecified.

[teckays]

This:

before(done => {
    return Promise
        .all([])
        .then(() => Model.insert(...)) // Bookshelf model returning a Bluebird Promise
        .then(() => done())
        .catch(done)
})

will result in an error Error: Resolution method is overspecified. Specify a callback *or* return a Promise; not both.

Docs say:

In Mocha v3.0.0 and newer, returning a Promise and calling done() will result in an exception, as this is generally a mistake:

The model call is resolving with a Promise.<Object> of the newly inserted entry, however if I omit .then(() => done()) then the test timeouts.

[elado]

async functions (babel) with done also break.

[papandreou]

As the error states, you're not supposed to provide a function with an arity of >0 (meaning that it's accepting a done callback), and return a Promise.

The easiest way to fix it would be to omit the return, but since you're using promises I suggest getting rid of the done callback instead, as it'll result in a much simpler construct:

before(() => Promise.all([]).then(() => Model.insert(...)));

[elado]

Here's an example of both async (essentially a promise) and done that breaks:

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

[papandreou]

@elado That's an interesting use case, although from mocha's point of view it's the same situation -- a function that both takes a done callback and returns a promise. I'd rewrite it to be fully promise-based:

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

... but I guess what you're getting at is that in this example, it would be better if mocha waited for both the promise to be resolved and the callback to be called?

Unfortunately that particular combo is most often a bug in the test, which is why this error message was added in the first place.

[ScottFreeCode]

...if I omit .then(() => done()) then the test timeouts.

This seems like a bug, in any case.

[papandreou]

@ScottFreeCode Hmm, yeah, it seems to be because the "overspecified" error is issued in the function provided as the done callback:

mocha/lib/runnable.js

Lines 357 to 373 in 4944e31

	var result = fn.call(ctx, function(err) {
	if (err instanceof Error || toString.call(err) === '[object Error]') {
	return done(err);
	}
	if (err) {
	if (Object.prototype.toString.call(err) === '[object Object]') {
	return done(new Error('done() invoked with non-Error: '
	+ JSON.stringify(err)));
	}
	return done(new Error('done() invoked with non-Error: ' + err));
	}
	if (result && utils.isPromise(result)) {
	return done(new Error('Resolution method is overspecified. Specify a callback *or* return a Promise; not both.'));
	}
	done();
	});

... but we can of course determine that we have to fail with that error as soon as the function has returned.

[boneskull]

@ScottFreeCode What's the fix here?

[boneskull]

I'm going to guess it's "wait for the promise to resolve or reject, then reject with the 'overspecified' error"?

[ScottFreeCode]

If we're going to consider done plus promise in the same test an error, I don't see any reason not to detect the error as soon as a test function that took done returns a promise, as @papandreou suggested. It doesn't make a lot of sense to me to try to figure out what other points should trigger the error unless we intend to allow promises and done together in some cases.

[boneskull]

@ScottFreeCode I'm in agreement. So it's

1. Detect problem; instantiate the Error but do not call done() with it
2. Wait until Promise fulfillment
3. Reject with the Error

Bonus question: What to do with the result of the Promise fulfillment?

[ScottFreeCode]

Ah, I think I get it -- even when we detect the error, we need to let the test run so it's not still running its code during the next test and, maybe, so we can also report the test result.

Could a test also end up calling done without resolving or rejecting the promise? If so, that's another end case we'll have to handle.

My inclination, re. what to do with the result of the test, is that if it times out (without having either called done or resolved/rejected the promise) we should just report the use of done with the promise (because confusion over the combination could very well be why it never got to either one and timed out instead), but if it does manage to finish by either means then presumably the result is valid (or at least somehow meaningful) and we should report both the possibly erroneous use of done and promise together and also the test result in hope that it's at least somewhat helpful.

That's the best I can come up with at the moment, anyway, but I might have more insight if I can find the time to dig into this problem.

[boneskull]

Well, we can do this in phases. The first would be to ensure that if a Promise is returned and a done callback is given, then Mocha will break in a friendly manner.

It's conceivable that one could do something like this (with Bluebird):

// to make this work, you need to omit `return`
it('should do something async for sure', function(done) {
  return somethingAsync()
    .then(makeAssertion)
    .asCallback(done);
});

But that's just something you could do; I have yet to determine a use case for this.

[boneskull]

I think I've confused myself. I'm not sure there's a bug here at all.

[boneskull]

This is more of a "poor UI"-type issue

[ScottFreeCode]

Timing out waiting for done to be called even though a returned promise gets resolved/rejected is definitely a bug, regardless of whether we want to disallow such tests from using done and promises together in the first place. This should either use the result of the promise and/or error because the promise and done were both utilized in the same test, but not just time out because one of the two never completed when the other one did (which is what it does currently):

it("should not time out", function(done) {
  return new Promise(function(resolve, reject) {
    setTimeout(resolve.bind(42), 50)
  })
})

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

I'll look at the PR in any case...

[boneskull]

Interested parties should watch #2413.

@BrianSipple

[ianaya89]

I get the same error using multiple async beforeEach hooks in version 3:

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

Is not possible to still using async hooks in that way?

[ianaya89]

I figured out my issue: my methods inside each hook are returning a promise (I didn't know that because I was not using promises in those cases) and obviously I am also passing cb function.

[RobertWHurst]

I don't think this is a good idea. If one requests a callback I think they have made their intentions clear. The error here is just an annoyance. Please remove it.

[boneskull]

This isn't really a matter of intention. From #1320's description:

When both a callback function is specified and a Promise object is
returned, the Runnable's resolution condition is ambiguous.

[RobertWHurst]

It's not ambiguous, the callback was requested. How is that ambiguous?

[ianaya89]

I am agree with that @RobertWHurst there is no ambiguous here.

Besides that I think this issue could be something "opinion based" and developers will have different point of views. I consider that is very extremist to do a breaking change and force people use that way.

[boneskull]

We didn't force anyone to do anything. We released a major, which by definition will have breaking changes. We didn't break semver.

[plroebuck]

@tamoyal, yeah -- as elado posted back in Aug 3, 2016.

Using async/await returns an implicit Promise. Use of the done callback is for
CPS-based async, as shown here. Mocha supports both... but not at the same time.
This is documented and has been standard behavior since the Mocha-3.0 release.

[plroebuck]

I was getting this error but I was accidentally returning a promise (supertest)

it('should do something', (done) => {
       return supertest(server)
           .get(`/api/endpoint`)
           .set('somekey', 'somevalue')
           .expect(200)
           .then(() => { done(); })
           .catch(done);
});

Removing the 'return' clause solved the problem

@victorsferreira, seems like this should have been the solution...

it('should do something', () => {
  return supertest(server)
    .get(`/api/endpoint`)
    .set('somekey', 'somevalue')
    .expect(200);
});

[mienaikoe]

@tamoyal Yep that was what tripped me up. It's very unintuitive to have a third party library looking into the parameters of a function I create and using that to make a decision without telling me that it did so.

[plroebuck]

@mienaikoe, this exact scenario is explicitly documented, you know...

[mienaikoe]

@plroebuck two things:

1. There's a difference between documenting something and being one of the few libraries in npm that introspects on a function's parameters. I could document that I take a background check of all my friends when I first meet them, but it's still strange and people would still complain about it unless I had a reason and explicitly told them the reason.

2. There's a flaw in the documentation:

In Mocha v3.0.0 and newer, returning a Promise and calling done() ...

It's not about calling done, it's about specifying done as a parameter whether or not you use it.

[plroebuck]

@mienaikoe

1. Mocha checks it's second parameter to see if function exists, and (if so) its arity to determine if user added a callback. This barely qualifies as introspection, and commonly used in JavaScript.
2. Send PR to correct the documentation if you think it's too vague.

[QauseenMZ]

Removing done as a param worked for me!

BEFORE:

AFTER(works!)

[plroebuck]

@QauseenMZ, see no difference between your "before" and "after" code.

You're also not returning your promise. Shouldn't it be more like the following?

  it('should receive successful RPC response', async () => {
    return await getResponse(unitData)
      .then((res) => {
        expect(res).to.have.status(200);
      });
  });

PS. Your Node callback argument ordering is whacked... error always goes first.

[QauseenMZ]

@plroebuck Thanks for mentioning! I just edited that.

I'm not handling the promise because getResponse function is returning a callback. It was the only way I got it working. getResponse function is as follows:

Here error is second parameter just because of the callback getResponse function is returning. Please let me know your thoughts about it. Thanks!

[plroebuck]

Some parts seem kinda illogical. Just for clarity, which request package are you using?
Why would you need to return the options object (what you named unitData)?

• Where does obj come from?
• Why would you have a res.body.error with a res.statusCode === 200?

PS. Please just paste the code itself rather than images of the code...

[maitrungduc1410]

For anyone else who was driven crazy by this...

This works:

it("should work", async () => {
  await something;
});

This does not:

it("should work", async (done) => {
  await something;
});

You must remove done as a parameter.

@tamoyal You saved my life <3

[YourFavouriteOreo]

This breaks and results into the same error:

it('Location Querying Works', async () => {
    await WeatherComponent.pullLocationData();
    Vue.nextTick(()=>{
      expect(WeatherComponent.$el.textContent).contain("Spain")
    })
  })

[Crowbrammer]

To give you a fast solution to this issue, wrap your entire test in a promise, and use resolve as you would done.

Turn this:

it.only("Works with the database", async (done) => {
    let browser = await puppeteer.launch();
    let query = db.collection('lists').where('ownerId', '==', 'UJIXXwynaCj8oeuWfYa8');
    let ct = 0;
    query.onSnapshot(async querySnapshot => {
        if (ct === 0) {
            await addAPurpose(browser, session, sessionSig); // A function with a bunch of Puppeteer code.
            ct++
        } else {
            expect(querySnapshot.size).to.equal(1);
            expect(querySnapshot.docs[0].get().title).to.equal("Understand Mocha");
            done();
        }
        console.log(`Received query snapshot of size ${querySnapshot.size}`);
    }, err => {
        console.log(`Encountered error: ${err}`);
    });
});

into this:

it.only("Works with the database", async () => {
    const onSnap = new Promise(async (res, rej) => {
        let browser = await puppeteer.launch();
        let query = db.collection('lists').where('ownerId', '==', 'UJIo1gGMueoubgfWfYa8');
        let ct = 0;
        let unsubscribe = query.onSnapshot(async querySnapshot => {
            if (ct === 0) {
                await addAPurpose(browser, session, sessionSig);
                ct++
            } else {
                expect(querySnapshot.size).to.equal(1);
                expect(querySnapshot.docs[0].data().title).to.equal("Evolution");
                // done(); 
                unsubscribe();
                res();
            }
            console.log(`Received query snapshot of size ${querySnapshot.size}`);
        }, err => {
            console.log(`Encountered error: ${err}`);
            rej()
        });
    });
    return onSnap;
});

And it works like you want.

The need to remove done surprised me, because the test ran until done was called. To make it more obvious that done shouldn't be used with async, async functions should fail immediately if done is passed. Mocha should start the test by:

1. Seeing if the function is async, and
2. Detecting the done argument.

If they're both present, it should throw instead of letting my tests run up until done is called and then blue-balling me. Then the error should suggest that you wrap your code in another promise, and use resolve as you would done.

I know you can use function.prototype.name === "AsyncFunction". Then it's

if (function.prototype.name === "AsyncFunction && arg1.name === "done") {
  throw new Error("Can't use done with an async function")
}

to implement this.

[NateZimmer]

I was nearly SOL on this. I need to execute async code AND its expected that code won't finish executing(ever) so I HAVE to use done. The annoying hack-a-round was to wrap my async test code in a self invoking async function but leave the it func as a sync func.

Solution:

it("It shouldn't be like this", function (done) {
    ( async function(){
        var life = require('InfiniteLife');
        var asyncRes = await life.someCoolstuff();
        assert(asyncRes);
        setTimeout( function(){
            done();
        },letsCallItQuitsNow);
    })();
});

[Haisum92]

An example of async functions with done breaking.

it('If the credentials exists in the system it should return the token generated against it.', async (done) => {
        let aObj = await admin.createAdmin();
        chai.request(server)
        .post("/authenticate")
        .set("Content-Type", "application/x-www-form-urlencoded")
        .send({username: aObj.login,password:aObj.password})
        .end((err, res) => {
            res.should.have.status(200);
            res.body.should.be.a("string");
            done();
        });
    });

Success Case

it('If the credentials exists in the system it should return the token generated against it.', async () => {
        let adminObj = await admin.createAdmin();
        chai.request(server)
        .post("/auth/login")
        .set("Content-Type", "application/x-www-form-urlencoded")
        .send({username: adminObj.login,password:adminObj.password})
        .end((err, res) => {
            res.should.have.status(200);
            res.body.should.be.a("string");
            // done();
        });
    });

[Munter]

@Haisum92

You need neither done nor async in your test. Chai http returns a promise. Mocha's tests work with async code if you return a promise.

it('If the credentials exists in the system it should return the token generated against it.', () => {
  let adminObj = await admin.createAdmin();
  return chai.request(server)
    .post("/auth/login")
    .set("Content-Type", "application/x-www-form-urlencoded")
    .send({username: adminObj.login,password:adminObj.password})
    .end((err, res) => {
      res.should.have.status(200);
      res.body.should.be.a("string");
    });
});

[Munter]

@NateZimmer

Your posted solution is for timing out the test if it takes too long. Mocha already has this functionality built in.

it("It should be liek this", async function () {
  this.timeout(letsCallItQuitsNow);

  var life = require('InfiniteLife');
  var asyncRes = await life.someCoolstuff();
  assert(asyncRes);
});

Your example also doesn't seem to fail the test if the timeout is exceeded, so be careful with depending on the correctness of it

[christos97]

` Using express/js, and firebase cloud functions, check api error responses, then get a token and try again. Make sure if using express you return .json and not .send, also functions logger bugs with mocha so use plain console.log.
Make sure you use async and emit done completely

  process.env.NODE_ENV='TESTING';
  const { FIREBASE_UID } = require('dotenv').config()?.parsed;
  const { red, green, white } = require('chalk');
  const chai = require('chai');
  const chaiHttp = require('chai-http');
  const server = require('../lib/api').API;
  const should = chai.should();
  const expect = chai.expect;
  chai.use(chaiHttp);
  const test = chai.request(server).keepOpen();

  const shouldPrint = (details, status) => {
      return white(`details: ${details}, status: ${status}`);
  };

describe('Authorization Middleware Error Check for bad token', () => {

    it(shouldPrint('Bad Request', red(400)), async () => {
        try {
            const res = await test.get('/').send()
            res.should.have.status(400);
            res.should.have.json
            const { details, status } = JSON.parse(res.text)
            expect(details).to.equal('Bad request')
            expect(status).to.equal(400)
        } catch (error) { 
            throw error 
        }
    })


    it(shouldPrint('Unauthorized', red(401)), async () => {
        try {
            const res = await test.get('/').set('Authorization', 'Bearer bad123token').send()
            res.should.exist
            res.should.have.status(401);
            res.should.have.json
            const { details, status } = JSON.parse(res.text)
            expect(details).to.equal('Unauthorized')
            expect(status).to.equal(401) 
        } catch (error) {
            throw error
        }
    })

})

 describe('Get token and ping', () => {
    let adminToken
    
    it(shouldPrint( 'Create custom jwt for testing', green(200)), async () => {
        try {
            const res = await test.get(`/createToken/${FIREBASE_UID}`).send()
            res.should.exist
            res.should.have.status(200);
            res.should.have.json
            adminToken = (JSON.parse(res.text)).token
        } catch (error) {
            throw error
        }
    })

    it(shouldPrint('PING', green(200)), async () => {
        try {
            const res = await test.get('/')
                .set('x-tenorders-testing', 'true')
                .set('Authorization', `Bearer ${adminToken}`).send()

            res.should.exist
            res.should.have.status(200);
            res.should.have.json
            const { details, status } = JSON.parse(res.text)
            expect(details).to.equal('PING')
            expect(status).to.equal(200)
        } catch (error) {
            throw error
        }   
    })

})
`

[lambertkevin]

So just to make clear what the problem is, as some are saying you shouldn't need to use done and async, here is an example where you would want to use both.

it('should error on fn', async function(done) {
  try {
    await fn();
  } catch (e) {
    // assert some things here
    done()
  }
});

Not using done will let the test pass if no error is thrown. Some have suggested using things like expect(await fn).to.throw('blah'), but sometimes you need to check more properties than are going to fit into a one-liner.

====

Has a solution to this use case been found ?

[tom-james-watson]

Yeah - it's not pretty, but you can wrap your code in an async closure:

it("should error on fn", function (done) {
  (async () => {
    try {
      await fn();
    } catch (e) {
      // assert some things here
      done();
    }
  })();
});

[lambertkevin]

Thanks for the answer @tom-james-watson !

Well, as you said, that's not pretty... I ended up doing this instead, not super pretty either, but a little bit clearer to me:

it("should error on fn", () => {
   try {
      await fn();
      assert.fail('Promise should have been rejected');
   } catch (e) {
      if (e instanceof AssertionError) {
         throw e;
      }
      // assert some things here
   }
});

It felt a little less confusing and a little more readable to me, but it's still pretty verbose, and it's a pain to add this every time...
I guess you could have this in just 2 lines if your eslint/prettier config allows it:

it("should error on fn", () => {
   try {
      await fn();
      assert.fail('Promise should have been rejected');
   } catch (e) {
      if (e instanceof AssertionError) throw e;
      // assert some things here
   }
});

[tom-james-watson]

For that specific case yeah. Or you can just directly assert that the call throws. There are still some cases where you do need a mix of await and callbacks that may require the use of done, though.

[ktalebian]

This is defn a bug on mocha that should be fixed. As others have pointed out, using async (done) is a common practice for checking negative cases of your code.

This is what I've done in the meantime:

it('hack for async done', async () => {
  try {
    await someFunction();
    assert.isFalse(true);  // This ensures we never actually get to here
  } catch (e) {
    // my assertion here
  }
});

[jospueyo]

For anyone else who was driven crazy by this...
This works:

it("should work", async () => {
  await something;
});

This does not:

it("should work", async (done) => {
  await something;
});

You must remove done as a parameter.

@tamoyal You saved my life <3

@maitrungduc1410 I'm using this pattern, but then the test never stops running. What I'm doing wrong?

        it('Returns error when token is not correct', async () => {
            let result = await addSciStudy({token: 'fake'})
            console.log(result)
            expect(result).to.have.key('error')
        });

// { error: 'Token is not correct, please contact the administrator' }

I'm using the same pattern with the same function in a previous test. And there stops when the test is done.

Here is the entire file (line 12): https://github.com/icra/snappAPI-v2/blob/main/tests/test-add-sci-study.js
And here the async function: https://github.com/icra/snappAPI-v2/blob/main/lib/add-sci-study.js

Thank you.

[tandrewnichols]

That test looks fine; I'd guess something else is wrong. But this thread is not where that debugging should happen.

(Used the wrong account the first time, sorry for the noise everyone).