WIP: Implementing snapshot editor support #2629

kentaromiura · 2017-01-18T18:17:53Z

Summary
Cross editor support for getting snapshot metadata from a test file

This will enable extending various editor to show something similar to this

Test plan

I've add a couple of tests, I might have missed something (that's why I've add WIP on the title) but seems pretty solid.

how to use it
look at SnapshotSupport-test.js
I've add a small configuration that should enable to change the parser if in the future we or someone else needs it, as well as the plugin array

cpojer · 2017-01-18T18:25:19Z

packages/jest-editor-support/package.json

@@ -11,7 +11,8 @@
    "typescript": "^2.0.10"
  },
  "dependencies": {
-    "babylon": "^6.13.0"
+    "babylon": "^6.13.0",
+    "astroll": "^1.0.0"


What's this? I can't even find this anywhere.

a 10 line ast walker that keeps track of the parents:
https://github.com/kentaromiura/astroll/blob/master/index.js#L15-L32

Can we just put this into jest-editor-support for now until it grows too big?

kentaromiura · 2017-01-18T18:52:05Z

packages/jest-editor-support/src/SnapshotSupport.js

+    if (!SnapshotSupport.parser) {
+      throw new Error('SnapshotSupport.parser has to exists');
+    }
+    const fileNode = SnapshotSupport.parser(fileContent, {


There is no try catch here as the it's better to move the responsibility to the caller, any throw from here means that the source is broken.

cpojer · 2017-01-18T21:54:52Z

@orta we are still getting the missing @flow annotations on test files :(

orta · 2017-01-18T22:16:00Z

That sucks, time to take a look

kentaromiura · 2017-01-19T11:13:29Z

Once danger support is solid it would be nice to add it to the lint script in the package.json.

codecov-io · 2017-01-23T14:37:35Z

Current coverage is 68.75% (diff: 95.65%)

Merging #2629 into master will increase coverage by 0.39%

@@             master      #2629   diff @@
==========================================
  Files           140        142     +2   
  Lines          5060       5127    +67   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3459       3525    +66   
- Misses         1601       1602     +1   
  Partials          0          0

Powered by Codecov. Last update 88d0503...2a324d1

cpojer · 2017-01-24T14:16:57Z

packages/jest-editor-support/src/__tests__/SnapshotSupport-test.js

+
+test('snapshot-test.example', () => {
+  const filePath = path.join(snapshotFixturePath, 'snapshot-test.example');
+  const required = require(


"required" seems a bit odd. Maybe "expected" (this is what it calls when the assertion fails) or "snapshotData"/"snapshotFile"/"snapshots"?

cpojer · 2017-01-24T14:17:50Z

packages/jest-editor-support/src/__tests__/SnapshotSupport-test.js

+  expect(results[0].node.loc.end.column).toBe(56);
+});
+
+test('react-native.example', () => {


How is this test different from the one above? It seems like there are two tests here that test the same thing.

In one case is describeless, other than that is the same (apart the different AST)

cpojer · 2017-01-24T14:18:11Z

packages/jest-editor-support/src/__tests__/SnapshotSupport-test.js

+
+  // -> expect(extractSummary(info).summary).toMatchSnapshot();
+  expect(results[0].node.loc.end.line).toBe(111);
+  expect(results[0].node.loc.end.column).toBe(56);


Can we add an object on top of the test with metadata for all of the snapshots in the file and then test for all of them rather then just asserting that the first one is correct?

We can but for .node it's dependant of the format returned from the parser, which might be a good thing to test against as it will break in case the parser breaks back-compat

cpojer · 2017-01-24T14:18:58Z

...jest-editor-support/src/__tests__/fixtures/snapshots/__snapshots__/react-native.example.snap

@@ -0,0 +1,105 @@
+exports[`test renders correctly 1`] = `


Both the two example tests and two example snapshots are rather long. Can we condense them a little bit to be smaller in size? That'll make it easier to maintain them and it gets rid of all the things in the code that we don't need.

I took two already existing tests and related snapshots from other tests in Jest, yes we can make them smaller, but I rather use a real example

cpojer · 2017-01-24T14:20:17Z

packages/jest-editor-support/src/SnapshotSupport.js

+SnapshotSupport.plugins = [
+  'jsx', 'flow', 'objectRestSpread', 'classProperties',
+];
+SnapshotSupport.parser = babylon.parse.bind(babylon);


Can we re-use code from here: https://github.com/cpojer/jest/blob/master/packages/jest-editor-support/src/parsers/BabylonParser.js#L46 ? We shouldn't make assumptions about the syntax that the user is using.

cc @orta

I don't think this should be exported, especially since the only other user is within this file. I'd recommend to either remove this export or to split it up into two modules (like proposed in my previous comment) so that this is actually modular, like:

getMetadata(parse(file))

The idea is that you can require this and override the default parser with a different one (TypeScript maybe?).
Ideally I should make the two Visitors I'm using configurable as well, despite the Visitiors I've used are pretty standard across different ASTs so it should work as it is.

I'd like suggestion for a better API, I though this was a good compromise to avoid having to pass all the configurations for the default case and still be a bit extensible.

Yeah, ideally the babel lookup should work here 👍

cpojer · 2017-01-24T14:24:54Z

packages/jest-editor-support/src/SnapshotSupport.js

+  },
+};
+
+SnapshotSupport.plugins = [


Should also be possible to get rid of this when we extract the parsing part in this package.

cpojer · 2017-01-24T14:25:51Z

packages/jest-editor-support/src/SnapshotSupport.js

+  exportName: string,
+  node: Node,
+  content?: string,
+};


Why would exists be false? Can we remove it?

I recommend using name instead of exportName. export doesn't add much value here.

I didn't want it to be confused with the filename, I guess if we need that I can just call it fileName.

Oh and yes exists can be false the first time you're writing the test as there's no snapshot yet.

Oh yeah, for exists, that makes sense.

cpojer · 2017-01-24T14:27:11Z

packages/jest-editor-support/src/SnapshotSupport.js

+);
+
+const SnapshotSupport: SnapshotSupportType = {
+  getMetadataFromTestFile(filePath: string): Array<SnapshotMetadata> {


I'd recommend calling this something like:

Snapshot.getMetadata(…).

I don't think either the "fromTestFile" or "Support" need to be there. The editor support package already signals it is a "support package" and we only have one getter for metadata. If we ever add different files that may have snapshots (like regular files), then it makes sense to split it up into getMetadataFromTestFile and getMetadataFromMockFile for example.

cpojer · 2017-01-24T14:27:32Z

packages/jest-editor-support/src/SnapshotSupport.js

+
+const SnapshotSupport: SnapshotSupportType = {
+  getMetadataFromTestFile(filePath: string): Array<SnapshotMetadata> {
+    const fileContent = String(fs.readFileSync(filePath));


This should already be a string, please pass utf8 as second arg instead of converting the buffer to a string.

cpojer · 2017-01-24T14:29:17Z

packages/jest-editor-support/src/SnapshotSupport.js

+      },
+    },
+    state
+  );


Why do you need to look for toMatchSnapshot here? When you are calling this function below, shouldn't you have the node and be able to pass it in here?

This is for counting the number of toMatchSnapshots inside the parent so I can append the position
${describeLess}${fullName} ${position}

Oh, I see now. You are using the walker to descend into toMatchSnapshot first and then you are going back up to find the name, right? Is there a way we could track the parent nodes when we descend into the AST so that we don't have to go down and then back up the AST (for perf reasons)?

The walker already does this, parents is an array of the parents, and I use that to select the nearest it or test parent.
Here though we would need to check for all toMatchSnapshots if they have a common ancestor before starting this, and group by it.
Since I already filter the parents, it should be just a matter of moving the parents.filter up inside the map and then comparing just the last parent (and I can even optimize this last bit by keeping a reference to the last parent and reset the index when it change or increment it)

Makes sense.

cpojer · 2017-01-24T14:29:50Z

packages/jest-editor-support/src/SnapshotSupport.js

+    // if `it` or `test` exists without a surrounding `describe`
+    // then `test ` is prepended to the snapshot fullName
+    describeLess = 'test ';
+  }


I was wondering if we could get rid of this in Jest but it would wipe away every single existing snapshot… :(

cpojer · 2017-01-24T14:30:40Z

packages/jest-editor-support/src/SnapshotSupport.js

+      const parents = toMatchSnapshot.parents.filter(
+        parent => (
+          parent.callee &&
+          ['test', 'it', 'describe'].indexOf(parent.callee.name) !== -1


Instead of doing an indexOf lookup here, could we use Object.create(null) here so that lookups are O(1)? :)

Oh, also, this should work for all of these here: https://github.com/cpojer/jest/blob/master/packages/eslint-plugin-jest/src/rules/no-identical-title.js#L15

Again, please use a map for these and hoist them to the top of the file :)

cpojer · 2017-01-24T15:40:01Z

packages/jest-editor-support/src/SnapshotSupport.js

+      const innerAssertion = parents[parents.length - 1];
+
+      if (!innerAssertion || innerAssertion.callee.name === 'describe') {
+        // dead code.


what do you mean by this comment?

it means that there's a toMatchSnapshot inside a describe and that will never get executed.

Instead of "dead code" can you elaborate with the explanation from your comment?

cpojer · 2017-01-24T15:50:12Z

packages/jest-editor-support/src/SnapshotSupport.js

+      const result = {
+        content: undefined,
+        exists: false,
+        exportName: 'N/A',


can we make this an empty string? I don't like "N/A" as a default.

cpojer · 2017-01-24T15:50:57Z

packages/jest-editor-support/src/SnapshotSupport.js

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


use 'utf8' instead of casting to string.

cpojer · 2017-01-24T15:52:46Z

packages/jest-editor-support/src/SnapshotSupport.js

+        // reading and evaluating the snapshot
+        // instead of just requiring to avoid filling node cache
+        // eslint-disable-next-line no-new-func
+        const populate = new Function(


This is interesting, I'm wondering if we should just use this for snapshots as well instead of require..?

cpojer · 2017-01-24T15:54:23Z

packages/jest-editor-support/src/SnapshotSupport.js

+        );
+
+        const snapshots = {};
+        populate(snapshots);


Doesn't this run the function once for each snapshot in the test? I feel like we should only do this once per file.

cpojer · 2017-01-24T15:54:51Z

packages/jest-editor-support/src/walker.js

+'use strict';
+
+
+const excludes = excludeList => key => excludeList.indexOf(key) === -1;


Use a prototype-less object please! :)

bookman25 · 2017-01-26T01:27:50Z

Should Snapshot be added to index.js exports?

cpojer · 2017-01-26T13:51:03Z

packages/jest-editor-support/src/Snapshot.js

+
+const fs = require('fs');
+const walker = require('./walker');
+const utils = require('jest-snapshot/build/utils');


can you make it so jest-snapshot exports the utils in index.js? Packages in Jest aren't allowed to be accessed through /build/ etc.

cpojer · 2017-01-26T13:52:17Z

packages/jest-editor-support/src/Snapshot.js

+};
+
+// create a lookup table from an array.
+const lookupFromArray = array => array.reduce(


Is this really necessary?

const describeVariants = Object.assign(Object.create(null), { describe: true, fdescribe: true, xdescribe: true, });

seems fine to me, without additional runtime overhead of looping over an array and modifying an object.

cpojer · 2017-01-26T13:56:41Z

packages/jest-editor-support/src/Snapshot.js

+
+  getMetadata(filePath: string): Array<SnapshotMetadata> {
+    const fileContent = fs.readFileSync(filePath, 'utf8');
+


rm, go easy on the newlines, man!

cpojer · 2017-01-26T13:56:53Z

packages/jest-editor-support/src/Snapshot.js

+    const fileContent = fs.readFileSync(filePath, 'utf8');
+
+    const fileNode = this._parser(fileContent, this._parserOptions);
+


this one too. You can group all the constants together.

cpojer · 2017-01-26T13:57:07Z

packages/jest-editor-support/src/Snapshot.js

+    }, state);
+
+    let lastParent = null;
+    let count = 1;


put let assignments after const, if possible.

cpojer · 2017-01-26T13:57:35Z

packages/jest-editor-support/src/Snapshot.js

+      };
+
+      if (!innerAssertion || isDescribe(innerAssertion.callee)) {
+        // an expectation inside a describe never get executed.


An expectation inside describe never gets executed.

cpojer · 2017-01-26T13:58:34Z

Nice! I think we are getting closer to something we can ship! :) @bookman25 is right, Snapshot should be exported in index.js.

cpojer

see comments

thymikee · 2017-04-11T08:09:23Z

:(

gcazaciuc · 2017-04-28T20:19:45Z

Not sure why this PR was closed. The functionality seems super helpful. Is there something we can give a hand on or is this functionality not desirable to be included in jest editor support ?

cpojer · 2017-04-28T20:37:04Z

Feel free to bring back this pull request and address all the outstanding comments, we are more than happy to merge it! :)

vvo · 2017-09-14T13:24:18Z

@cpojer The GitHub interface only shows outdated comments, I cannot understand which parts should be changed for this to happen. This would bring very good dev experience to editors. For example showing only the diff inline the editor and a button to accept the change.

cpojer · 2017-09-15T01:13:16Z

@vvo I'm not sure either, could you bring this PR back to life and make it work with the current codebase and we'll take it from there? :)

Following jestjs#2629, I rebased the code on current master. I still have to actually test the feature with jest-community/vscode-jest#73. What i would like to add ultimately is the ability to: - preview the snapshot - have the snapshot diff - have a button to accept diff per snapshot (would update it) WDYT?

* first iteration * Use babel-traverse * comments on the pr * feat(jest-editor-support): Add Snapshot metadata Following #2629, I rebased the code on current master. I still have to actually test the feature with jest-community/vscode-jest#73. What i would like to add ultimately is the ability to: - preview the snapshot - have the snapshot diff - have a button to accept diff per snapshot (would update it) WDYT?

vvo · 2017-10-01T13:10:38Z

(This PR was followed and merged in #4570)

* first iteration * Use babel-traverse * comments on the pr * feat(jest-editor-support): Add Snapshot metadata Following jestjs#2629, I rebased the code on current master. I still have to actually test the feature with jest-community/vscode-jest#73. What i would like to add ultimately is the ability to: - preview the snapshot - have the snapshot diff - have a button to accept diff per snapshot (would update it) WDYT?

github-actions · 2021-05-13T05:11:47Z

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.

facebook-github-bot added the CLA Signed ✔️ label Jan 18, 2017

cpojer reviewed Jan 18, 2017

View reviewed changes

kentaromiura commented Jan 18, 2017

View reviewed changes

kentaromiura mentioned this pull request Jan 19, 2017

Custom peek widgets microsoft/vscode#3220

Open

cpojer reviewed Jan 24, 2017

View reviewed changes

kentaromiura force-pushed the snapshot-editor-support branch from d33cdee to 065b59f Compare January 25, 2017 19:54

cpojer reviewed Jan 26, 2017

View reviewed changes

cpojer requested changes Jan 26, 2017

View reviewed changes

thymikee mentioned this pull request Feb 15, 2017

Add snapshot version support #2896

Merged

orta mentioned this pull request Feb 16, 2017

snapshot previews jest-community/vscode-jest#73

Merged

Cristian Carlesso added 2 commits March 20, 2017 16:14

first iteration

7d5969e

Use babel-traverse

7972ae6

kentaromiura force-pushed the snapshot-editor-support branch 2 times, most recently from 421adaa to b007791 Compare March 21, 2017 18:00

comments on the pr

7caa8c0

kentaromiura force-pushed the snapshot-editor-support branch from b007791 to 7caa8c0 Compare March 21, 2017 18:03

cpojer closed this Apr 11, 2017

vvo mentioned this pull request Sep 30, 2017

feat(jest-editor-snapshot): Add Snapshots metadata #4570

Merged

renovate bot mentioned this pull request Apr 25, 2018

Update dependency danger to v3.6.0 danreeves/react-tether#101

Merged

renovate bot mentioned this pull request May 8, 2018

chore(deps): update dependency danger to v3.6.5 MCS-Lite/mcs-lite#615

Merged

renovate bot mentioned this pull request May 25, 2018

chore(deps): update dependency danger to v3.7.14 apollographql/react-apollo#2016

Merged

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

		'use strict';


		const excludes = excludeList => key => excludeList.indexOf(key) === -1;


		getMetadata(filePath: string): Array<SnapshotMetadata> {
		const fileContent = fs.readFileSync(filePath, 'utf8');

		const fileContent = fs.readFileSync(filePath, 'utf8');

		const fileNode = this._parser(fileContent, this._parserOptions);

WIP: Implementing snapshot editor support #2629

WIP: Implementing snapshot editor support #2629

Conversation

kentaromiura commented Jan 18, 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

cpojer commented Jan 18, 2017

orta commented Jan 18, 2017

kentaromiura commented Jan 19, 2017

codecov-io commented Jan 23, 2017 • edited Loading

Current coverage is 68.75% (diff: 95.65%)

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

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

bookman25 commented Jan 26, 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

cpojer commented Jan 26, 2017

cpojer left a comment

Choose a reason for hiding this comment

thymikee commented Apr 11, 2017

gcazaciuc commented Apr 28, 2017

cpojer commented Apr 28, 2017

vvo commented Sep 14, 2017

cpojer commented Sep 15, 2017

vvo commented Oct 1, 2017

github-actions bot commented May 13, 2021

codecov-io commented Jan 23, 2017 •

edited

Loading