-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
// removed with `--updateSnapshot`. | ||
if (!(isInline && this._snapshotData[key])) { | ||
this._uncheckedKeys.delete(key); | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changelog 😶 |
Oops 😄 |
* 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) ...
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. |
Summary
Currently when a snapshot is migrated from
toMatchSnapshot
totoMatchInlineSnapshot
, 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
.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