-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[RFC] Inline Snapshots #6380
Conversation
9c04b50
to
084b4ec
Compare
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 |
Absolutely, I was going to do that but I forgot 😄. |
This only took around four hours to implement, which is a massive testament to the approachability of the codebase, by the way! |
There was a problem hiding this 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
packages/jest-snapshot/src/State.js
Outdated
this._dirty = true; | ||
this._snapshotData[key] = receivedSerialized; | ||
if (isInline) { | ||
const stack = new Error().stack.split(/\n/); |
There was a problem hiding this comment.
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?
packages/jest-snapshot/src/index.js
Outdated
@@ -72,6 +107,7 @@ const toMatchSnapshot = function( | |||
: currentTestName || ''; | |||
|
|||
if (typeof propertyMatchers === 'object') { | |||
const propertyMatchers = propertyMatchers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
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.
packages/jest-snapshot/src/index.js
Outdated
@@ -102,7 +138,12 @@ const toMatchSnapshot = function( | |||
} | |||
} | |||
|
|||
const result = snapshotState.match(fullTestName, received); | |||
const result = snapshotState.match( |
There was a problem hiding this comment.
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?
packages/jest-snapshot/src/utils.js
Outdated
snapshotData: {[key: string]: InlineSnapshot}, | ||
sourceFilePath: Path, | ||
) => { | ||
const sourceFile = fs.readFileSync(sourceFilePath, 'utf8'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Sorry for the extra notification, but I just wanted to come by and say THIS IS AMAZING! |
packages/jest-snapshot/src/utils.js
Outdated
matcher.loc.start.column === frame.column - 1 | ||
) { | ||
if ( | ||
node.arguments[0] && |
There was a problem hiding this comment.
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>
}
`);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
Need to figure out how this will work with 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. |
packages/jest-snapshot/src/utils.js
Outdated
}; | ||
|
||
const createParser = snapshots => (text, parsers) => { | ||
const ast = parsers.babylon(text); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 19c231a.
To run with circus locally, you can do |
Super awesome, @azz :) |
@SimenB Segfault in jest-circus run looks unrelated. Runs cleanly on my machine. |
|
@aaronabramov thoughts on the circle segfault?
If sourcemaps are set up correctly, it should just work. Have you got any examples of where it doesn't work? |
I haven't tested yet. How does |
We install |
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 |
14d8025
to
a161fe5
Compare
@SimenB hm.. it's not a segfault. apparently node process ran out of memory, which means there's a memory leak somewhere :( |
yeah i could reproduce it locally with |
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:
|
Config is loaded using Prettier's API, so
It already works 😄, Prettier supports bleeding edge proposals/features from JS, flow and TS, so we should be covered.
Jest already has a dependency on
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. |
Don't forget those who don't use Prettier, but are using Jest.
Sure, but (an additional) Babel parsing + Prettier code emitting is never going to be as blazing as string manipulation.
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. |
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 |
I could change it to use the Range API, that way it would only format the |
@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. |
@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. |
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#​2330) [Compare Source](jestjs/jest@v23.2.0...v23.3.0) ##### Features - `[jest-cli]` Allow watch plugin to be configured ([#​6603](`https://github.com/facebook/jest/pull/6603`)) - `[jest-snapshot]` Introduce `toMatchInlineSnapshot` and `toThrowErrorMatchingInlineSnapshot` matchers ([#​6380](`https://github.com/facebook/jest/pull/6380`)) ##### Fixes - `[jest-regex-util]` Improve handling already escaped path separators on Windows ([#​6523](`https://github.com/facebook/jest/pull/6523`)) - `[jest-cli]` Fix `testNamePattern` value with interactive snapshots ([#​6579](`https://github.com/facebook/jest/pull/6579`)) - `[jest-cli]` Fix enter to interrupt watch mode ([#​6601](`https://github.com/facebook/jest/pull/6601`)) ##### Chore & Maintenance - `[website]` Switch domain to https://jestjs.io ([#​6549](`https://github.com/facebook/jest/pull/6549`)) - `[tests]` Improve stability of `yarn test` on Windows ([#​6534](`https://github.com/facebook/jest/pull/6534`)) - `[*]` Transpile object shorthand into Node 4 compatible syntax ([#​6582](`https://github.com/facebook/jest/pull/6582`)) - `[*]` Update all legacy links to jestjs.io ([#​6622](`https://github.com/facebook/jest/pull/6622`)) - `[docs]` Add docs for 23.1, 23.2, and 23.3 ([#​6623](`https://github.com/facebook/jest/pull/6623`)) - `[website]` Only test/deploy website if relevant files are changed ([#​6626](`https://github.com/facebook/jest/pull/6626`)) - `[docs]` Describe behavior of `resetModules` option when set to `false` ([#​6641](`https://github.com/facebook/jest/pull/6641`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
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. |
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 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:
|
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. |
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, andbabel
for transformations, meaning if your code was already formatted with Prettier, nothing will jump around the screen, and you can usejest --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()
TeststoMatchInlineSnapshot()
toThrowErrorMatchingSnapshot()
expect()
is not in the test suite file.Use Range API. (This may be difficult).resolves.toMatchInlineSnapshot
,.rejects.toThrowErrorMatchingInlineSnapshot
, etc.