Ensure all after hooks are executed, even if one of them fail #3281

johanblumenberg · 2018-03-12T14:39:54Z

Description of the Change

If you have tests that require some cleanup, for example to close open files, network connections, processes, or other things, you usually have an after() or afterEach() hook to do that.

describe('suite A', function () {
  afterEach(function () {
    cleanupItemX();
    cleanupItemY();
  });

  .
  .
  .
});

Now, if there is an exception from the first cleanup method, the second one will not be run.

This PR enables you to write cleanup code like this:

describe('suite A', function () {
  afterEach(function () {
    cleanupItemX();
  });

  afterEach(function () {
    cleanupItemY();
  });

  .
  .
  .
});

This will make sure that both hooks are always executed, and errors are reported properly if any of them fail.
Before this PR, the second after each hook was not executed if the first one throws an exception. But the fact that hooks in the parent suite are executed leads me to believe that this was simply overlooked, maybe because several after hooks in the same suite is not very common.

Alternate Designs

It would be possible to fix this by adding a bunch of try/finally blocks. However, there are some problems with that.

The code is cluttered with a bunch of try/finally blocks
Only one error is reported from the test suite. You want to log all errors that happen.

describe('suite A', function () {
  afterEach(function () {
    try {
      cleanupItemX();
    } finally {
      try {
        cleanupItemY();
      } finally {
        cleanupItemZ();
      }
    }
  });

  .
  .
  .
});

Why should this be in core?

Because there is an inconsistency in how after hooks work. Hooks in the same suite are not run if the first one fails, but hooks in parent suites are run. To be consistent, it should work the same for all hooks.

Benefits

Consistency in how hooks work, and possibility to write less try/finally clauses in the after hooks.

Possible Drawbacks

Cannot see any drawbacks

Applicable issues

I say this is a patch release, because it only affects what is executed when something fails. Also you should not be surprised that your after hook is executed when something fails, because that is the purpose of having it.

jsf-clabot · 2018-03-12T14:39:55Z

All committers have signed the CLA.

coveralls · 2018-03-12T14:46:01Z

Coverage decreased (-0.2%) to 89.814% when pulling 50180c2 on johanblumenberg:master into d76f490 on mochajs:master.

boneskull · 2018-03-18T05:27:32Z

@johanblumenberg Thanks. Can you please sign the CLA?

johanblumenberg · 2018-03-18T16:31:17Z

@johanblumenberg Thanks. Can you please sign the CLA?

Done

boneskull

Hi, thanks. Mostly I have some questions. Was hoping you could take a look.

boneskull · 2018-03-19T17:22:03Z

lib/runnable.js

@@ -318,14 +318,15 @@ Runnable.prototype.run = function (fn) {
    self.clearTimeout();
    self.duration = new Date() - start;
    finished = true;
+    delete self.callback;


what's this for?

This is to ensure the done() function is only invoked once for a test

boneskull · 2018-03-19T17:24:13Z

lib/runner.js

-      this.hookUp('afterEach', this.next);
-      return;
-    }
+  if (runnable.isPassed() || !runnable.callback) {


I'm unclear what we're doing with the callback

The callback is what is to be executed if the test fails. It's the done() function of the current test.

If there is a current test, then that test should be cancelled and all after hooks should be run on any uncaught exceptions.
If there is no current test, then uncaught exceptions simply mark the current suite as failed.

boneskull · 2018-03-19T17:25:27Z

lib/runner.js

+          if (name === 'beforeAll' || name === 'beforeEach') {
+            // stop executing hooks, notify callee of hook err
+            return fn(err);
+          } else {


this else is not necessary

ok, it can be removed

boneskull · 2018-03-19T17:26:46Z

test/integration/uncaught.spec.js

@@ -16,7 +16,7 @@ describe('uncaught exceptions', function () {
      assert.equal(res.stats.failures, 1);

      assert.equal(res.failures[0].fullTitle,
-        'uncaught "before each" hook');
+        'uncaught "before each" hook for "test"');


where is this coming from?

It's just a bonus improvement, that came for free when improving the handling of uncaught exceptions, since it now invokes the current test callback.

It's better to have the error message contain which test that failed.

boneskull · 2018-03-19T17:27:47Z

lib/runner.js

        }
      }
      self.emit('hook end', hook);
      delete hook.ctx.currentTest;
-      next(++i);
+      next(++i, prevErr);


so I understand: we're trapping the first error we find, then passing it along to the rest of the after and afterEach hooks? then, if there's no hooks remaining, we finally bail?

yes

The prevErr is just to keep track of the last error that happened. The first after hook might throw an error, and the next ones succeed. Then the error has to be remembered so it can be used when calling the callback fn.

boneskull · 2018-03-19T17:29:18Z

lib/runner.js

-  var alreadyPassed = runnable.isPassed();
-  // this will change the state to "failed" regardless of the current value
-  this.fail(runnable, err);
-  if (!alreadyPassed) {


why was all of this removed?

All this just another implementation of what is already in the tests done() function. So it's duplicate code.

It's trying to figure out which hook is being executed, and take certain actions depending on that. All that is already done, in a better way, in the tests done() function.

boneskull · 2018-03-19T17:37:32Z

FWIW: I'm unsure if we want to merge this.

To some of your points:

Only one error is reported from the test suite. You want to log all errors that happen.

The user can log via console.error, etc. Currently, Mocha does not support "multiple" exceptions in any context. For example, if a test fails then fails again, Mocha swallows the second exception because there's no provision to handle such a state in the reporters:

it('fails twice', function (done) {
  process.nextTick(() => {
    done(new Error('once'));
    process.nextTick(() => {
      throw new Error('twice');
    });
  });
});

See #3223 for further information.

But the fact that hooks in the parent suite are executed leads me to believe that this was simply overlooked, maybe because several after hooks in the same suite is not very common.

So, to confirm, you're saying:

describe('parent', function () {
  afterEach(function () {
    // this gets run even after 'accck' is thrown?
  });
 
  describe('child', function () {
    afterEach(function () {
      throw new Error('accck!');
    });
   
    it('does stuff', function () {
    });
  }); 
});

plroebuck · 2018-05-08T03:09:33Z

This would seem to change the symmetry expectations of Mocha hooks. You only want to do this for after()/afterEach(), but not before()/beforeEach(), right?

Seems like you could just wrap the portions of your own code that might throw in a try/catch, and this problem goes away without changes to Mocha.

const _ = require('lodash');
const errors = [];

function justKeepSwimming(fn) {
  let maybeError = _.attempt(fn);
  if (_.isError(maybeError)) {
    errors.push(maybeError);
  }
}

function logErrorsAndReset() {
  if (errors.length > 0) {
    errors.forEach((err) => console.error(err));
    errors.length = 0;
  }
}

describe('suite A', function () {
  afterEach(function () {
    justKeepSwimming(cleanupItemX);
    justKeepSwimming(cleanupItemY);
  });

  afterEach(function () {
    logErrorsAndReset();
  });

  // tests...
});

boneskull · 2018-05-08T04:51:26Z

I'm calling this inactive. please rebase/reopen if interested

johanblumenberg · 2018-09-18T10:54:46Z

@boneskull

So, to confirm, you're saying:
// this gets run even after 'accck' is thrown?

yes

Currently, Mocha does not support "multiple" exceptions in any context. For example, if a test fails then fails again, Mocha swallows the second exception because there's no provision to handle such a state in the reporters:

In the example you provided, the test is completed when the done() function is called. All after hooks are run, and the next test is executed. The second exception, in the next tick, will be an unhandled exception, caught outside of any test. It will fail the current test suite. No exceptions are swallowed, they are all reported. I'm not changing any of that. And I think that is correct. If any code has a side effect that an exception is thrown asynchronously, that should still fail the test (if possible) or the test suite, so I notice the bug.

johanblumenberg · 2018-09-18T11:04:47Z

@plroebuck

This would seem to change the symmetry expectations of Mocha hooks. You only want to do this for after()/afterEach(), but not before()/beforeEach(), right?

Yes

It cannot be done for the before hooks, for the same reason that tests cannot be run if a before hook fails. There is no point in continuing execution after an exception in the before hooks. But you need to make sure that you do cleanup of whatever you were setting up.

Seems like you could just wrap the portions of your own code that might throw in a try/catch, and this problem goes away without changes to Mocha.

Problems with this code:

It's a lot more noisy
It doesn't handle asynchronous cleanup functions
It's logging the failures, but the test suite will still succeed, and no one will notice the errors
Only if you are looking at stdout will you see the errors, no reporters will be aware of the errors

johanblumenberg · 2018-09-18T11:31:49Z

I would like this ticket to be reopened, however I cannot find the button to do so

johanblumenberg · 2018-09-18T11:34:19Z

I'm not sure why this has to be a major semver update.

The only change is that all after hooks are run on test failures.
If tests are passing, there is no change.
If tests are failing, then possibly another after hook will be run. But since the tests are already failing, this won't affect the outcome of the test run.

outsideris · 2018-09-19T15:31:06Z

Reopen button is also disabled for me. You should open it as new pull request if you want.

boneskull · 2018-09-19T15:45:21Z

If tests are failing, then possibly another after hook will be run. But since the tests are already failing, this won't affect the outcome of the test run.

in that case, it doesn't necessarily need to be a semver-major, but I prefer to err on the side of caution with anything that fundamentally changes test execution.

plroebuck · 2018-09-20T15:37:15Z

@johanblumenberg

@plroebuck

Seems like you could just wrap the portions of your own code that might throw in a try/catch, and this problem goes away without changes to Mocha.

Problems with this code:
* It's a lot more noisy
* It doesn't handle asynchronous cleanup functions
* It's logging the failures, but the test suite will still succeed, and no one will notice the errors
* Only if you are looking at stdout will you see the errors, no reporters will be aware of the errors

No argument there.
I only spent about 10 mins on it. But I could add a done argument to the afterEach()...
See below...
Same

function logErrorsAndFail() {
  if (errors.length > 0) {
    var err;
    if (errors.length === 1) {
      err = errors.shift();
    } else {
      errors.forEach((err) => console.error(err));
      err = new Error('multiple errors occurred during teardown... see console');
    }
    this.test.error(err);
  }
}

johanblumenberg · 2019-02-25T14:22:13Z

@plroebuck I don't see how asynchronous errors would be handled in the code you provided.

You would need to provide a done argument. You would also need to check each cleanup function to see if it is a synchronous or asynchronous function, based on if it returns a promise or not. If it does return a promise, you need to wait with executing the next cleanup function, until the first one is done. Then, when all cleanup functions are done, you need to invoke the done callback.

This would be possible, of course, but it is a lot of code to write. It also puts the responsibility of making this work properly on the test writer, not on the test framework.

Also, all this code already exists in mocha. Why not leverage the existing code? Why require the user of the library to write the same code? Write it once, and it will work for everyone.

johanblumenberg · 2019-02-25T14:24:22Z

4. Same

@plroebuck I don't see how test reporters would be aware of the exceptions thrown. All they would receive would be the exception new Error('multiple errors occurred during teardown... see console'), no?

johanblumenberg · 2019-02-25T14:27:54Z

There is also the inconsistency that if I put after hooks in different describe contexts, they are always executed, as shown in the second example here: #3281 (comment)

johanblumenberg added 2 commits March 12, 2018 13:38

Timeout and uncaught exception handled the same way

4935e78

Invoke all after hooks even if the first one fails

50180c2

outsideris added the type: feature enhancement proposal label Mar 13, 2018

boneskull added the semver-major implementation requires increase of "major" version number; "breaking changes" label Mar 18, 2018

Bamieh approved these changes Mar 19, 2018

View reviewed changes

boneskull suggested changes Mar 19, 2018

View reviewed changes

boneskull added the status: waiting for author waiting on response from OP - more information needed label Apr 7, 2018

boneskull force-pushed the master branch 2 times, most recently from 4547268 to 7613521 Compare May 4, 2018 16:47

boneskull closed this May 8, 2018

johanblumenberg mentioned this pull request Oct 4, 2018

Ensure all after hooks are executed, even if one of them fail #3488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure all after hooks are executed, even if one of them fail #3281

Ensure all after hooks are executed, even if one of them fail #3281

johanblumenberg commented Mar 12, 2018

jsf-clabot commented Mar 12, 2018 •

edited

Loading

coveralls commented Mar 12, 2018

boneskull commented Mar 18, 2018

johanblumenberg commented Mar 18, 2018

boneskull left a comment

boneskull Mar 19, 2018

johanblumenberg Sep 18, 2018

boneskull Mar 19, 2018

johanblumenberg Sep 18, 2018

boneskull Mar 19, 2018

johanblumenberg Sep 18, 2018

boneskull Mar 19, 2018

johanblumenberg Sep 18, 2018

boneskull Mar 19, 2018

johanblumenberg Sep 18, 2018

boneskull Mar 19, 2018

johanblumenberg Sep 18, 2018

boneskull commented Mar 19, 2018

plroebuck commented May 8, 2018 •

edited

Loading

boneskull commented May 8, 2018

johanblumenberg commented Sep 18, 2018 •

edited

Loading

johanblumenberg commented Sep 18, 2018

johanblumenberg commented Sep 18, 2018

johanblumenberg commented Sep 18, 2018

outsideris commented Sep 19, 2018

boneskull commented Sep 19, 2018

plroebuck commented Sep 20, 2018 •

edited

Loading

johanblumenberg commented Feb 25, 2019

johanblumenberg commented Feb 25, 2019

johanblumenberg commented Feb 25, 2019

Ensure all after hooks are executed, even if one of them fail #3281

Ensure all after hooks are executed, even if one of them fail #3281

Conversation

johanblumenberg commented Mar 12, 2018

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

jsf-clabot commented Mar 12, 2018 • edited Loading

coveralls commented Mar 12, 2018

boneskull commented Mar 18, 2018

johanblumenberg commented Mar 18, 2018

boneskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Mar 19, 2018

plroebuck commented May 8, 2018 • edited Loading

boneskull commented May 8, 2018

johanblumenberg commented Sep 18, 2018 • edited Loading

johanblumenberg commented Sep 18, 2018

johanblumenberg commented Sep 18, 2018

johanblumenberg commented Sep 18, 2018

outsideris commented Sep 19, 2018

boneskull commented Sep 19, 2018

plroebuck commented Sep 20, 2018 • edited Loading

johanblumenberg commented Feb 25, 2019

johanblumenberg commented Feb 25, 2019

johanblumenberg commented Feb 25, 2019

jsf-clabot commented Mar 12, 2018 •

edited

Loading

plroebuck commented May 8, 2018 •

edited

Loading

johanblumenberg commented Sep 18, 2018 •

edited

Loading

plroebuck commented Sep 20, 2018 •

edited

Loading