🐛 Bug: afterEach halts suite and/or causes simultaneous failure and pass

[glenjamin]

This is a common pattern with tools like sinon or nock - to have afterEach do some standard verification at the end of each test.

I'm fairly sure this used to work - not sure what's changed.

var assert = require('assert');

describe("afterEach", function() {
    var verificationFlag;
    beforeEach(function() {
        verificationFlag = 0;
    });
    afterEach(function() {
        assert.equal(verificationFlag, 1);
    });

    it("0", function() { })
    it("1", function() { verificationFlag = 1 })
    it("2", function() { verificationFlag = 2 })
});

Expected output: 3 tests, 3 failures
Actual output: 1 test, 1 pass, 1 fail (from hook)

[m-reiniger]

I might be off with that, but I think you just forgot about done();

[danielstjules]

Looks like it's been the same behavior for a while. Mocha assumes that cleanup couldn't be done properly if an exception was thrown from the afterEach hook.

# --------------------------------
# 2.2.1
# --------------------------------
dstjules:~/git/mocha ((2.2.1))
$ ./bin/mocha test.js


  ․․

  1 passing (8ms)
  1 failing

  1) afterEach "after each" hook:
     AssertionError: 0 == 1
      at Context.<anonymous> (/Users/dstjules/git/mocha/test.js:9:16)
      at callFn (/Users/dstjules/git/mocha/lib/runnable.js:266:21)
      at Hook.Runnable.run (/Users/dstjules/git/mocha/lib/runnable.js:259:7)
      at next (/Users/dstjules/git/mocha/lib/runner.js:268:10)
      at Immediate._onImmediate (/Users/dstjules/git/mocha/lib/runner.js:289:5)
      at processImmediate [as _immediateCallback] (timers.js:357:17)

# --------------------------------
# 2.1.0
# --------------------------------
dstjules:~/git/mocha ((2.1.0))
$ ./bin/mocha test.js


  ․․

  1 passing (11ms)
  1 failing

  1) afterEach "after each" hook:
     AssertionError: 0 == 1
      at Context.<anonymous> (/Users/dstjules/git/mocha/test.js:9:16)
      at callFn (/Users/dstjules/git/mocha/lib/runnable.js:251:21)
      at Hook.Runnable.run (/Users/dstjules/git/mocha/lib/runnable.js:244:7)
      at next (/Users/dstjules/git/mocha/lib/runner.js:259:10)
      at Immediate._onImmediate (/Users/dstjules/git/mocha/lib/runner.js:276:5)
      at processImmediate [as _immediateCallback] (timers.js:357:17)

# --------------------------------
# 2.0.0
# --------------------------------
dstjules:~/git/mocha ((2.0.0))
$ ./bin/mocha test.js


  ․․

  1 passing (9ms)
  1 failing

  1) afterEach "after each" hook:
     AssertionError: 0 == 1
      at Context.<anonymous> (/Users/dstjules/git/mocha/test.js:9:16)
      at callFn (/Users/dstjules/git/mocha/lib/runnable.js:250:21)
      at Hook.Runnable.run (/Users/dstjules/git/mocha/lib/runnable.js:243:7)
      at next (/Users/dstjules/git/mocha/lib/runner.js:258:10)
      at Immediate._onImmediate (/Users/dstjules/git/mocha/lib/runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:357:17)

# --------------------------------
# 1.16.2 (Dec 23, 2013)
# --------------------------------
dstjules:~/git/mocha ((1.16.2))
$ ./bin/mocha test.js
child_process: customFds option is deprecated, use stdio instead.

  ․․

  1 passing (9ms)
  1 failing

  1) afterEach "after each" hook:
     AssertionError: 0 == 1
      at Context.<anonymous> (/Users/dstjules/git/mocha/test.js:9:16)
      at Hook.Runnable.run (/Users/dstjules/git/mocha/lib/runnable.js:221:32)
      at next (/Users/dstjules/git/mocha/lib/runner.js:263:10)
      at Immediate._onImmediate (/Users/dstjules/git/mocha/lib/runner.js:280:5)
      at processImmediate [as _immediateCallback] (timers.js:357:17)

[boneskull]

@glenjamin @danielstjules Can someone provide the breaking change?

[danielstjules]

@boneskull In my first reply above, I went as far back as 1.16.2 (Dec 23, 2013). The behavior's been the same in 2.2.1, 2.1.0, 2.0.0 and 1.16.2. Unless it was introduced in a patch by accident, then I don't think it's ever been supported in a 2.x release.

Just checked 1.3.0 (July, 2012), and it gave the same result:

dstjules:~/git/mocha ((1.3.0))
$ ./bin/mocha test.js
child_process: customFds option is deprecated, use stdio instead.

  ..

  ✖ 1 of 3 tests failed:

  1) afterEach "after each" hook:
     AssertionError: 0 == 1
      at Context.<anonymous> (/Users/dstjules/git/mocha/test.js:9:46)
      at Hook.Runnable.run (/Users/dstjules/git/mocha/lib/runnable.js:184:32)
      at next (/Users/dstjules/git/mocha/lib/runner.js:194:10)
      at /Users/dstjules/git/mocha/lib/runner.js:205:5
      at process._tickCallback (node.js:361:13)
      at Function.Module.runMain (module.js:453:11)
      at startup (node.js:123:18)
      at node.js:868:3

So I don't think it's ever been supported. I also don't think it's feasible for us to support, since there's no way for us to effectively distinguish between these types of errors from a hook.

[boneskull]

@danielstjules I'm not sure I understand why catching an AssertionError in a hook isn't feasible?

[danielstjules]

@boneskull Because some assertion libraries do no throw instances of AssertionError (ex: expect.js). However, both chai and should do.

[glenjamin]

Hrm, I must have imagined that this worked before - probably another test runner!

Do we think it's too much of a breaking change to continue onwards in the face of a failing hook? What if it was opt-in? afterEach(function() {}).continueAfterErrors() or similar?

[boneskull]

@danielstjules I would be fine with only catching AssertionErrors in v3.0.0.

[boneskull]

@danielstjules or rather, at any rate:

If expect.js is the only thing that doesn't throw AssertionError's, and we start explicitly catching them in hooks, then nothing will really "break" per se, because it never worked in the first place.

[boneskull]

...and perhaps expect.js will start to throw them if we bug them about it

[boneskull]

Furthermore, we could actually start looking for AssertionErrors everywhere. We could attempt to detect an AssertionError as well if the object has actual and expected properties. If expect.js has these properties, we could just go w/ that.

[boneskull]

(we might be already doing this)

[danielstjules]

detect an AssertionError as well if the object has actual and expected properties. If expect.js has these properties, we could just go w/ that.

Good call. :) Had completely forgotten about that. They do handle it: https://github.com/Automattic/expect.js/blob/9f0efa203e82b6a768aa40240344c5340ed27e3d/index.js#L96-L100

[glenjamin]

Are we in agreement that if a failure in an after hook is identified as an assertion failure, that the testsuite should continue?

[danielstjules]

I think so. :) Given its scope, it'd probably have to be directed at the next major release (3.0.0)

[RamonDonnell]

+1

[sarbbottam]

👍

[j3tan]

A common pattern that we use with sinonJS is

var sandbox;
beforeEach(function() {
    sandbox = sinon.sandbox.create();
});
afterEach(function() {
    sandbox.verifyAndRestore();
});
...
it('should do something when called', function() {
   sandbox.mock(obj).expects('foo');
   doSomething();
});

Mocks that fail expectations (and throw an ExpectationError) will end up halting the suite. In addition, the afterEach isn't really associated with the test case and sometimes displays the testcase as 'passing' when it actually failed. Perhaps we should consider the before/beforeEach/after/afterEach as part of the test case instead of separate pre/post process hooks?

The alternative is to put sandbox.verifyAndRestore() at the end of every test case, but that isn't very DRY and we have potentially thousands of tests.

[nathanboktae]

Perhaps we should consider the before/beforeEach/after/afterEach as part of the test case instead of separate pre/post process hooks?

That's a good idea, but I would just limit it to beforeEach and afterEach handlers.

As for ExpectationError, it's easy enough to wrap it to convert it to AssertionError (or accept both, I assume the implementation will use Object.getPrototypeOf(error).toString to check (or the ES3 friendly equivalent))

I just ended up monkey-patching it to work around this issue :( I'm very much looking forward to this being implemented.

[kownacki]

Seems like before/beforeEach/after/afterEach halt next tests because they are meant to be used as set up and cleanup - not for actual test assertion. What we need is something that will make it non blocking and thus do assertions like in normal tests - IMO best idea is to make it an option.

afterEach.nonBlocking( ... ) // or something similar

[glenjamin]

@kownacki - that's a reasonable suggestion IMO, do you see problems with the current suggestion of using AssertionError?

[nathanboktae]

nonblocking is not semantic and doesn't allow for cleanup and assertion to happen together like the AssertionError case, so my vote is the later.

[glenjamin]

We can bikeshed on the name, I was more interested in thoughts on an explicit opt-in, vs implicit by exception type

[kownacki]

@nathanboktae As it's in mocha API:

Mocha allows you to use any assertion library you want, if it throws an error, it will work!

Allowing only AssertionErrors in both test cases and hooks will result in breaking change - so it doesn't seem sensible.
But saying explicitly that in hooks throwing something other that AssertionError will result in halting next tests and throwing AssertionErrors won't - yeah, it's more reasonable, but kinda counter-intuitive and very confusing, because it's hard to say immidiately if the hook is for testing or cleanup/setup. That's why I'm opting for nonblocking or similar more semantic name.

[nathanboktae]

Good points, but something else to consider is the complexity of implementing say another set of hooks. How would it look like in your proposal?

before -> before.nonBlocking -> beforeEach -> beforeEach.nonBlocking -> test -> afterEach.nonBlocking -> afterEach -> after.nonBlocking -> after

Even if you removed the asserting before's, that's alot. Also I would guess the majority would do assertions while cleaning up, so in your suggestion, they'd have to explicit separate that out. Maybe that's a good thing?

[glenjamin]

From an implementation POV, i'd have that API create a hook using all the same objects as normal, but with an extra flag.

I think both options as as easy to implement (although I've been putting off actually doing it for aaaages now!).

[kownacki]

I'm not sure, but probably the order should stay the same. I mean, all befores -> all beforeEaches -> test -> all afterEaches -> all afters. nonBlocking flag would not make the hook special, and just put it after the last one of its kind. So it would result in for example multiple beforeEaches and non-blocking beforeEaches in mixed order.

From my POV these "non blocking hooks" will be used as an extension for multiple tests. Like it's in the first comment here - we want to check the same thing in multiple tests. Essentialy: fail the test that was just finished but doesn't pass the "post test".

Maybe nonBlocking is a misleading name. Now I think it should be simply another hook named postCheck or something like this. If the main test doesn't fail, its "post tests" would be run and till one fails, or all pass. Then all afterEaches and afters would run regardless of the test and post-tests result - just like it's now.

[glebec]

An ugly workaround alternative with existing API is described here: https://github.com/mochajs/mocha/wiki/Conditionally-failing-tests-in-afterEach()-hooks

So, you can do the following:

afterEach(function(){
  try {
    yourAssertion();
  } catch (err) {
    this.test.error(err);
  }
})

Just as a stopgap, at least. I am in favor of some way to make afterEach able to fail a spec if an error is thrown. I think that's how Jasmine does it, which is why the Angular docs include bare verifyNoOutstandingRequests() in their afterEach blocks. I get the desire not to introduce breaking changes, but sometimes it makes sense to DRY up your it blocks with repeated tests, not just setup and teardown.

[jjoos]

@glebec that behaviour (or 'ugly workaround') is currently broken, I'm trying to get it fixed in #1944.

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[j-funk]

This is not stale and should be fixed.

[Rush]

I have a workaround:

function wrapTests(wrapper) {
  var originalIt = global.it;
  var originalDescribe = global.describe;
  global.describe = function() {
    global.it = originalIt;
    global.describe = originalDescribe;
    return originalDescribe.apply(this, arguments);
  };
  
  global.it = function(title, test) {          // override the original it by a wrapper  
    return originalIt.call(this, title, function() {
      var promise;
      if(test.length > 0) {
        promise = Promise.fromCallback(function(done) {
          return test(done);
        });
      } else {
        promise = Promise.resolve(test());
      }
      return wrapper(promise);
    });
  }
}

describe('test', function() {

  // will fail each test with custom Error, do your conditional checking here
  wrapTests(function(promise) {
    return promise.then(function() {
      throw new Error('dd');
    });
  });

});

[Rush]

Actually I improved that solution and published it in an npm package: https://github.com/Rush/mocha-finalize-each

Below supports all types of tests: sync, async and promises. It unifies them to use promises so that finalizeEach can have a uniform interface.

var finalizeEach = require('mocha-finalize-each');

describe('some tests', function() {
  var sinonSandbox = sinon.sandbox.create();

  finalizeEach(this, promise => {
    return promise.then(() => {
      // will fail tests if sinon assertions are not satisfied
      sinonSandbox.verifyAndRestore();
    }).finally(() => {
      // clean in all cases
      sinonSandbox.restore();
    });
  });

  it('some test', function() {
     /* ...  some code using sinonSandbox */
  });
});

[j-funk]

@Rush does your workaround prevent failures created during afterEach from halting the test run?

[Rush]

@j-funk correct. Well, I actually added finalizeEach which you should instead of afterEach

[jaredsm]

@Rush, thanks for tackling this deficiency in mocha!

I'm having trouble with failed assertions in asynchronous code. It seems they prevent finally from running. Is that by design? If you apply the following patch to mocha-finalize-each/test.js, you can see for yourself:

2a3
> var assert = require('assert');
21a23
>       assert(false);

In case it matters, I'm using node v6.11.3 and mocha 2.5.3.

[Rush]

@jaredsm can you submit a full example which I can test?

[jaredsm]

var assert = require('assert');
var finalizeEach = require('./');

[false, true].forEach((assertionValue) => {
  describe('finalizeEach should run after', function() {
    var finalizeDidRun = false;
    finalizeEach(this, promise => {
      return promise.finally(() => { finalizeDidRun = true; });
    });
    afterEach(() => {
      if (!finalizeDidRun) throw new Error('Finalize should have run');
    });
    it('an asynchronous test asserting on ' + assertionValue, done => {
      setTimeout(() => {
        assert(assertionValue, 'intentional asynchronous assertion failure');
        done();
      }, 20);
    });
  });
});

[jaredsm]

Exploring further, it seems finalize doesn't run when a wrapped asynchronous test throws an AssertionError rather than calling back with an error. This pattern may be a misuse of mocha, but I've found it convenient to assert after asynchronous functions call back:

it('does the right thing', done => {
  setTimeout(() => {
    assert(false, 'expected the right thing');
    done();
  }, 20);
});

This works in plain mocha but breaks mocha-finalize-each.

[fgarcia]

I think the right solution should be the one mentioned by @glebec

Currently the whole test suite shutdowns thinking that throwing an error within a hook is a "setup -infrastructure" problem and not a test problem. Otherwise a coding error within a hook would trigger lots of false test errors when actually the test tear -down / -up is broken.

IMHO the solution I would like to see is solving #3345, probably with the pull provided by @jjoos in #1944 and also adding some documentation to the error explaining that this.test.error() should be used if the intention is failing the test, not a test setup exception.

[papb]

Hello, just wanted to point out that @glebec's workaround works for me in mocha v8.2.1:

afterEach(function(){
  try {
    yourAssertion();
  } catch (err) {
    this.test.error(err);
  }
})

However, mocha's output shows the test both passing and failing: #3345

[MarcCelani-at]

We attempted @glebec 's solution and ran into some issues. Here's the expected behavior:

• [✔️] Mocha does not halt
• [✔️] Mocha exits with an error code
• [ X ] Mocha's reporters report as a failure

In reality, what happens is that Mocha reports the test as both a success and a failure. For us, this breaks a bunch of assumptions when we post-process test results that I am afraid to change. Furthermore, we are a bit uncomfortable using an unsupported method.

I would still advocate for some kind of officially supported hook. This is our use case:

One of the things we generally want to verify in each of our tests is that there are no unexpected error logs. In tests, we wrap our logger so that we can check for errors logs at the end. We currently do this check in afterEach, but that causes our test suite to halt when the assertion fires.

Some alternatives we are considering in the absence of this type of hook:

1. Wrapping it(), which would be a pretty big migration for our codebase but doable.
2. Tracking errors but only doing the assertion at the end in after() so that the test is not halted.