Chai: Deep equality check for Errors

[janis91]

Hi,
because we use chai and chai-as-promised in our project, we recently tried to use .rejectedWith() in order to check for a custom error:

class HttpError extends Error {
    constructor(statusCode, message) {
        super(message);
        this.statusCode = statusCode;
        Object.setPrototypeOf(this, new.target.prototype);
        this.name = new.target.name;
    }
    toString() {
        return `${this.name}(${this.statusCode}): ${this.message}`;
    }
}

But the following check doesn't work:

it('', async () => {
   const httpError = new HttpError(404, 'Not found.');
   await expect(Promise.reject(new HttpError(404, 'Not found.'))).to.be.rejectedWith(httpError);
});

This is - as @meeber kindly pointed out here - because of the strict === comparison of the throws() assertion in Chai itself, as chai-as-promised just emulates Chai's throws().

I personally think that it'd be more useful for errors to have a deep equality check in place. Something like suggested in the linked issue:
.to.deep.throw() => to.be.deep.rejectedWith()
This deep equality check can't be the same as the normal deep equality check for objects, because of the Error's stack-trace property, which of course will be not the same in the most cases.
I guess this approach would also not break the existing usage, so it might be a bit simpler than changing the error comparison as a whole.

[keithamus]

Hey @janis91 thanks for the issue.

I'm inclined to agree. But actually right now the deep-eql algorithm doesn't do deep equality on errors, as you mention the .stack property is never going to be equal. Are two errors truly deep-eql if they have different stacks? This is a point of contention which has led us this far.

I would say it might be an easier point to make that .[error|throws|rejectedWith](someErrorInstance) could be special cased and documented that it'll only compare the message and constructor properties. However another problem we have is that often times people tack on other properties onto error objects, e.g. .code.

I have no firm opinion any way on this, just presenting the current state of mind here.

[janis91]

Hi @keithamus thanks for sharing your thoughts.

I basically like your suggestion. But I would go even further: Beside that we have to distinguish if it is an instanceof Error first, as it is possible to throw any expression in JS, I would suggest to compare every property beside the .stack property. Because as you already pointed out right, it is also possible and in my opinion, even very important that someone can extend the Error class and put some more information into it. This information shouldn't be lost when comparing them.

I hope this goes in a right way.

[keithamus]

While it is possible to throw any expression we should really only encourage the throwing of Error objects, so I'm not so interested in supporting generic objects.

Let's get more eyes on this problem before moving toward a particular solution. I guess @meeber might have some feedback, given https://github.com/domenic/chai-as-promised/issues/226#issuecomment-337773181

[meeber]

I agree with @janis91; I think our deep equality algorithm should special-case Error objects by ignoring the .stack property, but consider all other own and inherited enumerable properties.

Although the semantics are a bit weird, I also think Chai should support .deep.throw(errInstance) in order to perform a deep equality comparison against the thrown error, instead of strict equality. Such a change could ride in the wake of #1021.

[janis91]

@meeber @keithamus Is there any progress on this, so far? I could try to help if you let me know where I should start. I think a .deep.throw is a really nice suggestion. Is the PR #1021 still alive?

[meeber]

@janis91 Things are slow-going at the moment, but this is still on the radar, including a semi-related issue in #907. PR #1021 is still alive, waiting on a follow-up review of https://github.com/chaijs/check-error/pull/18.

[keithamus]

We've got a lot of PRs and issues open around this. It's definitely something on the radar, and we're trying to close a lot of issues so we can stay focussed. Let's close this so we can keep focussed.

[MicahZoltu]

I just upgraded Chai and a test that was previously passing is now failing because the following is true:

expect(new Error('apple')).to.not.deep.equal(new Error('apple'));

Reading through issue backlog, it appears that Error comparisons used to _only_ do a type check during deep compare, but were changed to do full comparisons. Unfortunately this puts me in an awkward situation because in my real scenario the error is a property on a nested object, so I don't see any way to compare other than to _manually_ compare every property of the entire object tree _except_ for the error.

Are there any workarounds for those of us that were broken by the behavior change? Is a rollback the most reasonable solution?

[keithamus]

@MicahZoltu we'll soon have an .to.be.an.error() matcher which will allow you to do comparisons like .to.be.an.error(Error, /apple/). We're going to put a lot of focus into releasing Chai 5 which will include matchers like this.

[MicahZoltu]

@keithamus Will that be able to be used as part of a .deep.equal(...)? My real problem looks like:

const error1 = [{a: 'hello', b: new Error('goodbye')}, ...]
const error2 = [{a: 'hello', b: new Error('goodbye')}, ...]
expect(error1).to.deep.equal(error2)

[keithamus]

@MicahZoltu part of the plan for Chai 5 is to have matchers, so you could do something like:

expect(error1).to.deep.equal({
  a: 'hello',
  b: expect.to.be.an.error(Error, /goodbye/)
})

The exact syntax and mechanics are not yet hashed out, but it would likely be close to something like this.

[janis91]

@keithamus That'd be really nice. I think this is quite a big step forward. I like this approach.

[tusharmath]

Actually the assert implementation by node.js actually works —

import * as assert from 'assert'

assert.deepStrictEqual(
  new Error('A'), 
  new Error('A')
) // passes