#3320 Native types exceptions can crash Mocha runner #3321

fargies · 2018-04-12T09:22:58Z

Any non-extensible type thrown in an async callback raises an exception in Mocha runner, stopping tests.

See issue #3320

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
All new code requires tests to ensure against regressions.

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Tests continue and reports errors properly

Possible Drawbacks

Applicable issues

coveralls · 2018-04-12T09:34:09Z

Coverage increased (+0.05%) to 90.063% when pulling 41fe763 on fargies:master into 1acea30 on mochajs:master.

boneskull

Hi, thanks for the PR.

Mocha does not (and will not) support throwing of anything except an Error or Error-like object.

If you wish to modify this PR, you could perform a check as seen here which re-throws the value as an Error (and sets the uncaught property).

fargies · 2018-04-12T17:12:54Z

You're right, with the modification uncaught was not set once transformed into an Error.

Patch modified accordingly.

outsideris · 2018-04-13T04:44:07Z

test/unit/throw.spec.js

@@ -26,6 +26,24 @@ describe('a test that throws', function () {
    });
  });

+  describe('extensible', function () {
+    it('should not crash if throwing non-extensible type', function (done) {
+      var test = new Test('im async and throw string async', function (done2) {


done2 is unused.

Neither it is in several places in the same file (I just reused an existing test), see :

L63

L77

L109

L123

The linter doesn't seem to report it for some reason.

It's normal to have done2 argument there to make the test an async test.

I added eslint-ignore comments.

Ooops! I got it.
It needed to make the test as async as you said.

The comment makes it clearer, so it's better anyway :)

fargies · 2018-04-13T08:03:10Z

unused arguments are not checked by the linter, I'd recommend you to add the following lines at the end of your .eslintrc.yml file:

  no-unused-vars:
    - error
    - args: after-used

I have the following result:

.../mocha/lib/interfaces/common.js
  13:45  error  'mocha' is defined but never used  no-unused-vars

.../mocha/test/browser/large.spec.js
  27:49  error  'done' is defined but never used  no-unused-vars

.../mocha/test/integration/fixtures/simple-reporter.js
   9:32  error  'suite' is defined but never used  no-unused-vars
  13:37  error  'err' is defined but never used    no-unused-vars
  17:31  error  'test' is defined but never used   no-unused-vars
  21:41  error  'err' is defined but never used    no-unused-vars

.../mocha/test/integration/hook-err.spec.js
  215:26  error  'line' is defined but never used  no-unused-vars

.../mocha/test/integration/reporters.spec.js
  49:54  error  'result' is defined but never used  no-unused-vars

.../mocha/test/jsapi/index.js
  27:25  error  'test' is defined but never used  no-unused-vars

.../mocha/test/node-unit/color.spec.js
  12:77  error  'stderr' is defined but never used  no-unused-vars

.../mocha/test/reporters/helpers.js
  136:3  error  'expectedBody' is defined but never used  no-unused-vars

.../mocha/test/unit/runnable.spec.js
  101:47  error  'done' is defined but never used      no-unused-vars
  289:52  error  'done' is defined but never used      no-unused-vars
  300:52  error  'done' is defined but never used      no-unused-vars
  314:52  error  'done' is defined but never used      no-unused-vars
  370:50  error  'done' is defined but never used      no-unused-vars
  388:38  error  'rejected' is defined but never used  no-unused-vars
  404:38  error  'rejected' is defined but never used  no-unused-vars

.../mocha/test/unit/throw.spec.js
   31:72  error  'done2' is defined but never used  no-unused-vars
   63:74  error  'done2' is defined but never used  no-unused-vars
   77:75  error  'done2' is defined but never used  no-unused-vars
  109:69  error  'done2' is defined but never used  no-unused-vars
  123:70  error  'done2' is defined but never used  no-unused-vars

Not sure that those aren't false-positive, and definitely not related to this modification, so won't touch this here.

plroebuck · 2018-04-24T23:56:22Z

Would prefer no-unused-vars: ["error", { "args": "none" }] if "eslint" can't add a different rule that can tell the difference between variables and function arguments. The function signature is just that -- if you don't use all its arguments, that's not an error.

plroebuck · 2018-08-29T15:00:25Z

lib/runner.js

@@ -704,6 +704,9 @@ Runner.prototype.uncaught = function (err) {
    debug('uncaught undefined exception');
    err = undefinedError();
  }
+  if (!(err instanceof Error || (err && typeof err.message === 'string'))) {
+    err = new Error('the ' + type(err) + ' ' + stringify(err) + ' was thrown, throw an Error :)');


Maybe create a string2Error function and have both .uncaught() [here] and .fail() [here] call it? DRY.

plroebuck · 2018-08-29T15:03:27Z

test/unit/throw.spec.js

@@ -26,6 +26,25 @@ describe('a test that throws', function () {
    });
  });

+  describe('extensible', function () {


ticky-tack, but aren't you rather testing "non-extensible"?

plroebuck · 2018-08-29T15:11:24Z

test/unit/throw.spec.js

+          throw 'error';
+        });
+      });
+      suite.addTest(test);


Possibly outside the scope, but rather than repeating these lines over and over, why don't we just use a function runTest(test, done) instead?

plroebuck · 2018-08-29T15:27:49Z

@fargies, I like this PR (making async work like sync). But it may be problematic to integrate. Typical workflow would be to create a local issue-related branch (e.g., "issue3320"), make your changes there, then push that branch to GitHub for the PR. Then the GitHub (remote) branch is eventually merged into a release. But not directly to master.

- Any non-extensible type thrown in an async callback raises an exception in Mocha runner, stopping tests.

fargies · 2018-08-30T10:32:08Z

Modification updated.

I can rename my remote branch, but where should I make a merge request ?

plroebuck · 2018-08-30T14:08:14Z

lib/runner.js

-        ' was thrown, throw an Error :)'
-    );
-  }
+  err = string2Error(err);


Somewhere else in the code, let's have:

/** * Check if argument is an instance of Error object or a duck-typed equivalent. * * @private * @param {Object} err - object to check * @param {string} err.message - error message * @returns {boolean} */ function isError(err) { return err instanceof Error || (err && typeof err.message === 'string'); }

And replace the check so it's clear why the other routine would be called.

if (!isError(err)) { err = thrown2Error(err); }

plroebuck · 2018-08-30T14:11:15Z

lib/runner.js

-        ' was thrown, throw an Error :)'
-    );
-  }
+  err = string2Error(err);


And again...

if (!isError(err)) { err = thrown2Error(err); }

plroebuck · 2018-08-30T14:23:58Z

lib/runner.js

+ * @param {Object} err
+ * @return {Error}
+ */
+function string2Error(err) {


Renamed for added clarity.

/** * Converts thrown non-extensible type into proper Error. * * @private * @param {*} thrown - Non-extensible type thrown by code * @return {Error} */ function thrown2Error(thrown) { return new Error( 'the ' + type(thrown) + ' ' + stringify(thrown) + ' was thrown, throw an Error :)' ); }

plroebuck · 2018-08-30T14:38:23Z

Rename your remote branch "issue3320" after updates.

fargies · 2018-08-31T06:51:15Z

New pull request from issue3320 branch -> #3471

boneskull suggested changes Apr 12, 2018

View reviewed changes

boneskull added area: usability concerning user experience or interface semver-minor implementation requires increase of "minor" version number; "features" labels Apr 12, 2018

fargies force-pushed the master branch from c178d02 to 41fe763 Compare April 12, 2018 17:10

outsideris reviewed Apr 13, 2018

View reviewed changes

fargies force-pushed the master branch from c644caa to 41fe763 Compare April 13, 2018 08:03

boneskull force-pushed the master branch 2 times, most recently from 4547268 to 7613521 Compare May 4, 2018 16:47

plroebuck reviewed Aug 29, 2018

View reviewed changes

Native types exceptions can crash Mocha

b7acffa

- Any non-extensible type thrown in an async callback raises an exception in Mocha runner, stopping tests.

fargies force-pushed the master branch from 0dd4db1 to b7acffa Compare August 30, 2018 10:21

fargies force-pushed the master branch 2 times, most recently from bf19d09 to b7acffa Compare August 30, 2018 10:38

Add string2Error to convert thrown object to Error

70494f7

plroebuck reviewed Aug 30, 2018

View reviewed changes

fargies mentioned this pull request Aug 31, 2018

#3320 Native types exceptions can crash Mocha runner #3471

Merged

fargies closed this Aug 31, 2018

rogerioyuuki mentioned this pull request Nov 1, 2019

Truffle doesn't catch error and crashes mocha runner trufflesuite/truffle#2497

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3320 Native types exceptions can crash Mocha runner #3321

#3320 Native types exceptions can crash Mocha runner #3321

fargies commented Apr 12, 2018

coveralls commented Apr 12, 2018 •

edited

boneskull left a comment

fargies commented Apr 12, 2018

outsideris Apr 13, 2018

fargies Apr 13, 2018

fargies Apr 13, 2018

outsideris Apr 13, 2018

fargies Apr 13, 2018

fargies commented Apr 13, 2018

plroebuck commented Apr 24, 2018

plroebuck Aug 29, 2018 •

edited

plroebuck Aug 29, 2018

plroebuck Aug 29, 2018

plroebuck commented Aug 29, 2018

fargies commented Aug 30, 2018

plroebuck Aug 30, 2018 •

edited

plroebuck Aug 30, 2018

plroebuck Aug 30, 2018 •

edited

plroebuck Aug 30, 2018 •

edited

plroebuck commented Aug 30, 2018

fargies commented Aug 31, 2018

#3320 Native types exceptions can crash Mocha runner #3321

#3320 Native types exceptions can crash Mocha runner #3321

Conversation

fargies commented Apr 12, 2018

Requirements

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

coveralls commented Apr 12, 2018 • edited

boneskull left a comment

Choose a reason for hiding this comment

fargies commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fargies commented Apr 13, 2018

plroebuck commented Apr 24, 2018

plroebuck Aug 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plroebuck commented Aug 29, 2018

fargies commented Aug 30, 2018

plroebuck Aug 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plroebuck Aug 30, 2018 • edited

Choose a reason for hiding this comment

plroebuck Aug 30, 2018 • edited

Choose a reason for hiding this comment

plroebuck commented Aug 30, 2018

fargies commented Aug 31, 2018

coveralls commented Apr 12, 2018 •

edited

plroebuck Aug 29, 2018 •

edited

plroebuck Aug 30, 2018 •

edited

plroebuck Aug 30, 2018 •

edited

plroebuck Aug 30, 2018 •

edited