Opt-out of truncation for custom classes? #727

Open
glenjamin opened this Issue Jun 10, 2016 · 10 comments

Projects

None yet

3 participants

@glenjamin

The https://github.com/astorije/chai-immutable plugin extends lots of core assertions to work with ImmutableJS.

The collections in Immutable all have a custom toString() and inspect() method which produces a readable version of their internal datastructures. This gets picked up correctly by chai's inspect util.

However, if this representation goes over the truncateThreshold, they are detected as objects and their first two properties are displayed instead - which isn't useful for these collections.

To work around this, many of the assertion methods convert the objects to a string before passing to the message formatting. This mostly works but it's easy to miss cases.

This problem should apply to any object with a custom inspect, would it be possible to tweak this behaviour somehow?

Some options I can think of would be to change the object detection to check for plain objects only, or to skip truncation for objects with a custom inspect() method, or to truncate the result of inspect() when objects have an inspect().

Any thoughts?

Two options I can think of

@glenjamin glenjamin referenced this issue in astorije/chai-immutable Jun 10, 2016
Closed

Use friendly object representation in more places #65

@meeber
Contributor
meeber commented Jun 11, 2016

@glenjamin If I understand the issue correctly, the problem only occurs when the length of the string exceeds truncateThreshold? If so, have you tried increasing truncateThreshold or disabling it entirely for the problematic tests (or even the entire test suite)?

I'd be hesitant to change Chai's logic in any way that potentially overrides a user's truncateThreshold setting since the whole reason it exists is to enable the user to dictate how large their assertion error messages are allowed to be.

Let me know if I'm misunderstanding the issue :D

@glenjamin
glenjamin commented Jun 11, 2016 edited

Changing truncateThreshold does work, but pretty much every user would need to do this.

The problem is that the shortened version uses the object's internal structure rather than the internal representation - and isn't useful (you can't even roughly tell which type of object it is).

I think this is in a similar territory to how strings don't get truncated because you might miss the important bit.

I think tweaking the detection so that objects with a custom inspect() get the result of that inspect() call truncated to the limit rather than their keys displayed might be a decent balance?

@glenjamin
glenjamin commented Jun 11, 2016 edited

Just realised I never actually gave an example, here's one from my testsuite. In this case the first argument has been forced into a string, but the second argument hasn't.

AssertionError: expected 'OrderedSet { Record { "sourceBlock": "7e7210f2-b01e-4829-81e1-358c0417ed5f", "sourceConnector": "18CED375-CD46-4F86-8EA7-E98022EEDB1B", "targetBlock": "55e2f6c6-6fcf-47da-b31e-9237a4bf1530", "targetConnector": "0219FCDC-8B15-4C7C-B398-7666471DDBEA" } }' to include { Object (_map) }

@meeber
Contributor
meeber commented Jun 11, 2016 edited

@glenjamin The objDisplay utility is where Chai decides how to stringify the object for the assertion error message.

In the best case scenario, it uses Chai's custom inspect utility, which is a close adaptation of Node's util.inspect, although work is being done here to improve upon that. This plays nicely with Immutable collections thanks to their custom .toString .inspect property. It's important to note that at no point in this process does Chai take into consideration an object's .inspect property.

If the string's length ends up exceeding truncateThreshold then it switches strategy and instead tries to succinctly summarize the object. In the case of functions, the string is reduced to "[Function]" or "Function: myFunctionName]". In the case of arrays, the string is reduced to "[ Array(length) ]". And in the case of objects, it displays the first two keys via Object.keys as "{ Object (key1, key2, ...) }". It's this final behavior that's producing unhelpful strings for you.

Personally, I'm not too crazy about how it's handling arrays and objects. I think I'd prefer that it instead take the original .toString string representation from .inspect and cut it down to truncateThreshold length by chopping off from the end. And then adding a "..." at the end of it. I believe this would be more helpful in general, not just in your examples with the Immutable library, and would still respect the user's truncateThreshold setting.

@lucasfcosta @keithamus Any thoughts on the above?

Also, I'm not in favor of any change that doesn't respect a user's truncateThreshold setting, even if this means that all users making use of a particular plugin would find themselves wanting to increase it.

I'm also not in favor of Chai having knowledge of an object's .inspect property. Unlike .toString, there's nothing standard about .inspect. I'd even venture to say that most objects which have a property named .inspect are using it as a method to perform some kind of domain-specific inspection on a provided argument, which would cause Chai's logic to break if it was assuming the behavior of every object's .inspect was to return a stringified self-representation.

@glenjamin

In the best case scenario, it uses Chai's custom inspect utility, which is a close adaptation of Node's util.inspect, although work is being done here to improve upon that. This plays nicely with Immutable collections thanks to their custom .toString property. It's important to note that at no point in this process does Chai take into consideration an object's .inspect property.

Immutable objects have .inspect() as an alias for .toString(), as far as I can tell this is what chai and/or loupe would pick:
https://github.com/chaijs/chai/blob/master/lib/chai/utils/inspect.js#L45
https://github.com/chaijs/loupe/blob/master/lib/loupe.js#L61

On a related note, you may find https://github.com/Xotic750/inspect-x interesting, it's also originally based on Node's inspect and does a good job of handling the newer ES6 datatypes.

Personally, I'm not too crazy about how it's handling arrays and objects. I think I'd prefer that it instead take the original .toString representation and cut it down to truncateThreshold length by chopping off from the end. And then adding a "..." at the end of it. I believe this would be more helpful in general, not just in your examples with the Immutable library, and would still respect the user's truncateThreshold setting.

👍 from me on this, assuming you mean the result of the inspect util (and loupe in future). that would solve my immediate problem and should work well in general. Would you still want to handle values that were originally strings differently?

I'm also not in favor of Chai having knowledge of an object's .inspect property. Unlike .toString, there's nothing standard about .inspect.

I think it's reasonably fair to say it's a de-facto standard in Node-land, thanks to https://nodejs.org/dist/latest-v4.x/docs/api/util.html#util_custom_inspect_function_on_objects having been in the docs since at least 0.4 that I know of. Whether that's a good enough reason to lean more heavily on it, I don't know.

@meeber
Contributor
meeber commented Jun 11, 2016

I stand corrected on inspect! Thanks for the references :D

@meeber
Contributor
meeber commented Jun 11, 2016 edited

Would you still want to handle values that were originally strings differently?

It's my current opinion that truncateThreshold should be respected for every expected and actual, including values that were originally strings.

Edit: This topic is also discussed in #710 and #703

@glenjamin

Is there enough consensus for me to submit a PR for something that would improve things?

At the moment chai-immutable is stringifying arguments as a way to opt-out of truncation, which is leading to quite a bit of wasted time in my suite producing large strings for tests which aren't actually failing.

It seems like the currently floated idea would be to remove the alternative output formats for objects and arrays and to always truncate the string-output of values?

@keithamus
Member

Hey, I'd like it if we sat on this one for a bit. I know it's dragging on - but I am working on deep-eql and then loupe, and I'll be improving the algos for loupe to better represent the differences between two values. Doing any work on this now could impact that in undesirable ways, IMO. Are people happy with this conclusion?

@glenjamin

That seems sensible, please let me know if there's anything on there that I could help contribute with.

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