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

Diff colors should show green for insert, red for delete #5430

Closed
Athaphian opened this issue Jan 31, 2018 · 8 comments · Fixed by #9132
Closed

Diff colors should show green for insert, red for delete #5430

Athaphian opened this issue Jan 31, 2018 · 8 comments · Fixed by #9132

Comments

@Athaphian
Copy link

FEATURE REQUEST

When snapshot testing, the console shows differences between old and new snapshots with a DIFF view. The colors in the DIFF view are reversed as opposed to most other popular tools. Normally green is for inserts and red is for deletions. Jest shows this the wrong way around which is very confusing when using jest in combination with other diff tools (like bitbucket, git diff, intelliJ, etc).

So this feature request is: make deleted rows show as RED and inserted rows show as GREEN.

@SimenB
Copy link
Member

SimenB commented Jan 31, 2018

See #2347 (comment) for some discussion.

I'm still in favour of doing something like this 🙂

@thymikee
Copy link
Collaborator

thymikee commented Jan 31, 2018

This is intended behavior.

Basically, in snapshot land GREEN is the accepted previous state. If something changed, Jest highlights new changes in RED to draw your attention, as this is potentially something that's failing, or just something that needs to be updated.

We get that it may be unintuitive at first glance, but it really makes sense. IMO we shouldn't change it.

@Athaphian
Copy link
Author

It might be true that green was the accepted state... but this is a diff between the old snapshot and the new snapshot (the new snapshot will in most cases be accepted anyway). I still feel that stuff that is/will be inserted after a change should be + green and stuff that is/will be removed should be - red..

The number of times me and my coworkers have been searching for a problem that wasn't there because we simply misinterpreted the diff does not weigh up to being 'correct in snapshot land'. At least for us it doesn't.

Maybe add a config option to allow users to configure their own colors? That way everybody's happy..

@thymikee
Copy link
Collaborator

the new snapshot will in most cases be accepted anyway

Let's not generalize about "most cases", as it varies for different projects. But I'd be supportive to adding a config option, if it's not going to introduce a lot of extra complexity.

@Athaphian
Copy link
Author

That would be awesome!

I agree that it might not be the case for every project indeed.

Thanks for being supportive.

@quantizor
Copy link
Contributor

I’d like to propose some coloring changes. Since the rest of the snap is grey, what if the original diff was just white with the change being yellow/amber?

That way the diff is more of a warning than an error, since you might actually intend the change.

@manovotny
Copy link

I and two others on my team spent more time that we care to admit publicly tripped up over this.

Because of the removal of undefined props in Jest 23, we started seeing these failures and kept thinking to ourselves, "Why is it adding undefined props when it should be removing them?!".

It drove us mad until I realized the colors were flipped when I noticed the - and + next to Snapshot and Received.

Screen Shot 2019-06-15 at 3 45 59 PM

I mean, we're fine now, as this will forever be seared into our brains going forward, but it was completely opposite compared to what we're all accustom to seeing in the dev world, like here on GitHub.

@silverwind
Copy link
Contributor

silverwind commented Nov 30, 2020

How about adding an option like invertSnapshotDiff? Would that be acceptable for a contribution?

Edit: Actually I was on an older jest version that did not have #9132 yet, looks better now. While I don't agree with the background colors there on a dark terminal, at least the foreground is expected now.

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

Successfully merging a pull request may close this issue.

6 participants