snapshot-diff icon indicating copy to clipboard operation
snapshot-diff copied to clipboard

Serializers not working as expected

Open DarkPurple141 opened this issue 4 years ago • 7 comments

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 avatar Jun 11 '20 06:06 DarkPurple141

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?

DarkPurple141 avatar Jun 14 '20 23:06 DarkPurple141

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

  • Ideal - diffing two React components (not rendered) displays the component output with Emotion generated classes, but no style output
    • We're passing in React components which aren't rendered, so I don't think Emotion's serializer is picking it up. Instead, it's all being serialized by the React serializer that comes as default in snapshot-diff, and so we just get the component output
  • vanilla toMatchSnapshot() - working as expected (nothing to do with snapshot-diff)
  • Using react-rest-render and using snapshotDiff() func - these two tests I believe are essentially doing the same thing; diffing two rendered React components displays the style output but not the component output
    • This one's pretty much the opposite of the first; we're passing in rendered React components which Emotion's serializer is picking up. But Emotion's serializer only deals with rendering the styles, so we're seeing [object Object] for the component output.

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.

alistairjcbrown avatar Jun 27 '20 14:06 alistairjcbrown

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

DarkPurple141 avatar Jun 27 '20 23:06 DarkPurple141

@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

alistairjcbrown avatar Jun 28 '20 11:06 alistairjcbrown

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 😜

alflennik avatar Oct 02 '20 15:10 alflennik

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

thymikee avatar Oct 02 '20 15:10 thymikee

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

RazerM avatar Aug 01 '22 16:08 RazerM