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

uncaughtException: fix "--allow-uncaught" with "this.skip()" #4030

Merged
merged 3 commits into from
Oct 8, 2019

Conversation

juergba
Copy link
Contributor

@juergba juergba commented Sep 26, 2019

Description

describe('test suite', () => {
  it('test1', function () { });
  it('test2', function () {
    this.skip();
  });
  it('test3', function () { });
  it('test4', function () {
    this.skip();
  });
});

mocha --allow-uncaught test.js silently aborts execution after the first this.skip():

test suite
    √ test1

Our docu states:

[...] It's also important to note that calling this.skip() will effectively abort the test.

We achieve this abortion of the test/hook execution by throwing a Pending exception. In async scenarios this results in an intended uncaughtException which has to be swallowed somehow. Obviously --allow-uncaught does not distinguish correctly between "bad" uncaughtException (user code) and "good" uncaughtException (this.skip()).

Description of the Change

  • independently of --allow-uncaught, we never allow the Pending exceptions to propagate.
  • directly in Runnable#skip() we set the test to pending and immediately catch/swallow the Pending exception (sync scenario).
  • for async this.skip() we register an addional uncaughtException handler for exceptions bubbling up after EVENT_RUN_END.
  • the only purpose of the Pending is to abort the test execution, we don't use it for anything else.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 92.781% when pulling f8f3542 on juergba/uncaught-skip into 5f8df08 on master.

@juergba juergba self-assigned this Sep 27, 2019
@juergba juergba added area: async related to asynchronous use of Mocha type: bug a defect, confirmed by a maintainer status: needs review a maintainer should (re-)review this pull request semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Sep 27, 2019
@juergba juergba marked this pull request as ready for review September 27, 2019 14:23
juergba added a commit that referenced this pull request Sep 28, 2019
@juergba
Copy link
Contributor Author

juergba commented Oct 7, 2019

I will do some additional testing and then merge this PR within the next few days.

@juergba juergba merged commit 3633a90 into master Oct 8, 2019
@juergba juergba deleted the juergba/uncaught-skip branch October 8, 2019 13:48
@juergba juergba removed the status: needs review a maintainer should (re-)review this pull request label Oct 8, 2019
@juergba juergba added this to the next milestone Oct 8, 2019
@juergba juergba modified the milestones: 6.2.1, 7.0.0 Oct 18, 2019
@juergba juergba added semver-major implementation requires increase of "major" version number; "breaking changes" and removed semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Oct 18, 2019
@juergba juergba added semver-patch implementation requires increase of "patch" version number; "bug fixes" and removed semver-major implementation requires increase of "major" version number; "breaking changes" labels Jan 4, 2020
@@ -910,6 +915,12 @@ Runner.prototype.run = function(fn) {
this.on(constants.EVENT_RUN_END, function() {
debug(constants.EVENT_RUN_END);
process.removeListener('uncaughtException', uncaught);
process.on('uncaughtException', function(err) {
Copy link

@weswigham weswigham Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is adding a new, unique uncaughtException handler every time the end event of a runner is called - this means for every finished runner, a new handler gets added (which is never removed). This, in turn, causes a Possible EventEmitter memory leak detected. 11 uncaughtException listeners added to [process]. Use emitter.setMaxListeners() to increase limit warning to be fired by node.

:(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: async related to asynchronous use of Mocha 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.

3 participants