#3320 Native types exceptions can crash Mocha runner

[fargies]

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

See issue #3320

See PR #3321 for additional discussion.

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

Fixes #3320

[plroebuck]

Sorry, I didn't think to mention this earlier - we should have a test for the original code as well since its implementation was (technically) modified. Could you make your describe() in "throw.spec.js" mirror the other two existing ones, adding missing the 'test (sync/async), throw sync' cases for non-extensible type. Make your test descriptions for the three test cases match those in the other describe()s as well.

In other words, add equivalent of this and this prior to the test in your describe().

[plroebuck]

Modify your PR above as follows:

• Update last line of original description.

See PR #3321 for additional discussion.

• Add the following text under the "Applicable Issues" section (so the issue will be closed automatically upon merge and label this as a bug fix).

Fixes #3320
(semver-patch)

[plroebuck]

After these last few changes are approved, squash your commits so your PR will be ready to merge. If needed, a tutorial on doing this can be found here.

[outsideris]

Thank you.

[plroebuck]

It's still missing the addition of other two test cases.

[fargies]

Sorry, I don't understand what you mean by "a test for the original code", the test-case that has been added will fail if you revert the modification which is what I understand in "a test for the original code".

As described in the issue #3320 this behaviour is inconsistent in Mocha and not happening in sync tests, thus the bug can't be tested and displayed when it's not happening.

This very small modification is already 6 months old, I've reworked it 3 times to please you for cosmetic changes, not sure if it's worth the effort.

Thank you

[plroebuck]

@fargies Outside of a comment about your proposed eslint rule changes, I had absolutely nothing to do with the previous PR until Aug29. At that time, I read through your issue and proposed change, reviewed it, and attempted to help push it through. On Aug31, I requested a couple extra tests be added. As you never added them (nor mentioned any confusion over what was requested), I don't find it fair for you to complain about the time required to get things merged after waiting a month before even responding. What you call cosmetic changes, I call maintenance - required to keep the codebase legible and less spaghetti-like.

[plroebuck]

Replace your changes in the file "throw.spec.js" with the code below (which adds requested tests for the other two cases for non-extensible type).

  describe('non-extensible', function() {
    it('should not pass if throwing sync and test is sync', function(done) {
      var test = new Test('im sync and throw string sync', function() {
        throw 'non-extensible';
      });
      suite.addTest(test);
      runner = new Runner(suite);
      runner.on('end', function() {
        expect(runner.failures, 'to be', 1);
        expect(test.state, 'to be', 'failed');
        done();
      });
      runner.run();
    });

    it('should not pass if throwing sync and test is async', function(done) {
      var test = new Test('im async and throw string sync', function(done2) {
        throw 'non-extensible';
      });
      suite.addTest(test);
      runner = new Runner(suite);
      runner.on('end', function() {
        expect(runner.failures, 'to be', 1);
        expect(test.state, 'to be', 'failed');
        done();
      });
      runner.run();
    });

    it('should not pass if throwing async and test is async', function(done) {
      var test = new Test('im async and throw string async', function(done2) {
        process.nextTick(function() {
          throw 'non-extensible';
        });
      });
      suite.addTest(test);
      runner = new Runner(suite);
      runner.on('end', function() {
        expect(runner.failures, 'to be', 1);
        expect(test.state, 'to be', 'failed');
        done();
      });
      runner.run();
    });
  });

[fargies]

Sorry, I did not understood that you were requesting more tests on Aug 31, I thought you were talking about the test description itself, I just realised it when reading your last comment.

[coveralls]

Coverage increased (+0.1%) to 90.405% when pulling c43744b on fargies:issue3320 into 04469bf on mochajs:master.

[plroebuck]

Looks good. Approved.