uncaughtException after runner's end: improve exit code

[juergba]

Description

This PR forked various PR's concerning uncaught exceptions:

• related PR: uncaughtException: fix double EVENT_RUN_END events #4025 uncaughtException: fix "--allow-uncaught" with "this.skip()" #4030 uncaughtException: report more than one exception per test #4033 uncaughtException: fix when current test is pending #4083 uncaughtException: fix recovery when current test is still running #4150
• regression: Fix leaking global 'uncaughtException' handler #4147

Description of the Change

• refactor and move existing tests
• add one additional test

Applicable issues

closes #3917

[juergba]

@medikoo I would highly appreciate your feedback to this draft PR, thank you!

[coveralls]

Coverage remained the same at 92.857% when pulling 147f763 on juergba/allowUncaught into b431609 on master.

[medikoo]

Great thanks @juergba for initiative. Having that fixed in Mocha will be really helpfiul.

Concerning #3917 I commented (it fixes issue but only for Node.js v10+, not sure if it's intended)

Concerning #3938 it fixes issue but only when--allowUncaught is set. If I see the complete fix would be to do console.error('\n', err); process.exit(1); also when runnable.isFailed() || runnable.isPending() is met here:

mocha/lib/runner.js

Line 830 in 6ec07d6

return;

[juergba]

Concerning #3938 it fixes issue but only when --allowUncaught is set.

TBH I don't really want to touch this recovery feature when --allowUncaught: false. I did some testing and the behavior is buggy and unpredictable. There are too many end events and the runner keeps going after the first one. I will have a deeper look at this topic though.
=> see #4025

For your needs is there anything opposing against using --allowUncaught: true and not using the recovery feature?

[juergba]

I have to investigate further about the dependencies of --allow-uncaught and --exit. While combining both options the outcome of my tests is suboptimal.

[juergba]

The more I'm testing this, the more weird behavior pops up.

describe('test', () => {
  it('test1', function() { });
  it('test2', function () {
    this.skip();
  });  
  it('test3', function () { });  
  it('test4', function () {
    this.skip();
  });
});

Running with node bin\mocha --allow-uncaught test.js:

test
    √ test1

There is no (uncaught) exception, but all this.skip() tests plus the test summary are truncated.
see => #4030

[juergba]

@medikoo above bug (this.skip() plus --allow-uncaught) is rather complex and I will need more time to solve this problem. It's mainly due to our implementation of (async) this.skip() by throwing an uncaught exception.

Node's docu states:

Attempting to resume normally after an uncaught exception can be similar to pulling out of the power cord when upgrading a computer — nine out of ten times nothing happens - but the 10th time, the system becomes corrupted.

Do you have some time left to comment this?

[medikoo]

Node's docu states

In my understanding, it's just to indicate that we cannot be sure of side effects/broken states that come with uncaught exceptions, and therefore we should not try to recover from them. Otherwise if we know what we're doing (e.g. recovering from plain throw new Error('Example')) it can be safe.

Still, I totally I agree, that designing this.skip() to work through uncaughtException channel is very controversial.
I assume it was decided back in a past, when async tests were not necessarily written with promises, and were settled via done callback.
Then idea was that any this.skip() between start of a test, and done callback invocation, should be caught and recognized, so uncaughtException seemed the only way to cover that.

Maybe it'll be good with a new major, to narrow async support just to promises, and that way errors handling could be simplified (?)

When sync tests and async tests resolved via promises are concerned, I believe that any uncaughtException should be treated as an error that should be reported (even one thrown by this.skip())) as it's a clear signal of orphaned async flow, which is an error on its own.

With how current implemention works, I would definitely not ignore uncaught exceptions that follows ones thrown via this.skip(). Fact that they're ignored now is source of #3938

[juergba]

@medikoo thank you.

Then idea was that any this.skip() between start of a test, and done callback invocation, should be caught and recognized, so uncaughtException seemed the only way to cover that.

it('test', function (done) {
      var self = this;
      setTimeout(function () {
        instruction1;
        self.skip();       // includes implicit 'done()' and throws a 'Pending' error
        instruction2;     // does not run
        instruction3;     // does not run
      }, 100);
    });

The idea was to abort an asyncly running callback and not to execute instruction2 and instruction3.

The user could easily get the same result by using if skip/else and we wouldn't need to throw this Pending error. We would just have to label the test as pending.

[medikoo]

I think fact that this.skip() throws is desirable, questionable is how it's caught :)

But in async tests reported via done, I believe the uncaughtException channel is the only way to go.

[juergba]

@medikoo thank you for your support!