-
-
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
do not eat exceptions thrown asynchronously from passed tests; closes #3226 #3257
Conversation
Looks good, I love the usage of |
yeah, but it's actually a hack around the real problem, which is that |
@boneskull regardless if it is triggered once or multiple times, we should be using |
Mochawesome has used: skipped = (!cleaned.pass && !cleaned.fail && !cleaned.pending); since adamgruber/mochawesome@2d4a63b7 (logic to record skipped tests, 2015-01-29). Do not emit 'test end' for skips or TODO (pending) tests, because Mochawesome has: obj.stats.skipped = obj.stats.testsRegistered - obj.stats.tests; since that same commit, so we need these tests entered in testsRegistered (via the 'test' event) but not in tests (via the 'test end' event). Do not emit 'pass' for skips either, otherwise we'll end up with failures + passes > tests. Also fix "pass" -> "passed" for .state. Mochawesome has been comparing the value to "passed" in its cleaner since adamgruber/mochawesome@d687633d (Initial commit, 2014-07-11), and Mocha itself has been comparing with "passed" since at least mochajs/mocha@2c720a35 (do not eat exceptions thrown asynchronously from passed tests, 2018-02-28, mochajs/mocha#3257).
On upgrading from mocha 3.5.3 to 5.2.0 (latest), we noticed that our test coverage started silently skipping a bunch of tests while still passing. I bisected and found that this commit is the commit that causes it. I'm aware now that this is because one or more of our tests are throwing an error in asynchronous code, possibly after they pass, but we've got quite a few tests, so it's going to take a while to pinpoint. For now we're downgrading to 4.1.0, which still runs the entire suite. Just want to leave in case it helps someone |
@rsheldiii, This commit? Two choices for your actual problem:
$ mocha --exit my-shitty-code.spec.js
$ wtfnode ./node_modules/.bin/_mocha my-shitty-code.spec.js |
Note that the change to |
Currently, Mocha eats the error thrown below, because the test has already passed:
If the above happens, Mocha can't recover gracefully b/c of #3223. This PR causes Mocha to bail if the above situation is encountered. Exactly how this is reported is "best effort", based on queued tasks and timing--the reporter output may not be pretty, but you should at least get a traceback.
One thing we can do to mitigate (some) #3223-related weirdness is force the reporters to only respond to the
end
event once, which is what I've done. Without this, machine-readable reporters in particular--such asjson
orxunit
--get their wires crossed. No reporter should be dumping the summary twice anyway.Also added a couple helper functions to
Runnable.prototype
.