Crashes that happen after test completion are reported with success exit code

[medikoo]

Description

Sort of follow up to #3226 which was fixed but it seems not fully.

Steps to Reproduce

1. Install locall mocha v6.1.4

2. Createtest.js test file:

describe('test', () => {
  it('test', () => {
    setTimeout(() => {
      throw new Error('Unexpected crash');
    }, 10);
  });
});

2. Run npx mocha test.js

Output is valid:

$ npx mocha test.js

  test
    ✓ test


  1 passing (5ms)

/Users/medikoo/npm-packages/serverless/test.js:6
      throw new Error('Unexpected crash');
      ^

Error: Unexpected crash
    at Timeout._onTimeout (/Users/medikoo/npm-packages/serverless/test.js:6:13)
    at listOnTimeout (internal/timers.js:531:17)
    at processTimers (internal/timers.js:475:7)

But exit code is not:

$ echo $?
0

This makes such crashes not detected as fails in CI

Versions

• The output of mocha --version and node node_modules/.bin/mocha --version: v6.1.4
• The output of node --version: v12.2.0
• Your operating system:
  • name and version: macOS-10.14.4
  • architecture (32 or 64-bit): 64-bit

[plroebuck]

Quite possible Mocha was in process of terminating by time your test threw.
Not sure what could be done in such case; what is your production scenario?

[medikoo]

Not sure what could be done in such case

Process should certainly exit with other code than 0.

Problem is that this scenario ends as success in CI, it shouldn't be the case.

[plroebuck]

Not sure what could be done in such case

Process should certainly exit with other code than 0.
Problem is that this scenario ends as success in CI, it shouldn't be the case.

It's not like I don't understand the issue here -- I'm saying it's a race condition... one that you lose!

Modify this function from "lib/cli/run-helpers.js" as such and rerun your test.

const exitMochaLater = code => {
  process.on('exit', () => {
    process.exitCode = Math.min(code, 255);
    console.log('exitCode set');
  });
};

Mocha finishes execution prior to your timeout, so the exit code is already set.

$ npm test

> testfailafter@1.0.0 test /var/tmp/testfailafter
> mocha

  test
    ✓ test

  1 passing (8ms)

exitCode set
/var/tmp/testfailafter/test.js:4
      throw new Error('Unexpected crash');
      ^

Error: Unexpected crash
    at Timeout.setTimeout [as _onTimeout] (/var/tmp/testfailafter/test.js:4:13)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)
$ 

[plroebuck]

My suggestion would be to migrate your specification to "test" directory and
add an async delay test in a file named "test/zzz.spec.js".

$ mkdir test
$ mv test.js test/my.spec.js
$ cat << EOF >> "test/zzz.spec.js"
describe('delay in case of async test failures', function() {
  const delay = 5000;  // 5 secs
  it('should delay Mocha exit in case of poorly written tests', function(done) {
    setTimeout(done, delay);
  });
});
EOF
$ npm test

Mocha will now implode when run, and its exitCode will be 1.

[medikoo]

@plroebuck This is about how specific programmer errors (of which developer is not aware of) are handled by Mocha (and not about tests written specifically like that for some reason).
So your suggestion with "test/zzz.spec.js" doesn't apply here.

Such error may happen due to missed to be returned promise that in later turn rejects (with unhandledRejection listener, configured to be thrown - a setup that many mocha users have, which we may see from discussions at #2640)

Currently Mocha hides such error from CI, and IMO It's a clear bug on Mocha side.

To me it seems that can be easily fixed by reconfiguring the exitMochaLater into

const exitMochaLater = code => {
  if (!code) process.on('uncaughtException', () => code = 1);
  process.on('exit', () => {
    process.exitCode = Math.min(code, 255);
  });
};

After that, process ends with non success code, which is highly desirable in such case.

---

Currently I workaround this issue with:

// Workaround for: https://github.com/mochajs/mocha/issues/3917
process.on('uncaughtException', err => {
  if (!process.listenerCount('exit')) return;
  // Mocha done it's report, and registered exit code which is not updated in face of a crash
  // Ensure we exit with non-success code
  process.removeAllListeners('exit');
  throw err;
});

[plroebuck]

You rather radically changed the subject at hand from deferred async to uncaughtException.
My simplistic workaround was just that -- to add one final test with a delay which might allow unfinished deferred async functions time to fail.

Feel free to send PR your other issues.

[medikoo]

You rather radically changed the subject at hand from deferred async to uncaughtException.

Since beginning it's all about uncaught exception that happens after mocha wraps report (and that could be only the result of orphaned async execution).

It's exactly what my initial example exposes.

Feel free to send PR your other issues.

I will, thank you. In a meantime it'll be great if you reopen and relabel it, as I hope we agree now, it's a bug, that can easily be fixed.

[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!

[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!

[linearbci]

any update on this? if mocha has import error before running the tests, it exit with 0 exit code, our CI reported succses for 2 weeks before we realize something is broke