Skip to content

[WIP] Replace jest-snapshot #1223

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[WIP] Replace jest-snapshot #1223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 7 additions & 21 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ const observableToPromise = require('observable-to-promise');
const indentString = require('indent-string');
const isObservable = require('is-observable');
const isPromise = require('is-promise');
const jestSnapshot = require('jest-snapshot');
const snapshotState = require('./snapshot-state');
const Snapshot = require('./snapshot');

const x = module.exports;
const noop = () => {};
Expand Down Expand Up @@ -152,38 +151,25 @@ x.ifError = (err, msg) => {
test(!err, create(err, 'Error', '!==', msg, x.ifError));
};

x._snapshot = function (tree, optionalMessage, match, snapshotStateGetter) {
// Set defaults - this allows tests to mock deps easily
const toMatchSnapshot = match || jestSnapshot.toMatchSnapshot;
const getState = snapshotStateGetter || snapshotState.get;

const state = getState();

const context = {
dontThrow() {},
currentTestName: this.title,
snapshotState: state
};
x._snapshot = function (tree, optionalMessage, customSnapshot) {
const snapshot = customSnapshot || Snapshot.getSnapshot();

// Symbols can't be serialized and saved in a snapshot, that's why tree
// is saved in the `__ava_react_jsx` prop, so that JSX can be detected later
const serializedTree = tree.$$typeof === Symbol.for('react.test.json') ? {__ava_react_jsx: tree} : tree; // eslint-disable-line camelcase
const result = toMatchSnapshot.call(context, JSON.stringify(serializedTree));
const result = snapshot.match(this.title, serializedTree);

let message = 'Please check your code or --update-snapshots';

if (optionalMessage) {
message += '\n\n' + indentString(optionalMessage, 2);
}

state.save();

let expected;

if (result.expected) {
// JSON in a snapshot is surrounded with `"`, because jest-snapshot
// serializes snapshot values too, so it ends up double JSON encoded
expected = JSON.parse(result.expected.slice(1).slice(0, -1));
if (!result.pass) {
expected = result.expected;

// Define a `$$typeof` symbol, so that pretty-format detects it as React tree
if (expected.__ava_react_jsx) { // eslint-disable-line camelcase
expected = expected.__ava_react_jsx; // eslint-disable-line camelcase
Expand Down
3 changes: 3 additions & 0 deletions lib/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const process = require('./process-adapter');
const serializeError = require('./serialize-error');
const globals = require('./globals');
const Runner = require('./runner');
const snapshot = require('./snapshot').getSnapshot();
const send = process.send;

const opts = globals.options;
Expand Down Expand Up @@ -48,6 +49,8 @@ function test(props) {
}

function exit() {
snapshot.save();

const stats = runner._buildStats();

send('results', {stats});
Expand Down
30 changes: 0 additions & 30 deletions lib/snapshot-state.js
View file
Open in desktop

This file was deleted.

58 changes: 58 additions & 0 deletions lib/snapshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
'use strict';
const path = require('path');
const fs = require('fs');
const isEqual = require('lodash.isequal');
const mkdirp = require('mkdirp');
const globals = require('./globals');

class Snapshot {
constructor(testPath, options) {
if (!testPath) {
throw new TypeError('Test file path is required');
}

this.dirPath = path.join(path.dirname(testPath), '__snapshots__');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I'd prefer snapshots too.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

this.filePath = path.join(this.dirPath, path.basename(testPath) + '.snap');
this.tests = {};
this.options = options || {};

if (fs.existsSync(this.filePath)) {
this.tests = JSON.parse(fs.readFileSync(this.filePath));
Copy link
Member

Choose a reason for hiding this comment

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

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) : {}

Copy link
Member

Choose a reason for hiding this comment

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

👍

}
}

save() {
mkdirp.sync(this.dirPath);
Copy link

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

fs.writeFileSync(this.filePath, JSON.stringify(this.tests, null, ' '));
Copy link
Member

Choose a reason for hiding this comment

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

' ' => '\t' :trollface:

}

match(testTitle, actual) {
const expected = this.tests[testTitle];
if (!expected || this.options.update) {
this.tests[testTitle] = actual;

return {pass: true};
}

const isMatch = isEqual(actual, expected);
if (isMatch) {
return {pass: true};
}

return {
pass: false,
actual,
expected
};
}
}

const x = module.exports = Snapshot;

Snapshot.getSnapshot = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why export the Snapshot class at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For tests.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I know. Just voicing my preference 😄

if (!x.snapshot) {
x.snapshot = new Snapshot(globals.options.file, {update: globals.options.updateSnapshots});
Copy link
Member

Choose a reason for hiding this comment

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

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?

}

return x.snapshot;
};
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@
"is-obj": "^1.0.0",
"is-observable": "^0.2.0",
"is-promise": "^2.1.0",
"jest-snapshot": "^18.1.0",
"last-line-stream": "^1.0.0",
"lodash.debounce": "^4.0.3",
"lodash.difference": "^4.3.0",
Expand All @@ -142,6 +141,7 @@
"max-timeout": "^1.0.0",
"md5-hex": "^2.0.0",
"meow": "^3.7.0",
"mkdirp": "^0.5.1",
"ms": "^0.7.1",
"multimatch": "^2.1.0",
"observable-to-promise": "^0.4.0",
Expand Down
71 changes: 34 additions & 37 deletions test/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
'use strict';
const path = require('path');
const uniqueTempDir = require('unique-temp-dir');
const test = require('tap').test;
const sinon = require('sinon');
const Snapshot = require('../lib/snapshot');
const assert = require('../lib/assert');

test('.pass()', t => {
Expand Down Expand Up @@ -487,43 +489,35 @@ test('.deepEqual() should not mask RangeError from underlying assert', t => {
t.end();
});

test('snapshot makes a snapshot using a library and global options', t => {
const saveSpy = sinon.spy();
const state = {save: saveSpy};
const stateGetter = sinon.stub().returns(state);
const matchStub = sinon.stub().returns({pass: true});
test('.snapshot()', t => {
t.plan(4);

assert.title = 'Test name';
const snapshot = new Snapshot(path.join(uniqueTempDir(), 'test.js'));
t.deepEqual(snapshot.tests, {});

t.plan(4);
assert.title = 'test title';

t.doesNotThrow(() => {
assert._snapshot('tree', undefined, matchStub, stateGetter);
assert._snapshot('tree', undefined, snapshot);
});

t.ok(stateGetter.called);

t.match(matchStub.firstCall.thisValue, {
currentTestName: 'Test name',
snapshotState: state
t.throws(() => {
assert._snapshot('grass', undefined, snapshot);
});

t.ok(saveSpy.calledOnce);
t.deepEqual(snapshot.tests, {'test title': 'tree'});

delete assert.title;

t.end();
});

test('snapshot handles jsx tree', t => {
const saveSpy = sinon.spy();
const state = {save: saveSpy};
const stateGetter = sinon.stub().returns(state);
const matchStub = sinon.stub().returns({pass: true});
test('.snapshot() handles JSX', t => {
t.plan(4);

assert.title = 'Test name';
const snapshot = new Snapshot(path.join(uniqueTempDir(), 'test.js'));
t.deepEqual(snapshot.tests, {});

t.plan(5);
assert.title = 'test title';

t.doesNotThrow(() => {
const tree = {
Expand All @@ -534,28 +528,31 @@ test('snapshot handles jsx tree', t => {

Object.defineProperty(tree, '$$typeof', {value: Symbol.for('react.test.json')});

assert._snapshot(tree, undefined, matchStub, stateGetter);
assert._snapshot(tree, undefined, snapshot);
});

t.ok(stateGetter.called);

const savedTree = JSON.parse(matchStub.firstCall.args[0]);
t.deepEqual(savedTree, {
__ava_react_jsx: { // eslint-disable-line camelcase
type: 'h1',
t.throws(() => {
const tree = {
type: 'div',
children: ['Hello'],
props: {}
}
});
};

Object.defineProperty(tree, '$$typeof', {value: Symbol.for('react.test.json')});

t.match(matchStub.firstCall.thisValue, {
currentTestName: 'Test name',
snapshotState: state
assert._snapshot(tree, undefined, snapshot);
});

t.ok(saveSpy.calledOnce);
t.deepEqual(snapshot.tests, {
'test title': {
__ava_react_jsx: { // eslint-disable-line camelcase
type: 'h1',
children: ['Hello'],
props: {}
}
}
});

delete assert.title;

t.end();
});
46 changes: 0 additions & 46 deletions test/snapshot-state.js
View file
Open in desktop

This file was deleted.

Loading