Skip to content
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

Deep Equal should not exact equal for error #1322

Closed
stephanwlee opened this issue Mar 9, 2017 · 3 comments
Closed

Deep Equal should not exact equal for error #1322

stephanwlee opened this issue Mar 9, 2017 · 3 comments

Comments

@stephanwlee
Copy link

stephanwlee commented Mar 9, 2017

Hi Sinon! Thank you for providing awesome library with cool new features added to the pre-release version. I have been using pre for newly introduced API (I was going to write extension myself and I found the code whiling digging through the source) but .6 release broke our large test suite.

  • Sinon version : 2.0.0-pre.6
  • Environment : macOS with Node6+
  • Example URL :
  • Other libraries you are using:
    Mocha, Babel

What did you expect to happen?
I did not expect .deep.equals assertion to fail when different but "equal" instance of error was passed
What actually happens
For any subclass of Error, deep equals does exact equals. https://github.com/sinonjs/sinon/blob/master/lib/sinon/util/core/deep-equal.js#L54-L56

@lucasfcosta
Copy link
Member

lucasfcosta commented Mar 16, 2017

Hi everyone!

This is indeed something simple to solve and I'm willing to do a PR for it. However, I'd like to suggest a different approach other than just fixing that if clause.
In Chai we've got a module called deep-eql which does deep equality checks. It has been extensively tested and fine tuned for performance and works fine on Chai's core.

By adopting the same library we would also be working on the same codebase in the future in order to improve it and make it better for every project using it.

Also, it still allows us to have the use method due to the comparator argument this library accepts.

If you don't want to adopt it it's fine too, but I just thought about suggesting it before solving this issue.

@mroderick
Copy link
Member

@lucasfcosta I like the proposed solution. It might be a bit of challenge to wrangle match to become a comparator option ... but it looks like you like the challenges ;)

I would love to see a PR that reduces the size of the codebase

@stale
Copy link

stale bot commented Jan 13, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 13, 2018
@stale stale bot closed this as completed Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants