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

feat(testing): improved strings diff #948

Merged
merged 12 commits into from
Jun 24, 2021
Merged

feat(testing): improved strings diff #948

merged 12 commits into from
Jun 24, 2021

Conversation

lowlighter
Copy link
Contributor

@lowlighter lowlighter commented Jun 2, 2021

This add a specialized tokenizer that is called when both operands are string before entering diff(), making changes in large strings assertions more visible.

A word diff has been implemented (similar to git diff --word-diff):
image

As the above is not practical when a lot of words have been changed, a multiline diff has also been implemented:
image

Todo

  • word diff
  • multiline diff
  • combine word and multiline diffs
  • link new code with diff() and buildMessage()
  • tests
    • _diff_test
    • asserts_test
  • find good colors
  • merge word diff token if a in-between token is a space

Closes #929

testing/_diff.ts Outdated Show resolved Hide resolved
@lowlighter lowlighter marked this pull request as ready for review June 3, 2021 12:23
@wperron wperron self-requested a review June 3, 2021 13:52
@wperron
Copy link
Contributor

wperron commented Jun 3, 2021

I created a small gist just to get a sense of how the new diff looks visually -- it seems to be working ok when both sides of the assertion are single-line string. Things get a little weird when the part of the string that differs contains a mix of newlines and spaces though.

Screenshot from 2021-06-03 10-44-17
Screenshot from 2021-06-03 10-42-15

When the strings are completely different, or when the string is inside an object or an array, it reverts back to the old diff. In the case where the string is nested in an object or array, I guess it's ok, but having to different diffs for strings seems a bit weird to me.

Screenshot from 2021-06-03 10-45-59
Screenshot from 2021-06-03 10-45-51
Screenshot from 2021-06-03 10-45-40

Here's the gist I used https://gist.github.com/wperron/77ba7b170799cc7b925c1734b38624e1

@lowlighter
Copy link
Contributor Author

Thanks for linking the gist

Yes I wasn't too sure how to handle both multilines and word diffs. I was actually waiting for some input about it. I can see multiple paths to takes:

  • Only keep multiline diff and scrap word diff
  • Combine them both to achieve something like this
  • Or else I can check if it's possible to group alternating tokens together (like the ones you reported) but I think one of the above solution would be better

Currently it's an "autodetect" which choose between word-diff/multiline-diff based on the changes ratio and the number of lines, so that's why a full line diff revert back to old diff system but it's kind of hack, I'm not very convinced myself it 😅

If you have others suggestions please let me know 👍


For nested strings, I chose to not support them because I think it would quite a lof of work because of how diffs are computed. From my understanding, the generic diff() method actually takes from Deno.inspect(). So to support it, it would requires to catch strings properties from both actual and expected objects, and anyway I think a mixed diff would probably be more confusing I guess.

Current behaviour where both root operands must be strings seems fine to me, in the sense of when you're asserting two strings together you'd probably like to see a detailed diff between them, while if you assert whole objects/arrays you're probably more insterested in a more global view. If someone wants detailed string diff within a complex structure, one can always assert it explicitely by checking members

@lowlighter
Copy link
Contributor Author

Here's what it looks like now:

image

Not sure about the colors though, I think it has some accessibility issues 🤔

testing/asserts.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Jun 21, 2021

Looks very useful 👍 Does anyone have any concern about introducing this?

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lowlighter LGTM. Great feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] assertEquals multiline string diffs
3 participants