[WIP] Replace jest-snapshot #1223

vadimdemedes · 2017-02-03T22:04:03Z

After investigating #1218, I realized jest-snapshot is too much for us and I believe we can bundle a simpler solution. We cut down on unnecessary dependencies and lay more groundwork for universal (React/Preact/Inferno/more) assertions by not making assumptions about the content of your snapshots.

Since our replacement is basically just born, please report any bugs or missing/unexpected behavior.

If this PR lands and if you've used t.snapshot already, make sure to update them using --update-snapshots flag.

Todo:

Snapshot feature will modify existing snapshots without passing --update-snapshots flag #1218 (comment)
Needs to handle removing stale snapshots, and do it correctly with test.only/test.skip. (As discussed on Hangouts) (Added by @sindresorhus)
Update readme regarding the changes we do and new behavior. (Added by @sindresorhus)
Change directory to __snapshots__ if inside a __tests__ directory, otherwise snapshots (added by @novemberborn)
Fix when snapshots are saved Snapshot feature will modify existing snapshots without passing --update-snapshots flag #1218 (comment) (added by @novemberborn)

lusentis · 2017-02-06T16:05:47Z

Hi,
I was testing this PR on a project, and found an issue with objects containing undefined values.
I've created a repo to reproduce this: https://github.com/lusentis/ava-test-1223

Let me know if I can further help,
thanks!

vadimdemedes · 2017-02-06T19:18:30Z

Hey @lusentis, I don't think it can be considered a bug, since d is still contained in that object, we can't skip it because it's undefined. If you try the following, it will also error:

t.deepEqual({a: 1, b: undefined}, {a: 1});

lusentis · 2017-02-06T19:27:31Z

Sure, I didn't explain it very well...
The issue is that the snapshot file does not contain the value which is undefined, so t.snapshot() fails.

This code:

        const obj = {
                a: 123,
                b: 'bar',
                c: null,
                d: undefined
        }
        t.snapshot(obj);

produces this snapshot when running ava -u:

{
  "example snapshot of an object with undefined values": {
    "a": 123,
    "b": "bar",
    "c": null
  }
}

which is missing the "d" key from the Object.
Obiously, t.snapshot() fails when run again because it complains that d: undefined is not in the snapshot.

vadimdemedes · 2017-02-06T19:47:26Z

Right, it is an unexpected behavior for sure. This is happening, because we use JSON.stringify to serialize snapshot data and it skips undefined values.

vadimdemedes · 2017-02-06T19:48:17Z

@lusentis Thanks for trying out this PR and reporting the issue!

vadimdemedes · 2017-02-06T20:04:41Z

A quick fix would be to JSON.parse(JSON.stringify(actual)), when comparing to expected value stored in the snapshot. Not sure about the drawbacks of this though. Otherwise, we'd have to manually serialize incompatible values like undefined and Function using a replacer parameter.

lusentis · 2017-02-07T10:15:04Z

Found this: https://github.com/kaelzhang/node-code-stringify it might help (I'm playing with this: https://github.com/lusentis/ava/commit/a39f1be2a48aa7c7e3b2f9a2aa6958a46b2e302b).
Thanks for your work on this! Let me know if I can help testing out.

novemberborn

I think at this stage we should be a little more opinionated (and conservative) in what snapshot testing supports. Notably, using JSON for serialization means:

undefined, Symbol() and function property values are dropped
undefined, Symbol() and function array items are replaced by null
regular expression property values and array items are replaced by {}
no support for built-ins such as Map and Set
if the input value utilizes toJSON() functions then the serialized value will definitely not match

I'm OK with that actually, though we could refine in the future.

What we should do, however, is when t.snapshot() is first called (and there is no saved snapshot), is to serialize and deserialize the actual value to make sure it behaves the same as it would on the next run (when there is a saved snapshot).

We should also document these caveats and perhaps link to that documentation when displaying the difference.

novemberborn · 2017-02-07T11:03:11Z

lib/snapshot.js

+		this.options = options || {};
+
+		if (fs.existsSync(this.filePath)) {
+			this.tests = JSON.parse(fs.readFileSync(this.filePath));


I'd just read the file and ignore any errors:

let contents try { contents = fs.readFileSync(this.filePath); } catch (err) {} this.tests = contents ? JSON.parse(contents) : {}

novemberborn · 2017-02-07T11:06:19Z

lib/snapshot.js

+
+const x = module.exports = Snapshot;
+
+Snapshot.getSnapshot = () => {


Why export the Snapshot class at all?

Fair enough. Perhaps export both Snapshot and getSnapshot separately (rather than exporting the class as the main?)

Alternatively you could use proxyquire and inject the globals dependency, though you'd have to reload for each test.

Perhaps export both Snapshot and getSnapshot separately (rather than exporting the class as the main?)

This doesn't seem to change anything. What is the benefit of this?

I think it better separates the "public" API (getSnapshot()) from the truly internal API (Snapshot constructor) which is needed for tests only.

Hm, but the thing is, both getSnapshot() and Snapshot aren't public. t.snapshot is an access point for these, users won't ever need to access the above mentioned stuff.

I know. Just voicing my preference 😄

novemberborn · 2017-02-07T11:07:25Z

lib/snapshot.js

+
+Snapshot.getSnapshot = () => {
+	if (!x.snapshot) {
+		x.snapshot = new Snapshot(globals.options.file, {update: globals.options.updateSnapshots});


Could you document that this is only loaded in a worker process, so we only need one snapshot instance for the file that worker is testing?

dinoboff · 2017-02-11T00:39:30Z

@novemberborn Assuming it serialise the new value and deserialise it before comparing it to the deserialised old value, it can support toJSON, no?

dinoboff · 2017-02-11T00:46:54Z

ps: when storing a new value, snapshot could warn about any value lost in the process: it could show a diff of the value with the deserialised stored value.

capaj · 2017-02-11T11:44:22Z

@novemberborn For me as a user of ava, I'd expect the same level of robustness as I get from jest snapshots.
I think work done on https://www.npmjs.com/package/pretty-format is quite awesome and I don't think ava comunity needs to reinvent this piece of snapshoting feature. Maybe you could used that instead of doing JSON.parse(JSON.stringify(actual)) ? I know it's not taking ava too far from the current state, but do we really need to? Jest snapshots are easily the most awesome feature of jest.

Also when you've listed the features not supported by raw json snapshots, you failed to mention Type support: https://www.npmjs.com/package/pretty-format#type-support

novemberborn · 2017-02-12T16:20:20Z

Maybe you could used that instead of doing JSON.parse(JSON.stringify(actual)) ?

@capaj yea that's a good idea.

@vadimdemedes if we go this route we'd have to do string comparisons on the formatted values. Can we do string diffs in case the new value doesn't match the saved snapshot?

novemberborn · 2017-02-12T16:22:14Z

lib/snapshot.js

+			throw new TypeError('Test file path is required');
+		}
+
+		this.dirPath = path.join(path.dirname(testPath), '__snapshots__');


Jest convention aside, is there a reason we should stick to this format? Our naming scheme is helpers and fixtures, not __fixtures__.

I agree, I'd prefer snapshots too.

Should this be configurable though? Folks who follow a __tests__ scheme will want to use __snapshots__.

I would really prefer not introducing more options. Can we make it __snapshots__ if inside a __tests__ directory, otherwise snapshots?

I would really prefer not introducing more options. Can we make it snapshots if inside a tests directory, otherwise snapshots?

Yea that sounds great to me.

vadimdemedes · 2017-02-12T16:45:33Z

We could use pretty-format to serialize values into snapshots, but then we wouldn't get a highlighted diff (when t.snapshot fails). I think it'd look weird, if only t.snapshot didn't support diff highlighting, while other assertions did.

sindresorhus · 2017-02-14T09:57:40Z

lib/snapshot.js

+			throw new TypeError('Test file path is required');
+		}
+
+		this.dirPath = path.join(path.dirname(testPath), '__snapshots__');


sindresorhus · 2017-02-14T09:58:17Z

lib/snapshot.js

+		this.options = options || {};
+
+		if (fs.existsSync(this.filePath)) {
+			this.tests = JSON.parse(fs.readFileSync(this.filePath));


sindresorhus · 2017-02-14T09:59:02Z

lib/snapshot.js

+
+	save() {
+		mkdirp.sync(this.dirPath);
+		fs.writeFileSync(this.filePath, JSON.stringify(this.tests, null, '  '));


' ' => '\t'

sindresorhus · 2017-02-14T10:11:26Z

I added a couple of todo items to the PR description.

sindresorhus · 2017-02-14T13:26:11Z

I just realized the benefit of doing like Jest is with a JS snapshot file is that they can have template literals with multiple line, so they can have good git diffing when updating snapshots.

novemberborn · 2017-02-17T15:13:06Z

Needs to handle removing stale snapshots, and do it correctly with test.only/test.skip. (As discussed on Hangouts) (Added by @sindresorhus)

@sindresorhus what are you intending here? I'm under the impression that if all tests are run, the snapshots will be updated correctly. If you use t.only then at least you'll see an overly large snapshot diff. Is the only issue one where a test file is removed but we don't clean up the snapshot file? And does Jest handle this?

novemberborn · 2017-02-17T15:57:54Z

I've also added a few TODO items. Let's decide our direction first though: #1275.

sindresorhus · 2017-02-20T07:45:24Z

Is the only issue one where a test file is removed but we don't clean up the snapshot file?

Not just removing a test file. Imagine the user removes a single test. There will now be a stale entry in the snapshot file. I would expect AVA to remove the stale test entry in the snapshot when I run --update-snapshots.

This is made harder by t.skip()/.only() though. Here's my discussion with Vadim about it. I'm too lazy to summarize.

And does Jest handle this?

Yes

novemberborn · 2017-02-20T10:17:17Z

Not just removing a test file. Imagine the user removes a single test. There will now be a stale entry in the snapshot file. I would expect AVA to remove the stale test entry in the snapshot when I run --update-snapshots.

Yes, I'd imagine it would do that. That's easy to do though. Removing a snapshot file after a test file has been removed / renamed isn't too hard either since we glob all test files and can compare that to the existing snapshot files.

This is made harder by t.skip()/.only() though

I see. As you said, we can simply not clean up snapshots for skipped tests.

TzviPM · 2017-02-21T21:02:32Z

One more item for the TODO:

Allow multiple snapshots per test

Currently, the snapshots are indexed by test name, it seems; however, when I tested in my project, I received false positives when there were multiple snapshots in a test. The current implementation in master (using jest-snapshot) appends a number after each one.

For example, if my test is called "foo is consistent", and it has 2 snapshots, We could do something like: "foo is consistent 1" and "foo is consistent 2".

What I'm currently doing is a bit nicer to read (diff-wise), which is the snapshot a hash with the different snapshots. That way they each have a name associated with them.

TzviPM · 2017-02-21T21:17:16Z

lib/snapshot.js

+	}
+
+	save() {
+		mkdirp.sync(this.dirPath);


We should add something like:

let hasTests = false; for(var key in obj) { if(obj.hasOwnProperty(key)) hasTests = true; break; } } if (!hasTests) return;

at the beginning of this function. That way we don't end up making a bunch of empty files/divs.

We also need to ask ourselves what we want to do if someone deletes a test. Should we delete the snapshots too? Or leave them there?

FYI, I forked off and tried this out and it works quite well: TzviPM@c95962a

TzviPM · 2017-02-21T21:31:15Z

On my project, the diffs seem to be backwards. For example, if my test does: t.snapshot({foo: 'foo'}), and then I change the test to t.snapshot({bar: 'bar'}), I think the diff should be:

Object {
- foo: foo,
+ bar: bar,
}

but I see the opposite:

Object {
- bar: bar,
+ foo: foo,
}

TzviPM · 2017-02-21T21:45:12Z

I tried playing around with non-native objects and it became quite messy quite fast. Perhaps the MVP doesn't need to support anything that can't be serialized / stringify-ed.

novemberborn · 2017-04-13T16:57:35Z

Humbly, I think this is superseded by #1341 (comment) 😄

novemberborn · 2017-06-25T15:52:30Z

Superseded by #1341.

replace jest-snapshot with a simpler solution

34c3817

vadimdemedes mentioned this pull request Feb 3, 2017

Snapshot feature will modify existing snapshots without passing --update-snapshots flag #1218

Closed

vadimdemedes changed the title ~~Replace jest-snapshot~~ [WIP] Replace jest-snapshot Feb 3, 2017

novemberborn requested changes Feb 7, 2017

View reviewed changes

novemberborn mentioned this pull request Feb 10, 2017

Snapshot is doubly-escaped / hard to see diffs #1254

Closed

novemberborn reviewed Feb 12, 2017

View reviewed changes

sindresorhus reviewed Feb 14, 2017

View reviewed changes

novemberborn mentioned this pull request Feb 17, 2017

Snapshot recap #1275

Closed

TzviPM reviewed Feb 21, 2017

View reviewed changes

novemberborn mentioned this pull request Apr 13, 2017

Alternative for deepEqual, formatting, diffs and snapshots #1341

Merged

6 tasks

novemberborn closed this Jun 25, 2017

novemberborn deleted the replace-snapshot branch June 25, 2017 15:52


		const x = module.exports = Snapshot;

		Snapshot.getSnapshot = () => {

[WIP] Replace jest-snapshot #1223

[WIP] Replace jest-snapshot #1223

Conversation

vadimdemedes commented Feb 3, 2017 • edited by novemberborn Loading

lusentis commented Feb 6, 2017 • edited Loading

vadimdemedes commented Feb 6, 2017

lusentis commented Feb 6, 2017 • edited Loading

vadimdemedes commented Feb 6, 2017 • edited Loading

vadimdemedes commented Feb 6, 2017 • edited Loading

vadimdemedes commented Feb 6, 2017

lusentis commented Feb 7, 2017 • edited Loading

novemberborn left a 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

dinoboff commented Feb 11, 2017

dinoboff commented Feb 11, 2017

capaj commented Feb 11, 2017 • edited Loading

novemberborn commented Feb 12, 2017

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

vadimdemedes commented Feb 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Feb 14, 2017

sindresorhus commented Feb 14, 2017

novemberborn commented Feb 17, 2017

novemberborn commented Feb 17, 2017

sindresorhus commented Feb 20, 2017

novemberborn commented Feb 20, 2017

TzviPM commented Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TzviPM commented Feb 21, 2017

TzviPM commented Feb 21, 2017 • edited Loading

novemberborn commented Apr 13, 2017

novemberborn commented Jun 25, 2017

vadimdemedes commented Feb 3, 2017 •

edited by novemberborn

Loading

lusentis commented Feb 6, 2017 •

edited

Loading

lusentis commented Feb 6, 2017 •

edited

Loading

vadimdemedes commented Feb 6, 2017 •

edited

Loading

vadimdemedes commented Feb 6, 2017 •

edited

Loading

lusentis commented Feb 7, 2017 •

edited

Loading

capaj commented Feb 11, 2017 •

edited

Loading

TzviPM commented Feb 21, 2017 •

edited

Loading

TzviPM commented Feb 21, 2017 •

edited

Loading