Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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 azz force-pushed the obsolete-inline branch from 702aa7d to d90341d Compare August 7, 2018 05:34
@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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
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