Chai: Don't print array enumerable properties via inspect by default

[DingoEatingFuzz]

The Problem

When using a library that modifies the array prototype, the expected [ ] to deeply equal [ ] becomes very noisy.

Here is a screenshot of this issue while using Ember:

Notice how small the vertical scroll thumb is.

I also created a minimum viable example as a gist.

The History

I then began to dig into why this ever came to be this way because it doesn't seem useful to me. This led me to #120, #125, and https://github.com/chaijs/chai/commit/f76eedd46f282e0c948f0c9ad76221e9ae7d53b6

The tl;dr of those issues is inspect was originally borrowed from node, which does not print enumerable properties on arrays. This feature was added partly in support of .eql support for DOM nodes and matching that support when printing. This is reasonable since something like AssertionError: expected [ ] to deeply equal [ ] would lead to confusion.

The Proposal

I suggest that printing enumerable properties is an optional behavior just like show hidden is.

This way the more common case of plain arrays with modified prototypes get printed properly, and in cases where the enumerable properties are relevant, inspect can be invoked with the showEnumerable flag.

I think the only point of contention here is whether or not it's safe to assume that whenever enumerable properties should be printed, an addon is preferred over a primitive assertion like .eql.

[meeber]

@DingoEatingFuzz Thank you for the detailed write-up, research, and gist example! :D

A small but crucial correction: Node's util.inspect does print enumerable properties on arrays. For example, assigning the doubleAll function directly to an array instance instead of to Array.prototype will cause it to be displayed by Node's util.inspect.

The real difference between implementations is that Chai's inspect includes inherited enumerable properties in the output whereas Node's util.inspect only prints own enumerable properties.

So this leaves two questions:

1. Is it appropriate for a deep equality assertion to consider inherited enumerable properties for arrays?
2. If yes, is it appropriate for these same inherited enumerable properties to show up (by default) in the assertion failure message?

To me, the second question is a definite yes. Otherwise, it'd be possible (especially in ES6 with extending Array) to end up with an assertion failure message like this:

AssertionError: expected [ 2, 4, 6, 8 ] to deeply equal [ 2, 4, 6, 8 ]

Whoops! But that could happen if you compared two Arrays that have different inherited enumerable properties but are otherwise equal. It's too dangerous for an assertion to consider inherited properties during the comparison but not include those same properties (by default) in the failure message.

As for the first question... Ehhh... My first reaction is "probably" but I need to think about it more.

I kinda feel like the real issue here is that Ember is creating enumerable properties on Array.prototype that should really be non-enumerable. But I could be wrong. Maybe it was just a performance decision.

By the way, a partial workaround is to set chai.config.truncateThreshold to a low number before the troublesome tests, and then reset it afterward. The failure messages won't be as helpful but at least it won't be spam.

[lucasfcosta]

That is what I call a great issue! Thanks for the detailed report! 😄

As @meeber just said above, it comes down to two questions and my answer to both is yes.

Question 1 Answer Explanation:
By definition a deep comparison should check every single property of an object, this includes inherited properties. If this wasn't true we could have two objects with the same own properties which inherit from different prototypes and they would still be considered deep equal even if they have completely different prototypes.
IMO, deep means every single property (inherited or not) will be checked for equality.

Question 2 Answer Explanation:
As @meeber said, it is very dangerous to end up with an error message that is a paradox itself.
The error message should warn the user exactly what happened based on the criteria used for the assertion, which, in this case includes inherited enumerable properties.

---

Possible Solution

IMO this is definitely an output format problem.
As said above, we could set truncateThreshold to limit the output, but I think it doesn't really solves the problem, it just mitigates it (in an excellent way, but it still doesn't solve the problem).

Perhaps we could adopt the solution you just provided but with another default behaviour.
We could show inherited enumerable properties by default and hide them only when a certain flag is set. Additionally we could also add the prototype's name to the output in order to guarantee the user is aware of the inherited properties comparison on the assertion without having to print every single one of its inherited properties.

What do you guys think about this?
I feel like this would be an extremely specific setting, but it really solves the problem.

Oh, and thanks again for this awesome report!

[meeber]

Following up on @DingoEatingFuzz's gist, it's worth noting that this:

expect([ 1, 2, 3, 4 ].doubleAll()).to.have.same.deep.members([2,4,6,9]);

Results in this:

AssertionError: expected [ 2, 4, 6, 8, doubleAll: [Function] ] to have the same members as [ 2, 4, 6, 9, doubleAll: [Function] ]

I wouldn't expect inherited properties such as the doubleAll function to be included in the error message here since a members assertion (even with the deep flag) doesn't consider inherited properties, only the array's elements. I feel like we can classify this behavior specifically as a bug. Thoughts?

@lucasfcosta Hmm. You mean a flag set in the same way as deep or not, except it would only control output, not the actual assertion? I'm not sure how I feel about that. What did you have in mind naming-wise? And what about a config value instead?

[keithamus]

There's a sort of unwritten plan to refactor our inspection lib - loupe - and make it much more intelligent. It's on my todo list after refactoring deep-eql (which will likely, hopefully, be this week). After the refactor this kind of issue will likely no longer be present - as our error messages will (hopefully) prioritise showing the _different_ values of the two objects.

[lucasfcosta]

@meeber nice catch. IMO we could hide that kind of property for the members assertion, since it's not taken into account we would have no problems showing a reduced part of the current output.

I was thinking about a flag which is config value, not an assertion chain, I'm sorry, I should've expressed myself better. Maybe comparePrototypeProps would be a clear name for it, but feel free to suggest other options, anyway, I feel like this won't be needed at all after reading @keithamus' post.

Since he has a nice solution for this I think we should wait for that refactor before moving on to more changes on top of this behavior.

[DingoEatingFuzz]

I just tested this again with the 4.0 canary release and the problem still halfway exists.

First, the diffing you mentioned, @keithamus, is there and it's wonderful. However, the main AssertionError line still includes the inherited properties.

Unfortunately, that line is all you get when you use the Tap reporter:

[meeber]

@DingoEatingFuzz Unfortunately, we weren't able to get a rewrite of loupe into 4.0. But it's still a huge priority for us moving forward.

[lucasfcosta]

Hi @DingoEatingFuzz, thank you very much for this issue! 😄

This is definitely one of our priorities when it comes to v5. Right now we're making sure that we have cards setup for everything that is on our roadmap so that we can tackle those things as soon as possible and get things working.

As you can see in this card, refactoring our inspect utility (loupe) is in progress and is one of our next priorities.

For house-cleaning purposes, I'll close this issue for now.

Thank you very much!