Fixes an issues when both .throws() and not.throws() produce failures.

[jails]

This PR solves an issue when both .throws() and not.throws() produce failures. Exemple:

expect(function() { throw new Error(); }).to.throw(Error, 'hello'); // fails
expect(function() { throw new Error(); }).to.not.throw(Error, 'hello'); // fails too

Imo the notspec above should pass.

This PR solves this issue and also modify the initial behavior by perfoming lazy comparison on exceptions to allows this syntax to make it works out of the box:

expect(function() { throw new Error('testing'); }).to.throw(new Error('testing')); 

Indeed previoulsy a === comparison was made on objects which requires the following syntax:

expect(function() { throw new Error('testing'); }).to.throw(Error, 'testing'); 

Now both syntax can be used either way. the only difference is one is assuming the error message to be strictly identical since the second way only requires testing to be included in the error message.

Solves #436, #430 and probaly #470

[jails]

Shoud I still maintain it ?

[keithamus]

Thanks for the PR @jails. It's a bit of a big one, so I'm going to stick it on my todo list for this weekend. Just sit tight and I'll definitely get to looking at it!

[niftylettuce]

👍

[jdorfman]

👍

[diego-pacheco]

+1

[s-silva]

+1

[keithamus]

I'd prefer an explicit String() conversion here, as if a class has valueOf() it will be called over toString(), e.g.:

'' + { valueOf() { return 1; }, toString() { return 'hello'; } }

[keithamus]

@jails as you can see above I've made some notes about the code.

I really like the general direction you're going with this, I feel like the error checking could be made much simpler though. We can distill this down to two essential checks:

• Same constructor
  • This check is ignored when passed a single regexp
• Same message
  • Skipped if not given a message, or if given error doesn't have one.

We also currently check for referential equality, although I really wonder how much utility that has... I'd happily release a breaking version where we drop referential equality checking, but I suppose it is an easy piece of code to shortcut the longhand logic and just say if (givenError === thrownError).

Anyway - my point is that I get the feeling the functions should match these checks. You're very close with what you have, but I think it could be more concise, following the above list

[jails]

Thanks for the feedback, I updated the PR with the following changes:

• an explicit string conversion here: https://github.com/chaijs/chai/pull/551/files#diff-6f8be24f0197a0249fde03c55e7397dbR1292
• using message.indexOf(expected.errMsg) !== -1 instead of !!~

For the code duplication part I wasn't able to find a better option. So I'll leave it up to you since you know the util codebase better than me.

Otherwise for the boths checks, I'm not sure to get what you meant since it's actually the implemented behavior. However I renamed similarException()/similarMessage() by compatibleConstructor()/compatibleMessage() to make it more clear.

So the constructor check is ignored when using a single regexp (or a string):
https://github.com/chaijs/chai/pull/551/files#diff-c7f56e7ef2d6759cc445c937a78f61cfR932

The exception message is also ignored when a exception with no message is expected:
https://github.com/chaijs/chai/pull/551/files#diff-c7f56e7ef2d6759cc445c937a78f61cfR902

Finally I wasn't able to find any alternative for the referential equality except having a BC break here.
Indeed when givenError === thrownError the constructor and the message will be both comptatible so the test will pass either way. However the to.not.throw() will require an explicit failure when givenError !== thrownError which is not compatible with this implementation which is looking for compatible exeptions even when givenError !== thrownError. So I'm not sure we can deal with both strategies here.

Hope it makes sense.

[keithamus]

Sorry @jails I should clarify better.

What I mean to say about the checks, is that parseParams is there to convert the args into an object that can be consumed by compatibleMessage and compatibleConstructor - but then these functions do the same typechecking, so it seems like parseParams could just go.

I feel like something like this approximation would be close to passing our requirements (and tests)

function compatibleErrorConstructor (expected, thrown) {
 if (typeof expected === 'undefined' || expected instanceof RegExp || typeof expected === 'string' || expected === thrown) {
   return true;
 } else if (typeof expected === 'function') {
   return thrown instanceof expected;
 } else if (typeof expected.constructor === 'function') {
   return thrown instanceof expected.constructor;
 }
 return false;
}

function compatibleErrorMessage (expected, thrown) {
 if (typeof expected === 'undefined' || typeof expected === 'function' || expected === thrown) {
   return true;
 }
 var isThrownedException = typeof thrown === 'object' && thrown.hasOwnProperty('message');
 var message = isThrownedException ? thrown.message : '' + String(thrown);
 if (expected instanceof RegExp) {
   return expected.test(message);
 } else if (typeof expected === 'object' && expected.hasOwnProperty('message')) {
   return expected.message === '' || message === expected.message;
 }
 return message.indexOf(expected) !== -1;
}
function assertThrows (constructor, errMsg, msg) {
 if (msg) flag(this, 'message', msg);
 var obj = flag(this, 'object');
 new Assertion(obj, msg).is.a('function');
 var expectedString = ((constructor||{}).name || 'Error') + (errMsg ? ': ' + errMsg : '');

 var thrown;
 try {
   obj();
 } catch (err) {
   thrown = err;
 }

 if (thrown === undefined) {
   this.assert(
       false
     , 'expected #{this} to throw ' + expectedString
     , 'expected #{this} to not throw ' + expectedString
   );
   return this;
 }

 var hasCompatibleConstructor = compatibleErrorConstructor(constructor, thrown);
 var hasCompatibleMessage = compatibleErrorMessage(errMsg || constructor, thrown);
 this.assert(
     hasCompatibleConstructor && hasCompatibleMessage
   , 'expected #{this} to throw error #{exp} but got #{act}'
   , 'expected #{this} to not throw error #{exp}'
   , expectedString
   , String(thrown)
 );
 flag(this, 'object', thrown);
 return this;
};

(Note how most of the detection logic is pulled into the compatible* methods, and the main assert method does no type checking).

[jails]

Definitely 👍 ! It was just a proof of concept so feel free to use the implementation you prefer !

[keithamus]

@jails sorry, that comment wasn't meant to discourage you from continuing the work on this PR - more as notes to improve it. I'd definitely like to merge this, but my aim was to steer you more into a direction like the above (incomplete) implementation.

Please, if you would like to continue working on this PR, I'd really like it you to do so and I really do want to merge your work! Do you have any critical thoughts of the sketch I gave? Do you think you could work your PR into something like that?

[niftylettuce]

👍 Let's get thsi into chai!

[oravecz]

I was going to address a shortcoming I find, but it also impacts this part of the code. Perhaps I can explain and have this issue addressed, or I can open a different issue. Otherwise, perhaps there is a better approach to handle what I am struggling with?

The throws() assertion seems to deal with Error objects well, but not the throwing of Objects.

it( 'should test an exception', function () {
    var func = function () {
        throw { code: 400, message: 'some exception' }
    }
    func.should.throw( { code: 400, message: 'some exception' } ) // passes
    func.should.throw( /some exception/ ) // fails
} )

The second assertion fails with:

 AssertionError: expected [Function] to throw error matching /some exception/ but got '[object Object]'

There is code in place that seems to attempt to pull a message property from an Error type, but it seems a bit convoluted, and ends up producing the never-wanted [object Object] string for comparison. The code in question has been removed by this pull request, but I would expect both of my above assertions to pass in the new code.

[keithamus]

Hey @oravecz - that is somewhat intentional. You shouldn't be throwing non-error objects due to the fact that they provide no stack trace information, which causes problems when debugging. You'll note that any error state within JavaScript or the DOM is always an error object. However if you feel strongly about it you should raise a separate issue and we can discuss it further there.

[oravecz]

Turns out there is a workaround...

it( 'should test an exception', function () {
    var func = function () {
        throw { code: 400, message: 'some exception' }
    }.should.throw().and.has.property( 'message' ).to.match( /exception/ )
} )                

It was unexpected to me that the exception object would be available to inspect after a throw() call.

[JamesMGreene]

+1

[lucasfcosta]

@keithamus' implementation seems adequate and shrugs some code (also making things more efficient).
When it comes to referential equality I think we could just add a function like isSameError (where we do some type checking and then expectedErr === thrownErr) and then instead of using hasCompatibleConstructor && hasCompatibleMessage we could use isSameError || (hasCompatibleConstructor && hasCompatibleMessage).

Are you still working on this @jails? Your PR seems great and I think it would be awesome to merge it if we took @keithamus considerations into account.

Please let me know if you need anything, I would certainly be very happy to help you.

[keithamus]

@lucasfcosta 👍 💯 keep up the good work 😄

[lucasfcosta]

Hello guys, I've asked @jails on twitter if he's still working on this and unfortunately he said that he gave up on this.

I also noticed that @brunops sent a PR (#635) related to this, so I feel like he could use this discussion and code as "inspiration" for his implementation. Would like to take it forward, @brunops?

What are your thoughts on this, @keithamus ?

[keithamus]

@lucasfcosta hold off on this one just for now - I want to make a little lib for checking equality errors, as we have a few places it can be used and also other libs like chai-as-promised could use it. I'll get the lib done over the weekend and we can look to integrate it into the error matchers in Chai.

[keithamus]

But thanks for trying to get this one resolved, and reaching out to @jails 😄

[lucasfcosta]

@keithamus seems awesome!
Let me know if you need any help with it.

[meeber]

@keithamus I know I'm late to the game but I couldn't help but notice how similar this issue is to the global.err function from #677. I wonder if something in the same spirit might be appropriate for assertThrows? This would then allow the global.err to either go away or be a simple wrapper around assertThrows. Examples:

Validate an Error is thrown of any type with any properties:

expect(fn).to.throw();

Validate an Error is thrown that's an instance of the given type with any properties:

expect(fn).to.throw(TypeError);

Validate an Error is thrown that's referentially equal to the given Error:

var err = new TypeError("Test");
expect(fn).to.throw(err);

Validate an Error is thrown that's an instance of the given type with an exact message:

expect(fn).to.throw(TypeError, "oops");

Validate an Error is thrown that's an instance of the given type with a matching message:

expect(fn).to.throw(TypeError, /oops/);

Validate an Error is thrown that's an instance of the given type with deeply equal properties:

expect(fn).to.throw(TypeError, {message: "oops", stack: "Type Error: poo at blahblah"});

Validate an Error is thrown that's an instance of the given type with one or more matching properties:

expect(fn).to.throw(TypeError, {message: "oops", stack: /Type Error: poo at blahblah/});

Validate an Error is thrown of any type with an exact message:

expect(fn).to.throw("oops");

Validate an Error is thrown of any type with a matching message:

expect(fn).to.throw(/oops/);

Validate an Error is thrown of any type with deeply equal properties:

expect(fn).to.throw({message: "oops", stack: "Type Error: doh at blahblah"});

Validate an Error is thrown of any type with one or more matching properties:

expect(fn).to.throw({message: "oops", stack: /Type Error: doh at blahblah/});

The not.throws would then pass as long as an Error wasn't thrown of the given type with the given properties.

[lucasfcosta]

Hi @meeber, thanks for your feedback!
Those features look really useful, but I'm not sure about introducing this kind syntax, it doesn't seem as fluid as the rest of our API. Currently, as .throws changes the assertion subject, you can chain other assertions and test things against the Error thrown, including checking the stack. You can see some tests that look like it at #661 (take a look at the test files).

Giving a single String/RegExp argument to match only the error match, however, seems like an excellent idea, if @keithamus agrees I'd be happy to include it in the PR I'm working on, so lets hear his opinion.

EDIT: Oh, I forgot this is already supported, as said by @meeber below, srry

Currently I'm working to move checkError into a separate module and make the .throws assertion simpler, as said on #457. I may finish it until the end of this weekend and then we can review and define features/behaviours in a more solid ground (if you want to).

Thanks again for sharing your ideas! 😄

[meeber]

@lucasfcosta Thanks for the reply! I can't take credit for the single argument String / RegExp; it's already a part of the current functionality. The webpage shows the following being allowed (so I really only added property matching in my suggestion):

var err = new ReferenceError('This is a bad function.');
var fn = function () { throw err; }
expect(fn).to.throw(ReferenceError);
expect(fn).to.throw(Error);
expect(fn).to.throw(/bad function/);
expect(fn).to.not.throw('good function');
expect(fn).to.throw(ReferenceError, /bad function/);
expect(fn).to.throw(err);

You make a good point about fluidity. The current implementation is a bit convoluted with the argument overloading, and my suggestion exacerbates that further. By instead relying on chained assertions, you lose a bit of succinctness in the API, but you gain sweet simplicity and cleaner functions. Easier to test too. Hard to argue with that ;)

I look forward to the new modular approach!

[keithamus]

@lucasfcosta do we need to do anything with this issue now we have #683 merged?

[lucasfcosta]

@keithamus I think we're done with this one since the spec is now fully compliant with the behavior described in the first post, we even have tests similar to the ones on the first post of this one.

Before closing this one I would also like to thank everyone here, especially @jails for contributing and sharing your thoughts on this matter. Great job everyone!

[keithamus]

Great work @lucasfcosta.

Also @jails thanks so much for working on this, and thank you so much for your patience through all of this - even after it has ultimately not been merged. Please don't let us closing this dissuade you from making more awesome contributions like this!