adding checkError utility to compare errors against criteria

[vikki]

Refactoring as per chaijs/chai-as-promised#47 to add a checkError utility. The purpose of this is to make it easier for plug ins to use the same logic as chai when verifying errors.

A couple of things I wasn't sure about :

• I've kept the API the same as the variable names in the original contents of assertThrows (i.e. the params passed into checkError on assertionArgs) but it might be clearer if they were renamed.
• checkError currently returns an array because in some cases the original code asserted more than once. This makes it a bit more complex but also more flexible.
• The only pre-existing test(s) I had to change were the ones who check for a message with ReferenceError without quotes (e.g. https://github.com/chaijs/chai/blob/5c5cbd1c36355a1261b3652da927ded36803174b/test/expect.js#l875). A lot of other tests specified Errors in quotes, when specified as constructors, instances or matches, in quotes, and I think its more consistent now, but I might just have misunderstood the logic.

Anyway happy to fix up stuff if this is helpful. Thanks!

[vikki]

Hi guys, does this look like it could work? Happy to make any changes as necessary.

[keithamus]

Hi @vikki sorry for not looking at this yet. I'll take a look in the next couple of days and leave some proper feedback. Feel free to pester me if I haven't by then 😄

[keithamus]

Okay @vikki, some general comments to start us off with this PR:

• There is no need to compile chai.js - it is done on every build. Would you kindly remove it from the commit please? This makes it easier to keep a good git history, and easier to see what you've done in this commit.
• I'm not sure I like the idea of having one function (checkError) that returns an Array of errors. For starters, while this certainly does move out some of the logic from the method in assertions.js, it means there's a whole new set of logic just there to decipher what checkError is giving back to us.
• The assertion code is a little difficult to follow. I think this is a symptom of the above issue. The main cause is that it builds up an error message through objects and then does one assert at the end, rather than just making asserts in if/else statements (e.g. have a look at assertAbove as a random example - its just two different this.asserts wrapped in an if, throw should be similar.
• Typically, chai always does a this.assert - even if the result is true, while it looks that with foundErrors.forEach there is a possibility of throw never calling this.assert, which breaks coding conventions and is also a little worrying as assertions could pass through side-effects.

Here is what I imagine throw to look like, which gives you some idea of what the checkError api could look like.

function assertThrows (constructor, errorLike, msg) {
   if (msg) {
     flag(this, 'message', msg);
   }
   var obj = flag(this, 'object');
   new Assertion(obj, msg).is.a('function');

   try {
     obj();
   } catch (err) {
     caughtError = err;
   }

   if (!caughtError) {
     this.assert(
       caughtError,
       'expected #{this} to throw an error but did not',
       'expected #{this} to not throw an error but threw #{act}',
       errorLike,
       caughtError
     );
     return this;
   }

   var expectedConstructorName = checkError.getConstructorName(errorLike);
   var actualConstructorName = checkError.getConstructorName(caughtError);
   var expectedMessage = checkError.getMessage(errorLike);
   var actualMessage = checkError.getMessage(caughtError);

   this.assert(
     checkError.sameInstance(caughtError, errorLike),
     'expected #{this} to throw #{exp} but #{act} was thrown',
     'expected #{this} to not throw #{exp}',
     expectedConstructorName,
     actualConstructorName
   );

   this.assert(
     checkError.sameConstructor(caughtError, errorLike),
     'expected #{this} to throw #{exp} but #{act} was thrown',
     'expected #{this} to not throw #{exp}',
     expectedConstructorName,
     actualConstructorName
   );

   this.assert(
     checkError.sameMessage(caughtError, errorLike),
     'expected #{this} to throw error matching #{exp} but got #{act}',
     'expected #{this} to not throw #{exp}',
     expectedMessage,
     actualMessage
   );

   flag(this, 'object', err); 
 }

This keeps the throw function to a set of simple asserts, which is easy to read and has no logic to decipher (apart from checking that an error was actually thrown) and checkError has a simple API for interfacing with Error and "ErrorLike" (String, RegExp, Error instance, and constructor function) objects. With the above example I'd expect:

• sameInstance checks the caughtError is an instance of the errorLike constructor (if it has one). If errorLike is not an Error instance, then it is assumed to be Error. Always returns a boolean
• sameConstructor is a more rigid check to ensure that if errorLike is an instance of Error - that the specific constructor from errorLike matches caughtError - otherwise if errorLike is not an Error instance, return true. Always returns a boolean
• sameMessage checks against the errorLike message to ensure that it is equal to (in whole or part) caughtError - if errorLike is a RegExp, do the appropriate thing. Always returns a boolean.

I understand this is all quite a change from your original PR - I'd love to hear your thoughts on this, and it certainly isn't meant to be fully prescriptive; just my interpretation on how I'd implement it.

[keithamus]

Hey @vikki. What do you think about the feedback I gave? Are you okay to make the changes requested?

[vikki]

Hey @keithamus, Sorry I missed your responses in the end. So I originally wrote the PR to deviate as little as possible from the original code, and also keeping to what I understood by the original comments in chaijs/chai-as-promised#47

I like that your implementation of assertThrows is certainly clearer than what I ended up with, but it seems a shame that so much logic has to remain outside of the helper (checkError). I was keen to avoid changing the code that was already there too much but if you'd be open to a more substantial change, in order to make it more readable, then I'll definitely have a go at that :) Will update the branch asap

[keithamus]

@vikki I'm pretty confident in our test suite - so substantial changes are welcome. I'd love to see the ideas you have to get the logic down inside assertThrows. The more logic out of big assertions like this, the better 😄

[lucasfcosta]

Hi everyone, as I was moving this I got confused about the description of the sameInstance method.

According to @keithamus this should be its behaviour:

sameInstance checks the caughtError is an instance of the errorLike constructor (if it has one). If errorLike is not an Error instance, then it is assumed to be Error. Always returns a boolean

But when should it return true and when should it return false?

By reading the name of this method I expect it to strictly compare two objects if they're both instances of Error, otherwise they're already compatible.

After taking a look at how throws currently works, this is the implementation I was thinking about:

function compatibleInstance(thrown, errorLike) {
  if (errorLike instanceof Error) {
    return thrown === errorLike;
  } else {
    return true;
  }
}

Am I doing it wrong? If the answer for the previous question is "yes", what do you guys think should be compatibleInstance's behavior?

Thanks in advance 😄

[keithamus]

@lucasfcosta I think my intent with sameInstance was that if the caughtError and errorLike are both error instances, then they should be checked for referential equality (much like the code you have written). However now I feel like we're close enough to 4.0 that we could have a breaking change whereby we only check the constructor & message, as that's the behaviour most of our users want. Thoughts?

[lucasfcosta]

Hi @keithamus ,
I think we could have both behaviors, therefore this won't be a breaking change and we also won't eliminate this feature.
The way I'm currently coding guarantees that when someone gives throws a constructor, the instances will be compatible and compatibleConstructor will compare the thrown error constructor with the passed argument (expected) instead of comparing it to the expected.constructor.
These are simple checks and won't hurt performance too.

However, I agree with you that's not a feature which is often used, so dropping it won't be bad too, but I just think that if we can keep it, there's no downside to it.

What do you think it's better? Both options seem viable.
Thanks for sharing your opinion 😄

[keithamus]

Hey @vikki thanks for your great work with this. I'm going to close down this PR as #683 supersedes it. Please continue to contribute to chai in the future, as we'd love to have more of your awesome input!