Uncaught exceptions are silenced if happen in timeframe of skipped test

[medikoo]

Possibly related to #3740

Steps to Reproduce

describe('test', () => {
  it('test1', () => {
    setTimeout(() => {
      throw new Error('Uncaught');
    }, 3);
  });
  it('test2', function () {
    this.skip();
  });
  it('test3', function () {
    this.skip();
  });
  it('test4', function () {
    this.skip();
  });
});

Expected behavior:

Tests should fail unconditionally

Actual behavior:

In most cases suite passes (there is a race condition,due to which occasionally error gets exposed)

Versions

mocha: 6.1.4
Node.js: 12.3.1
macOS: 10.14.5

[plroebuck]

Test1 should pass, though the error may get caught outside the test itself (depending on timing).
Recode as async test (using done or Promise) if you want to catch Error from setTimeout.

All other tests should be marked pending regardless.

[medikoo]

It's a question of how mocha handles the case where user misconfigured the test so it results with unhandled exception.

I believe the goal is to expose such errors unconditionally, in this case it doesn't happen, therefore it's a bug.

[plroebuck]

Which part of my reply are you implying Mocha does incorrectly here -- Test1 or the other three?

Your "Expected Behavior" is hard to tell what you expect -- did you mean "all" tests or only the attempted fail of first one?

[medikoo]

Which part of my reply are you implying Mocha does incorrectly here

Incorrect is assumption that developers do not make mistakes.

In discussed case, test by mistake was configured that it ends before an actual job it started ends. The left over job additionally raises an uncaught exception (currently silenced by Mocha, therefore developer is left with no clue that tests setup is broken)

I believe Test runner should not silence uncaught exceptions, no matter in what context they exploded, whether it was failed test, pending test, or maybe after all tests were registered as ended.

It general it goes down to rule: unhandled errors should not be silenced

[juergba]

Which is the result when you use --allow-uncaught option?

[plroebuck]

Fail to understand why you continue to create new issues over the same problem though; you're creating synchronous Mocha tests which use asynchronous functions.
We went through all this in #3917.

If you want it fixed, send a PR.

[medikoo]

We went through all this in #3917.

No, it's a different issue.

All things I report are actually issues I discover when working with quite extensive Mocha setup in real world project (currently it comes with four Mocha bug patches, so it behaves reasonably)

[plroebuck]

Seems like another aspect of same problem -- an uncaught error occurs in asynchronous code run from a synchronous Mocha test.

Notwithstanding whether Mocha is lacking in this department, some chunk of the solution for you would be to write your tests using Mocha's asynchronous API instead...

[medikoo]

Which is the result when you use --allow-uncaught option?

In this case it ends with output as:

  test
    ✓ test1
    1) test2

Which indicates an error in test2, but provides no details.

And process exists with 0 success code.

So it's also an invalid outcome

[wnghdcjfe]

Can you give me a more detailed explanation?
I tested

describe('test', () => {
    it('test1', () => {
      setTimeout(() => {
        throw new Error('Uncaught');
      }, 3);
    });
    it('test2', function () {
        console.log(2)
      this.skip();
    });
    it('test3', function () {
        console.log(3)
      this.skip();
    });
    it('test4', function () {
        console.log(4)
      this.skip();
    });
  });

and result

 test
    ✓ test1
2
    - test2
3
    - test3
4
    - test4


  1 passing (7ms)
  3 pending

/Users/juhongchul/Desktop/dev/test/a.js:4
        throw new Error('Uncaught');
        ^

Error: Uncaught
    at Timeout.setTimeout [as _onTimeout] (/Users/juhongchul/Desktop/dev/test/a.js:4:15)
    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)

is very clear, 1 passing because move to event loop and other things pending
you think 2, 3, 4, and 1 failed?

[medikoo]

@wnghdcjfe run it few times, you'll see that in some cases test suite ends with success, not reporting the error.

It happens only when race condition (throw new Error('Uncaught'); being caught between this.skip() and following it run) occurs.
Also the more further tests going, the more likely issue is to occur, try e.g.:

describe("test", () => {
    it("test1", () => {
        setTimeout(() => {
            throw new Error("Uncaught");
        }, 3);
    });
    it("test2", function() {
        this.skip();
    });
    it("test3", function() {
        this.skip();
    });
    it("test4", function() {
        this.skip();
    });
    it("test5", function() {
        this.skip();
    });
    it("test6", function() {
        this.skip();
    });
    it("test7", function() {
        this.skip();
    });
    it("test8", function() {
        this.skip();
    });
    it("test9", function() {
        this.skip();
    });
    it("test10", function() {
        this.skip();
    });
    it("test11", function() {
        this.skip();
    });
});