🐛 Bug: Test stops recognizing exceptions once done has been called

[feckertson]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

Mocha stops recognizing exceptions once done has been called.

Steps to Reproduce

Find a complete example in git, but an outline is given below.

Source

exports.handler = function eventHandler(callback) {
  callback(null, 'oops');
}

Test

const expect = require('chai').expect;
const subject = require('../mocha-done-issue.js');

it('problematic test', (done) => {
  subject.handler((e, d) => {
    done();
    expect(true).to.be.false;
  });
});

Expected behavior: We expect the test to fail because of the failed expect assertion.

Actual behavior: The test passes, but if the two lines are reversed the test fails.

Reproduces how often: 100%

Versions

Multiple including mocha 5.2.0/7.1.0, node 8.10.0/10.16.0 (64-bit) on Windows cmd and Linux bash.

Additional Information

This came to our attention when faulty code similar to the following was submitted for review. Mocha failed to indicate there was a problem with the code even though Stryker reported 100% mutation coverage.

The expect fails during the extraneous call to callback since the statusCode is 200. Consequently no duplicate call to done occurs. If it weren't for the assert failure mocha would catch the duplicate call to done.

exports.handler = function eventHandler(condition, callback) {
    if ( ! condition ) {
        callback(null, {statusCode: 422, body: 'missing required value'});
    }
    callback(null, {statusCode: 200, body: 'OK'});
}

it('test false condition', (done) => {
    subject.handler(false, (e, d) => {
        expect(d.statusCode).to.equal(422);
        done();
      });
});

[craigtaub]

Yes that seems like a bug. Can replicate via

done();
equal(true, false);
done();

It should throw a failure but it passes, not good. Guessing initial done flags as passing which cant be updated or something. Needs more investigating.

[boneskull]

@craigtaub Your example is not a problem. writing that JS another way:

done();
throw new Error();
done();

You wouldn't expect the second done() to be called 😄

@feckertson Can you show that your code does not do something like the above? If there's an exception thrown before the second call to done(), there's really nothing we can do about it, since there won't be a second call to done().

Regardless: if Mocha is eating errors, please provide a minimal, reproducible case.

[feckertson]

@boneskull
If "assert fails" = "exception thrown", then no, but....

Isn't the point of a unit testing framework to help code authors discover coding mistakes?
In that sense, mocha is not doing the best possible job.

My simple example at the beginning has a an assert after a call to done which is one type of coding mistake, Currently, such a mistake can cause a test to pass for the wrong reason.

I gave the actual coding error which led to this issue under the "Additional Information" heading. One path through that code leads to a duplicate call to the callback function. There is a test that ought to expose that mistake by either failing an assert or by generating a duplicate call to done error, but neither error occurs.

It seems to me like an error should occur if any code is executed by a test after done is called.

[boneskull]

I misunderstood @craigtaub; I thought he was saying that the code doesn't throw a "multiple calls to done()" exception when it should.

That should throw. This is the circumstance where done is provided, but the code neither returns a Promise nor calls done asynchronously. This code, OTOH, will error as expected:

it('should fail', function(done) {
  setTimeout(() => {
    done();
    throw new Error();
  });
});

I haven't had time to bisect, but it may be a regression introduced by a fix for #4030.

Isn't the point of a unit testing framework to help code authors discover coding mistakes?
In that sense, mocha is not doing the best possible job.

Uh... have you ever written a bug? Please keep the entitlement out of our issue tracker; we don't work for you.

[boneskull]

cc @juergba; the code here was most recently changed when fixing an issue with this.skip()

[juergba]

Versions
Multiple including mocha 5.2.0/7.1.0, node 8.10.0/10.16.0 (64-bit) on Windows cmd and Linux bash.

#4030 was released in v7.0.0, so probably this is an older issue. I will have a look.

[juergba]

const assert = require('assert');
it('problematic test', function(done) {
    done();
    console.log('after done');
    assert(false);
});

Above test does not run correctly neither in Mocha v5.2.0, nor v7.2.0, nor in v8.0.1.
It's an async test with explicit call to done, but the function body is running synchronously.

Similar test cases with real async functions are handled by Mocha's uncaughtException listener and work correctly.

[JoshuaKGoldberg]

Confirmed that that last code snippet (#4202 (comment)) doesn't fail as expected in Mocha 10.2.0:

const assert = require("assert");

it("problematic test", function (done) {
  done();
  console.log("after done");
  assert(false);
});

$ npm run test

> example-browser@1.0.0 test
> mocha test.spec.js



  ✔ problematic test
after done

  1 passing (5ms)

...while the earlier setTimeout snippet (#4202 (comment)) does fail as expected:

it("should fail", function (done) {
  setTimeout(() => {
    done();
    throw new Error();
  });
});

$ npm run test

> example-browser@1.0.0 test
> mocha test.spec.js



  ✔ should fail
  1) should fail

  1 passing (3ms)
  1 failing

  1) should fail:
     Uncaught 
  Error
      at Timeout._onTimeout (test.spec.js:4:11)
      at listOnTimeout (node:internal/timers:573:17)
      at process.processTimers (node:internal/timers:514:7)

Fun!