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

Improve snapshot diff display #36

Closed
antfu opened this issue Dec 9, 2021 · 9 comments
Closed

Improve snapshot diff display #36

antfu opened this issue Dec 9, 2021 · 9 comments
Labels
enhancement New feature or request pr welcome

Comments

@antfu
Copy link
Member

antfu commented Dec 9, 2021

No description provided.

@antfu antfu mentioned this issue Dec 9, 2021
Closed
@antfu antfu added enhancement New feature or request pr welcome labels Dec 10, 2021
@tony-go
Copy link
Contributor

tony-go commented Dec 10, 2021

Hi 👋

Do you have an idea about what you're expecting? ^^

I think the entry point is here ? https://github.com/antfu-sponsors/vitest/blob/4742d44466bafea5253eaf744fe4721ca1620e43/src/integrations/chai/snapshot/utils/jest-reporters-lite.ts#L35

@antfu
Copy link
Member Author

antfu commented Dec 10, 2021

Yes. That snapshot folder is basically a port of https://github.com/mochiya98/mocha-chai-jest-snapshot, and we will need to rewrite the printing to use our reporter interface and match with the overall CUI design.

Currently, I see the improvement is to make the diff of large objects or long strings easier to read.

Thanks for interesting in it btw!

@tony-go
Copy link
Contributor

tony-go commented Dec 10, 2021

You're welcome.

I'd like to give it a try on this. If it's okay for you^^

@tony-go
Copy link
Contributor

tony-go commented Dec 10, 2021

I looked a bit at the code and seems that what we want to improve is there:

https://github.com/antfu-sponsors/vitest/blob/main/src/integrations/snapshot/client.ts#L68-L74

The expect function is the black box that:

  • make the diff between actual and expected
  • pretty-print in the console the diff

I think that If we want to provide a different experience we should implement:

  • a diff util (take two strings as input and return a DiffObject with relevant data
  • a pretty-print util which will consume our DiffObject and display it nicely in the console
    ... and obviously, get rid of the expect().equals function.

This approach seems ok for you?

☝🏼 Blocking ☝🏼 : Impossible to log something in the vitest/src/integrations/snapshot/client.ts file. I feel that the only way to print things is by throwing. I probably miss something in the workflow ...
Maybe the third argument of addMethod utils from chai doesn't allow you to log anything.

@antfu
Copy link
Member Author

antfu commented Dec 10, 2021

We already have the diff here:
https://github.com/antfu-sponsors/vitest/blob/2ddbf7e40b7771494404327c789560df8350cd53/src/reporters/error.ts#L19

So maybe we could have some thing to mark the snapshot check in the error and let the main thread do some conditional formating

@tony-go
Copy link
Contributor

tony-go commented Dec 12, 2021

Hey 👋

Thanks for merging the quick refactor ^^ I think that the incremental approach it's the better one.

Now I have a better vision about where I should play:

https://github.com/antfu-sponsors/vitest/blob/d9d2075c4db43850ec1999d81347d1dd9c82bcc3/src/reporters/error.ts#L256

What I have imagined in terms of improvement is:

Display the object with (image it will more color than that ^^ and more polished):

👋🏼 Hey hacker: we found a diff between what is expected and what is actual:

1 - Where:

Object {
     "this": Object {
       "is": Set {
  ->   🙈 Here (Something goes wrong at this position)   
         "snapshot",
       },
     },
   }

2 - What we got:

-> "of",

3 - What we want:

-> "hey"

Then, If objects/strings become very long we have them isolated.
I also imagine that for people that are comfortable with the previous approach we could add an option to let them see the merged one version.

Let me know. I prefer to ask first, to be sure that on your side you agree with this approach.

If you don't like it, I can imagine something else.

@antfu
Copy link
Member Author

antfu commented Dec 12, 2021

I would like to have this minimal as possible. As we might expect multiple snapshot reporting in the same run, making it short could make improve the readability.

I think we could:

  • For large objects with small changes, we could only frame ±5 lines to avoid printing the entire object
  • Currently, the error message contains the entire stringified expect and receive object, which is necessarily duplicated and not really readable since it does not wrap. I think we could find some way to get rid of them to say Snapshot "xxx" mismatched and show the detailed diff below.

@tony-go
Copy link
Contributor

tony-go commented Dec 13, 2021

Here a diff between a string and a big array (length: 300)

Screenshot 2021-12-13 at 10 14 39

What do you think @antfu?

@antfu
Copy link
Member Author

antfu commented Dec 13, 2021

Looks good

@antfu antfu closed this as completed Dec 18, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request pr welcome
Projects
None yet
Development

No branches or pull requests

2 participants