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

feat: make prettier optional for inline snapshots #7792

Merged
merged 110 commits into from
Nov 15, 2020

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Feb 3, 2019

foxsportsnl-WQeuUqJkRAh151wLqk

Summary

Inline snapshots are awesome, but they use prettier to format the test file whenever they're added or updated. This is bad for two reasons:

  1. It affects code outside of the snapshots.
  2. It creates a dependency on prettier.

In projects that use prettier, both of these are fine. But in projects that use other style tools, this can mean that prettier will fight with those other tools, and their configurations need to be kept in sync.

This PR addresses (1), but not (2). I would expect it could address (2) fairly easily too - prettier is now effectively only being used as a parser-finder, which provides a consistent AST to be walked. It's very possible there are more direct ways to do that, but I'm not familiar with prettier or babel, so I've left that for now. Any tips welcome - otherwise maybe it could be done as a follow up.

This PR uses prettier when an up-to-date version is found; otherwise falls back to using Babel 7 to walk the AST and locate the snapshots.

I opened an issue in prettier (prettier/prettier#5807) but the suggestion was to fix here instead. I tried using the the range API (as I think did @azz when he first implemented inline snapshots in #6380), but there were a lot of gotchas and this approach is closer to getting rid of the prettier dependency entirely.

The basic implementation is - walk the AST in the same way as before, but instead of modifying at and using prettier to output a 'fixed' version, mark each snapshot with the character offsets of where they should be inserted. Then, walk backwards through each snapshot, and slice up the source file, inserting the snapshot text. Walking backwards means the offsets aren't messed up by the insertions.

Tagging a few people who took part in #6380:
@azz @SimenB @cpojer

Closes #7744

Test plan

Added some tests to inline_snapshots.test.js:

  • one features hideous formatting outside of a snapshot assertion, and makes sure that the hideous formatting is preserved.
  • another adds a test case for creating a new snapshot when a property matcher exists. This was just a missing test case and I wanted to make sure I didn't break this behaviour.

Command run:
npm run jest packages/jest-snapshot/src/__tests__/inline_snapshots.test.js
Output:

 PASS  packages/jest-snapshot/src/__tests__/inline_snapshots.test.js
  √ saveInlineSnapshots() replaces empty function call with a template literal (171ms)
  √ saveInlineSnapshots() leaves formatting outside of snapshots alone (9ms)
  √ saveInlineSnapshots() replaces existing template literal - babylon parser (3ms)
  √ saveInlineSnapshots() replaces existing template literal - flow parser (2ms)
  √ saveInlineSnapshots() replaces existing template literal - typescript parser (2ms)
  √ saveInlineSnapshots() replaces existing template literal with property matchers (3ms)
  √ saveInlineSnapshots() creates template literal with property matchers (3ms)
  √ saveInlineSnapshots() throws if frame does not match (28ms)
  √ saveInlineSnapshots() throws if multiple calls to to the same location (6ms)
  √ saveInlineSnapshots() uses escaped backticks (3ms)
  √ saveInlineSnapshots() works with non-literals in expect call (5ms)

Test Suites: 1 passed, 1 total
Tests:       11 passed, 11 total
Snapshots:   0 total
Time:        2.374s, estimated 5s

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

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.

I like this approach! However, I think for this to be tenable we need to use something that's not prettier to walk the AST (I realize you talked about this in the OP). The approach of just walking backwards and modifying makes sense to me.

You have POC in the title - what do you think is missing? The only thing I can think of is to be able to still use prettier - otherwise I'll probably have to format my file after Jest has done it's thing. I really like that it's properly formatted automatically.

(also, please update the changelog (and docs))

@jeysal
Copy link
Contributor

jeysal commented Feb 3, 2019

Regarding the optional usage of Prettier:
Maybe we can make it work both for Prettier users and non-users without requiring config by running Prettier on the old test file (without writing) to check if it changes anything. If it doesn't, we run Prettier after inserting the snapshot.
It's a bit expensive for Prettier users, but it would happen only on snapshot update, so it should be fine.

@mmkal
Copy link
Contributor Author

mmkal commented Feb 3, 2019

@SimenB the main reason for POC is I wanted to see what the feedback was on using prettier to walk the AST without using it for formatting. Given there already is a dependency on prettier, my hope was that this at least solves the problem of unwanted formatting as a side-effect, so it may be good enough as a first step.

Since babel now supports typescript, maybe @babel/parser would be good enough.

@azz
Copy link
Contributor

azz commented Feb 3, 2019

This is a good optimisation of the current implementation of inline snapshots, but might be missing the broader context and reason I used Prettier in the first place.

The end-game for inline snapshots is to be able to automatically create assertions that look like regular tests. For example, saving this file:

test('inline snapshot', () => {
  const obj = {
    foo: 'bar',
    fn: () => {},
  };
  expect(obj).toMatchInlineSnapshot();
});

Would auto-generate this:

test('inline snapshot', () => {
  const obj = {
    foo: 'bar',
    fn: () => {},
  };
  expect(obj).toMatchInlineSnapshot({
    foo: 'bar',
    fn: expect.any(Function),
  });
});

Using strings for the snapshots were just a way of getting the feature of the ground (MVP if you will), and creating the framework for implementing more powerful, JavaScript/expect snapshots, which come with better diagnostics, edit-ability, static analysis, etc.

So as much as this PR improves the way the feature is currently implemented, it may be pre-maturely optimising.

@cpojer do you agree?

@mmkal
Copy link
Contributor Author

mmkal commented Feb 3, 2019

@azz that does sound good, but out of scope for this PR - at least I wouldn't be able to make that big a change.

@SimenB @jeysal in the end I went with making prettier optional. If there's no prettier, or if the version is too low, it uses @babel/parser to try to parse the AST, then string manipulation. If a new enough prettier version is found, the behaviour is the same as before.

IMHO, long-term prettier should just be dropped. But I'm biased since I don't use it, hence this PR. I guess if people care enough about formatting the file after insertion, it's worth maintaining both code paths.

@azz
Copy link
Contributor

azz commented Feb 4, 2019

My point was Prettier provides the ability to parse, manipulate and re-print the AST without doing string manipulation. This will be required when generating more complex ASTs than just a template string literal. You could do this with @babel/core, @babel/parser and @babel/generator, but generator has never really been optimized for humans.

@mmkal
Copy link
Contributor Author

mmkal commented Feb 4, 2019

@azz I see - if anything I think that's all the more reason to put this in now, before the coupling with prettier is increased. I think you should do that with @babel/generator, not prettier. That's exactly the sort of thing @babel/generator is for. So I don't really see this change as an optimisation. Side note - babel now has support for typescript, but only in version 7, which hadn't been released when inline snapshots were first added. There's no reason I'm aware of that it wouldn't be possible to do everything we need with babel now.

I got the impression from the original PR that using prettier was a stopgap to get the feature out:
#6380 (comment)
#6380 (comment)

@mmkal mmkal changed the title [POC] Uglier inline snapshots Uglier inline snapshots Feb 4, 2019
had to do this manually; node-gyp is not playing nicely with windows. if CI complains I'll try on a mac.
@jeysal jeysal requested a review from SimenB November 14, 2020 19:50
@jeysal
Copy link
Contributor

jeysal commented Nov 14, 2020

image
What a monster 😅

@jeysal
Copy link
Contributor

jeysal commented Nov 14, 2020

So after benchmarking this on 64 test files with a noop test case each, it appears to be between 1.05x and 1.1x as fast as master due to the reduced overhead of loading prettier once per worker instead of once per test file.
I've also tried loading some other things in the dependency graph of jest-circus from outside, but e.g. all of {pretty-format,jest-diff,chalk} (some of the biggest ones) only gets us another 1.05x to 1.1x on top, and I'd expect it to cause all sorts of realm-related problems so it's nowhere near a 'free' perf gain either.
But still, of course we needed this anyway because without it loading prettier so many times OOMed CI lol, plus we now have the possibility to easily load big dependencies from inside the test runner if we need more in the future. I'm also still looking into some other areas where it could be applied for perf gains, we'll see what comes out of that.

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.

I pushed one cleanup commit to reduce the diff. @jeysal PTAL and merge if you're happy with it 👍 (with a more desriptive commit message than "uglier")

@jeysal jeysal changed the title Uglier inline snapshots feat: make prettier optional for inline snapshots Nov 15, 2020
@jeysal jeysal merged commit c8418df into jestjs:master Nov 15, 2020
@jeysal
Copy link
Contributor

jeysal commented Nov 15, 2020

🎉

@SimenB
Copy link
Member

SimenB commented Nov 15, 2020

took almost 2 years 😅 Thanks @mmkal!

connorjclark added a commit to connorjclark/jest that referenced this pull request May 7, 2021
jestjs#7792 suggests that Prettier is optional, but I assumed that all I needed to do was remove `prettier` from my dependencies. That still results in Jest trying to require `prettier`, causing Jest to fail. Took awhile to learn I needed to set `prettierPath` to `null` or `''`, so I added this to the docs.
@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 11, 2021
@mmkal mmkal deleted the uglier-inline-snapshots branch August 6, 2021 15:05
@mmkal mmkal restored the uglier-inline-snapshots branch August 12, 2021 22:14
@mmkal mmkal deleted the uglier-inline-snapshots branch October 24, 2023 21:15
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.

9 participants