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

Diff showing on unchanged snapshot lines #355

Closed
fritz-c opened this issue Feb 24, 2021 · 3 comments · Fixed by #419
Closed

Diff showing on unchanged snapshot lines #355

fritz-c opened this issue Feb 24, 2021 · 3 comments · Fixed by #419

Comments

@fritz-c
Copy link
Contributor

fritz-c commented Feb 24, 2021

There is an issue with diff output from jest when changes are found in snapshots. Specifically, it will show extraneous diffs in lines that have no changes:

  .c0 > span {
-   margin-left: 0;
-   margin-right: 0;
+   margin-left: 0;
+   margin-right: 0;
  }

The detection of changes itself is not flawed. This diff issue only happens when legitimate changes are detected elsewhere in the snapshot. The issue resides in the logic printing the changes.

Another CSS-in-JS library, emotion, and its jest library, @emotion/jest, had a very similar issue with diff output. It was identified in emotion-js/emotion#1847 and resolved with a serializer overhaul in emotion-js/emotion#1850 .

The root cause of the issue starts with some behavior introduced with Jest v25. Prior to Jest v25, snapshot diffs printed to stdout were sensitive to indentation, but a pull request made it insensitive to indentation like so: indent diff before-after

However, this new diff logic requires that snapshot serializers respect the indentation config in order for it to work properly. Older serializers that don't take the indentation config into account can get false positives on changed lines (like the diff I shared above). There is actually a fallback in the jest-snapshot logic to account for this, but in the case of jest-styled-components and presumably @emotion/jest, there is a mixture of compliant serializers and non-compliant serializers called to produce the output, which results in the fallback failing to work correctly.

The fix for this issue must be made in the logic provided by jest-styled-components.

First, the serializer must be updated from the old plugin style ({ test(){}, print(){} }) to the new plugin style ({ test(){}, serialize(){} }), in order to receive indentation configuration in the arguments. This may lose support for very old versions of Jest (I have confirmed at least jest >= 22.0.0 would be fine), so it might need to be considered a breaking change if the jest version is not enforced somewhere (I didn't see it in peerDependencies). Following that change, we have two options to fix the diff issue:

  1. Respect the indentation configuration provided by jest. This is made difficult by the dependency on the css library for the serialization of style rules. That library has yet-unfixed bug in that indentation set as an empty string is ignored.

  2. Modify the indentation config being passed on by the serializer in jest-styled-components such that all serializers it calls are equally ignorant to indentation configuration, helping the fallback in jest-snapshot be triggered. This would allow us to fix the issue just with changes in this library, but it has the potential to change users' existing snapshots if they were using custom indentation before, and therefore might be considered a breaking change.

fritz-c added a commit to fritz-c/jest-styled-components that referenced this issue Feb 24, 2021
Helps the library comply with Jest's serializer's indent configuration,
which in turn fixes styled-components#355
@fotopixel
Copy link

Hey there... this fix would be really helpful. I look forward to having this fix included in the next release :-)

jan-bitgrip added a commit to jan-bitgrip/jest-styled-components that referenced this issue Jun 29, 2021
@Bazif-Khan
Copy link

Bazif-Khan commented Jun 21, 2022

Hi @fritz-c, is there any update on this?

The fix for this issue would be really useful.

This is quite an important issue, which needs to be fixed because the latest version of the library is not usable due to this bug.

I think this issue was introduced in v7.0.7 because if I use v7.0.6, then it works fine.

@fritz-c
Copy link
Contributor Author

fritz-c commented Jun 22, 2022

@Bazif-Khan I haven't had the time to work on a fix for it, though the path to resolution seemed pretty clear at the time I left it. The code is up on GitHub for anyone who has some working hours to take over implementing the fix.

cincodenada pushed a commit to cincodenada/jest-styled-components that referenced this issue Aug 9, 2022
Helps the library comply with Jest's serializer's indent configuration,
which in turn fixes styled-components#355
cincodenada pushed a commit to cincodenada/jest-styled-components that referenced this issue Aug 9, 2022
Helps the library comply with Jest's serializer's indent configuration,
which in turn fixes styled-components#355
cincodenada pushed a commit to cincodenada/jest-styled-components that referenced this issue Aug 9, 2022
Helps the library comply with Jest's serializer's indent configuration,
which in turn fixes styled-components#355
quantizor pushed a commit that referenced this issue Aug 31, 2022
* create failing tests for indent config compliance

* use new serializer plugin API

* pass indentation config to css.stringify

Helps the library comply with Jest's serializer's indent configuration,
which in turn fixes #355

* add comments to serialize util in tests

* Manual updates post-rebase

 - Update test for new class format
 - Upgrade pretty-format to reduce indirect deps

Co-authored-by: fritz-c <4413963+fritz-c@users.noreply.github.com>
Co-authored-by: Ell Bradshaw <joelbr@tailormed.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants