set allowUncaught on hooks

[givanse]

Requirements

Description of the Change

When passing --allow-uncaught, uncaught exceptions thrown inside hooks are not surfaced. This PR enables that. Original issue: #2302

Alternate Designs

The hook-runner is the best place to set this flag, since it has to be read from the parent context (Runner). I can't see any other way of doing given that strong dependency.

Why should this be in core?

It is a missing feature, already marked as a confirmed bug (#2302 )

Benefits

Tests authors can be more strict about not accepting code that throws errors as side effects. Debugging failing hooks becomes easier.

Possible Drawbacks

There shouldn't be drawbacks. Tests that throw errors within their hooks are most likely not setting allowUncaught, otherwise the browser would hang. So it remains an opt-in feature.

Applicable issues

closes #2302 , patch release

[coveralls]

Coverage increased (+0.04%) to 91.722% when pulling de9cc78 on givanse:issue-2302 into df1937b on mochajs:master.

[boneskull]

This LGTM, thanks. I just had that question about the test.

[boneskull]

I'm inclined to merge this as-is unless @givanse is interested in doing further work here (see my comment on the assertions).

@givanse, please let me know if you would like to do this, or you are finished. There's also a conflict that needs resolving, which might mean we don't have to stub Runner.immediately here.

[givanse]

Before you merge I'll resolve the conflicts and address the feedback on the assertions.

[givanse]

@boneskull I resolved the conflict and took another approach, instead of immediately now I'm using the hook event. If you are ok with these changes, then this is ready to merge.

[boneskull]

@givanse Looks like this is failing some tests

[juergba]

@givanse I will review within the next few days, I haven't understood this PR completely, yet.

[juergba]

describe('suite', function() {
    beforeEach('beforeEach', function() {
        throw new Error('uncaught error');
    })
    it('test', function() {
        console.log('test runs');
    });
});
describe('suite2', function() {    
    it('test2', function() {
        console.log('test runs');
    });
});

I would have expected a different result, when run with

• --no-allow-uncaught (default)
• --allow-uncaught

Both ways the second describe() does run and the result is:

suite
    1) "before each" hook: beforeEach for "test"

  suite2
test runs
    √ test2

  1 passing (11ms)
  1 failing

  1) suite
       "before each" hook: beforeEach for "test":
     Error: uncaught error     // Uncaught Error

The error type changes from Error to Uncaught Error though.
What is the sense of allowing uncaught exceptions to propagate? Is there an uncaughtException event emitted?

[givanse]

Error: uncaught error vs Uncaught Error: uncaught error

The difference is that Error is just an entry in the log. Mocha captures the error and logs it. An Uncaught Error is an actual JS error that can stop JS execution, this is useful for:

• Testem bail_on_uncaught_error
• Chrome dev tools Pause on exceptions

You can verify this by executing your code snippet with the flag --inspect-brk.

[juergba]

@givanse Thanks for this PR! It does work, however with the tests I'm not really happy. But I admit I haven't found a better solution so far. I tried with catching the uncaughtException event, then patching Runner.prototype.uncaught()... all just weird solutions.

I requested some small changes and will merge after that. If you don't agree, please say so, you seem to know what you are doing.