jest-snapshot: Improve colors when snapshots are updatable #9132
Conversation
obsolete first proposalWhat is your first impression: which way did it change?
In the preceding picture, do counts of changed lines confirm your impression? Here are adjacent replaced lines:
with simulated 4-bit color chalk level 1 in older Microsoft Windows Command Prompt:
|
obsolete first proposalHere are multiple changes with 8-bit color chalk level 2 in macOS Terminal:
Here are substring and line differences in multi-line strings:
Here are substring differences in one-line strings:
In the preceding picture, does the background tint help you recognize a concise report as an updatable snapshot that requires a decision? |
obsolete first proposalNew snapshot was not written is also an updatable snapshot:
When the matcher has properties that match, it is updatable:
When the matcher has properties that do not match, it is not updatable:
The preceding picture with same green/red colors as |
obsolete first proposal Accessibility: color visionSee second proposal #9132 (comment) https://wearecolorblind.com/articles/a-quick-introduction-to-color-blindness/
deuteranomaly: malfunctioning green cone affects 4.63% 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:
24-bit color chalk level 3 improved at left:
8-bit color chalk level 2 baseline at left:
8-bit color chalk level 2 improved at left:
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:
4-bit color chalk level 1 improved at left:
|
obsolete first proposal Accessibility: color contrastSee second proposal #9132 (comment) https://webaim.org/resources/contrastchecker/ For small text, WCAG 2.0 level AAA contrast ratio is >= 7.0
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
This is exciting! 👏 I'll look over this soonish, but the updated screenshots all look better to me 👍 |
|
This is great and a clear improvement. Thanks for going an extra mile and not settling on the simple "let's invert the colors". |
|
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. |
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:
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:
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 :) |
There was a problem hiding this comment.
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?
| export const bForeground3 = [0, 95, 0]; // #005f00 | ||
| export const bBackground3 = [215, 255, 215]; // #d7ffd7 | ||
|
|
||
| export const getSnapshotColor = (chalkInstance: Chalk): DiffOptionsColor => |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
You get bonus points for leading me to a mistake, because the similar code below be if without else but the condition was backward.
packages/jest-snapshot/src/index.ts
Outdated
| @@ -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()( | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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);
|
@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. |
|
Is there an option to revert to the normal colors (for us who are not colorblind)? |
|
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. |
Summary
Delightful diff colors suggest that the decision you need to make:
Fixes #5430 and closes #8572 /cc @brunotavares @cpojer @thymikee
See short summary of second proposal to replace
greenwith teal #9132 (comment)Delightful diff colors are magenta and teal instead of red and green:
A background tint for changed lines provides:
isUpdatablefalsefalsefalsetruetrue[email protected]has type declarations foransi256andbgAnsi256Test plan
<g>and<r>with<m>and<t>Update
convertAnsifor consistent snapshot independent of test environmentSee 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