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

Human-readable colors for jest-matchers snapshots #3119

Merged
merged 3 commits into from
Mar 10, 2017

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Mar 10, 2017

Summary

Storing ANSI characters in snapshots of jest-matchers isn't very helpful and adds unnecessary noise, making it harder to review. This is a proposal to change it to something more human-readable like <green> or <red> or </> for color reset sequence.

Test plan

Look at snapshots diff

@thymikee
Copy link
Collaborator Author

cc: @DmitriiAbramov

@@ -12,5 +12,10 @@
"jest-matcher-utils": "^19.0.0",
"jest-message-util": "^19.0.0",
"jest-regex-util": "^19.0.0"
},
"devDependencies": {
"ansi-regex": "^2.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are ansi-regex and ansi-styles added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I use them in here: https://github.com/facebook/jest/pull/3119/files#diff-5d7e608b44e8080e7491366117a2025fR20
They're also already available through chalk

Copy link
Member

Choose a reason for hiding this comment

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

Can you make ansi-styles and ansi-regex devDependencies of the top level package.json instead? jest-snapshot should stay here but please use the latest version (19.0.2).

@codecov-io
Copy link

codecov-io commented Mar 10, 2017

Codecov Report

Merging #3119 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3119      +/-   ##
=========================================
- Coverage    69.4%   69.2%   -0.21%     
=========================================
  Files         156     157       +1     
  Lines        5514    5530      +16     
  Branches        3       3              
=========================================
  Hits         3827    3827              
- Misses       1686    1702      +16     
  Partials        1       1
Impacted Files Coverage Δ
packages/pretty-format/src/plugins/ConvertAnsi.js 0% <0%> (ø)

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 a8c0a47...2112c92. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Mar 10, 2017

Lovely! This makes snapshot files a lot more readable. Can we make this a utility function somewhere and use it for all the matcher snapshots? I think we also do this in other files.

@thymikee
Copy link
Collaborator Author

@kentaromiura advised that it may be a good idea to write a custom snapshotSerializer so we don't have to create new matchers. What do you think @cpojer?


return toMatchSnapshot.call(this, error.message);
return toMatchSnapshot.call(this, errorMessage);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We pass a string to toMatchSnapshot so we either need to:

  1. add a symbol, so we can test for it in the snapshot serializer
  2. change nothing and test for ansi characters inside the plugin (which will alter our regular snapshots too)
  3. pass whole Error object and test for JestAssertionError in toMatchSnapshot and extract its message if necessary

@cpojer
Copy link
Member

cpojer commented Mar 10, 2017

I like the idea overall but tagging Jest code with Symbol.for('toThrowErrorMatchingSnapshot') simply to make our own tests better doesn't seem like a great way to go about it :(

@cpojer cpojer merged commit 202d452 into jestjs:master Mar 10, 2017
@maximderbin
Copy link
Contributor

Wow. That's sweet

@thymikee thymikee deleted the ansi-snapshot branch March 10, 2017 16:29
@aaronabramov
Copy link
Contributor

i wonder if it would make sense to go the other direction.
Use XML-like tags for coloring our text and then convert them to the weird ANSI sequences. This way we could probably make it more compatible with other platforms (like generating HTML reports)

@rogeliog
Copy link
Contributor

🎉

@jest-bot
Copy link
Contributor

Warnings
⚠️ Changes were made to package.json, but not to yarn.lock - Perhaps you need to run `yarn install`?

Generated by 🚫 dangerJS

skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
* Human-readable colors for jest-matchers snapshots

* Use ConvertAnsi pretty-format plugin as snapshotSerializer

* Convert ansi escapes in all snapshots
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Human-readable colors for jest-matchers snapshots

* Use ConvertAnsi pretty-format plugin as snapshotSerializer

* Convert ansi escapes in all snapshots
@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 13, 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.

9 participants