Skip to content
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

[RFC] Inline Snapshots #6380

Merged
merged 34 commits into from
Jun 28, 2018
Merged

[RFC] Inline Snapshots #6380

merged 34 commits into from
Jun 28, 2018

Conversation

azz
Copy link
Contributor

@azz azz commented Jun 2, 2018

In the Jest 23 event recently, @cpojer mentioned that he would like to have snapshots in the source file, removing the indirection. I thought I'd have a crack at it, and it works!

It is a very simple API:

  • expect(x).toMatchInlineSnapshot()

  • expect(fn).toThrowErrorMatchingInlineSnapshot()

Snapshot updates are done with prettier for formatting, and babel for transformations, meaning if your code was already formatted with Prettier, nothing will jump around the screen, and you can use jest --watch and watch the snapshots appear before your 👀!

The code is still rough around the edges, but I wanted to get some early feedback on whether Jest wants this kind of feature.

Todo:

  • saveInlineSnapshots() Tests
  • E2E tests: toMatchInlineSnapshot()
  • E2E tests: toThrowErrorMatchingSnapshot()
  • E2E tests: Prettier resolution/config
  • Handle case where expect() is not in the test suite file.
  • Support TypeScript.
  • Use Range API. (This may be difficult)
  • Make prettier path configurable
  • Documentation
  • Support .resolves.toMatchInlineSnapshot, .rejects.toThrowErrorMatchingInlineSnapshot, etc.

@cpojer
Copy link
Member

cpojer commented Jun 2, 2018

This is fucking outstanding. I didn't think it would happen so soon. Amazing. I will take a look at this soon, we should absolutely make this happen and ship it with Jest.

One early nit: I think it would be better to re-use the indentation level of the expect call – what do you think?

@azz
Copy link
Contributor Author

azz commented Jun 2, 2018

One early nit: I think it would be better to re-use the indentation level of the expect call – what do you think?

Absolutely, I was going to do that but I forgot 😄.

@azz
Copy link
Contributor Author

azz commented Jun 2, 2018

This only took around four hours to implement, which is a massive testament to the approachability of the codebase, by the way!

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is super awesome, thanks for working on it!

Left some inline comments, but overall this looks really good

this._dirty = true;
this._snapshotData[key] = receivedSerialized;
if (isInline) {
const stack = new Error().stack.split(/\n/);
Copy link
Member

Choose a reason for hiding this comment

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

We can use callsites here, but it's way easier with sourcemaps to use errors.

We have an error already from the matcher on expect, but we might not pass it through?

@@ -72,6 +107,7 @@ const toMatchSnapshot = function(
: currentTestName || '';

if (typeof propertyMatchers === 'object') {
const propertyMatchers = propertyMatchers;
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already removed locally, I had some argument shuffling happening earlier that I didn't completely clean up.

@@ -102,7 +138,12 @@ const toMatchSnapshot = function(
}
}

const result = snapshotState.match(fullTestName, received);
const result = snapshotState.match(
Copy link
Member

Choose a reason for hiding this comment

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

Should we start passing an object?

snapshotData: {[key: string]: InlineSnapshot},
sourceFilePath: Path,
) => {
const sourceFile = fs.readFileSync(sourceFilePath, 'utf8');
Copy link
Member

@SimenB SimenB Jun 2, 2018

Choose a reason for hiding this comment

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

Would be awesome to reuse the hastemap to save the fs operation.

Then we should be able to cache prettier config lookup as well

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'm unfamiliar with hastemap, got any pointers to how I would do that?

Copy link
Member

Choose a reason for hiding this comment

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

I'll have to take a look when I'm at a computer, but basically the context contains the full filesystem (and dependencies between files) in an abstraction called haste map (with haste fs in it). I'm not sure how to best access it (or if it's even possible) in jest-snapshot

@orta
Copy link
Member

orta commented Jun 2, 2018

Honestly, this might just be good enough for me to deprecate #5718

Great work @azz

@kentcdodds
Copy link
Contributor

Sorry for the extra notification, but I just wanted to come by and say THIS IS AMAZING!

matcher.loc.start.column === frame.column - 1
) {
if (
node.arguments[0] &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: It won't always be the first argument, it should always be the last one. Property matchers should probably go first.

expect(x).toMatchInlineSnapshot(
  { r: expect.any(Number) },
  `
  Object {
    r: Any<Number>
  }
`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

😍

@azz
Copy link
Contributor Author

azz commented Jun 2, 2018

Need to figure out how this will work with jest-each or just simple loops. Maybe switch to an array if more than one snapshot is found?

for (const i of [1, 2, 3]) {
  expect(i).toMatchInlineSnapshot([
    "1",
    "2",
    "3"
  ]);
}

Edit: Just going to fail the test for now, it's a bit fiddly to get working.

};

const createParser = snapshots => (text, parsers) => {
const ast = parsers.babylon(text);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using prettier.getFileInfo() to determine which parser to use, and throw error for parser other than babylon, flow or typescript? since we only care the astFormat to be estree here.

Copy link
Contributor Author

@azz azz Jun 2, 2018

Choose a reason for hiding this comment

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

I tried parser: 'flow' manually when I was originally writing this, and babel-traverse threw an error, something like Couldn't find Program or Function, will need to write a bit of code to patch it, I think. cc. @existentialism

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 19c231a.

@SimenB
Copy link
Member

SimenB commented Jun 2, 2018

To run with circus locally, you can do JEST_CIRCUS=1 ./jest

@suchipi
Copy link
Contributor

suchipi commented Jun 2, 2018

Super awesome, @azz :)

@azz
Copy link
Contributor Author

azz commented Jun 3, 2018

@SimenB Segfault in jest-circus run looks unrelated. Runs cleanly on my machine.

@azz
Copy link
Contributor Author

azz commented Jun 3, 2018

TypeScript support for this might not land straight away - need to:

@SimenB
Copy link
Member

SimenB commented Jun 3, 2018

@aaronabramov thoughts on the circle segfault?

Figure out how to map the stack trace back to the TS source

If sourcemaps are set up correctly, it should just work. Have you got any examples of where it doesn't work?

@azz
Copy link
Contributor Author

azz commented Jun 3, 2018

I haven't tested yet. How does new Error know about sourcemaps?

@SimenB
Copy link
Member

SimenB commented Jun 3, 2018

We install source-map-support into the environment, and tell it to use the sourcemaps returned by transformers: https://github.com/facebook/jest/blob/3a1afceb1789d1e054de947019dfc8fe433c4576/packages/jest-runner/src/run_test.js#L118-L146

@azz
Copy link
Contributor Author

azz commented Jun 3, 2018

It now works if you do this:

// my.test.js
test('...', () => {
  require('./other')();
});

// other.js
module.exports = () => {
  expect(value).toMatchInlineSnapshot();
};

But I just realized, since tests can be ran in parallel there's no way to ensure that the external toMatchInlineSnapshot is only called once. 🤔

@aaronabramov
Copy link
Contributor

@SimenB hm.. it's not a segfault. apparently node process ran out of memory, which means there's a memory leak somewhere :(

@aaronabramov
Copy link
Contributor

yeah i could reproduce it locally with JEST_CIRCUS=1 ./jest -i
is there anything we're not cleaning up? or maybe we already had a memory leak and just started attaching more stuff?

@ArtemGovorov
Copy link
Contributor

ArtemGovorov commented Jun 3, 2018

Great feature!

@azz I may be missing something, but wouldn't it be possible to save the inline snapshot without AST manipulation in this case? Given that it seems like a straightforward replacement, couldn't just string manipulation be used (without involving babel and prettier)? It would address the following concerns:

  • right now the feature is only usable for Prettier enabled code bases (with no settings overrides or all overrides in prettier config). Those who are not using Prettier, or using it from various editor extensions that support loading prettier settings from sources other than prettier config, such as .editorconfig, or editor settings, will have their code reformatted. With string manipulation, the feature will seamlessly work for any code base.
  • with string manipulation there's no need to worry about parser, so Typescript/Flow/whatever would just work;
  • using string manipulation approach eliminates dependency on Babel and Prettier for the feature;
  • string manipulation is faster, and while not much of a concern for a single/small test file change, things can add up when updating multiple inline snapshots in multiple files.

@azz
Copy link
Contributor Author

azz commented Jun 4, 2018

or using it from various editor extensions that support loading prettier settings from sources other than prettier config, such as .editorconfig

Config is loaded using Prettier's API, so package.json, .prettierrc, .editorconfig, etc will be respected. Notice in the GIFs that there is no bracket spacing. The only caveat is if people are using editor-specific overrides (like .vscode/settings.json) will not be picked up.

Typescript/Flow/whatever would just work

It already works 😄, Prettier supports bleeding edge proposals/features from JS, flow and TS, so we should be covered.

using string manipulation approach eliminates dependency on Babel and Prettier for the feature;

Jest already has a dependency on babel, so it's just the addition of Prettier.

string manipulation is faster, and while not much of a concern for a single/small test file change, things can add up when updating multiple inline snapshots in multiple files.

Prettier is Blazing 🔥

The main value prospect of using Prettier instead of string manipulation is probably demonstrated by the second GIF in the original post. When the string is short it will be inlined, when it is too long it will be on the next line, when there are property matchers (first arg object) everything will be in the right place. Doing that reliably with raw string manipulation would be complex and error-prone.

@ArtemGovorov
Copy link
Contributor

ArtemGovorov commented Jun 4, 2018

The only caveat is if people are using editor-specific overrides (like .vscode/settings.json) will not be picked up.

Don't forget those who don't use Prettier, but are using Jest.

Prettier is Blazing

Sure, but (an additional) Babel parsing + Prettier code emitting is never going to be as blazing as string manipulation.

When the string is short it will be inlined, when it is too long it will be on the next line, when there are property matchers (first arg object) everything will be in the right place. Doing that reliably with raw string manipulation would be complex and error-prone.

It is handy, but can be done outside of the testing framework feature, can't it? It can be done with Prettier (outside Jest) for those who are using Prettier, and for those using other formatting tools, those tools can do it differently (the way those tools are configured to do so), and everyone would be happy.

I have nothing against Prettier as a separate tool that does its job very well. My concern is that if a testing framework feature can avoid being a specific code formatter dependent, then it probably should be.

@azz
Copy link
Contributor Author

azz commented Jun 4, 2018

I have nothing against Prettier as a separate tool that does its job very well. My concern is that if a testing framework feature can avoid being a specific code formatter dependent, then it probably should be.

That's a fair criticism, and I'm obviously biased here. Happy to roll with what the collaborators here think the right way forward is.

@ArtemGovorov
Copy link
Contributor

That's a fair criticism, and I'm obviously biased here. Happy to roll with what the collaborators here think the right way forward is.

I love Prettier too, also I'm a big fan of AST manipulations. What bothers me is that it can make the feature less usable/attractive for those who configure Prettier differently or are not using Prettier. If only the bit of code around the toMatchInlineSnapshot(...) was formatted, it'd be ok, but changing the whole source file formatting when updating a single inline snapshot may affect the feature adoption.

@azz
Copy link
Contributor Author

azz commented Jun 4, 2018

I could change it to use the Range API, that way it would only format the expect() statement.

@ArtemGovorov
Copy link
Contributor

@azz 👍 This should address the re-formatting concern. Those who are using Prettier will have their code formatted as a standalone Prettier would do, others can reformat it on the fly with their code formatters - at least it's going to be just one place to do it.

@ArtemGovorov
Copy link
Contributor

ArtemGovorov commented Jun 4, 2018

@azz I'd still consider string manipulation though to try to not create a dependency on Prettier. I may be missing something, correct me if I'm wrong, but seems like a simple replacement.

schalkneethling referenced this pull request in mdn/interactive-examples Jul 6, 2018
This Pull Request renovates the package group "jest monorepo".


-   [jest-environment-node](https://github.com/facebook/jest) (`devDependencies`): from `23.2.0` to `23.3.0`
-   [jest](https://github.com/facebook/jest) (`devDependencies`): from `23.2.0` to `23.3.0`

# Release Notes
<details>
<summary>facebook/jest</summary>

### [`v23.3.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#&#8203;2330)
[Compare Source](jestjs/jest@v23.2.0...v23.3.0)
##### Features

- `[jest-cli]` Allow watch plugin to be configured ([#&#8203;6603](`https://github.com/facebook/jest/pull/6603`))
- `[jest-snapshot]` Introduce `toMatchInlineSnapshot` and `toThrowErrorMatchingInlineSnapshot` matchers ([#&#8203;6380](`https://github.com/facebook/jest/pull/6380`))
##### Fixes

- `[jest-regex-util]` Improve handling already escaped path separators on Windows ([#&#8203;6523](`https://github.com/facebook/jest/pull/6523`))
- `[jest-cli]` Fix `testNamePattern` value with interactive snapshots ([#&#8203;6579](`https://github.com/facebook/jest/pull/6579`))
- `[jest-cli]` Fix enter to interrupt watch mode ([#&#8203;6601](`https://github.com/facebook/jest/pull/6601`))
##### Chore & Maintenance

- `[website]` Switch domain to https://jestjs.io ([#&#8203;6549](`https://github.com/facebook/jest/pull/6549`))
- `[tests]` Improve stability of `yarn test` on Windows ([#&#8203;6534](`https://github.com/facebook/jest/pull/6534`))
- `[*]` Transpile object shorthand into Node 4 compatible syntax ([#&#8203;6582](`https://github.com/facebook/jest/pull/6582`))
- `[*]` Update all legacy links to jestjs.io ([#&#8203;6622](`https://github.com/facebook/jest/pull/6622`))
- `[docs]` Add docs for 23.1, 23.2, and 23.3 ([#&#8203;6623](`https://github.com/facebook/jest/pull/6623`))
- `[website]` Only test/deploy website if relevant files are changed ([#&#8203;6626](`https://github.com/facebook/jest/pull/6626`))
- `[docs]` Describe behavior of `resetModules` option when set to `false` ([#&#8203;6641](`https://github.com/facebook/jest/pull/6641`))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
@lgandecki
Copy link
Contributor

So amazing! I also think this: "When we render an Object, why do we not instead print the real JavaScript object right there?" would be game changing.
@cpojer if you don't have time to work on this, maybe you thought about it a bit and have some pointers how would be a good way to do it? I'd love to give it a try :)

@SimenB
Copy link
Member

SimenB commented Jul 19, 2018

That's basically #4442, IDK if @orta has any thoughts on the implementation

@orta
Copy link
Member

orta commented Jul 19, 2018

Yeah, at least me and @cpojer are on board with basically doing that @lgandecki - and I agree, doing that completely eradicates the need for #4442 IMO

The most naive implementation could be to do prettier(JSON.stringify(theObj)) to have it turn it into a real AST of the object that can be inserted.

I think what might make more sense in the long run is to have sentinels for functions which turn into expect.any`s for functions, so that they can still feel like real tests e.g:

expect(obj).toMatchSnapshot({
  myFunc: expect.any(Function),
  number: 1,
  value: {
    one: "two"
  }
})

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.