Fix for async hook error handling issue

[ajaks328]

• The issue causes sibling suites too fail when there is an async error thrown in the hooks of one suite
• Added unit test cases for async hook error handling
• Fixed minor issue in existing sync hook error unit test case

Detail:
Hook failures are handled in the same manner as outlines in this PR - #1043

This fix simply ensures that uncaught aysnc errors are being handled appropriately rather than bailing.

This fix should also resolve issue - #534

[ajaks328]

This PR is in relation to this filed issue - #1769

[danielstjules]

Hm, not your fault, but the pattern used for integration tests in https://github.com/ajaykodali/mocha/blob/fix/1769-asynchronous-error-handling/test/integration/hook.err.js is resulting in some pretty verbose tests. It's hard to follow what's being tested. Perhaps some of this could get moved into fixtures?

[ajaks328]

Are you recommending it for this PR or general observation? I ask cause I am not entirely familiar with the testing setup (and a lot of the rest of the code :)) but if it needs to be addressed in this PR I will give it a try. Some background on the intent of fixtures etc. would probably help.

[ajaks328]

@danielstjules, seemed straight forward so I moved some of the code to 'fixtures' to make this hook.err.js file more readable.

[danielstjules]

Awesome! Thank you :) Would you mind squashing?

Also, is there maybe some unnecessary code in some of the fixtures? Could they be slimmed down a bit? For example, couldn't afterEach.hook.async.error.js be simplified? Since it's testing afterEach (and not the other hooks), and some of the nesting of suites is redundant:

describe('spec 1', function () {
  describe('spec 1 nested', function () {
    it('should not be called, because hook error will happen in parent suite', function () {
      console.log('test nested');
    });
  });
  beforeEach(function () {
    console.log('before');
  });
  afterEach(function (done) {
    console.log('after');
    process.nextTick(function () {
      throw new Error('after each hook error');
    });
  });
  it('should be called because error is in after each hook', function () {
    console.log('test');
  });
});
describe('spec 2', function () {
  before(function () {
    console.log('before 2');
  });
  after(function () {
    console.log('after 2');
  });
  it('should be called, because hook error was in a sibling suite', function () {
    console.log('test 2');
  });
});

// =>

describe('spec 1', function () {
  afterEach(function (done) {
    console.log('afterEach');
    process.nextTick(function () {
      throw new Error('after each hook error');
    });
  });
  it('should be called because error is in after each hook', function () {
    console.log('test');
  });
  it('should not be called', function () {
    console.log('test nested');
  });
});
describe('spec 2', function () {
  it('should be called, because hook error was in a sibling suite', function () {
    console.log('test 2');
  });
});

If you agree, but don't have time, I'd be happy to make those changes. Either way, really appreciate your help with this!

[ajaks328]

Yeah, I agree. I can work on this later today, will squash and update.

[ajaks328]

Done. I trimmed the fixture code for the hook errors and squashed the commits. Note: I did not touch the multiple.hook.error.js and multiple.hook.async.error.js files and that is intentional. This fixture code does some pretty thorough usage of the different hooks and errors thrown at the different layers which seems like a good idea.

Let me know what you think.

[danielstjules]

does some pretty thorough usage of the different hooks and errors thrown at the different layers which seems like a good idea

Agreed :)

Done. I trimmed the fixture code for the hook errors and squashed the commits.

Oh, sorry, could you try squashing again? I'm still seeing 4 commits.

Also, thanks a lot for this PR. Looks solid! :) That test coverage will be extremely helpful in avoiding any future regressions.

[ajaks328]

Ok, think I did the squashing right, second time around.

[danielstjules]

Awesome, thanks again!

@mochajs/mocha

[ajaks328]

Not a problem :). This looks like an issue that more than a few people are waiting on.

[ajaks328]

@danielstjules , any estimate on when this might be merged?

[danielstjules]

@ajaykodali You've got a +1 from me! Just hoping to get a second from the team before hitting the merge button, since it is touching core runner logic. Sorry for the wait!

[ajaks328]

Makes sense, thanks!

[danielstjules]

Some small nitpicks that I'll fix in the merge if you don't have time:
Renaming afterAllHookCalledOnce => afterAllCalled or afterAllHookCalled

[ajaks328]

done

[ajaks328]

Addressed the feedback changes

[boneskull]

Is there a better way to detect an afterEach than examining fullTitle?

[danielstjules]

As of right now, it doesn't look like it

{
  "title": "\"after each\" hook",
  "async": 1,
  "sync": false,
  "timedOut": false,
  "type": "hook",
  "parent": "#<Suite>",
  "ctx": "#<Context>",
  "timer": {
    "0": null
  },
  "state": "failed",
  "err": {
    "uncaught": true
  }
}

At least not from the uncaught method of the runner. You could probably inspect the suite's _afterEach or _beforeEach, but that'd be messier. Maybe a property on these hooks (a type) could be introduced as part of a later PR?

[ajaks328]

Yeah, I agree. I couldn't figure out a better way to handle this within the current implementation.

[danielstjules]

@mochajs/mocha @boneskull +1 from anyone else :) While the PR is large, the change to mocha is 36 loc. The rest are some awesome tests verifying its functionality.

[ajaks328]

Any update on this?

[dasilvacontin]

I'll try reviewing this tomorrow.

[danielstjules]

For reference, here's an example of what this fixes. Note that the third spec isn't invoked prior to this PR.

$ cat test/integration/fixtures/hooks/afterEach.hook.async.error.js
describe('spec 1', function () {
  afterEach(function (done) {
    console.log('after');
    process.nextTick(function () {
      throw new Error('after each hook error');
    });
  });
  it('should be called because error is in after each hook', function () {
    console.log('test 1');
  });
  it('should not be called', function () {
    console.log('test 2');
  });
});
describe('spec 2', function () {
  it('should be called, because hook error was in a sibling suite', function () {
    console.log('test 3');
  });
});

// --------------------------
// BEFORE PR
// --------------------------

$ ./bin/mocha test/integration/fixtures/hooks/afterEach.hook.async.error.js

test 1

  ․after
․

  1 passing (12ms)
  1 failing

  1) spec 1 "after each" hook:
     Uncaught Error: after each hook error
      at test/integration/fixtures/hooks/afterEach.hook.async.error.js:5:13

// --------------------------
// AFTER PR
// --------------------------

$ ./bin/mocha test/integration/fixtures/hooks/afterEach.hook.async.error.js

test 1

  ․after
․test 3
․

  2 passing (13ms)
  1 failing

  1) spec 1 "after each" hook:
     Uncaught Error: after each hook error
      at test/integration/fixtures/hooks/afterEach.hook.async.error.js:5:13

[danielstjules]

@mochajs/mocha Unless anyone objects, I'm hoping to merge this weekend.

[boneskull]

I don't object. What I'm wondering is whether we plan on a minor or major at this point.

[boneskull]

cc @mochajs/mocha

[travisjeffery]

what warrants a major upgrade?

[danielstjules]

minor or major at this point.

Did you mean minor vs patch? I think master has enough functionality and changes to warrant a minor (as does the 2.3.0 branch) So maybe we could consolidate the changes in those two branches and release a new minor?

If you did mean minor/major, then I didn't think it was worth a new major yet? I thought the big goal for that was the plugin system.

[travisjeffery]

yeah i look at the commit list after i wrote that. looks like minor to me

[boneskull]

dunno, I heard some rumblings about trying for v3.0.0. please let me look at the commit list as well

[danielstjules]

No one objected, so merging in :) Thanks again @ajaykodali - Awesome work, really appreciate it!

[ajaks328]

Awesome! Thanks for pulling this in. Is there a decision made on whether this will come out soon as a patch or in a major version bump?

[danielstjules]

Not yet, will likely move that discussion to another issue :)

[ajaks328]

ok, can you point me to that discussion so I can keep track as to when this might come out.

[jugglinmike]

I just now ran into this bug, so I'm excited to see it's already been fixed in master. Thanks, all! (Also, gh-1769 can now be closed)

[danielstjules]

Thanks for the reminder! :)

[glenjamin]

Just stumbled across these changes, and wanted to mention that the choice to have the tests included in the file instead of in a fixture was a conscious one, discussed in #1781 (comment)

Personally I find the fixture approach in this case to be rather opaque, as you have to flick between the fixture file and the test itself to see why the assertion expects ['before', 'test 3'].

Anyway, not a huge issue either way - but I wanted to explain why they seemed so weird before 😄

[danielstjules]

Personally I find the fixture approach in this case to be rather opaque, as you have to flick between the fixture file and the test itself to see why the assertion expects ['before', 'test 3'].

You may have to click back and forth, but I prefer the distinction. Skimming through the original tests, it was difficult to tell at a glance what was an actual integration test, and what was an "inlined fixture" to be ran in a spawned process. E.g. https://github.com/glenjamin/mocha/blob/092ea4684478aa99e32f3acaa0e5963ee5c0cb7d/test/integration/hook.err.js#L166-L332

But I do appreciate your work in adding those tests in the first place - they've been really helpful! :)

[glenjamin]

Skimming through the original tests, it was difficult to tell at a glance what was an actual integration test, and what was an "inlined fixture" to be ran in a spawned process.

Fair point - the fixtures did look exactly like the tests themselves!