Add snapshot version support #2896

thymikee · 2017-02-15T20:12:47Z

Summary

Here's how the error looks like:

Test plan
I think I should also include one more test for checking if saveSnapshotFile() is called when updating, although test-examples catch this issue.

thymikee · 2017-02-15T20:13:52Z

integration_tests/__tests__/transform-test.js

@@ -98,7 +98,7 @@ describe('multiple-transformers', () => {
  });

  it('transforms dependencies using specific transformers', () => {
-    const {json, stderr} = runJest.json(dir, ['--no-cache']);
+    const {json, stderr} = runJest.json(dir, ['--no-cache', '-u']);


Had to add that, so the snapshot version is updated, otherwise it fails.

hm. can't you update snapshot in the underlying test directory?

Oh I know what happened, I've updated it manually before this change #2896 (comment), so it wasn't updated. You're totally right and fix is already applied, thanks!

thymikee · 2017-02-15T20:15:55Z

packages/jest-cli/src/reporters/__tests__/__snapshots__/utils-test.js.snap

@@ -8,9 +10,9 @@ exports[`trimAndFormatPath() trims dirname 1`] = `"[2m...234567890/[22m[1m123

 exports[`trimAndFormatPath() trims dirname and basename 1`] = `"[1m...1234.js[22m"`;

-exports[`wrapAnsiString() returns the string unaltered if given a terminal width of zero 1`] = `"This string shouldn\'t cause you any trouble"`;
+exports[`wrapAnsiString() returns the string unaltered if given a terminal width of zero 1`] = `"This string shouldn't cause you any trouble"`;


Shouldn't this be fixed?

omg yes. Not sure what is going on here?

Can you remove this snapshot and run the test again and use the result of that?

Wait, actually this is correct now. I don't understand why it wasn't updated before. cc @vjeux.

Maybe because before it was checking the unserialized state so they were both the same and therefore didn't needed an update? idk

thymikee · 2017-02-15T20:17:41Z

packages/jest-snapshot/src/State.js

@@ -83,7 +83,7 @@ class SnapshotState {

    const isEmpty = Object.keys(this._snapshotData).length === 0;

-    if ((this._dirty || this._uncheckedKeys.size) && !isEmpty) {
+    if ((this._dirty || this._uncheckedKeys.size || update) && !isEmpty) {


This is added because otherwise saveSnapshotFile wouldn't be called (adding a version comment doesn't mark snapshot "dirty")

Can we make this so we only write the file if update is given & the snapshot header is either missing or there is a mismatch? I wanna prevent writes from happening if we don't need them.

Updated. I'm making snapshot "dirty" if it is in update mode and would throw invalid version error. I'm not very happy with returning a tuple from getSnapshotData but it's past midnight here and it seems to work :p

thymikee · 2017-02-15T20:19:18Z

packages/jest-snapshot/src/utils.js

+      snapshotContents = fs.readFileSync(snapshotPath, 'utf8');
+      // eslint-disable-next-line no-new-func
+      const populate = new Function('exports', snapshotContents);
+      populate(data);


As already changed here: #2629

cpojer · 2017-02-15T21:22:02Z

examples/react-native/__tests__/__snapshots__/Intro-test.js.snap

@@ -1,3 +1,5 @@
+// Jest Snapshot v1


Let's do:

// Jest Snapshot v1, https://goo.gl/fbAQLP

which links to our snapshot testing guide.

cc @kentcdodds who will be pleased.

Added the link

cpojer · 2017-02-15T21:32:30Z

packages/jest-snapshot/src/utils.js

+  `// Jest Snapshot v${SNAPSHOT_VERSION}`;
+
+const validateSnapshotVersion = (snapshotContents: string) => {
+  const versionTest = SNAPSHOT_VERSION_REGEXP.exec(snapshotContents);


I guess you are already making sure we only match the beginning of the string with ^. I'm not sure why the regex has "g" though? We only want to find the version in the beginning of the file, once.

cpojer · 2017-02-15T21:50:40Z

packages/jest-snapshot/src/utils.js

+
+const validateSnapshotVersion = (snapshotContents: string) => {
+  const versionTest = SNAPSHOT_VERSION_REGEXP.exec(snapshotContents);
+  const version = versionTest && versionTest[1] || '0';


can you do (versionTest && versionTest[1]) || '0'; just for readability?

cpojer · 2017-02-15T22:27:50Z

packages/jest-snapshot/src/utils.js

+      `Stored snapshot version is outdated.\n` +
+      `Expected: v${SNAPSHOT_VERSION}, but received: v${version}\n` +
+      `Update the snapshot to remove this error.`
+    );


This is a great error but I think we should aim to make this more informative and absolutely solid in messaging. Here is my recommendation:

If no snapshot header is not found:

Outdated snapshot: No snapshot header found. Jest 19 introduced versioned snapshots to ensure all people on a project are using the same version of Jest. Please update all snapshots during this upgrade of Jest. More information: jest-19-blogpost (I will update this later)

If snapshot header is lower than current version of Jest:

Outdated snapshot: The version of the snapshot file associated with this test is outdated. The snapshot file version ensures that all people on a project are using the same version of Jest. Please update all snapshots during this upgrade of Jest. Expected: v1 Received: v0 More information: jest-19-blogpost (I will update this later)

If snapshot header is higher than current version of Jest:

Outdated Jest version: the version of this snapshot file indicates that this project is meant to be used with a newer version of Jest. The snapshot file version ensures that all people on a project are using the same version of Jest. Please update your version of Jest and re-run the tests. Expected: v1 Received: v2 More information: jest-19-blogpost (I will update this later)

We must also include a warning (I think this was brought up by @gaearon yesterday) for the first two to indicate that local change to tests shall be reverted during this update to avoid saving wrong snapshot state. For the first two, how about:

Warning: It is advised to revert any local changes to tests or other code during this upgrade to ensure that no invalid state is stored as a snapshot.

What do you think?

I'm all for it. Updated.

cpojer · 2017-02-15T22:38:58Z

I think this is really solid. Thanks for working on this. I'm pretty excited about providing a solid user experience for this workflow – nothing worse than snapshots changing back and forth from one version to another because two people use different versions of Jest.

cpojer · 2017-02-16T13:42:43Z

packages/jest-snapshot/src/utils.js

+  if (update && snapshotContents) {
+    try {
+      validateSnapshotVersion(snapshotContents);
+    } catch (error) {


I kinda feel like we could clean this up to return an error and throw in #105 if an error is returned. Here on #109, instead of try-catch, call the validator and set dirty to true if it returns a value. What do you think as a follow-up?

* Add snapshot version support * Add missing snapshots * Add link to snapshot guide, small regexp fixes * Update examples snapshots * Add parens for readability * Remove unnecessary -u flag from transform test * Update snapshot version messages to be more exhaustive * Fix error formatting * Don't write to fs if not needed

github-actions · 2021-05-13T22:07:10Z

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.

Add snapshot version support

02e65fb

facebook-github-bot added the CLA Signed ✔️ label Feb 15, 2017

thymikee commented Feb 15, 2017

View reviewed changes

Add missing snapshots

77d282e

cpojer reviewed Feb 15, 2017

View reviewed changes

thymikee added 2 commits February 15, 2017 22:36

Add link to snapshot guide, small regexp fixes

b6339c0

Update examples snapshots

1cd8bee

cpojer reviewed Feb 15, 2017

View reviewed changes

thymikee added 2 commits February 15, 2017 22:51

Add parens for readability

3c04d8b

Remove unnecessary -u flag from transform test

280d442

cpojer reviewed Feb 15, 2017

View reviewed changes

thymikee added 3 commits February 15, 2017 23:56

Update snapshot version messages to be more exhaustive

6c327a6

Fix error formatting

263fd38

Don't write to fs if not needed

165e0a6

cpojer reviewed Feb 16, 2017

View reviewed changes

cpojer merged commit 22c89b4 into jestjs:master Feb 16, 2017

thymikee deleted the snaphot-version branch February 19, 2017 16:30

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snapshot version support #2896

Add snapshot version support #2896

thymikee commented Feb 15, 2017 •

edited

Loading

thymikee Feb 15, 2017

aaronabramov Feb 15, 2017

thymikee Feb 15, 2017

thymikee Feb 15, 2017

cpojer Feb 15, 2017

cpojer Feb 15, 2017

cpojer Feb 15, 2017

vjeux Feb 15, 2017

thymikee Feb 15, 2017

cpojer Feb 15, 2017

thymikee Feb 15, 2017

thymikee Feb 15, 2017

cpojer Feb 15, 2017

cpojer Feb 15, 2017

thymikee Feb 15, 2017

cpojer Feb 15, 2017

thymikee Feb 15, 2017

cpojer Feb 15, 2017

thymikee Feb 15, 2017

cpojer Feb 15, 2017

thymikee Feb 15, 2017

cpojer commented Feb 15, 2017

cpojer Feb 16, 2017

github-actions bot commented May 13, 2021

Add snapshot version support #2896

Add snapshot version support #2896

Conversation

thymikee commented Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Feb 15, 2017

Choose a reason for hiding this comment

github-actions bot commented May 13, 2021

thymikee commented Feb 15, 2017 •

edited

Loading