Skip to content
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

Use diff-match-patch for string diffing #3428

Closed
wants to merge 4 commits into from

Conversation

alextes
Copy link

@alextes alextes commented May 1, 2017

Summary
I did not dive that deep into the issue yet. Although swapping the diff lib is the suggested solution correct me if I'm wrong in assuming it is the full solution. I could see about adding a test that currently hangs jest as described in a couple of comments on the issue.

It's hard to share some of the test output because the highlighting in it is essential. I'd be happy to provide some screens if you prefer this to grabbing my branch!

To be done:

  • Figure out what seems to be different diffing for some whitespace

Decisions made:

  • Set diff timeout to 0 (meaning none)

Thanks go to @cpojer for giving me the little encouragement for which I'd hoped!

Test plan

  • Pass all current tests
  • Add a test that currently grinds diff to a halt if desired.

Sidenotes

  • The API docs for diff-merge-patch are no longer available, find the web.archive here.
  • Took a peak at how AVA does it. Interestingly they opt for the default 1s timeout if I'm reading things right. Maybe that makes more sense. Do share your thoughts.

Fixes #1772.

@thymikee
Copy link
Collaborator

thymikee commented May 1, 2017

Thanks for your PR!
The test should compare 1 large string (eg. 'jest'.repeat(100000)) to smaller one, and two of similar sizes (something around 10000 chars each).

@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

This is a great PR, and thank you for doing it. Currently we don't really have time to work on integrating it into Jest, so I'll close it. If you'd like to pick it back up, please ask to reopen this or send a new PR :)

@cpojer cpojer closed this Aug 24, 2017
@alextes
Copy link
Author

alextes commented Aug 24, 2017

I'll take another look at how tricky the whitespace issues look months later cpojer but I vaguely remember having no clue what exactly was going on there and a hard time reading the related code to get an idea of why the whitespace behaves as it does.

If it still looks like hours of work let this be a reference for the next guy.

@pedrottimark
Copy link
Contributor

@alextes For your info, there will be changes in packages/jest-diff/src/diff_strings.js when #3429 is merged soon. We expect some additional changes for snapshot tests after Jest 21.

A big difference (pardon pun :) between current diff and proposed diff-match-patch packages:

  • diff returns lines in “chunks” for --expand option or “hunks” by default
  • diff-match-patch returns array of deleted/inserted/equal substrings which
    • is probably why it became tricky for you to fix the test failure (negative point)
    • could make it possible to display differences within lines (positive point)

If you are interested to look at this again after Jest 21 is released, comment this issue with a direct mention to me, so we can divide and conquer with minimal merge conflicts.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest stalls after comparing to complex objects
5 participants