Deep equal with symbols as keys does not fail

[fflorent]

Hi,

The following test should fail but doesn't:

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

Thanks!
Florent

[meeber]

@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]

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]

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]

@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]

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]

@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]

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]

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]

@keithamus wil look into that, thanks

[inventive-endurance]

Definitely useful for testing out queries with sequelize. +1

[Vicnovais]

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

[keithamus]

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]

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);