Chai: expect.eql() returns true when deeply comparing different Error objects

[MarcL]

I've run into an issue which I _think_ is a bug when using deeply equals to compare two Error objects. Even though the message of the errors is different the expectation still returns that they're equal.

Here's an example using Mocha with expect().to.eql.

it('this test should fail', () => {
    const expectedError = new Error('An error');
    const error = new Error('A different error');
    expect(error).to.eql(expectedError);
});

Mocha passes this test even though I'd expect this to fail as the two errors have different message strings.

Is this a bug? I couldn't find any similar problems when searching the issues.

Thanks,
Marc

[keithamus]

Hey @MarcL thanks for the issue.

This definitely looks like a bug - we want your assertion to fail. The reason why it's not is a little... nuanced though:

• deep-eql (our deep equality algo) checks all enumerable keys (effectively using `Object.keys) (see here)
• Error, for some reason, sets Error.message to be non-enumerable by default (see here).

This kind of leaves us in a tricky position, because we could solve this with several different solutions, but each one has it's own pitfalls:

• We could switch to Object.getOwnPropertyNames, but this would be a breaking change for lots of people - potentially causing more issues. This also may cause issues because stack is a enumerable property and trying to do equality on this will always fail because the line/column numbers will be different for every error. I am sure we had a discussion about enumerability and deep equality somewhere but I can't find the issue now.
• We could specifically detect when you're comparing Errors and specifically check for message - but this could be a slippery slope - how many types do we treat specially? How do we document to users that these behaviours are happening?
• We could add a new assertion - something like expect(error).to.be.same.error(otherError) but of course this wont solve the fact that eql wont work and so its not really _fixing_ your issue.

I'm not sure what the right solution is to this. As such - I've labeled it as more-discussion-needed

[JamesMessinger]

My $0.02... I think option 1 is the way to go. But yes, it would definitely be a breaking change and would require a major version bump.

Comparing stacks shouldn't be a problem. In fact, if two errors have a different stack, then I'd _expect_ them to be be unequal. The only time two errors should be considered equal is if they occurred under the exact same conditions - including the call stack. IMHO

[keithamus]

@BigstickCarpet the problem with comparing stacks is that unless they are the exact same error^* then they're going to have different stacks. If they're exactly the same error, we can just compare for referential equality (expect(error).to.equal(otherError)).

*:

> var a = new Error(), b = new Error()
> a.stack === b.stack
false

[MarcL]

Hey @keithamus,

Thanks for the detailed reply. I had no idea that Error.message was non-enumerable!

• Option 3 definitely sounds like the worst of the three solutions given that it still leaves the unexpected behaviour of .eql with Error as is.
• Option 2 _feels_ like you could open a potential can of worms of having to match more than just the Error type. Like you said, the potential for the slippery slope of adding more and more types is there.
• Option 1 does sound like the best option but I appreciate it's a breaking change. Is it worth considering this as part of a v4.0 release in order to avoid breaking the rest of the world?! Is there an _accepted_ way to deeply compare the two Error object which avoids the stack issue that you mentioned above? Or would it just be a case of avoid the stack comparison of the two errors?

[keithamus]

Is there an accepted way to deeply compare the two Error object which avoids the stack issue that you mentioned above? Or would it just be a case of avoid the stack comparison of the two errors?

There's not really a way to do this. I mean, we could do random things like maintain a blacklist of keys (so we know not to check stack) or only check literal values (as in most implementations, stack is actually a getter) - but neither of those are desirable at all.

I feel like I probably misspoke when I said we have 3 solutions. Option 1 is pretty much unworkable because of the stack key (even if you get over the huge breaking change that it is). As you rightly say, Option 3 doesn't really fix anything, so again its not a workable solution.

As for option 2 - well... we already do have branches for a bunch of existing types, with more coming, so I guess adding an Error object to the mix is is not making the slope much more slippery.

Thoughts?

[MarcL]

Option 2 sounds like the fairest solution to the issue then, given that you're already explicitly looking for specific types in that branch. Looks like it would be easy enough to add an errorEquals function if the type is an Error and then I'd assume you'd compare against name and message?

Does your type-detect module detect Error objects or is it just a case of doing an instanceof Error?

Thanks for looking into this for me. :+1:

[keithamus]

@MarcL Eyeballing this, type-detect wont detect Errors - it uses @@toStringTag (through Object.prototype.toString.call()) and Error.prototype doesn't implement this, so comes back as [object Object]. So yeah, it'll have to be instanceof Error.

Before we mark this as pr-wanted; @BigstickCarpet has your position changed about the stack property or do you still strongly feel this should be part of the deep equality check?

[JamesMessinger]

@keithamus - Sorry to be a PITA, but I want to clarify what I mean when I say that "_the only time two errors should be considered equal is if they occurred under the exact same conditions - including the call stack_".

In your example var a = new Error(), b = new Error(), the stacks are different because those are two different errors. I wouldn't expect Chai to treat them as equal.

A more realistic example is some business logic that throws an error based on some input. In that situation, the error stack would always be the same, since the only difference is the input data. This is the _only_ situation in which I would expect Chai to treat two errors as equal.

Here's an example:

function setSalary(employee, salary) {
  if (salary > 0) {
    console.log(employee + "'s salary is now $" + salary);
  }
  else {
    throw new Error("Salary must be a positive number!");
  }
}

// Input data
// In a real system, this data would be read from a file, a database, user-input, etc.
var employees = ['Adam', 'James', 'Sarah', 'Bob'];
var salaries = [50000, -10000, 85000, 0];
var errors = [];

// Process the input data
function setSalaries() {
  employees.forEach(function(employee, index) {
    try {
      setSalary(employee, salaries[index]);
    }
    catch (e) {
      errors.push(e);
    }
  });
}

setSalaries();
errors[0] !== errors[1] && console.log('Different Error instances');
errors[0].message === errors[1].message && console.log('Same Error.message property');
errors[0].stack === errors[1].stack && console.log('Same Error.stack property');

[JamesMessinger]

btw... I hope my tone doesn't seem hostile or anything. I'm really just tossing in my two cents on the matter. Please don't interpret my _emphasized text_ as yelling :smiley:

[keithamus]

@BigstickCarpet not a PITA, and you're tone is fine :smile:. It's good to get conflicting opinions because it helps iron out potential issues.

@MarcL what are your thoughts on the above?

[MarcL]

@BigstickCarpet's message above has clarified my misunderstanding of the internals of Error. :smile:

I'd assumed that Error was a simple object like: {name: "Error", message: "Error message"} and didn't realise / had forgotten that the stack was an integral part of an error.

So yes, I agree with @BigstickCarpet that two Errors will be equal if, and only if, their stacks are also the same, in addition to the name and message.

My test code which revealed the bug (which I've simplified below) now seems incorrect in my assumption that the Error I create outside of the function I'm testing is the same as the thrown error internally.

it('should reject with expected error', () => {
    const expectedError = new Error('Invalid response returned from internal thrown error');
    return functionToTest()
        .should.eventually.be.rejected
        .then((error) => {
            expect(error)
                .to.equal(expectedError);
        });
});

I think my test is better off testing that error.message is equal, or perhaps comparing a typed error, instead in order to validate that I'm getting the error I expect.

Perhaps we need to add something to the Chai docs to clarify this @keithamus as it's a potential gotcha?

[keithamus]

Okay. I'm glad you reached that conclusion because after sleeping on it I think @BigstickCarpet makes a valid point and Error.stack should come into consideration.

As this is not the first time deep-eql has passed when it shouldn't, I think the correct solution is to have deep-eql test for referential equality on objects with no enumerable keys. This means the following will all fail:

expect(new Error('foo')).to.eql(new Error('bar'));
expect(new Error('foo')).to.eql(new Error('foo'));

// Also Promises have no enumerable keys!
expect(Promise.resolve()).to.eql(Promise.resolve());

The caveat being that we need to fix https://github.com/chaijs/deep-eql/issues/4 before a PR is accepted for this - because Maps/Sets have no enumerable keys and supporting them after the fact would be a breaking change.

I think we could also revisit Option 3 (having a new assertion method for matching Errors). We have expect(fn).to.throw(ErrorLike) so we could easily take the logic from that, minus the call behaviour and have expect(error).to.be.same.error(ErrorLike) or something to that tune.

---

I'll mark this as PR wanted; under the proviso that https://github.com/chaijs/deep-eql/issues/4 gets fixed first.

To add this feature, you'd would need to check the length of the enumerable keys around L239 and if both are a length of 0, return a check for referential equality. Something like:

if (!ka.length && !kb.length) {
  return a === b;
}

Of course, we'd also need tests to back this up, so some additions to test.js would be warranted. If you need any assistance with this, I'd be happy guide you through them.}

[hijonathan]

Just +1 this to watch. For anyone who wants a simpler test case to get up to speed on the issue:

``````
const expect = require('chai').expect;

describe('wat', () => {

it('should fail if error messages are different', () => {
    const err1 = new Error('Some error');
    const err2 = new TypeError('Some other error');


    expect(err1).to.not.equal(err2);  // true
    expect(err1).to.not.deep.equal(err2);  // false
    expect({err: err1}).to.not.deep.equal({err: err2});  // false
});

});```
``````

[osluf]

I'm interested on taking this one. @keithamus, could you confirm that we're on the same situation nowadays? Could I proceed on this new error assertion?

Just to wrap up:

• Reuse throw logic on a new error assertion.
• Change deep-eql objectEqual function to check this new behavior (still foggy on this).

[keithamus]

@oswaldoferreira I believe this will be fixed in the next release of deep-eql, so I don't think there's anything to do here. I'll mark it as such.

[keithamus]

Just FYI - this is already fixed in an outstanding PR on deep-eql; see https://github.com/chaijs/deep-eql/pull/14/files#diff-910eb6f57886ca16c136101fb1699231R268

[meeber]

@keithamus Can this be closed?

[MarcL]

OP here. Happy for it to be closed.