Skip to content
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

Merged
merged 1 commit into from
Mar 5, 2018

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Mar 1, 2018

Currently, Mocha eats the error thrown below, because the test has already passed:

it('should bail if a successful test asynchronously fails', function(done) {
  done();
  process.nextTick(function () {
    throw new Error('global error');
  });
});

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 as json or xunit--get their wires crossed. No reporter should be dumping the summary twice anyway.

Also added a couple helper functions to Runnable.prototype.

@boneskull boneskull requested a review from Bamieh March 1, 2018 06:36
@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Mar 1, 2018
@coveralls
Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage increased (+0.07%) to 90.014% when pulling 8186723 on issue/3226 into ec8901a on master.

@Bamieh
Copy link
Contributor

Bamieh commented Mar 4, 2018

Looks good, I love the usage of once instead of on.

@boneskull
Copy link
Contributor Author

Looks good, I love the usage of once instead of on.

yeah, but it's actually a hack around the real problem, which is that end shouldn't be emitted more than once.

@boneskull boneskull merged commit 2c720a3 into master Mar 5, 2018
@boneskull boneskull deleted the issue/3226 branch March 5, 2018 21:44
@boneskull boneskull added this to the next milestone Mar 5, 2018
@Bamieh
Copy link
Contributor

Bamieh commented Mar 6, 2018

@boneskull regardless if it is triggered once or multiple times, we should be using once to automatically detach the event listener.

wking added a commit to wking/tap-mocha-reporter that referenced this pull request Jun 8, 2018
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).
@rsheldiii
Copy link

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

@plroebuck
Copy link
Contributor

plroebuck commented Nov 10, 2018

@rsheldiii, This commit?

Two choices for your actual problem:

  • Run Mocha-5.2+ with --exit cmdline option which provides pre-4.0+ exit behavior
$ mocha --exit my-shitty-code.spec.js 
  • Find and fix problems in your test suite. The wtfnode package can help with this.
$ wtfnode ./node_modules/.bin/_mocha my-shitty-code.spec.js 

@plroebuck
Copy link
Contributor

plroebuck commented Nov 10, 2018

Note that the change to .once for the reporter event handler also prevents mocha instance from being reusable for those using it programmatically. We need another method to recreate the internal pieces/parts (e.g., reporters, growl) that have event listeners using .once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants