-
-
Notifications
You must be signed in to change notification settings - Fork 692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expect.eql() returns true when deeply comparing different Error objects #608
Comments
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:
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:
I'm not sure what the right solution is to this. As such - I've labeled it as |
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 |
@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 ( *:
|
Hey @keithamus, Thanks for the detailed reply. I had no idea that
|
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 I feel like I probably misspoke when I said we have 3 solutions. Option 1 is pretty much unworkable because of the 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? |
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 Does your Thanks for looking into this for me. 👍 |
@MarcL Eyeballing this, Before we mark this as |
@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 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'); |
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 😃 |
@BigstickCarpet not a PITA, and you're tone is fine 😄. It's good to get conflicting opinions because it helps iron out potential issues. @MarcL what are your thoughts on the above? |
@BigstickCarpet's message above has clarified my misunderstanding of the internals of I'd assumed that So yes, I agree with @BigstickCarpet that two My test code which revealed the bug (which I've simplified below) now seems incorrect in my assumption that the 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 Perhaps we need to add something to the Chai docs to clarify this @keithamus as it's a potential gotcha? |
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 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 chaijs/deep-eql#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 I'll mark this as PR wanted; under the proviso that chaijs/deep-eql#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.} |
Just +1 this to watch. For anyone who wants a simpler test case to get up to speed on the issue:
|
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:
|
@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. |
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 |
@keithamus Can this be closed? |
OP here. Happy for it to be closed. |
Error objects are compared by reference now in deep-eql: https://github.com/chaijs/deep-eql See Chai issue #608: expect.eql() returns true when deeply comparing different Error objects #608 chaijs/chai#608
Error objects are compared by reference now in deep-eql: https://github.com/chaijs/deep-eql See Chai issue #608: expect.eql() returns true when deeply comparing different Error objects #608 chaijs/chai#608
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
.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
The text was updated successfully, but these errors were encountered: