Chai needds a better way to assert the existence of an error

[gabegattis]

I often find myself doing this:

should.exist(err);
err.should.be.an.instanceOf(Error);
err.message.should.equal('this is an error');

This feels pretty awkward. Is there a better way to do this?

If not, something like

should.existError('this is an error');

would be great.

[keithamus]

@gabegattis thanks for the issue.

You could use chaining here to make this look a little nicer, and also read more like natural language:

should.exist(err).and.be.an.instanceOf(Error).with.property('message', 'this is an error');

Hope this is sufficient for you. I'll close this for now, but let's continue the discussion if you have any more thoughts 😄

[gabegattis]

Doing it that way would put everything in one statement, but it is still about the same amount of typing I have to do. That is one of the main things that I am concerned with.

Having a convenience method for this case would make things easier. I understand that we don't want a convenience method for every conceivable use case, but this kind of error checking seems like a very common use case, so I think it is justified.

Having this would also encourage devs to do more thorough error checking. I have seen tests where someone just did should.exist(err) without checking type and message, and the test eventually concealed bugs where err was either not an error or was an unexpected error.

I would be happy to make a pull request if you are willing to merge something like this.

[meeber]

Can your tests be refactored to use the throw assertion (documented here)? Or are you provided the err object in a different way?

[gabegattis]

I do use the throw assertion a lot if I am testing a function that I expect to throw. That is not what this issue is about though. This issue is mainly for functions that callback an error.

Something like

it('should error if given an invalid param', function(done) {
  database.findAccount('thisIsAnInvalidParam', function(err, account) {
    should.exist(err);
    err.should.be.an.instanceOf(Error);
    err.message.should.equal('this is an error');
    done();
  });
});

[meeber]

Gotcha. It makes sense to me for Chai to have an assertion that's exactly like .throw except the target is an existing error object. And then .throw can use that assertion internally, so there's no duplication between the two.

except(myErr).to.be.an.error();
except(myErr).to.be.an.error(TypeError);
except(myErr).to.be.an.error(myErr); // No point in doing this instead of .equal, but supported by .throws
except(myErr).to.be.an.error("some substring");
except(myErr).to.be.an.error(/some regex match/);
except(myErr).to.be.an.error(TypeError, "some substring");
except(myErr).to.be.an.error(TypeError, /some regex match/);

Of course, it suffers from the same problem as #918:

except(myErr).to.be.an.error;  // Looks correct but actually does nothing

For the record, I've found myself wanting this in the past for a slightly different use case: when the error is thrown in a try/catch block in the before section of a test. Rough example:

describe("blah blah", function () 
  let err = "__PRETEST__";

  before(async function () {
    try {
      await someAsyncThing();
    } catch (_err) {
      err = _err;
    }
  });

  it("throws a TypeError blahblah", function () {
    expect(err).to.be.an.error(TypeError, "blahblah");
  });

  it("has code 42", function () {
    expect(err).to.have.property("code", 42);
  });
});

[lucasfcosta]

I think we can have this because Node's error-first callbacks are very common out there and I'm impressed nobody asked for this before. I'm in favor of this because then it would make throws code more clear and still give people another useful assertion.

Also, regarding your second example, I never thought of writing tests this way but I'll start doing it from now on. I always duplicated code inside of test cases and then did different assertions inside both, but since they all depend on the exact same code running (which should be deterministic btw) it makes sense to write the "core" code inside the before section.

Well noticed, Mr. @meeber, +1 for that!

[keithamus]

Agreed for all the reasons above. This .error assertion would also be very helpful in places like chai-as-promised, where we've heard that .throw is not the right fit and using check-error sounded like too much work for them to handle. So big 👍 from me.

PR's welcome! This would be quite a big PR. The best place to start is by refactoring the internal assertThrows function - splitting it into assertError and assertThrows functions -
where assertThrows mostly just try/catches and calls out to assertError. Then the assertError function will need to be aliases to an actual assertion, just like the throws assertions are. Assert will need it's own aliases added just like this one, plus we'll need some tests, just like these.

[meeber]

Just a small warning: There are a couple of PRs open (#920 and #922) that touch the same part of the codebase, so if anyone submits a PR for this prior to the release of v4, there will need to be some follow-up merge conflict resolution.

[meeber]

Gonna start work on this.

2017-07-26 update: Still working on this. Turned out more complicated than I thought. Had to do more refactoring of the error checking logic.

[keithamus]

Hey @gabegattis thanks for the issue.

We've added this to our Roadmap https://github.com/chaijs/chai/projects/2! We'll be releasing chai 5 soon, but for now I'll close this issue because it is tracked on our roadmap.