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

jest-snapshot: Distinguish empty string from external snapshot not written #8880

Merged
merged 4 commits into from
Aug 30, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

When I tested the report for edge case that snapshot is empty string from custom serializer and received value is non-empty string, Jest confused it with new snapshot not written:

snapshot empty string 1

Fixed three places where the code distinguished snapshot from no snapshot:

  • baseline: truthy versus falsey
  • improved: string versus undefined (or nil, see Question) following example of code snippet:
    const hasSnapshot = isInline
      ? inlineSnapshot !== ''
      : this._snapshotData[key] !== undefined;

Question: What do you think about the conditional expression where undefined becomes null in the object that match returns? I am trying to keep the changes in complicated code to a minimum, but it fails the grep test to be able to find undefined :(

Residue:

  • The test feedback above does not include the snapshot name
  • Inline snapshots also have problems with empty string versus undefined

Test plan

The test does not save snapshots in CI mode by default still passes in e2e snapshot.test.ts

Also a local test of new snapshot with --ci option passed before and after

Here is the report when the local test fails for the correct reason:

snapshot empty string 2

I guess I had better get comfortable writing e2e tests for snapshots ;)

@codecov-io
Copy link

codecov-io commented Aug 26, 2019

Codecov Report

Merging #8880 into master will increase coverage by 0.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8880      +/-   ##
==========================================
+ Coverage   64.11%   64.13%   +0.01%     
==========================================
  Files         275      275              
  Lines       11623    11623              
  Branches     2846     2844       -2     
==========================================
+ Hits         7452     7454       +2     
  Misses       3545     3545              
+ Partials      626      624       -2
Impacted Files Coverage Δ
packages/jest-snapshot/src/State.ts 0% <0%> (ø) ⬆️
packages/jest-snapshot/src/index.ts 28.66% <66.66%> (+1.33%) ⬆️

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 fb7b132...dd49593. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

A test would be nice 😀

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Small test maybe? :)

@pedrottimark
Copy link
Contributor Author

Yes, after beating my head against the original e2e snapshot tests, I found the more recent tests, which I will adapt for this purpose tomorrow. It requires a new test because of custom serializer.

@pedrottimark
Copy link
Contributor Author

Added e2e test to when expected external snapshot is empty string, then received value non-empty string causes ordinary test failure instead of bogus new snapshot not written

In the same file, I temporarily tested a future test (pardon pun) when expected internal snapshot is empty string, then received value non-empty string causes ordinary test failure instead of:

  • overwriting snapshot without -u option
  • new snapshot not written, if --ci option

Also added SnapshotReturnOptions type to verify that || '' was not needed

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

5 participants