-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Bug: afterEach halts suite and/or causes simultaneous failure and pass #1635
Comments
I might be off with that, but I think you just forgot about done(); |
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) |
@glenjamin @danielstjules Can someone provide the breaking change? |
@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. |
@danielstjules I'm not sure I understand why catching an |
@boneskull Because some assertion libraries do no throw instances of AssertionError (ex: expect.js). However, both chai and should do. |
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? |
@danielstjules I would be fine with only catching |
@danielstjules or rather, at any rate: If expect.js is the only thing that doesn't throw |
...and perhaps expect.js will start to throw them if we bug them about it |
Furthermore, we could actually start looking for |
(we might be already doing this) |
Good call. :) Had completely forgotten about that. They do handle it: https://github.com/Automattic/expect.js/blob/9f0efa203e82b6a768aa40240344c5340ed27e3d/index.js#L96-L100 |
Are we in agreement that if a failure in an after hook is identified as an assertion failure, that the testsuite should continue? |
I think so. :) Given its scope, it'd probably have to be directed at the next major release (3.0.0) |
+1 |
1 similar comment
👍 |
A common pattern that we use with sinonJS is
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. |
That's a good idea, but I would just limit it to As for ExpectationError, it's easy enough to wrap it to convert it to I just ended up monkey-patching |
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.
|
@kownacki - that's a reasonable suggestion IMO, do you see problems with the current suggestion of using |
|
We can bikeshed on the name, I was more interested in thoughts on an explicit opt-in, vs implicit by exception type |
@nathanboktae As it's in mocha API:
Allowing only AssertionErrors in both test cases and hooks will result in breaking change - so it doesn't seem sensible. |
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? |
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!). |
I'm not sure, but probably the order should stay the same. I mean, all befores -> all beforeEaches -> test -> all afterEaches -> all afters. 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 |
An 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 |
I am a bot that watches issues for inactivity. |
This is not stale and should be fixed. |
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');
});
});
}); |
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 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 */
});
}); |
@Rush does your workaround prevent failures created during |
@j-funk correct. Well, I actually added |
@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:
In case it matters, I'm using node v6.11.3 and mocha 2.5.3. |
@jaredsm can you submit a full example which I can test? |
|
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:
This works in plain mocha but breaks mocha-finalize-each. |
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 |
We attempted @glebec 's solution and ran into some issues. Here's the expected behavior:
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:
|
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.
Expected output: 3 tests, 3 failures
Actual output: 1 test, 1 pass, 1 fail (from hook)
The text was updated successfully, but these errors were encountered: