Avoid error instantiation if possible on assertThrows (aka assert.throws). Closes #220

[lorenzogrv]

Given a constructor as function, try to get the error name from its prototype or itself before relying on the old behaviour. Closes #220

[vbardales]

+1

[qraynaud]

+1

[qraynaud]

The problem is that the current patch breaks 2 tests. It should not. Maybe we could write something like that instead to have all tests running but still making a best effort out of it :

name = constructor.prototype.name || constructor.name;
if (name === 'Error' && constructor !== Error) {
  name = new constructor().name;
}

Wouldn't this be "better" ? This would less likely break existing tests but still prevent constructing an error as much as possible...

[lorenzogrv]

Hi!

This was my first pull request to an open source project, so I expected to do something wrongly 😄.

@qraynaud Your proposition seems good enough for me as its principle is the same: Given an error constructor as function, try to get the error name from its prototype or itself before relying on the old behaviour.

Just to note, name = new constructor().name; should be name = ( new constructor() ).name;.

I'm going to test it this time, and commit if everything succeed.

[lorenzogrv]

Everything works fine with the @qraynaud suggestion. I created a new pull request to avoid useless comits, so I'm closing this one now. Please follow to #232 for further discussion.

[RobinQu]

Hi, everyone.

The code above really screwed me up.

name = constructor.prototype.name || constructor.name;

Imagine you are going to make a custom error class.

function ValidationError(message, value) {
    this.message = message;
    this.value = value;
    Error.call(this);
    this.name = this.constructor.name;
  }
ValidationError.prototype = new Error();

And then, we meet the following:

if (name === 'Error' && constructor !== Error) {
        name = (new constructor()).name;
}

Are you creating an instance just for detecting its name?

Although it seems fine for most circumstances, a slight more complicated error class will make unexpected errors:

function ValidationError(message, value) {
    this.message = message;
    this.value = value;

    //Watch this
     assert(this.message && this.value, 'both message and value are required');
    Error.call(this);
    this.name = this.constructor.name;
  }
ValidationError.prototype = new Error();

Many custom constructors require parameter checks, and a simple constructor call with no arguments cause disasters.

I really hope we could make better decisions about the name detection.

[RobinQu]

In fact, I am writing tests for code that uses https://github.com/composed-validations/composed-validations

And composed-validations have many custom error classes. When I am trying to do tests like:

expect(function() {validator.test('http://taobao.com/'); }).to.throw(cv.MultiValidationError)

I got an error related to the constructed call mentioned above:

     TypeError: Cannot read property 'length' of undefined
    at Object.module.exports.map (/Workspace/taobao-tms/tms-validator/node_modules/composed-validations/lib/util.coffee:43:5)
    at MultiValidationError.module.exports.MultiValidationError.errorMessages (/Workspace/taobao-tms/tms-validator/node_modules/composed-validations/lib/errors/multi_validation_error.coffee:9:7)
    at MultiValidationError.errorMessages (/Workspace/taobao-tms/tms-validator/node_modules/composed-validations/lib/errors/multi_validation_error.coffee:1:1)
    at new MultiValidationError (/Workspace/taobao-tms/tms-validator/node_modules/composed-validations/lib/errors/multi_validation_error.coffee:6:12)
    at Assertion.assertThrows (/Workspace/taobao-tms/tms-validator/node_modules/chai/lib/chai/core/assertions.js:1152:17)
    at Assertion.ctx.(anonymous function) [as throw] (/Workspace/taobao-tms/tms-validator/node_modules/chai/lib/chai/utils/addMethod.js:40:25)
    at Context.<anonymous> (/Workspace/taobao-tms/tms-validator/test/url_test.coffee:12:7)
    at callFn (/Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runnable.js:266:21)
    at Test.Runnable.run (/Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runnable.js:259:7)
    at Runner.runTest (/Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runner.js:387:10)
    at /Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runner.js:470:12
    at next (/Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runner.js:312:14)
    at /Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runner.js:322:7
    at next (/Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runner.js:257:23)
    at Immediate._onImmediate (/Workspace/taobao-tms/tms-validator/node_modules/mocha/lib/runner.js:289:5)
    at processImmediate [as _immediateCallback] (timers.js:358:17)

Better ideas?

[keithamus]

@RobinQu it looks as though the error is coming from the composed-validations codebase, because of required arguments of the MultiValidationError constructor. MultiValidationError - despite extending from Error, does not offer the same contract, and the constructor has a chance of throwing a new error (because it is trying to map on undefined). I've raised a PR which will fix this in your tests.