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 with symbols as keys does not fail #1054

Closed
fflorent opened this issue Sep 21, 2017 · 18 comments · May be fixed by #1457
Closed

Deep equal with symbols as keys does not fail #1054

fflorent opened this issue Sep 21, 2017 · 18 comments · May be fixed by #1457

Comments

@fflorent
Copy link

Hi,

The following test should fail but doesn't:

let test = Symbol('test')
expect({[test]: 1}).to.deep.equal({[test]: 2})

Thanks!
Florent

@meeber
Copy link
Contributor

meeber commented Sep 22, 2017

@fflorent Thanks for the issue. I believe that these lines are responsible for this behavior; for..in only returns enumerable string properties.

It's worth noting that the deep equality algorithm in the Node.js assert module skips symbols as well. However, lodash's algorithm compares enumerable symbols. My initial feeling is that Chai's algorithm should compare enumerable symbols too.

Thoughts @keithamus @shvaikalesh @lucasfcosta @vieiralucas?

@keithamus
Copy link
Member

Yes I think we should compare symbols too. However this will be a breaking change. This is also tangentially related to chaijs/deep-eql#38.

We could implement it right now in deep-eql then perhaps hide it behind an optio like {compareSymbols: true}. This way for chai@4 folks could get by with expect(deepEql({[test]: 1]}, {[test]: 2})).to.equal(false) or possibly work on a userland plugin in the interim? By chai@5 we could switch those options to true by default.

@vieiralucas
Copy link
Member

vieiralucas commented Sep 25, 2017

I agree that we should compare symbols.

@keithamus, can we do something like start a v5 branch and launch v5.0.0-canary.n versions?
In this way, users who want to use this and more features that will be breaking changes for v4 can use these canary versions and we do not need to do things like add settings to v4.

Please let me know if this does not make any sense at all 😸

@keithamus
Copy link
Member

@vieiralucas I'm not the BDFL - we are equals here, if you want to start a v5 branch you are welcome! I would warn though; we did this with v4 and it was quite a pain to merge to master, as we weren't immediately porting bug fixes over to v4 from master, so if we are to do this, then I would recommend v5 needs more attention than just a dumping ground for breaking changes.

What are your thoughts around having it behind an option in deep-eql though?

@shvaikalesh
Copy link
Contributor

My initial thought: we should compare symbols by default; if someone would like to store metadata on objects (like observers), they should resort to Weak{Map,Set}.

However, we lack real-world use cases of symbols to be confident that opt-in will be better in terms of DX than opt-out. Also, if we are comparing symbols, we should be careful when detecting polyfilled ones.

@nemisj
Copy link

nemisj commented Oct 4, 2017

@shvaikalesh Hi. I have one real-world use case for symbols. Sequelize module recently implemented their operators using Symbols, since it's more secure ( http://docs.sequelizejs.com/manual/tutorial/querying.html#operators ). Now, it's much more tedious to write unit tests, in order to test these operators. It's still possible, but quite challenging.

Check this. this is how it could look if chai would assert Symbols correctly:

expect(result).to.be.deep.equal({
  'name': {
    [Op.and]: [
      {
        [Op.or]: [
          { [Op.notIn]: [ 'lolipop', 'zork' ] },
          { [Op.eq]: null }
        ]
      },
      { [Op.in]: [ 'good', 'very-good' ] },
    ]
  }
});

( All the values inside Op are symbols.)

But, now, in order to test this, i have to do some totally crazy stuff.

@skn3
Copy link

skn3 commented Feb 22, 2018

yeah I just faced this issue with sequelize. You just have to write proeprty tests by hand and it seems to work.

            //test symbols independently
            expect(out).to.have.property(symbol1).that.equals('hello');
            expect(out).to.have.property(symbol2).that.deep.equals(['merge', 'symbols']);
            expect(out).to.have.property(symbol3).that.equals('world');
            expect(out).to.have.property(symbol4).that.deep.equals({
                hello: 'hello',
                overlap: 'new',
                goodbye: 'goodbye',
            });

@keithamus
Copy link
Member

Right now deep-eql has a comparator func that you could use to determine symbol-equality. It is not exposed within chai (but it could be). It'd be a fair chunk of code to write - but it might be useful as a plugin for sequelize specifically?

@nemisj
Copy link

nemisj commented Mar 5, 2018

@keithamus wil look into that, thanks

@inventive-endurance
Copy link

Definitely useful for testing out queries with sequelize. +1

@Vicnovais
Copy link

Vicnovais commented May 6, 2018

Any update on this? It'd be really awesome to test symbols. +1

@keithamus
Copy link
Member

Hey @fflorent thanks for this issue. Thanks to everyone else for commenting on this and showing your support.

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.

@janlukasschroeder
Copy link

There is a simple workaround: Node.js util.inspect

Example:

const { inspect } = require('util');
const { expect } = require('chai');

const foo = { [Symbol('a')]: ['1'] }; // also works with Sequelize operator, eg [Op.in]
const bar = { [Symbol('a')]: ['1'] };

const fooSerialized = inspect(foo, { depth: null });
const barSerialized = inspect(bar, { depth: null });

expect(fooSerialized).to.deep.equal(barSerialized);

@mstmustisnt
Copy link

@janlukasschroeder thank you, your advice helped me a lot!

@Sednaoui
Copy link

Sednaoui commented Feb 2, 2021

For those who do not want to compare objects using inspect, you can instead use lodash method isEqual.

import { isEqual } from 'lodash'

const foo = { [Symbol('a')]: ['1'] }; 
const bar = { [Symbol('a')]: ['1'] };

expect(isEqual(foo, bar)).to.be.true;

@Rest11
Copy link

Rest11 commented Aug 20, 2021

For those who do not want to compare objects using inspect, you can instead use lodash method isEqual.

import { isEqual } from 'lodash'

const foo = { [Symbol('a')]: ['1'] }; 
const bar = { [Symbol('a')]: ['1'] };

expect(isEqual(foo, bar)).to.be.true;

it doesn't work

@Sednaoui
Copy link

@Rest11 have you used a Sequelize symbol?

@snewcomer
Copy link
Contributor

chaijs/deep-eql#81

Hi all! I put up a PR that seems to fix the deep-eql package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.