allowUncaught in 2.3.0 doesn't work for async errors

[kevinbarabash]

Runner.prototype.run adds on a window.onerror handler when it calls

process.on('uncaughtException', uncaught);

This causes tests that throw async errors to fail even if I add my own window.onerror handler. To fix this it should check the value of this.allowUncaught first, e.g.

if (!this.allowUncaught) {
   process.on('uncaughtException', uncaught);
}

I made this change and the tests I was writing passed. Would you be willing to accept a pull request for this change?

[danielstjules]

Hi @KevinB7, a PR would be great! :) An additional spec in test/runner.js, like those in #1662, would also be greatly appreciated!

[kevinbarabash]

It looks like this is going to be a little harder to get right. I'm using expect.js which throws an exception if an expect fails. I was thinking that maybe there could be a customizable filter that will rethrow errors in uncaught if filter's predicate returns true.

[lolmaus]

I've been writing my tests like this:

it('foo', function (done) {
  visit('/bar');

  andThen(() => {
    expect(baz).equal(quux);
    done();
  });
});

andThen is my framework's helper that is aware of all promises within the app and executes its callback once all promises have completed.

Then I switched to async/await for readability:

it('foo', async function (done) {
  await visit('/bar');
  expect(baz).equal(quux);
  done();
});

When the assertion fails, I receive an "Unhandled promise rejection" error in browser console. To avoid it, I had to do this:

it('foo', async function (done) {
  try {
    await visit('/bar');
    expect(baz).equal(quux);
    done();
  } catch (e) {
    done(e);
  }
});

I hate try/catch, but the try block is still more readable than with andThen.

It works now, except that allowUncaught won't work because I'm catching the error.

To resolve this, done(e) should rethrow e when allowUncaught is enabled.

[lolmaus]

This monkey patch is a proof of concept:

    Mocha.Runnable.prototype.run = function (fn) {
      var self = this;
      var start = new Date();
      var ctx = this.ctx;
      var finished;
      var emitted;

      // Sometimes the ctx exists, but it is not runnable
      if (ctx && ctx.runnable) {
        ctx.runnable(this);
      }

      // called multiple times
      function multiple(err) {
        if (emitted) {
          return;
        }
        emitted = true;
        self.emit('error', err || new Error('done() called multiple times; stacktrace may be inaccurate'));
      }

      // finished
      function done(err) {
        var ms = self.timeout();
        if (self.timedOut) {
          return;
        }
        if (finished) {
          return multiple(err || self._trace);
        }

        self.clearTimeout();
        self.duration = new Date() - start;
        finished = true;
        if (!err && self.duration > ms && self._enableTimeouts) {
          err = new Error('timeout of ' + ms + 'ms exceeded. Ensure the done() callback is being called in this test.');
        }
        fn(err);
      }

      // for .resetTimeout()
      this.callback = done;

      // explicit async with `done` argument
      if (this.async) {
        this.resetTimeout();

        if (this.allowUncaught) {
          return callFnAsync(this.fn);
        }
        try {
          callFnAsync(this.fn);
        } catch (err) {
          done(Mocha.utils.getError(err));
        }
        return;
      }

      if (this.allowUncaught) {
        callFn(this.fn);
        done();
        return;
      }

      // sync or promise-returning
      try {
        if (this.pending) {
          done();
        } else {
          callFn(this.fn);
        }
      } catch (err) {
        done(Mocha.utils.getError(err));
      }

      function callFn(fn) {
        var result = fn.call(ctx);
        if (result && typeof result.then === 'function') {
          self.resetTimeout();
          result
            .then(function () {
                done();
                // Return null so libraries like bluebird do not warn about
                // subsequently constructed Promises.
                return null;
              },
              function (reason) {
                done(reason || new Error('Promise rejected with no or falsy reason'));
              });
        } else {
          if (self.asyncOnly) {
            return done(new Error('--async-only option in use without declaring `done()` or returning a promise'));
          }

          done();
        }
      }

      function callFnAsync(fn) {
        fn.call(ctx, function (err) {
          if (err instanceof Error || Object.prototype.toString.call(err) === '[object Error]') {
            if (mocha.options.allowUncaught) {
              throw(err);
            }
            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));
          }
          done();
        });
      }
    };

Too bad we can't override callFnAsync directly. :(

[boneskull]

Please see if 5e1cd44 addresses this issue.

[lolmaus]

@boneskull I've just tried it and it does help, but there's a quirk.

I have a test like this:

  it('works', function() {
    const foo = {}
    foo.bar()
  });

It fails with "TypeError: foo.bar is not a function".

When I enable allowUncaught, the test disappears from the list of tests, and I get a false positive result.

But if I run the test directly (via the grep query param in my HTML reporter), I do see the test failing and I do see the error in the console.

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[boneskull]

not stale

[boneskull]

This has since been fixed