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

jest-snapshot: Improve colors when snapshots are updatable #9132

Merged
merged 12 commits into from Nov 18, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Nov 4, 2019

Summary

Delightful diff colors suggest that the decision you need to make:

  • is prerequisite to a code review (for example, git diff, GitHub, GitLab)
  • differs from a decision about failure of Expected/Received matchers

Fixes #5430 and closes #8572 /cc @brunotavares @cpojer @thymikee

See short summary of second proposal to replace green with teal #9132 (comment)

Delightful diff colors are magenta and teal instead of red and green:

A background tint for changed lines provides:

isUpdatable description colors
false Snapshot state must be initialized black
false Received function did not throw red
false Expected properties / Received value green / red
true Snapshot / Received magenta / teal
true New snapshot was not written teal

chalk@3.0.0 has type declarations for ansi256 and bgAnsi256

Test plan

  • Add 4 tests for chalk level independent of test environment
  • Update 35 snapshots to replace <g> and <r> with <m> and <t>

Update convertAnsi for consistent snapshot independent of test environment

See pictures of first proposal in following comments and second proposal in #9132 (comment)

Example pictures baseline at left and improved at right

Most are 24-bit color chalk level 3 in Visual Studio Code Terminal

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Nov 4, 2019

obsolete first proposal

What is your first impression: which way did it change?

1 which way

In the preceding picture, do counts of changed lines confirm your impression?

Here are adjacent replaced lines:

2a replace lines

with simulated 4-bit color chalk level 1 in older Microsoft Windows Command Prompt:

2b replace lines

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Nov 4, 2019

obsolete first proposal

Here are multiple changes with 8-bit color chalk level 2 in macOS Terminal:

3 change lines

Here are substring and line differences in multi-line strings:

4 or lines

Here are substring differences in one-line strings:

5 one line

In the preceding picture, does the background tint help you recognize a concise report as an updatable snapshot that requires a decision?

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Nov 4, 2019

obsolete first proposal

New snapshot was not written is also an updatable snapshot:

6 not written

When the matcher has properties that match, it is updatable:

7 properties true

When the matcher has properties that do not match, it is not updatable:

8 properties false

The preceding picture with same green/red colors as toMatchObject difference is a reason why not to swap red/green for snapshot difference, in addition to accessibility (see below)

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Nov 4, 2019

obsolete first proposal Accessibility: color vision

See second proposal #9132 (comment)

https://wearecolorblind.com/articles/a-quick-introduction-to-color-blindness/

  • 8% to 10% of the male population is color blind
  • 0.5% of the female population is color blind

deuteranomaly: malfunctioning green cone affects 4.63% of people
deuteranopia: missing green cone affects 1.27% of people

Problem colors green and red have triadic relationship: 120° – 0° = 120°

Delightful colors magenta and green have complementary relationship: 300° – 120° = 180°

Problem colors green and gray are a reason why changed lines have background tint to distinguish from common lines which have dim color

For snapshot colors, protan (red) color vision deficiency is similar to deuteran.

The following pictures have simulated deuteranomaly in middle and deuteranopia at right

https://developer.mozilla.org/en-US/docs/Tools/Accessibility_inspector/Simulation

24-bit color chalk level 3 baseline at left:

4-baseline-color

24-bit color chalk level 3 improved at left:

4-improved-color

8-bit color chalk level 2 baseline at left:

3-baseline-color

8-bit color chalk level 2 improved at left:

3-improved-color

Special thanks to @dubzzz for several rounds of testing to discover that PowerShell 4 magenta is the same as the blueish-black terminal background color (that is, invisible unless changed lines have a background color).

Any mistakes in the following simulated pictures are from my interpretation of

https://devblogs.microsoft.com/commandline/updating-the-windows-console-colors/

4-bit color chalk level 1 baseline at left:

2-baseline-color

4-bit color chalk level 1 improved at left:

2-improved-color

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Nov 4, 2019

obsolete first proposal Accessibility: color contrast

See second proposal #9132 (comment)

https://webaim.org/resources/contrastchecker/

For small text, WCAG 2.0 level AAA contrast ratio is >= 7.0

level depth Snapshot Received example
3 24 7.36 7.27 Visual Studio Code Terminal
2 8 6.78 7.29 macOS 10.12.6 Terminal
1 4 8.77 5.13 improved colors in older Command Prompt
1 4 2.56 5.14 baseline colors on light in Code Terminal

@codecov-io
Copy link

codecov-io commented Nov 4, 2019

Codecov Report

Merging #9132 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9132      +/-   ##
==========================================
+ Coverage   64.85%   64.93%   +0.08%     
==========================================
  Files         279      280       +1     
  Lines       11744    11771      +27     
  Branches     2887     2892       +5     
==========================================
+ Hits         7616     7643      +27     
  Misses       3510     3510              
  Partials      618      618
Impacted Files Coverage Δ
packages/jest-snapshot/src/index.ts 73.23% <100%> (ø) ⬆️
packages/jest-snapshot/src/printSnapshot.ts 100% <100%> (ø) ⬆️
packages/jest-snapshot/src/colors.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d0b5e2...eeeef10. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Nov 5, 2019

This is exciting! 👏 I'll look over this soonish, but the updated screenshots all look better to me 👍

@gaearon
Copy link
Contributor

gaearon commented Nov 6, 2019

This is great and a clear improvement. Thanks for going an extra mile and not settling on the simple "let's invert the colors".

@jeysal
Copy link
Contributor

jeysal commented Nov 6, 2019

Making the diff visually similar to familiar code review tools is a really interesting approach to resolving the two opposing approaches. I also like that the color contrasts are easier on the eye in general.
Going beyond how diffs are rendered on GitHub etc., you also changed red to magenta, which further distinguishes it from Expected/Received comparisons. Do you think we could do something to the green as well, such as making it a bit turquoise?

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Nov 8, 2019

obsolete half step forward

@jeysal Super question! Which reminds me to acknowledge the hard work by you and Simen to give constructive critique on the prerequisite pull requests that enabled us to take this step ❤️

A hue clockwise from green on the color wheel was also my intuition, before I tested with color vision simulations. Let’s reserve teal at 180° for a possible future improvement.

Complementary colors (opposite on color wheel) are a straightforward way for non-experts (like me) to solve accessibility problems like green-red, which remains for non-snapshot matchers.

For example, pair one “familiar” color with its complementary “unfamiliar” color:

  • familiar green with unfamiliar magenta for snapshot matchers now
  • familiar red with unfamiliar teal for snapshot properties and other matchers as a visual branding if and when we can replace serialization diff with data-driven diff (see below)

That is, familiar green helps us learn that magenta is a substitute for red.

However, there is a real risk (implied by your question) that green might influence people to update snapshots without review. Therefore, we need to emphasize the analogy to code review.

Here are pictures of baseline compared to an imagined data-driven diff:

d3 toMatchSnapshot

d3 toMatchObject

Does familiar red help us learn that teal on cyan/aqua is a substitute for green? To my eyes, the analogy to green is stronger for foreground teal than background cyan/aqua, even though they have similar hue-saturation-value relationship as the other fg-bg pairs.

But a positive point in favor looking obviously different is that red-cyan/aqua versus magenta-green gives fast pattern recognition (to 90% of people who have full color vision) which interpretation and decision to evaluate each of a variety of test failure reports in terminal.

Your comments are invited, but your approval of possible future is not expected now :)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is really awesome work and the amount of time spent on research and sharing your findings and thought process is way beyond what I'd ever expect. Thank you so much for that!

As for my review, it's just about code readability, nothing really jumped out. Great work, as always!

Could you bump to chalk 3 all around?

packages/jest-snapshot/src/__tests__/printSnapshot.test.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/__tests__/printSnapshot.test.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/index.ts Outdated Show resolved Hide resolved
packages/jest-snapshot/src/printSnapshot.ts Outdated Show resolved Hide resolved
export const bForeground3 = [0, 95, 0]; // #005f00
export const bBackground3 = [215, 255, 215]; // #d7ffd7

export const getSnapshotColor = (chalkInstance: Chalk): DiffOptionsColor =>
Copy link
Member

Choose a reason for hiding this comment

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

I find this hard to read, could we do some ifs and returns rather than nested ternaries?

export const bBackground2 = 194; // #d7ffd7

// Contast 7.36 AAA
export const aForeground3 = [128, 0, 128]; // #800080
Copy link
Member

Choose a reason for hiding this comment

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

Add as const at the end or : [number, number, number] so it's typed as a tuple rather than number array? Probably the latter extracted to a type Rgb = [number, number, number] so it can be reused

if (!isUpdatable) {
options.secondArgumentColor = noColor;
}
options.secondArgumentColor = isUpdatable
Copy link
Member

Choose a reason for hiding this comment

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

if-else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You get bonus points for leading me to a mistake, because the similar code below be if without else but the condition was backward.

@@ -384,7 +385,7 @@ const _toMatchSnapshot = (config: MatchSnapshotConfig) => {
`must be explicitly passed to write a new snapshot.\n\n` +
`This is likely because this test is run in a continuous integration ` +
`(CI) environment in which snapshots are not written by default.\n\n` +
`Received:${actual.includes('\n') ? '\n' : ' '}${RECEIVED_COLOR(
`Received:${actual.includes('\n') ? '\n' : ' '}${getReceivedColor()(
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why this needs to be an extra function call? Can this be withReceivedColor(actual) or something, and that can on the fly check chalk.level? the thing()(otherThing) looks weird to me

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's due to the getSnapshotColor() further along. I still think it'd be preferable to keep it at a minimum, but I guess keeping the differences between the apis down is good as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, good question. I was thinking about “lazy evaluation” in case the tests pass and we never need the colors for a report.

I am happy to simplify to more intuitive: bReceivedColor(actual)

export const aSnapshotColor = getSnapshotColorForChalkInstance(chalk);
export const bReceivedColor = getReceivedColorForChalkInstance(chalk);

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that :)

packages/jest-snapshot/src/__tests__/printSnapshot.test.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@jeysal
Copy link
Contributor

jeysal commented Nov 10, 2019

@pedrottimark Trying to provide perspective as someone in the "assertion" camp (as opposed to the "git diff" camp), the reason why I am not comfortable with either red for Snapshot or green for Received is that red makes the snapshot seem "wrong", or green makes the Received seem "right", implying that most of the times a snapshot assertions fails you will want to update it.
Upholding the benefits of complementary colors you are referring to, have you considered orange (red-yellow) for Snapshot and light blue (cyan-blue) for Received? Color wheel for reference

@MrOggy85
Copy link

Is there an option to revert to the normal colors (for us who are not colorblind)?

@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 11, 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.

Diff colors should show green for insert, red for delete
7 participants