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

Prevent drifting inline snapshots #8424 #8492

Merged

Conversation

petternordholm
Copy link
Contributor

Fixes #8424 by avoiding indenting inline snapshot if second line of inline snapshot already has been indented.

Summary

As described in #8424, existing and unmodified inline snapshot drifts every time inline snapshots are updated.

This pull request fixes this behavior.

Test plan

  1. Added unit test to trigger faulty behavior
  2. Fixed code and verified that the faulty behavior was fixed by re-running the unit tests.

@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!

@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!

Fixes jestjs#8424 by avoiding indenting inline snapshot if second
line of inline snapshot already has been indented.
@petternordholm petternordholm force-pushed the prevent-drifting-inline-snapshots branch from 0417ae6 to ef5a9e3 Compare May 24, 2019 08:12
@codecov-io
Copy link

codecov-io commented May 24, 2019

Codecov Report

Merging #8492 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8492      +/-   ##
==========================================
+ Coverage   62.25%   62.26%   +0.01%     
==========================================
  Files         266      266              
  Lines       10746    10749       +3     
  Branches     2625     2628       +3     
==========================================
+ Hits         6690     6693       +3     
  Misses       3471     3471              
  Partials      585      585
Impacted Files Coverage Δ
packages/jest-snapshot/src/inline_snapshots.ts 81.81% <100%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c24934c...9c92314. Read the comment docs.

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 it, thanks!

@SimenB SimenB requested a review from scotthovestadt May 24, 2019 11:16
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Awesome, this really bugged me

packages/jest-snapshot/src/inline_snapshots.ts Outdated Show resolved Hide resolved
Also added a short comment to the code describing its purpose.
@jeysal
Copy link
Contributor

jeysal commented May 27, 2019

@scotthovestadt mind taking a look since you build the indentation feature initially?

@petternordholm
Copy link
Contributor Author

@scotthovestadt any news on this??? Drifting inline snapshots are driving us nuts. We are using a lot of them in our project to verify calls to mocks and all unnecessary changes in code creates lots of disturbance when reviewing code.

So, we would really like this PR to be merged.

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

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

Thank you for tracking this down and fixing it!

@scotthovestadt scotthovestadt merged commit 9ac8ca7 into jestjs:master Jun 7, 2019
mmkal added a commit to mmkal/pgkit that referenced this pull request Jul 21, 2019
Fix has been merged but not released: jestjs/jest#8492
mmkal added a commit to mmkal/pgkit that referenced this pull request Jul 21, 2019
* chore: patch jest snapshot bug

Fix has been merged but not released: jestjs/jest#8492

* chore: bump package-locks
@jedrichards
Copy link

Has this been released? I'm still seeing inline snapshot drifting on 24.9.0 ...

@SimenB
Copy link
Member

SimenB commented Sep 24, 2019

it's in 24.9 yeah (just check the changelog). if you're still seeing drifting snapshots, please open up a new issue with reproduction steps

@petternordholm
Copy link
Contributor Author

We are using jest 24.9.0 we don't have any problems with drifting inline-snapshots.

Timer added a commit to Timer/next.js that referenced this pull request Dec 10, 2019
timneutkens pushed a commit to vercel/next.js that referenced this pull request Dec 11, 2019
* CSS Module Support

* Fix Server-Side Render of CSS Modules

* Fix Jest Snapshots
jestjs/jest#8492

* Fix snapshots

* Add test for CSS module edit without remounting

* Add tests for dev and production style being applied

* Add missing TODOs

* Include/exclude should only be applied to issuer, not the CSS file itself

* Add CSS modules + node_modules tests

* Test that content is correct

* Create Multi Module Suite

* Add client-side navigation support for CSS

* Add tests for client-side nav

* Add some delays

* Try another fix

* Increase timeout to 3 minutes

* Fix test

* Give all unique directories
@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
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.

Inline snapshots drift to the right
8 participants