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] Convert remaining enzyme tests to testing-library #26832

Merged
merged 3 commits into from
Jun 19, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jun 18, 2021

This removes direct usage of enzyme from our internal testing API. It'll be only used internally-internally (i.e. not by collaborators writing tests but collaborators working on the testing API).

Testing with enzyme is still valueable for us. It was just too easy to write bad tests with enzyme.

This'll make it easier to debug these tests in React 18

Closes #22911

@eps1lon eps1lon added the test label Jun 18, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 18, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against b99e8aa

);
expect(wrapper.text()).to.equal('');

expect(container.firstChild).to.equal(null);
Copy link
Member Author

Choose a reason for hiding this comment

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

Argueably a clearer assertion since there's a different between an element with no text and no element.

@eps1lon eps1lon marked this pull request as ready for review June 18, 2021 20:05
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Testing with enzyme is still valuable for us

Do you mean for some of the tests in the describeConformance, or as a mean to make sure that our components have minimum level of support of enzyme?

@eps1lon
Copy link
Member Author

eps1lon commented Jun 18, 2021

Do you mean for some of the tests in the describeConformance

As part of describeConformance i.e. strictly internal

@eps1lon eps1lon merged commit 2569eb9 into mui:next Jun 19, 2021
@eps1lon eps1lon deleted the test/enzyme branch June 19, 2021 06:22
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 19, 2021

As part of describeConformance i.e. strictly internal

Ok, so we could potentially accept a PR that rewrites the describeConformance tests without enzyme, if it yields the same test guarantees.
I'm not suggesting that we should prioritize it, only wanted to make sure, that we could completely remove enzyme from the dev dependencies if we do no longer have the describeConformance use case.

@eps1lon
Copy link
Member Author

eps1lon commented Jun 20, 2021

Ok, so we could potentially accept a PR that rewrites the describeConformance tests without enzyme, if it yields the same test guarantees.

A big "If" though.

I don't understand why this is important though. I wouldn't waste a second thinking about dropping enzyme.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 20, 2021

@eps1lon Ok, thanks for the extra input. I wouldn't say it's important. I was wondering about the contingency plan in case of enzyme prevents us to upgrade React: enzymejs/enzyme#2524 (comment).

@eps1lon
Copy link
Member Author

eps1lon commented Jun 20, 2021

In the past we just forked the adapter which is fairly straight forward for the set of features we need. So there's no point in being concerned about this right now. Especially since the 17 adapter works just fine with React 18 alpha.

@siriwatknp siriwatknp mentioned this pull request Jun 23, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[test] Migrate tests from enzyme to react-testing-library
3 participants