Throw using checkError module

[lucasfcosta]

Hello everyone!
This one is pretty big and we've got plenty of special cases at the throws assertion, so be careful when reviewing 😄

This one aims to solve an issue for the 4.0.0 release (as stated on our Roadmap #457) and finish #470 implementation. I thought that before moving checkError to a repository of its own it would be nice to have it reviewed and merged by more people here and only then we would move it to a new repo, but I'll do what you guys think it's better, this is just my opinion, please let me know if you disagree 😉

---

This PR includes:

• The checkError module including DocStrings, compatibleInstance, compatibleMessage, compatibleConstructor, getConstructorName and getMessage
• Tests for each one of the checkError functions
• A full BC refactor of the throws assertion in order for it to use checkError

I couldn't make the throws assertion more simple than this, because we've got lots of edge/special cases, so this is the most generic and clean code I could get to while maintaining BC. If any of you can think of a better way to implement this please share your idea, it will certainly be welcome 😃

I look forward to hearing your feedback 👍

---

EDIT: This will be added to the check-error repo as requested by @keithamus.
I'll write the complete repo structure and submit a PR there as soon as it's done.

Please don't merge this since it requires changes to use the external module instead of the local one.

[keithamus]

Hey @lucasfcosta this is looking great. I've created a check-error repo, if you could please make a PR putting the util js in there, that'd be aaaweeesome. Then we can rework this PR to depend on that lib 😄

[lucasfcosta]

Ah, of course @keithamus!
As you wish buddy, I'll do it, np 😄

[meeber]

Wow! This was quite an undertaking. Great work :D

I have a few questions which I'll add line comments for in a moment.

I also noticed that the issue mentioned in #551 still persists:

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

Is this the desired behavior?

[meeber]

I'm not crazy about the terms errorLike and errMatcher, partially because one is "error" and the other "err", and partially because I feel some context is lost with "errMatcher".

What do you think about errLike and errMsgMatcher?

[lucasfcosta]

Seems awesome!
It really looks semantically better, thanks for the insight.

[lucasfcosta]

Thanks for the review @meeber, great catches!
I'll do some fixes this afternoon.

I will also take a look at the behaviour you've just reported here, that shouldn't happen indeed, I'll make sure to add some tests to cover this.

[lucasfcosta]

Hello everyone,
I've spent the whole afternoon thinking about this and I came to the conclusion that there isn't a "clean" way to pull it off.

This is the code I currently have:

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

    if (errorLike instanceof RegExp || typeof errorLike === 'string') {
      errMsgMatcher = errorLike;
      errorLike = null;
    }

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

    // If we have the negate flag enabled and at least one valid argument it means we do expect an error
    // but we want it to match a given set of criteria
    var everyArgIsUndefined = errorLike === undefined && errMsgMatcher === undefined;

    // Checking if error was thrown
    if (everyArgIsUndefined || !everyArgIsUndefined && !negate) {
      // We need this to display results correctly according to their types
      var errorLikeString = 'an error';
      if (errorLike instanceof Error) {
        errorLikeString = '#{exp}';
      } else if (errorLike) {
        errorLikeString = errorLike.name;
      }

      this.assert(
          caughtErr
        , 'expected #{this} to throw ' + errorLikeString
        , 'expected #{this} to not throw an error but #{act} was thrown'
        , errorLike && errorLike.toString()
        , (caughtErr instanceof Error ?
            caughtErr.toString() : (typeof caughtErr === 'string' ? caughtErr : caughtErr && caughtErr.name))
      );
    }

    // If we've got the negate flag enabled and both args, we should only fail if both aren't compatible
    // See Issue #551@GitHub
    var everyArgIsDefined = errorLike && errMsgMatcher;
    var constructorError;
    var messageError;

    if (errorLike && caughtErr) {
      // We should compare instances only if `errorLike` is an instance of `Error`
      if (errorLike instanceof Error) {
        this.assert(
            _.checkError.compatibleInstance(caughtErr, errorLike)
          , 'expected #{this} to throw #{exp} but #{act} was thrown'
          , 'expected #{this} to not throw #{exp}' + (caughtErr && !negate ? ' but #{act} was thrown' : '')
          , errorLike.toString()
          , caughtErr.toString()
        );
      }

      try {
        this.assert(
            _.checkError.compatibleConstructor(caughtErr, errorLike)
          , 'expected #{this} to throw #{exp} but #{act} was thrown'
          , 'expected #{this} to not throw #{exp}' + (caughtErr ? ' but #{act} was thrown' : '')
          , (errorLike instanceof Error ? errorLike.toString() : errorLike && _.checkError.getConstructorName(errorLike))
          , (caughtErr instanceof Error ? caughtErr.toString() : caughtErr && _.checkError.getConstructorName(caughtErr))
        );
      } catch(e) {
        if (everyArgIsDefined && negate) {
          constructorError = e;
        } else {
          throw e;
        }
      }
    }

    if (errMsgMatcher) {
      // Here we check compatible messages
      var placeholder = 'including';
      if (errMsgMatcher instanceof RegExp) {
        placeholder = 'matching'
      }

      try {
        this.assert(
          _.checkError.compatibleMessage(caughtErr, errMsgMatcher)
          , 'expected #{this} to throw error ' + placeholder + ' #{exp} but got #{act}'
          , 'expected #{this} to throw error not ' + placeholder + ' #{exp}'
          ,  errMsgMatcher
          ,  _.checkError.getMessage(caughtErr)
        );
      } catch(e) {
        if (everyArgIsDefined && negate) {
          messageError = e;
        } else {
          throw e;
        }
      }
    }

    if (everyArgIsDefined && negate) {
      if (constructorError && messageError) {
        throw constructorError;
      }
    }

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

This code passes every test, including:

expect(function() { throw new Error(); }).to.throw(Error, 'hello'); // this should fail
expect(function() { throw new Error(); }).to.not.throw(Error, 'hello'); // this should pass

The problem is: when running, for example, expect(function() { throw new Error('hello'); }).to.not.throw(Error, 'hello') the error we get back is only the error caused by the uncompatible constructors, and not both (constructor and message). To get a perfectly useful error message we would have to throw a new AssertionError, which I'm not sure it's a good idea.

Also, even if I do it, we will still have to change tests, since some of them use not combined with both errorLike and errMsgMatcher arguments, such as this.

So, what do you guys think? Should I throw a new AssertionError with an useful message or should I leave this as it is without any changes to the tests other than the addition of what @meeber suggested above?

[meeber]

What if we performed all of our validations near the top and stored the results in booleans?

For example, we already do this here:

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

But what if we immediately went on to do this too, prior to any assertions:

var hasCompatibleMessage = _.checkError.compatibleMessage(caughtErr, errMatcher)
  , hasCompatibleInstance = _.checkError.compatibleInstance(caughtErr, errorLike)
  , hasCompatibleConstructor = _.checkError.compatibleConstructor(caughtErr, errorLike);

Would that give us more freedom to organize the assertions and make sure the best error message is thrown on a failure (especially for compound tests)? It might also allow some of the errorLike instanceof Error type logic to move into the checkError utility functions.

[lucasfcosta]

Thanks for the suggestion, @meeber! 😄

Unfortunately I'm not sure this would be a very good idea, because we would still need some ifs to make the code work, because the not keyword inverts the assertion expression value and expects it to be true, so we would not be able eliminate if conditions and put them inside the assertion expressions.

Also, doing that prior to the assertions themselves would require us to add logic which is too tightly related to the throw assertion to the check-error module, and I think code should be as modular as possible, IMO we should think about adapting throws to check-error and not the other way round.

And after all, the main problem here still persists:
What Error will we throw when the failure you've described above happens? Should we throw a new AssertionError or only one of the previous errors?
That's what I'm still unsure about.

I'll explore the code some more and see if I can find another solution for this, thanks again for sharing your thoughts, it's been great to have such a good reviewer 😄

---

Edit: I've just pushed a new commit with the latest working version and new tests. It throws a constructor error instead of a new AssertionError for the cases @meeber described. Are we going to define this as the correct behaviour?
I will rebase this to use the external check-error module as soon as check-error is published.

[meeber]

You're right! We definitely don't want any throw-specific logic in the helper functions, and we'd end up doing many of the same if checks anyway.

I think there may still be some value in lifting the compatibleMessage, compatibleInstance, and compatibleConstructor checks out of their this.asserts. Doing so might allow the logic to flow more cleanly and not require having to wrap assertions in try or throw a new assertion.

Before I ramble any further, lemme see if I can cook up an example to express my thoughts in code :D

[meeber]

Okay, after playing around with the code, thinking about the problem, staring at my cat, and eating a chicken sammich, here's my analysis:

The reason this is so tricksy is because the negate logic is fundamentally different than the default logic.

With the default logic, you can proceed assertion-by-assertion and short-circuit the function as soon as an assertion fails. Conversely, with the negate logic, you must perform all relevant assertions ("relevant" being based on which combination of errLike and errMsgMatcher is provided); you can never short-circuit.

Because the default and negate logic are so different, you have three options:

1. Try to conform the negate logic into the default logic path.
2. Try to conform the default logic into the negate logic path.
3. Split default/negate into two separate logic paths, only sharing helper functions and the like.

There are issues with every option. Traditionally this function has gone with the first. I think that worked okay until trying to fix that one bug with the .not.throws behavior. It's my suspicion that the second approach could be significantly cleaner, avoiding the try blocks and having to rethrow errors. I haven't thought much about the third option, so I don't have much of an opinion of it yet, though you've kind of laid some groundwork for it with the checkError module.

Of course, refactoring to use the 2nd or 3rd option is made a bit trickier by having to respect the current failure message precedence for backward compatibility purposes, but only a minor hurdle I think.

[lucasfcosta]

Thanks for the help @meeber!

The issue with the 3rd option is that it will create code repetition, because we've got to do some of the same things for both cases (negate and !negate). The second option may be viable, but I really can't guarantee that until I get to code it effectively, since the throws logic is a very intricate one.

I'm also not a big fan of having to rethrow errors as we're currently doing, so my first goal on the next refactor will be to find a clean way of doing that.

I will take some time this week for a new full refactor of throws now that I know more about its inner workings and I will take your considerations into account.

You've been very helpful, thanks! 😄

Oh, and just in case you want to stare at cats other than yours, here you go, hahaha

[meeber]

You've created a portal to eternal bliss. Well done :D

I agree, the 3rd option likely isn't viable due to code repetition.

An alternative to the current approach that avoids try/catch is to start with var perfectMatch = true, and then slightly change each of the compatibleConstructor, compatibleInstance, and compatibleMessage functions from:

      try {
        this.assert(
            _.checkError.compatibleConstructor(caughtErr, errorLike)
          , 'expected #{this} to throw #{exp} but #{act} was thrown'
          , 'expected #{this} to not throw #{exp}' + (caughtErr ? ' but #{act} was thrown' : '')
          , (errorLike instanceof Error ? errorLike.toString() : errorLike && _.checkError.getConstructorName(errorLike))
          , (caughtErr instanceof Error ? caughtErr.toString() : caughtErr && _.checkError.getConstructorName(caughtErr))
        );
      } catch(e) {
        if (everyArgIsDefined && negate) {
          constructorError = e;
        } else {
          throw e;
        }
      }

To something like this:

        if (!_.checkError.compatibleConstructor(caughtErr, errorLike)) {
          this.assert(
            , false
            , 'expected #{this} to throw #{exp} but #{act} was thrown'
            , 'expected #{this} to not throw #{exp}' + (caughtErr ? ' but #{act} was thrown' : '')
            , (errorLike instanceof Error ? errorLike.toString() : errorLike && _.checkError.getConstructorName(errorLike))
            , (caughtErr instanceof Error ? caughtErr.toString() : caughtErr && _.checkError.getConstructorName(caughtErr))
          );
          perfectMatch = false;
        }

The non-negate behavior remains unchanged: If a compatibility check returns false, it performs the assertion and throws an error, thus short-circuiting the function as expected.

But the negate behavior changes: If a compatibility check returns true, it no longer performs the assertion and throws an error. It no longer short-circuits. Instead, it merely keeps track of whether or not the caughtErr is still a perfectMatch of our errLike and errMsgMatcher.

At the end of the function, if negate is true and caughtErr has an error in it, then we'd perform: this.assert(perfectMatch, blah, blah, blah, blah). If perfectMatch is true, then the negation causes the assertion to fail and throw an error as expected.

The only challenge becomes filling in the blahs with the most appropriate info ("expected {this} to not throw {errLike and/or errMsgMatcher}").

There are other ways to achieve the same thing I'm sure. What it boils down to is not allowing the assertions to short-circuit the logic for negate until it has validated compatibleInstance, compatibleConstructor, and compatibleMessage as needed.

[lucasfcosta]

Good idea, @meeber.
I've taken a look at it yesterday and I think I already know how to do this.
This is gonna be a busy week but I hope to refactor this taking your tips into account until the weekend.
Thanks for the help!

[lucasfcosta]

It's finally done!

My brain was broken into so many pieces I can't even think straight anymore, that was an awesome puzzle!

I made some minor tweaks to @meeber's ideas and I could get it working. I had to create two new variables to indicate whether a check has failed or not and then I would mark them with false if both arguments were defined and we've got the negate turned on instead of failing an assertion.

This was the most elegant way I could get it to work, please feel free to suggest improvements if you find room for any.

I would also like to thank @meeber for the help with this PR, without you this wouldn't have been possible, you've done a great job buddy, thanks! 😄

---

Important:

Currently the fail message is the same as if the constructor check had failed, if you guys want this to be fully BC we can't change it, because maybe (it's a remote possibility, but we can't discard it) someone is expecting an specific error message (the constructor fail message), so changing it could break someone's tests.

What do you guys think about this?
Do you want me to submit another PR against 4.x.x or will we merge this here and change error messages at the 4.x.x branch?

[meeber]

Well done, sir! LGTM. That was not an easy one. I concur with your approach to maintain backward compatibility for now; we can consider improving the error message for a future breaking change.

Thanks for all your hard work :D

[lucasfcosta]

Thanks @meeber!
I wish I could put your name into those commits too, you were as responsible for this as I was!

Well, so @keithamus can merge this if he's happy with it 😄
Thanks folks!

[keithamus]

@lucasfcosta @meeber is anything outstanding on chaijs/check-error#1? We should merge that and release it as a module, so we can then rework this PR to use the external module 😄

[lucasfcosta]

@keithamus chaijs/check-error#1 is complete, it just has minor linting-related improvements for the linting task to pass without errors, but they're the same.

I will refactor this to use the external module as soon as it's published. 😄

[lucasfcosta]

The build is failing because the check-error module doesn't cover a special case regarding poorly-constructed errors. I have tested this with the changes I submitted at this PR at check-error and every test is now passing. As soon as it gets merged there we will be good to go. I will just need to update package.json with the new version number.

I have already:

• Added check-error to the package.json
• Used the external module instead of the local one
• Removed tests for check-error module from this, since they're already being done externally at check-error

[lucasfcosta]

@meeber Done 🎉 🎉

[meeber]

@lucasfcosta Amazing job! LGTM! Where are you going on vacation to celebrate??

[lucasfcosta]

Thanks @meeber. There's two months left until I get my vacation, that's too much time to wait, let's throw a commit party on github 😄

[meeber]

@lucasfcosta I dunno that's more than a little terrifying :D

Someone should definitely double-check this but I think we'll be able to close the following once this gets merged: #470 #551 #596 #634 #635

It doesn't seem to close #430, #436, or #559, but I'm not sure they're using a valid form of the .throw assertion by doing expect(fn).to.throw(new Error("blah"))? If that's an invalid form, then we can suggest a different form, and close them.

It doesn't seem to close #355 either; stack trace still doesn't include original error frame.

[lucasfcosta]

I think we can close all of those except #355.

Here are the reasons for the ones you were not sure:

• expect .to.throw does not work with new Errors #430 because we didn't drop strict comparisons. The strict comparisons (===) will run whenever the user passes an error instance to the .throws assertion. That was a design decision.
• Expect to throw force string equality instead of inclusion #436 because the initial example will now work correctly since when we've got 2 arguments (Error and Message) both will need to match in order for the assertion to pass. Using new Error() will trigger strict comparison instead
• AssertionError: expected [Function] to throw 'Error: baseURI option is required' but 'Error: baseURI option is required' was thrown #559 because as I said before,new triggers strict comparison and also because the example here will now work, as you can see in this test and this test for example.

[meeber]

@lucasfcosta Thanks for double checking! I agree with your assessment. You might be setting the world record for most issues and PRs closed by a single commit.

[meeber]

@keithamus I forgot to ping you on this. Sorreh!

[meeber]

pester pester @keithamus

[meeber]

@lucasfcosta Oh hey I just thought of something. We should add check-error to README.md under "Related Projects".

Also!! I saw you ask a question on a youtube video!! :D

[lucasfcosta]

@meeber Yes we should!
I'll open another PR for that since we've got other modules that are not listed into README.md so we can keep this one on scope.

PS.: Yes I did it! MPJ rocks, hahahaha, I love his channel 😄

[keithamus]

Sorry for taking so long on this one. LGTM!

[lucasfcosta]

@keithamus We all get plenty busy sometimes, don't worry about it Keith.
Thanks for merging. I also ticked the check-error checkbox at our Roadmap.

Thanks everyone for your support 🎉 🎉 🎉 🎉

[meeber]

One last hurdle: Looks like a couple IE tests are failing.

[lucasfcosta]

@meeber I'll check this out after lunch, that's strange since check-error itself works on every IE version.
Thanks for noticing!

[lucasfcosta]

Just updating everyone:

Today I'm gonna take some time to:

• Fix tests on IE as @meeber pointed out
• Do some polishment on the assert interface as we talked about on Apply the same logic in .throws() to .doesNotThrow() so it matche… #635

We will also need some more time to investigate and fix #355 in the future.
I expected these two things to be done until tomorrow.

[keithamus]

#596 and #430 are also still open. Can I ask they also be checked on to see if they can be closed or at least updated with the impact of this merge.