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

chore(deps): migrate from enzyme to @testing-library/react #635

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

denkristoffer
Copy link
Collaborator

@denkristoffer denkristoffer commented Nov 11, 2020

Purpose of PR

Migrate from enzyme to @testing-library/react. This is to align testing libraries used across components, to more quickly allow for major React upgrades, and to unblock #611.

There's still a single component on enzyme, InViewport.test.tsx:

These tests use enzyme's instance() which testing-library doesn't support:

As we already discussed, we are against testing implementation details and things that uses are not aware of it, and we aim to test and interact with the component like how our users would.

I'm not sure how to test it without rewriting the component completely, which defeats the purpose.

PR Checklist

  • I have read the relevant readme.md file(s)
  • All commits follow our Git commit message convention
  • Tests are added/updated/not required
  • Tests are passing
  • Storybook stories are added/updated/not required
  • Usage notes are added/updated/not required
  • Has been tested based on Contentful's browser support
  • Doesn't contain any sensitive information

@netlify
Copy link

netlify bot commented Nov 11, 2020

Deploy preview for forma-36-react-components ready!

Built with commit 003c8ce

https://deploy-preview-635--forma-36-react-components.netlify.app

@denkristoffer denkristoffer force-pushed the react-testing-library branch 3 times, most recently from 022c137 to 6e011f3 Compare November 12, 2020 07:52
`gridColumnGap` -> `columnGap`
`gridRowGap` -> `rowGap`

Why did it work before? Not sure.
@denkristoffer denkristoffer marked this pull request as ready for review November 12, 2020 13:20
Copy link
Collaborator

@mshaaban0 mshaaban0 left a comment

Choose a reason for hiding this comment

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

Very nice, well done 👏

Copy link
Contributor

@m10l m10l left a comment

Choose a reason for hiding this comment

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

Awesome. We'll be able to deprecate InViewport soon, I wonder what peoples thoughts are on just removing the test so that we can completely get rid of Enzyme?

@denkristoffer
Copy link
Collaborator Author

@m10l I'm for it, but I don't really have an idea of how much InViewport is used outside of Forma.

Copy link
Contributor

@gui-santos gui-santos left a comment

Choose a reason for hiding this comment

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

GREAT!

I vote for removing the enzyme test as @m10l suggested

@denkristoffer
Copy link
Collaborator Author

I've deleted the test and removed enzyme completely 👍

The test requires enzyme and isn't easy to refactor to testing-library
@denkristoffer denkristoffer merged commit cc45c76 into master Nov 13, 2020
@denkristoffer denkristoffer deleted the react-testing-library branch November 13, 2020 09:18
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.

4 participants