Testing: Try adding snapshot-diff #28897
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarayourfriend
added
[Type] Automated Testing
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
6 tasks
sarayourfriend
commented
Feb 9, 2021
Comment on lines
3
to
+23
| font-weight: var(--wp-g2-font-weight); | ||
| margin: 0; | ||
| display: block; | ||
| max-height: 100%; | ||
| max-width: 100%; | ||
| min-height: 0; | ||
| min-width: 0; | ||
| display: var(--wp-g2-flex-item-display); | ||
| -webkit-flex: 1; | ||
| -ms-flex: 1; | ||
| flex: 1; | ||
| } | ||
|
|
||
| @media (prefers-reduced-motion) { | ||
| .emotion-1.emotion-1.emotion-1.emotion-1.emotion-1.emotion-1.emotion-1 { | ||
| -webkit-transition: none !important; | ||
| transition: none !important; | ||
| } | ||
| } | ||
|
|
||
| [data-system-ui-reduced-motion-mode="true"] .emotion-1.emotion-1.emotion-1.emotion-1.emotion-1.emotion-1.emotion-1 { | ||
| -webkit-transition: none !important; | ||
| transition: none !important; | ||
| } | ||
|
|
||
| .emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2 { | ||
| box-sizing: border-box; | ||
| -moz-osx-font-smoothing: grayscale; | ||
| -webkit-font-smoothing: antialiased; | ||
| font-family: Inter,system-ui,-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,"Helvetica Neue",sans-serif; | ||
| font-family: var(--wp-g2-font-family); | ||
| font-size: 13px; | ||
| font-size: var(--wp-g2-font-size); | ||
| font-weight: normal; | ||
| font-weight: var(--wp-g2-font-weight); | ||
| margin: 0; | ||
| display: -webkit-box; | ||
| display: -webkit-flex; | ||
| display: -ms-flexbox; | ||
| display: flex; | ||
| --wp-g2-flex-gap: calc(4px * 2); | ||
| --wp-g2-flex-gap: calc(var(--wp-g2-grid-base) * 2); | ||
| --wp-g2-flex-item-margin-bottom: 0; | ||
| --wp-g2-flex-item-margin-right: calc(4px * 2); | ||
| --wp-g2-flex-item-margin-right: var(--wp-g2-flex-gap); | ||
| --wp-g2-flex-item-margin-left: 0; | ||
| -webkit-align-items: flex-start; | ||
| -webkit-box-align: flex-start; | ||
| -ms-flex-align: flex-start; | ||
| align-items: flex-start; | ||
| -webkit-flex-direction: row; | ||
| -ms-flex-direction: row; | ||
| flex-direction: row; | ||
| -webkit-box-pack: justify; | ||
| -webkit-justify-content: space-between; | ||
| -ms-flex-pack: justify; | ||
| justify-content: space-between; | ||
| width: 100%; | ||
| } | ||
|
|
||
| @media (prefers-reduced-motion) { | ||
| .emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2 { | ||
| -webkit-transition: none !important; | ||
| transition: none !important; | ||
| } | ||
| } | ||
|
|
||
| [data-system-ui-reduced-motion-mode="true"] .emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2 { | ||
| -webkit-transition: none !important; | ||
| transition: none !important; | ||
| } | ||
|
|
||
| .emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2 > * + *:not(marquee) { | ||
| margin-left: calc(4px * 2); | ||
| margin-left: calc(var(--wp-g2-grid-base) * 2); | ||
| } | ||
|
|
||
| .emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2.emotion-2 > * { | ||
| min-width: 0; | ||
| } | ||
| "Snapshot Diff: | ||
| - First value | ||
| + Second value | ||
|
|
||
| @@ -7,14 +7,10 @@ | ||
| class=\\"components-flex-item wp-components-flex-item css-uyw6h0\\" | ||
| data-g2-c16t=\\"true\\" | ||
| data-g2-component=\\"FlexItem\\" | ||
| /> | ||
| <div | ||
| - class=\\"css-1nqjm19\\" | ||
| - /> | ||
| - <div /> | ||
| - <div | ||
| class=\\"components-flex-item wp-components-flex-item components-flex-block wp-components-flex-block css-2e3tg1\\" | ||
| data-g2-c16t=\\"true\\" | ||
| data-g2-component=\\"FlexBlock\\" | ||
| /> | ||
| </div>" | ||
| `; |
There was a problem hiding this comment.
We use the regular toMatchDiffSnapshot here because we only care about the markup, not the styles.
sarayourfriend
commented
Feb 9, 2021
Comment on lines
25
to
46
| exports[`props should render align 1`] = ` | ||
| "Snapshot Diff: | ||
| - First value | ||
| + Second value | ||
|
|
||
| - Snapshot Diff: | ||
| - - First value | ||
| - + Second value | ||
| - | ||
| - @@ -1,11 +1,11 @@ | ||
| - Object { | ||
| - \\"--wp-g2-flex-gap\\": \\"calc(var(--wp-g2-grid-base) * 2)\\", | ||
| - \\"--wp-g2-flex-item-margin-bottom\\": \\"0\\", | ||
| - \\"--wp-g2-flex-item-margin-left\\": \\"0\\", | ||
| - \\"--wp-g2-flex-item-margin-right\\": \\"var(--wp-g2-flex-gap)\\", | ||
| - - \\"align-items\\": \\"flex-start\\", | ||
| - + \\"align-items\\": \\"center\\", | ||
| - \\"box-sizing\\": \\"border-box\\", | ||
| - \\"display\\": \\"flex\\", | ||
| - \\"flex-direction\\": \\"row\\", | ||
| - \\"font-family\\": \\"var(--wp-g2-font-family)\\", | ||
| - \\"font-weight\\": \\"var(--wp-g2-font-weight)\\"," |
There was a problem hiding this comment.
Here is an example of toMatchStyleDiffSnapshot. You'll notice that the diff notes that align-items has changed between the two values.
sarayourfriend
commented
Feb 9, 2021
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
|
This repository notes the problems with emotion and snapshot-diff: https://github.com/DarkPurple141/jest-emotion-typescript-minimal In particular, this line is what I was running into as well, where the styles would be serialized but not the markup: https://github.com/DarkPurple141/jest-emotion-typescript-minimal/blob/master/src/__snapshots__/app.test.tsx.snap#L53 |
ItsJonQ
approved these changes
Feb 9, 2021
There was a problem hiding this comment.
❤️ Wow! This cuts down on the noise by a lot!
It took me a second to get the diffing flow (just a second), but it makes sense after that click.
I've never used this particular library before, but I can imagine how this strategy will scale. I think this will be tremendously helpful and make snapshots feel less of a chore to work around.
Thank you @saramarcondes !!
P.S. The \\ problem is interesting. I'd take a couple of \\ over gigantic snapshots any day!
|
This is aaaammmmaaazzzing! Exactly what I had in mind. We can iterate further once we have more tests converted. For now it's way better than trying to distill the same information from the full snapshot. I think it's perfectly fine to have 3 different matchers for snapshots. In fact, there is more like the one that inlines the output in the same file 😃 Let's get it in and iterate based on the experience in the upcoming PRs. |
gziolo
reviewed
Feb 10, 2021
| window.getComputedStyle( expected )._values | ||
| ); | ||
| const difference = snapshotDiff( receivedStyles, expectedStyles ); | ||
| return snapshotDiff.toMatchDiffSnapshot.call( |
There was a problem hiding this comment.
Did you check what this function returns? As far as I remember a matcher is an object so maybe you can remove // at this stage.
There was a problem hiding this comment.
See https://github.com/WordPress/gutenberg/blob/master/packages/jest-console/src/matchers.js
It was created before Jest added some internal references to the API that is used there but it probably still works very similar.
There was a problem hiding this comment.
Ooo I did not check that! I'll look and see if I can do something about it there.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR add's the
snapshot-difflibrary for diffing two values and using the diff as a snapshot. The rationale behind the usage of this library (as opposed to raw snapshot testing) can be found here: https://kentcdodds.com/blog/effective-snapshot-testing#snapshot-diffFor our purposes, the goal here is to minimize the size of a snapshot so that only the important parts are visible.
For example, snapshots that used to include upwards of ten emotion classes with hundreds of styles are reduced to just the difference in style between two elements.
To accomplish this, I had to write a simple custom matcher that diffs the
getComputedStylesresult.getComputedStylesis what@testing-library/jestuses fortoHaveStyleso I think it's safe to say this is fine to use in this way.The only difference here is that we're reducing what we're testing down to just the styles of the two elements rather than diffing the entire elements.
Of course, this introduces some cognitive overhead as now we have three (yes, three) different snapshot testing tools:
toMatchSnapshottoMatchDiffSnapshottoMatchStyleDiffSnapshotFurther work on this PR could include adding documentation around how to decide which to use when. The jist of it is this:
toMatchSnapshot-> use when wanting to take a snapshot of the entire output of something (logs, an entire component tree that should not change, etc)toMatchDiffSnapshot-> use when wanting to snapshot the diff of the markup of two elements but not the stylestoMatchStyleDiffSnapshot-> use when wanting to snapshot the diff of styles between two elements.Note: I'm unsure why those extra
\\are included in the snapshot test. I'd love to get rid of this noise but couldn't figure out exactly where to do that. I think they're being added bysnapshotDiffbut I'm not sure how to get rid of them.String.prototype.replacedidn't work in the couple places I tried (in theserializefunction and on the result ofsnapshotDiff). If anyone has further ideas on how to get rid of that noise I'd love input. Otherwise, it's not too bad, I can personally live with it if it means our snapshot tests are reduced by such a significant amount.How has this been tested?
Snapshot tests updated. Please take a look at them and see if they make sense.
Types of changes
New feature.
Checklist: