Add quotes around string values in diff output

[rgroothuijsen]

Purpose (TL;DR)

This PR fixes issue #2084 by adding quotes to diff values if said values are strings, thus producing clearly distinct string representations and preventing the expected and actual values from being reduced to a single value.

Background (Problem in detail)

When the expected and actual values of an assertion do not match but they have the same string representation (for example the number 1234 and the string "1234"), the output does not show the difference, but reduces it to a single value. In this example, this output is produced:

expected spy to be called with arguments
<black>1234</black>

while this output was expected:

expected spy to be called with arguments
<red>1234</red>
<green>1234</green>

Solution

A solution was proposed in the ticket itself which was also implemented here: adding quotes around a diff value if the value in question is a string. This process is skipped if the value is undefined, not a string, or already wrapped in quotes.

An alternative solution was considered which involved adding a check for type differences without altering the output, though this might still produce confusing output if the string representations of a diff are the same without making it clear whether a value is a string or not. This situation can be seen in the expected output example above.

Instead, diffs are now displayed as follows, given the example where the string 1234 was expected but the number 1234 was found instead:

expected spy to be called with arguments
<red>1234</red>
<green>'1234'</green>

How to verify

1. Check out this branch
2. npm install
3. Run the test included with this PR

[macsj200]

Excellent work, you beat me to it!

Thinking about edge cases here, what if the string contains single quotes? Should we add some escape logic? Since we're just doing basic string concatenation I'd imagine it'd just print something like 'it's yours' which seems a bit odd.

Also, are there any other tricky "stringlike" cases that could bite someone, i.e. Symbol?

[macsj200]

I'm not super experienced collaborating on OSS across forks, but I put together this PR against your branch that handles the single-double quote cases. If you'd like I think you could merge my PR into your branch.

rgroothuijsen#1

[rgroothuijsen]

@macsj200 Thanks! I think the regex in my code covers it, though, since it looks looks for a quote mark at the start/end and will accept anything else inbetween. I have no experience with Symbol, so I can't really answer that.

[macsj200]

I understand your regex can catch the scenario, but it doesn't look like you have any branching logic to make sure it's wrapped in the "other" string delimiter...i.e. wouldn't we end up with something like this?
'it's mine' as opposed to "it's mine"?

[rgroothuijsen]

What to do with double quotes is something to be considered, yes. If a string was found to have double quotes, it would get turned into'"it's mine"'. On the one hand, this might look not very pretty, but my idea behind always wrapping a string in single quotes was clearly indicating that it's the literal contents of the string.

[mantoni]

How about simply doing JSON.stringify(value) if it's a string? This would handle all escaping edge cases and we don't need the regexp check.

[rgroothuijsen]

That's a straightforward way of doing it, even if it doesn't look as fancy in all cases. I gave it a try, and "1234" (literal quote marks included) in this case was stringified as "\"1234\"". If that's acceptable, I can go with that.

[mantoni]

@mroderick Do you have additional thoughts or other ideas?

[rgroothuijsen]

@mantoni Awaiting further comments from @mroderick, does the idea seem good to you?

[mroderick]

I am working on some experiements to see if I can retire @sinonjs/formatio and use Node's util.inspect instead. That will do the quoting in the desired way (and save a lot of maintenance).

[rgroothuijsen]

@mroderick If it works, would it make this PR obsolete since the feature would be implemented that way?

[mantoni]

@rgroothuijsen Yes, that is precisely what it means.