(re-)enable after each hooks to fail tests.

[jjoos]

In version 1.2.1, support was added to fail test in the after each hook:

• Added this.test.error(err) support to after each hooks. Closes add 'test verify' hook #287

While migrating from teaspoon-mocha to karma-mocha, we discovered that
in the current mocha version this isn't supported anymore. That behaviour has been changed in: ebc6aee without any documentation change.

If this was a conscious decision adding some documentation about dropping this functionality would then be nice. The old behaviour is documented in: https://github.com/mochajs/mocha/wiki/Conditionally-failing-tests-in-afterEach()-hooks .

[jjoos]

As far as I can tell this is a bug that has been fixed by my changes, this is the content of the test fixture:

describe('Multiple Done calls', function(){
  it('should report an error if done was called more than once', function(done){
    done();
    done();
  })

  it('should report an error if an exception happened async after done was called', function (done) {
    done();
    setTimeout(done, 50);
  })

  it('should report an error if an exception happened after done was called', function(done){
    done();
    throw new Error("thrown error");
  })
})

[danielstjules]

Are you sure that code corresponds to the right test? Running that with mocha --reporter json on master yields:

  "stats": {
    "suites": 1,
    "tests": 3,
    "passes": 3,
    "pending": 0,
    "failures": 2,

[danielstjules]

Nvm, the fixture highlighted above is the wrong one. It's actually: https://github.com/jjoos/mocha/blob/master/test/integration/fixtures/multiple.done.js Which makes sense then :) Since its contents are:

it('should fail in a test-case', function(done) {
  process.nextTick(function(){
    done();
    done();
  });
});

In that case, your code is an improvement! I'd be curious if you fixed the issue here as well: https://github.com/jjoos/mocha/blob/master/test/integration/fixtures/multiple.done.js#L4-L11

[jjoos]

Ah sorry for quoting the wrong fixture.

I actually looked at the commented out section of that fixture, but didn't know what the the correct behaviour is.

If I uncomment that fixture, the change to the test to make it pass looks like this:

     it('results in failures', function() {
       assert.equal(res.stats.pending, 0);
-      assert.equal(res.stats.passes, 0);
-      assert.equal(res.stats.failures, 1);
-      assert.equal(res.code, 1);
+      assert.equal(res.stats.passes, 1);
+      assert.equal(res.stats.failures, 2);
+      assert.equal(res.code, 2);
     });

Which seems strange since the beforeEach error should cancel the execution of the test, but doesn't do that (I confirmed that the test is executed) But this could be an acceptable limitation of the async beforeEach implementation. If you clarify if this is acceptable I can uncomment it in my PR.

[danielstjules]

Nvm, I'd leave that be then :)

[danielstjules]

Related: #1635

[danielstjules]

Mind rebasing on top of master? :)

[danielstjules]

Heh, anytime we touch the runner code, it's a lot to QA 😅

[jjoos]

@danielstjules Ah missed #1635, this pr will fix that issue. Just rebased this commit on master.

[jjoos]

Ah missed #1635, this pr will fix that issue.

I spoke too soon, it won't fix that issue, but it will give them a reasonable workaround.

Fixing #1635 would mean a far bigger overhaul to consolidate the way the assertion errors are caught. And then apply that to a subset of the hooks.

[danielstjules]

spoke too soon, it won't fix that issue, but it will give them a reasonable workaround.

Exactly what I'm thinking :)

[glenjamin]

I think if this gets merged it'd be reasonable to close #1635 as the main reason I requested it would be to do exactly what this PR enables.

[j-funk]

Was very excited when I found the wiki article - alas it is not working - can we get this soon?

https://github.com/mochajs/mocha/wiki/Conditionally-failing-tests-in-afterEach()-hooks

[j-funk]

@jjoos any chance you're up for fixing the merge conflicts? This would be a huge help if/when it gets merged!

[glenjamin]

I'm happy to adopt this if it's likely to be merged afterwards.

There was a time when it was mergeable but wasn't integrated - so I'm not sure whether there was some other reason it wasn't accepted.

[j-funk]

@danielstjules sry to push on this so much, are you a potential gatekeeper for this?

[hrimhari]

Any news about this?

[arikon]

Anybody?

[Rush]

Any updates? Does a workaround exist?

[Rush]

FYI, pasted a workaround here #1635 (comment)

[boneskull]

can't use this unless the CLA is signed; please reopen if interested.