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

chore: Align snapshots in jest-diff #8959

Merged
merged 1 commit into from
Sep 17, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

Display colors concisely and clearly so you can review future changes quickly and confidently

Also omits distracting escape of double quote marks

  • Use addSnapshotSerializer for only the expected colors with one-letter tag names
  • Use stripAnsi only for edge case error messages
  • Replace stripAnsi with options and toMatch with toBe in assertions for comparison lines (which as a future chore could become snapshots with the serializer and omitAnnotationLines option)

Question: Before I go on to 5 snapshot test files in expect package, do I need to consider an alternative to repetition of the same or very similar inline serializer?

Test plan

  • diff.test.ts.snap updated 35

@codecov-io
Copy link

Codecov Report

Merging #8959 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #8959   +/-   ##
======================================
  Coverage    64.2%   64.2%           
======================================
  Files         276     276           
  Lines       11718   11718           
  Branches     2868    2869    +1     
======================================
  Hits         7523    7523           
  Misses       3563    3563           
  Partials      632     632

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 6c0a16a...bec979a. Read the comment docs.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Question: Before I go on to 5 snapshot test files in expect package, do I need to consider an alternative to repetition of the same or very similar inline serializer?

Would probably make sense to extract this - packages/test-utils is a bit of a mess but maybe we can start using that package for some more things?

import { ansiSnapshotSerializer } from '@jest/test-utils';

@pedrottimark pedrottimark merged commit bc0f55d into jestjs:master Sep 17, 2019
@pedrottimark pedrottimark deleted the align-jest-diff branch September 17, 2019 16:17
@pedrottimark
Copy link
Contributor Author

@jeysal Yeah, test-utils/README.md confused me. To limit the scope of escape code conversions to what is relevant for a specific package, what do you think about, for example:

packages/expect/src/__tests__/serializers/convertAnsi.ts

and include the subdirectory as item of testPathIgnorePatterns in jest.config.js file?

@jeysal
Copy link
Contributor

jeysal commented Sep 18, 2019

Excluding some specific files under __tests__ seems unintuitive to me.
Would one ever need a different ansi serializer? Otherwise it seems fine to me if it can be used from all packages?

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

4 participants