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

Serializers not working as expected #162

Open
DarkPurple141 opened this issue Jun 11, 2020 · 7 comments · May be fixed by #174
Open

Serializers not working as expected #162

DarkPurple141 opened this issue Jun 11, 2020 · 7 comments · May be fixed by #174

Comments

@DarkPurple141
Copy link

I've filed a mirror issue here: emotion-js/emotion#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
Copy link
Author

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
Copy link
Contributor

alistairjcbrown commented Jun 27, 2020

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.

@DarkPurple141
Copy link
Author

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: emotion-js/emotion#1898 (comment)

@alistairjcbrown
Copy link
Contributor

alistairjcbrown commented Jun 28, 2020

@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 added a commit to alistairjcbrown/snapshot-diff that referenced this issue Jun 28, 2020
snapshot-diff provides a convenience serializer for React components
which renders them and serializes them before diffing. Support is
already in place for allowing custom serializers to be added which will
work at the root level; serializing unknown types (e.g. React component
rendered with Enzyme).

However, there is no ability to update the serializers used within the
React component serializer. The use case for this need to when a
CSS-in-JS solution, such as Emotion, is used and custom serilization
is required to support outputting the styles attached. Emotion provides
this serializer, but adding at the root level then only outputs the
styles and does not output the component.

To support this, the same `defaultSerializers` and `setSerializers` API
as provided by `snapshot-diff` for adding root level serializers has
been applied to the React component serializer to allow
"sub-serializers" to be added which will then be passed into
`pretty-format`. This then provides an API which will allow the Emotion
serializer to work as expected with the rest of the React component
serialization process.

Fixes jest-community#162
alistairjcbrown added a commit to alistairjcbrown/snapshot-diff that referenced this issue Jun 28, 2020
snapshot-diff provides a convenience serializer for React components
which renders them and serializes them before diffing. Support is
already in place for allowing custom serializers to be added which will
work at the root level; serializing unknown types (e.g. React component
rendered with Enzyme).

However, there is no ability to update the serializers used within the
React component serializer. The use case for this need to when a
CSS-in-JS solution, such as Emotion, is used and custom serilization
is required to support outputting the styles attached. Emotion provides
this serializer, but adding at the root level then only outputs the
styles and does not output the component.

To support this, the same `defaultSerializers` and `setSerializers` API
as provided by `snapshot-diff` for adding root level serializers has
been applied to the React component serializer to allow
"sub-serializers" to be added which will then be passed into
`pretty-format`. This then provides an API which will allow the Emotion
serializer to work as expected with the rest of the React component
serialization process.

Fixes jest-community#162
alistairjcbrown added a commit to alistairjcbrown/snapshot-diff that referenced this issue Jun 30, 2020
snapshot-diff provides a convenience serializer for React components
which renders them and serializes them before diffing. Support is
already in place for allowing custom serializers to be added which will
work at the root level; serializing unknown types (e.g. React component
rendered with Enzyme).

However, there is no ability to update the serializers used within the
React component serializer. The use case for this need to when a
CSS-in-JS solution, such as Emotion, is used and custom serilization
is required to support outputting the styles attached. Emotion provides
this serializer, but adding at the root level then only outputs the
styles and does not output the component.

To support this, the same `defaultSerializers` and `setSerializers` API
as provided by `snapshot-diff` for adding root level serializers has
been applied to the React component serializer to allow
"sub-serializers" to be added which will then be passed into
`pretty-format`. This then provides an API which will allow the Emotion
serializer to work as expected with the rest of the React component
serialization process.

Fixes jest-community#162
alistairjcbrown added a commit to alistairjcbrown/snapshot-diff that referenced this issue Jul 18, 2020
snapshot-diff provides a convenience serializer for React components
which renders them and serializes them before diffing. Support is
already in place for allowing custom serializers to be added which will
work at the root level; serializing unknown types (e.g. React component
rendered with Enzyme).

However, there is no ability to update the serializers used within the
React component serializer. The use case for this need to when a
CSS-in-JS solution, such as Emotion, is used and custom serilization
is required to support outputting the styles attached. Emotion provides
this serializer, but adding at the root level then only outputs the
styles and does not output the component.

To support this, the same `defaultSerializers` and `setSerializers` API
as provided by `snapshot-diff` for adding root level serializers has
been applied to the React component serializer to allow
"sub-serializers" to be added which will then be passed into
`pretty-format`. This then provides an API which will allow the Emotion
serializer to work as expected with the rest of the React component
serialization process.

Fixes jest-community#162
@alflennik
Copy link

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
Copy link
Member

thymikee commented Oct 2, 2020

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

@RazerM
Copy link

RazerM commented Aug 1, 2022

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

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

Successfully merging a pull request may close this issue.

5 participants