Skip to content

[jest-emotion] snapshots show every style changed when any html attribute or style changes #1847

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

Closed
Jimmydalecleveland opened this issue Apr 19, 2020 · 8 comments
Labels

Comments

@Jimmydalecleveland
Copy link
Contributor

Current behavior:

Snapshot errors do not show only the changed styles or attributes of a styled component when using the snapshotSerializer.

To reproduce:
Here is a minimal repo I created with the snapshot failure committed. Simply install (I'm using yarn) and run yarn test to see it.
emotion-snapshot-issue-reproduction

  1. Update snapshots
  2. Change any style or html attribute (such as the id value)
  3. Snapshot will fail as expected but show that every style in the component has changed

Expected behavior:

Snapshots should only show diff lines for the things that changed in a component. This used to be the behavior for all our tests but at some point it changed. I'm sorry I can't figure out when, despite trying.

Environment information:

  • react version: ^16.12.0
  • @emotion/core version: ^10.0.27
  • @emotion/styled version: ^10.0.2
  • jest-emotion version: ^10.0.27
@Andarist
Copy link
Member

This is actually a bug in jest-snapshot package. I've located the problem and gonna try to prepare a fix.

@Jimmydalecleveland
Copy link
Contributor Author

Oh really? Well good luck, I am shivering with anticipation.

@Jimmydalecleveland
Copy link
Contributor Author

I've confirmed locally that [email protected] introduced the bug when they changed to the new diff highlighting, so I'm going to close this issue. Thanks for narrowing it down to that @Andarist .

@Andarist
Copy link
Member

Ok, so I actually was mistaken - although how things are handled in jest around this are somewhat confusing. The actual snapshot is being dedented (the indentation is completely removed) but the received snapshot is not - it relies on the printer indenting it correctly based on the received arguments.

And that's why I have thought it's a bug in jest-snapshot because I would expect it to both actual and expected snapshots being handled the same way. It's still might be a bug there - depends on how one looks at it. The printed leading whitespace doesn't have to be strictly related to classic code-indentation purposes which most commonly only improves the readability of nested logic and structures, whereas indentation can be used for other reasons. I think I'm going to propose a fix for this to the jest team regardless of handling this issue here.

To get back on track - the issue is here, in emotion. We don't use the received indenter and quite frankly, given the fact how we are printing styles (using css module from npm), it is not possible to use it right now as its a function and css.stringify expects indent option to be a string.

It turns out though that we are using the old serializer plugin APIs and it would be easier to fix this when using the new API. Even though it has been introduced in Jest 21 and most likely jest-emotion doesn't work with older jest anyway, but switching to the new API seems like a potential breaking change. We are closer and closer to releasing v11 and I believe it would be best to just switch to the new API, and thus fix this, in that version. I'm going to take care of this soon.

@Jimmydalecleveland
Copy link
Contributor Author

You probably already know, but inline snapshots require prettier to be installed for formatting which might have influenced why standard snapshots have that indenting. Just a hunch, I haven't looked into it as much as you.

It sounds like you'd like me to leave this closed since you are planning on fixing it through an indirect solution (switching to the new API) in v11. Thank you for the update.

@Andarist
Copy link
Member

While I'm going to address this soon on the next branch - I've started the discussion around this issue in Jest here: jestjs/jest#9863

@Andarist
Copy link
Member

And... you might also be interested that I've implemented a "fix" on our side here: #1850

@Jimmydalecleveland
Copy link
Contributor Author

You're damn right I'm interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants