-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Tests: improve diffing for values of different types #5454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mgol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment
f093c59 to
23ccedc
Compare
| // Diff everything else as words | ||
| diff = Diff.diffWords( | ||
| quoteWrap( error.expected ), | ||
| quoteWrap( error.actual ) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the necessary insight here is that we want to diff type-mismatched values after serialization, which quoteWrap a) fails to convey, and b) currently does so inconsistently (e.g., it escapes " in strings by \-prefixing, but does not similarly escape \).
| // Diff everything else as words | |
| diff = Diff.diffWords( | |
| quoteWrap( error.expected ), | |
| quoteWrap( error.actual ) | |
| ); | |
| // Diff everything else as words (post-serialization) | |
| diff = Diff.diffWords( | |
| serializeForDiff( error.expected ), | |
| serializeForDiff( error.actual ) | |
| ); |
with
function serializeForDiff( value ) {
// Use naive serialization for everything except types with confusable values
if ( typeof value === "string" ) {
return JSON.stringify(value);
} else if ( typeof value === "bigint" ) {
return `${ value }n`;
} else if ( typeof value === "symbol" ) {
const ctor = Symbol.keyFor(value) !== undefined ? "Symbol.for" : "Symbol";
return `${ctor}(${JSON.stringify(value.description)})`;
}
return `${ value }`;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that, too! Using JSON.stringify on strings is a great idea. ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try it and post some examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I switched to just show the diff all the time when at least one is non-null. I found it reassuring to confirm those are the values in diff form.
mgol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following with the review! LGTM, Richard’s suggestions also look good to apply before merging.
Co-authored-by: Richard Gibson <[email protected]>


Summary
Differences are:
diffCharswhen both expected and actual are stringsdiffWordsfor everything after thatObjects, arrays, and numbers are diffed as before.
Checklist