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

test: enhance emotion serializer on React 18 to remove random values in class names #6948

Conversation

tkajtoch
Copy link
Member

Summary

This PR extends the scope of our emotion serializer to replace random emotion prefixes with emotion for class attributes in DOM snapshots. It is necessary to work with React 18 and is backwards compatible.

QA

  1. Checkout this branch
  2. Run yarn to ensure modules are up to date
  3. Run REACT_VERSION=17 yarn test-unit and verify all tests are passing
  4. Run yarn test-unit and verify that all snapshots are passing and there are no failed tests caused by mismatched class attribute. It is expected for some other tests to fail.
    Please note that jest will never finish due to EuiIcon unit tests running indefinitely on React 18.

@tkajtoch tkajtoch requested review from cee-chen and a team July 13, 2023 21:07
@tkajtoch tkajtoch self-assigned this Jul 13, 2023
Comment on lines +25 to +27
// attribute values. Sometimes the style elements are not available
// in the document when running Enzyme with the hacky React 18 adapter,
// but the serialize function is still being called. We need to find the emotion
Copy link
Contributor

@cee-chen cee-chen Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[this may not make sense to do, just thinking out loud]

  • Just to clarify, does this only affect Enzyme and not RTL?
  • If so, does it make sense at all to try and detect Enzyme vs RTL?
    • I believe you can import ShallowWrapper or ReactWrapper from enzyme and check whether the constructor name equals ShallowWrapper.name or ReactWrapper.name to determine if it's an Enzyme val/node

No worries if it doesn't make sense to do this - the goal of this question is to see if we can future-proof this for when we've moved off of Enzyme and fully onto RTL. It would be nice to have slightly cleaner code if so - but if we can't, that's fine too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debugged my code again to remember what it did when I first implemented this logic. Emotion serializer is called with already processed react test objects that are indistinguishable between Enzyme and RTL renders, and I don't know if it would even be possible to reliably detect what library they're coming from.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible - I did so within my snapshot serializer spike (#6769, ctrl+F for isShallowWrapper, isReactWrapper, and isCheerioWrapper specifically).

I can pull this PR down here in a bit to play with it if you can confirm this only affects Enzyme. TBH, I am a little surprised that this hasn't come up on Emotion's issue repo, but I'm guessing potentially not a lot of people are using the new React 18 adapter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah my assumption is that running Enzyme with the hacky React 18 adapter AND emotion is such a rare case that nobody really had the same issue...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I know what you mean, I'm actually reusing the test function from @emotion/jest now and it seems to return correct results for RTL and Enzyme. We still need to run emotion serializer for both RTL and Enzyme and serialize doesn't seem to have constructor available at all as it's already converted to test objects.

I'm open to any ideas you may have to adding this check :)

Copy link
Contributor

@cee-chen cee-chen Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From local testing: this appears to primarily only affect render/Cheerio from Enzyme and not mount or shallow snapshots. This definitely feels like something we can write a conditional for and ideally remove once we're fully off Enzyme, or at least off Enzyme render. 🤞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkajtoch Actually, since this only affects Enzyme's render, one other option could be for us to do a wholesale find+replace of Enzyme render with RTL render in main (and then update the React 18 feature branch against latest main).

We mostly only use Enzyme render for snapshots, so it should be a fairly 1:1 replacement. WDYT? We get to pay down some minor tech debt and skip having to add extra serializer logic (that will go stale once we're off Enzyme) at the same time 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a try!

@tkajtoch
Copy link
Member Author

Superseded by #6968, #6969, #6970, #6971, #6972, #6973, #6974, #6975, #6976, #6977

@tkajtoch tkajtoch closed this Jul 20, 2023
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 this pull request may close these issues.

2 participants