Chai: assertKeys cannot handle some valid ES6 keys

[meeber]

There seems to be two ES6 issues with assertKeys:

Issue 1

If expected is a regular object with a key that is a Symbol, then assertKeys will not behave as expected. For example, the following assertion fails:

var key1 = Symbol()
  , key2 = 42
  , obj = {};
obj[key1] = 'val1';
obj[key2] = 'val2';
expect(obj).to.have.all.keys([key1, key2]);
// AssertionError: expected { '42': 'val2' } to have keys 'Symbol()', and '42'

val1 is missing from the AssertionError's actual because the behavior of Object.keys is to not include Symbols in its result set. (https://github.com/chaijs/chai/blob/6b640f71451fa575f24b302e56b99379c86d8bd5/lib/chai/core/assertions.js#L1173)

val2 is showing up as the string "Symbol()" in the AssertionError's expected because it's explicitly converted to a string here: (https://github.com/chaijs/chai/blob/6b640f71451fa575f24b302e56b99379c86d8bd5/lib/chai/core/assertions.js#L1187)

Issue 2

A TypeError is thrown if any of the expected or actual keys in a keys assertion cannot be implicitly converted to a string by Array.prototype.sort here: https://github.com/chaijs/chai/blob/6b640f71451fa575f24b302e56b99379c86d8bd5/lib/chai/core/assertions.js#L1251-L1252

Examples that fall victim to this issue but not the previous issue:

// key1 has no toString
var key1 = Object.create(null)
  , key2 = 42;
expect(new Map([[key1, 'val1'], [key2, 'val2']])).to.have.all.keys([key1, key2]);
// TypeError: Cannot convert object to primitive value

// key1 has non-function toString
var key1 = {toString: null}
  , key2 = 42;
expect(new Map([[key1, 'val1'], [key2, 'val2']])).to.have.all.keys([key1, key2]);
// TypeError: Cannot convert object to primitive value

// key1 has Symbol's special toString which rejects implicit conversion
var key1 = Symbol()
  , key2 = 42;
expect(new Map([[key1, 'val1'], [key2, 'val2']])).to.have.all.keys([key1, key2]);
// TypeError: Cannot convert a Symbol value to a string

Possible Resolutions

The first issue could perhaps be resolved by checking if Symbol is implemented, and if so, first concatenating Object.getOwnPropertyNames (Edit: Or still Object.keys, so no unexpected behavior with enumerability) with Object.getOwnPropertySymbols instead of using Object.keys, and then only converting the keys to strings if they aren't Symbols. (In the future Reflect.ownKeys could be used for the first part).

The second issue could perhaps be resolved by removing the two sorts linked above. The sorts were added a long time ago to make the diffs more readable (https://github.com/chaijs/chai/pull/264). Unfortunately, removing the sorts will break any user-created assertKeys test that catches the AssertionError and directly references its .actual and/or .expected members and relies on at least one of them to be in sorted order when it isn't already in sorted order prior to the sort call. To preserve current behavior, but still resolve the issue, here are some alternative solutions:

• Wrap the sorts in a try/catch and fall back to unsorted version on error
• Write a custom sort method that mimics Array.prototype.sort except instead of throwing it dumps unsortable elements at the end of the array in the order they appear
• Write a custom compareFunction to feed into Array.prototype.sort that has special behavior to dump unsortable elements to the end of the array

[lucasfcosta]

Hi, @meeber, thanks for this issue and for detailing it so well :smile:

I think your solution for the first issue is valid. IMO we should still use Object.keys as you've said in order to avoid unexpected behavior with enumerability and then Object.getOwnPropertySymbols.

About the second issue: what if we add support for symbols in inspect and use it into the compareFunction? Is this idea too crazy? This way we could have nice filtered output for every type of key. I'm not sure this is a valid approach, but it might be worth to think about.

[meeber]

Using inspect for the sort is great! And no changes to it are needed; it already converts Symbol to "Symbol()". Here's a basic test using https://github.com/meeber/chai/tree/es6-keys-asserts

Test

var key1 = "purr"
  , key2 = Symbol()
  , key3 = "hiss"
  , key4 = "meow"
  , obj = {};

obj[key1] = 'val1';
obj[key2] = 'val2';
obj[key3] = 'val3';
obj[key4] = 'val4';

expect(obj).to.have.all.keys(key4, key3, key2, key1, "newp");

Result

AssertionError: expected { Object (purr, hiss, ...) } to have keys 'meow', 'hiss', {}, 'purr', and 'newp'
+ expected - actual

 [
    "hiss"
    "meow"
+   "newp"
    "purr"
    Symbol()
 ]

(Disclaimer: Latest commit of Mocha was used so Symbol appears as "{}" in the above message)

AssertionError's actual property

[ 'hiss', 'meow', 'purr', Symbol() ]

AssertionError's expected property

[ 'hiss', 'meow', 'newp', 'purr', Symbol() ]

[lucasfcosta]

@meeber awesome job testing it!
We just need to validate if Symbol(symbolHere) is really the desired output (it's good for me, but it would be good to hear other's opinions before taking a final decision), since we have no special handlers for it at inspect.js.

If you're willing to open a PR for that and need any help, please let me know.
This is looking good :smiley:

[meeber]

"Symbol(description)" is just the built-in toString conversion of a Symbol (it only failed in the Array.prototype.sort because Symbol has special behavior to forbid implicit conversion). I can't think off the top of my head of any alternatives regarding output; not much else can be derived from a Symbol because it's all magical with its internal unique identifier. But maybe someone has an idea!

I should be able to cook up a PR, just need to add some tests.

[lucasfcosta]

@meeber ah yes, I was just wondering if someone has another idea on how a Symbol should be represented, but I totally agree with you.

I will be very happy to review and merge your PR after it's done :smile:

[meeber]

Whoops, I misinterpreted the output of the test above: Chai's inspect is stringifying it to {}, not Symbol(). It just seemed that way because of a combination of console.log and whatever outputs the diff to the screen. So, cribbing a couple of lines from node's util.inspect!