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

Mark snapshots as obsolete when moved to an inline snapshot #6773

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

azz
Copy link
Contributor

@azz azz commented Jul 28, 2018

Summary

Currently when a snapshot is migrated from toMatchSnapshot to toMatchInlineSnapshot, the external snapshot becomes orphaned and there is no way to remove it (other than manually editing/deleting the file).

This change marks the migrated snapshot as obsolete, thus it can be deleted with --updateSnapshot.

Snapshots:   1 obsolete, 1 written, 1 total

Test plan

Added an E2E test that writes an external snapshot, changes the code to toMatchInlineSnapshot, then runs Jest without -u to verify the snapshot is marked as obsolete, then with -u to verify the snapshot is removed.

Fixes #6655

@codecov-io
Copy link

codecov-io commented Jul 28, 2018

Codecov Report

Merging #6773 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6773      +/-   ##
==========================================
- Coverage   63.63%   63.62%   -0.01%     
==========================================
  Files         235      235              
  Lines        9017     9018       +1     
  Branches        4        4              
==========================================
  Hits         5738     5738              
- Misses       3278     3279       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-snapshot/src/State.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 277c547...d90341d. Read the comment docs.

// removed with `--updateSnapshot`.
if (!(isInline && this._snapshotData[key])) {
this._uncheckedKeys.delete(key);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could potentially add a check that fails the test if the inline snapshot doesn't match the external one. Thoughts?

Copy link
Member

@rickhanlonii rickhanlonii Jul 28, 2018

Choose a reason for hiding this comment

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

When a test name changes we write the new snapshot and mark the the old for removal so I think doing the same (without failing) for moving to the inline makes sense. Besides - how would you know that they changed the matcher fn instead of removing the old fn and snapshotting something else in line?

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

@SimenB
Copy link
Member

SimenB commented Aug 6, 2018

Changelog 😶

@azz
Copy link
Contributor Author

azz commented Aug 7, 2018

Oops 😄

@thymikee thymikee merged commit 56411e0 into jestjs:master Aug 7, 2018
thymikee added a commit to rhysawilliams2010/jest that referenced this pull request Aug 8, 2018
* upstream/master: (122 commits)
  fix: don't report promises as open handles (jestjs#6716)
  support serializing `DocumentFragment` (jestjs#6705)
  Allow test titles to include array index (jestjs#6414)
  fix `toContain` suggest to contain equal message (jestjs#6810)
  fix: testMatch not working with negations (jestjs#6648)
  Add test cases for jestjs#6744 (jestjs#6772)
  print stack trace on calls to process.exit (jestjs#6714)
  Updates SnapshotTesting.md to provide more info. on snapshot scope (jestjs#6735)
  Mark snapshots as obsolete when moved to an inline snapshot (jestjs#6773)
  [Docs] Clarified the use of literal values as property matchers in toMatchSnapshot() (jestjs#6807)
  Update CHANGELOG.md (jestjs#6799)
  fix changelog entry that is not in 23.4.2 (jestjs#6796)
  Fix --coverage with --findRelatedTests overwriting collectCoverageFrom options (jestjs#6736)
  Update testURL default value from about:blank to localhost (jestjs#6792)
  fix: `matchInlineSnapshot` when prettier dependencies are mocked (jestjs#6776)
  Fix retryTimes and add e2e regression test (jestjs#6762)
  Release v23.4.2
  Docs/ExpectAPI: Correct docs for `objectContaining` (jestjs#6754)
  chore(packages/babel-jest) readme (jestjs#6746)
  docs: noted --coverage aliased by --collectCoverage (jestjs#6741)
  ...
@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 12, 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.

Jest ignores obsolete file snapshots if there are inline snapshots
6 participants