jest-community / snapshot-diff

Diffing snapshot utility for Jest
MIT License
602 stars 38 forks source link

Serializers not working as expected #162

Open DarkPurple141 opened 4 years ago

DarkPurple141 commented 4 years ago

I've filed a mirror issue here: https://github.com/emotion-js/emotion/issues/1898

Can't get the setSerializer API to work as expected.

I've created a minimal reproduction repo here explaining the issue in more detail: https://github.com/DarkPurple141/jest-emotion-typescript-minimal/tree/master

DarkPurple141 commented 4 years ago

It appears the underlying issue is that this API integrates with the older jest.SerializerPlugin and so doesn't support a number of more current serializers. Is this something the maintainers are looking at fixing?

alistairjcbrown commented 4 years ago

the underlying issue is that this API integrates with the older jest.SerializerPlugin

Do you have any more details on this? That may be an issue, but I'm not sure it's the issue here.

From your repo - let's run through each test

The expectation was, that by adding the Emotion serializer that we'd also see styles being output in React components. However, serializers added through setSerializer are independent - the example in the docs/tests are either replacing the built-in React serializer (using React renderer) with Enzyme's serializer, or adding serialization support for some new type. In this case, we want to augment React component serialization to also deal with CSS-in-JS styles. The current setup means when adding the Emotion serializer, either it will run or the React serializer will run (depending on the result from test), not both. Which is what we're seeing from the snapshot data in your repository.

Proposed solution

  1. Update documentation for setSerializer with more example and clearer instructions on how this works
  2. Currently, the React serializer uses pretty-format to serialize a rendered React component, using the same default serializers as jest-snapshot. To have this work with Emotion, we'd need to provide the ability to enhance the React serializer to also pass Emotion's serializer into pretty-format. This would make the Ideal solution work as expected, with the React serializer taking care of rendering with react-test-renderer and pretty-format using all available serializers for generating the snapshot.
DarkPurple141 commented 4 years ago

Hi @alistairjcbrown thanks for getting back.

Sorry I did investigate further but didn't update this ticket.

The rub seems to be in the second of your proposed solutions. Essentially, the pretty-format call for React specific serialization is wrapped and is never updated (even by calling setSerializer you don't actually hook into the snapshot-diff's serializer.

I think there may be an additional issue thought with the fact that snapshot-diff uses the test() print() interface and newer libs now use test() serializer() see: https://github.com/emotion-js/emotion/issues/1898#issuecomment-642949145

alistairjcbrown commented 4 years ago

@DarkPurple141

the pretty-format call for React specific serialization is wrapped and is never updated (even by calling setSerializer

setSerializer is for adding new top-level serializers (e.g. a serializer for serializing React components rendered by Enzyme), not for enhancing the existing React component serializer. The proposed solution above is to "provide the ability to enhance the React serializer to also pass Emotion's serializer into pretty-format"; this would be new functionality and probably a new API to allow that existing serializer to be enhanced.

I think there may be an additional issue

I think you're right that that may be an issue as serializers without print() are added, but it sounds like it's unrelated to this specific issue on using the Emotion serializer; it looks like Emotion currently provide their serializer with the test() print() interface, which is why we're successfully seeing snapshot data in your repo, rather than getting an exception on missing print function.

We should probably create an issue to track updating snapshot-diff to support both print() and serialize(), as documented in the pretty-format plugins section: https://github.com/facebook/jest/blob/a8129a7e4520ec545e0ddf97048fbc9e8a573076/packages/pretty-format/README.md#serialize

alflennik commented 4 years ago

I've been playing around with snapshot-diff and it seems like it can really simplify my tests - the only thing is, whenever I change the internal implementation of my component, even if it doesn't affect any functionality, all the emotion classNames break.

Screen Shot 2020-10-02 at 11 23 57 AM

It looks like the PR could potentially fix this issue! If it makes it in I can promise it will make my day 😜

thymikee commented 4 years ago

Ugh, I need to get back to reviewing that 😅 hopefully this weekend!

RazerM commented 2 years ago

I was surprised to learn that snapshot-diff doesn't pass the serializers to pretty-format, I'd expect it to do what jest-snapshot does: https://github.com/facebook/jest/blob/03cf86f60c42036a183c4fecac24882a06835427/packages/jest-snapshot/src/utils.ts#L162-L175