🐛 Bug: Tests pass without running after uncaught exception

[JoshuaKGoldberg]

Bug Report Checklist

• I have read and agree to Mocha's Code of Conduct and Contributing Guidelines
• I have searched for related issues and issues with the faq label, but none matched my issue.
• I have '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, my usage of Mocha, or Mocha itself.
• I want to provide a PR to resolve this

Expected

If an uncaught exception occurs, I would think Mocha should report a failure and continue running subsequent tests.

Actual

If a test throws an error inside a process.nextTick callback, Mocha correctly reports that test failing, but then doesn't run subsequent tests. It reports them as passed.

For example, in the MRE, there are three tests -a, b, and c- and b throws an error inside process.nextTick. The report is:

  a
    ✔ should pass

  b
    ✔ should pass, then fail
    1) should pass, then fail


  2 passing (3ms)
  1 failing

  1) b
       should pass, then fail:
     Uncaught Error: uncaught!!
      at /Users/josh/repos/repros/test/example-b.js:4:15
      at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Minimal, Reproducible Example

https://github.com/JoshuaKGoldberg/repros/tree/mocha-skip-test-after-uncaught

git clone https://github.com/JoshuaKGoldberg/repros -b mocha-skip-test-after-uncaught
npm i
npx mocha

Versions

• Mocha: 10.8.2
• Node: 18, 20, and 22

Additional Info

Discovered in #5250 -> 6b5ff68.

Somewhat similar-looking to #5184 but not the same.

[wnghdcjfe]

Hello@JoshuaKGoldberg
I think this error can be resolved in the following two ways.
Are you talking about the part where the fail should appear when an error in process.nextTick occurs?

1. try - catch

describe('b', function() {
  it('should pass, then fail', function(done) {
    process.nextTick(function() {
      try {
        throw new Error('uncaught!!');
      } catch (err) {
        done(err); // Mocha에 에러를 알림
      }
    });
  });
}); 

2. promise

describe('b', function() {
  it('should pass, then fail', function() {
    return new Promise((resolve, reject) => {
      process.nextTick(function() {
        reject(new Error('uncaught!!'));  
      });
    });
  });
});

[JoshuaKGoldberg]

I don't follow what you're asking, sorry. But if you can send a PR that would help me understand :)

[wnghdcjfe]

Okay. I'll PR it later.
First of all, I found that it occur "pass" in beforeEach hook.

If the code is error-handling well, it should appear as follows.

[wnghdcjfe]

Hello sorry for leaving a reply so late, I try to do it every weekend but it's not easy haha

In my opinion, the callback of process.nextTick() runs asynchronously, because there is no logic with the parameter "done"

For example
The next code will be error-handled normally.

    it('should pass, then fail', function(done) {
        process.nextTick(function() { 
            throw new Error('uncaught!!'); 
        });
      });

So, how should we solve this?
Here's how I'm currently thinking.

let's see this function.

Runable(title, fn) 

If the parameter 'done' is included, the fn.length becomes 1 and it is recognized as an asynchronous function.
Therefore, when "the nextTick " is included in fn, it would be better to check whether there is 'done' in it and add 'done' appropriately. Is it okay to proceed like this?

For example
runnable.js

  if (this.fn && typeof this.fn.call !== 'function') {
    done(
      new TypeError(
        'A runnable must be passed a function as its second argument.'
      )
    );
    return;
  }
   
  if (this.fn.toString().includes('process.nextTick') && fn.length === 0) {
    this.async = 1;
  } 

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

https://github.com/mochajs/mocha/blob/main/lib/runnable.js#L326

@JoshuaKGoldberg

[JoshuaKGoldberg]

That seems reasonable at first glance. But it's hard to say without looking at a PR. I say: go for it, let's try it out! 🚀

[alxndrsn]

Actual

If a test throws an error inside a process.nextTick callback, Mocha correctly reports that test failing, but then doesn't run subsequent tests. It reports them as passed.

I think this section, and the current issue title, are slightly mis-stating the problem: I don't think that subsequent tests are reported as passed, I think:

1. subsequent tests are not run at all, and
2. the failing test is reported as failing twice

A longer set of subsequent tests can demonstrate this more clearly, e.g. this longer suite will fail with an identical report to the original repro:

describe("a", function () {
  it("should pass", function () {
  });
});

describe('b', function() {
  it('should pass, then fail', function() {
    process.nextTick(function() {
      throw new Error('uncaught!!');
    });
  });
});

describe("c", function () {
  it("should fail 1", function () {
    throw new Error("Oh no 1!");
  });
  it("should fail 2", function () {
    throw new Error("Oh no 1!");
  });
  it("should fail 3", function () {
    throw new Error("Oh no 1!");
  });
  it("should fail 4", function () {
    throw new Error("Oh no 1!");
  });
  it("should fail 5", function () {
    throw new Error("Oh no 1!");
  });
  it("should fail 6", function () {
    throw new Error("Oh no 1!");
  });
});

[alxndrsn]

...although maybe there is a bigger issue?

Consider the following:

describe('unhandled async failure within a test', () => {
  it('should pass 1', () => {});

  it('should pass, then fail', () => {
    setTimeout(() => { throw new Error('uncaught!!'); }, 100);
  });

  it('should pass after some time 1', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 2', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 3', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 4', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 5', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 6', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 7', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 8', () => new Promise(resolve => setTimeout(resolve, 50)));
  it('should pass after some time 9', () => new Promise(resolve => setTimeout(resolve, 50)));
});

For me, this fails like so:

$ yarn mocha test.js 


  unhandled async failure within a test
    ✔ should pass 1
    ✔ should pass, then fail
    ✔ should pass after some time 1 (51ms)
    1) should pass after some time 2
    2) should pass after some time 2
    ✔ should pass after some time 3 (50ms)
    ✔ should pass after some time 4 (50ms)
    ✔ should pass after some time 5 (51ms)
    ✔ should pass after some time 6 (50ms)
    ✔ should pass after some time 7 (50ms)
    ✔ should pass after some time 8 (50ms)
    ✔ should pass after some time 9 (50ms)


  10 passing (461ms)
  2 failing

  1) unhandled async failure within a test
       should pass after some time 2:
     Uncaught Error: uncaught!!
      at Timeout._onTimeout (test.js:5:30)
      at listOnTimeout (node:internal/timers:569:17)
      at process.processTimers (node:internal/timers:512:7)

  2) unhandled async failure within a test
       should pass after some time 2:
     Error: done() called multiple times in test <unhandled async failure within a test should pass after some time 2> of file test.js

This failure looks incorrect and very misleading. It seems like mocha is ascribing the unhandled async failure to whatever test happens to be running at the time the async failure happens. Personally I'd expect that either

1. mocha just errors out the whole test suite if there's an unhandled promise rejection somewhere, and states that it doesn't know what's going on, or
2. somehow mocha keeps track of which test was running when a promise chain was initiated, so that it can point the user in the right direction when an unhandled rejection occurs

Even in case 2, short-circuiting the whole suite seems like a more helpful default than continuing, as whatever was happening in the background may be affecting subsequent tests.