-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: Ignore indentation for most serialized objects #9203
jest-snapshot: Ignore indentation for most serialized objects #9203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so good, I continue to be amazed over how much you're able to do with these matchers. Great job! 👏
Question, does this also work for inline snapshots? I'd assume so, but since we already mess with indentation in them, I thought I'd ask.
Another q - should GlobalConfig.expand
show more (full) diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
}; | ||
const indented = formatLines2(val); | ||
|
||
expect(dedentLines(indented)).toEqual(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's quite a bunch of repetition in these tests. It's quite readable and I don't say it's bad. However, have you considered using test.each
where it makes sense (e.g. for smaller inputs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, super suggestion!
Here is a picture to answer 2 questions:
Many thanks Simen and Michał for patient review of so many prerequisite pull requests to reduce complexity so that we can spend some where it makes a difference (pardon pun :) |
Thoughts on some sort of gutter indicator whitespace is not highlighted when |
@SimenB You mean a visual hint where indentation differs, but lines are considered common? I used cyan instead of dim for non-snapshot matchers, but deleted it in the
Just to stretch our imaginations, this problem would have been solved automatically for markup, if the plugins did not indent tags but did indent the additional lines of multi-line text nodes: <div>
<pre
className="language-js"
>
for (key in foo) {
if (Object.prototype.hasOwnProperty.call(foo, key)) {
doSomething(key);
}
}
</pre>
</div> |
* master: chore: upgrade to fsevents 2 (jestjs#9215) docs: remove expect.assertions(1) in rejects example of Tutoria… (jestjs#9149) chore: bump to istanbul alphas (jestjs#9192) Fix typo in JestPlatform.md (jestjs#9212) jest-snapshot: Ignore indentation for most serialized objects (jestjs#9203) fix(jest-types): tighten Config types and set more defaults (jestjs#9200) jest-snapshot: Improve colors when snapshots are updatable (jestjs#9132) jest-snapshot: Omit irrelevant received properties when property matchers fail (jestjs#9198) chore: make changedFiles option optional in `shouldInstrument` (jestjs#9197) fix(pretty-format): correctly detect memo (jestjs#9196) chore: regenerate lockfiles in e2e tests (jestjs#9193) chore: bump handlebars
We need to re-indent the lines printed from `graphql-js` in `astSerializer`, because Jest has started to ignore indentation when diffing snapshots, and it does this by invoking snapshot serializers with these values set to 0. Without re-indenting, every line printed from `graphql-js` would be shown as changed. See jestjs/jest#9203.
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
Fixes #8626
Call
diffLinesUnified2
to compare without indentation:indent
arg to serialize received without indentationdedentLines
heuristic to remove indentation from snapshotFall back to baseline
diffLinesUnified
in edge case of multi-line strings:Test plan
dedent.test.ts
printSnapshot.test.ts
(although my eyes have already adjusted to magenta and teal in console,<m>
and<t>
in snapshots will take a while longer)See also pictures in following comment
Example pictures baseline at left and improved at right