-
Notifications
You must be signed in to change notification settings - Fork 23
fix: Support diffing single-line strings #27
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
src/index.js
Outdated
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.
If one value is single-line and other value is multi-line, does the if statement append newline to both?
How about separate if statement for each value which tests RegExp only if typeof string? Although this seems like the primary use case, from quickly reading code in index.js of jest-diff package (but not trying it, I admit :) I think the snapshot-diff assertion works for other objects, not just React elements, true?
if (typeof valueA === 'string' && !MULTILINE_REGEXP.test(valueA)) { valueA += '\n'; // eslint-disable-line no-param-reassign } if (typeof valueB === 'string' && !MULTILINE_REGEXP.test(valueB)) { valueB += '\n'; // eslint-disable-line no-param-reassign }
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.
Yea, makes sense, addressed
363b2f2 to
0a3fda0
Compare
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.
this seems weird, IMO. Maybe not, though :D
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.
Maybe that's a regression?
jestjs/jest#1633
cc @pedrottimark
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.
Hah, there's even a test for that.
Is this because jest-diff was supposed to provide an inline diff, but it was not supported at the time (and nothing change in that matter for now)?
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.
Y’all ask a good question about this special case. I added test to document it, not endorse it :)
I guess it’s because jest-diff omits output that seems redundant when expect calls it:
but it’s confusing when snapshot-diff calls it.
Not to sound critical of awesome effort to integrate expect matchers with diff display, and then snapshots, it feels like the whole is less than the sum of the two parts in some cases. What do you think if expect omits Expected value to equal and Received lines when a diff follows? EDIT: Then jest-diff could drop the special case, as a breaking change.
Yesterday I updated a local copy of code from dormant Jest PR for inline substring diff to call diff-sequences package. This version is somewhat easier to grok than the original :)
Any feedback how to expose that new function for Jest and other callers is very welcome.
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.
@pedrottimark you could expose some useful utils like diff lib (talking about something like diffLines and structuredPatch we use).
Back to the topic, diff + expect behavior feels like a hack to me. It's .toEquals() matcher that should be responsible for determining whether to display the diff or not.
Do you think it makes sense to file a PR to Jest?
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.
Yeah, we might refactor to more clearly separate assertion logic at one extreme and diff display at another extreme from the responsibility you mention, which I think is divided between expect/src/matchers.js and jest-diff/src/index.js files.
pedrottimark
commented
Feb 1, 2018
Have started on first of a series of pull requests for expect and jest-matcher-utils which are prerequisite to end goal to remove limitation from jest-diff so it returns a diff for single-line strings.
Oh, great, thanks for the heads-up! boolean and number need these updates too.
Closing, this is fixed upstream in Jest 23. Thank you all for chiming in!
Fixes #22
Also simplifies our test components btw, because it annoyed me it's this long unnecessarily 😛.