dequelabs / cauldron

https://cauldron.dequelabs.com/
Mozilla Public License 2.0
89 stars 21 forks source link

Migrate from Enzyme to React Testing Library #1197

Closed anastasialanz closed 9 months ago

anastasialanz commented 1 year ago

Background

In order to update Cauldron to React 18, we need to update our testing framework. Currently, Enzyme does not support React 17 or React 18.

Related resources: https://dev.to/wojtekmaj/enzyme-is-dead-now-what-ekl https://stackoverflow.com/questions/72029189/does-enzyme-support-react-version-18

Recommendations

✨ Switching to React Testing Library

Pros

In this branch https://github.com/dequelabs/cauldron/tree/rfc--testing-library, I've used the matching React Testing Library to the React version 16.3.1 and updated the tests for the Button component as a proof of concept. We can run Enzyme and RTL together until we finish all of the test migrations.

Migration Plan

https://testing-library.com/docs/react-testing-library/migrate-from-enzyme

To ensure a successful migration, we recommend doing it incrementally by running the two test libraries side by side in the same application, porting your Enzyme tests to React Testing Library one by one. That makes it possible to migrate even large and complex applications without disrupting other business because the work can be done collaboratively and spread out over time.

⚠️ Switching to Playwright Component Testing

@scurker has mentioned in the past about trying out Playwright Component Testing.

Pros

Cons

Screenshot 2023-09-18 at 8 21 11 AM

Additional resources: https://github.com/microsoft/playwright/tree/main/packages/playwright-ct-react https://www.lambdatest.com/learning-hub/playwright-component-testing

scurker commented 1 year ago

Thanks for putting this together!

I think switching to RTL probably makes sense as our teams are likely already familiar with it, even with some of the caveats that might exist in RTL.

A couple of questions/thoughts:

I don't want to scope creep here as this will already be a fairly large undertaking, but I would be curious how much effort the above might add vs the benefit.

scurker commented 1 year ago

There is a list of components to do test migrations with here: https://github.com/dequelabs/cauldron/issues/1159

anastasialanz commented 1 year ago

I'm wondering if rewriting tests should also co-locate tests next to their components, that way it would be easier to target a separate test runner (if needed). We would also need to consider what the implications are for code coverage if we did this.

I'm a fan of tests in their component folders, but we can also leave the tests in the __tests__ folder and run both test runners side by side without issue. I can test this in a different branch based on my button test branch and see what the code coverage implications would be.

Related, if a component test file has 5 tests and we update functionality for that component, should the new test be written using RTL in the same file or should the entire file be converted to RTL in addition to the new test? This could allow someone to only focus on the new code and leave other test rewrites for a separate PR.

I'm thinking if we're doing a full rewrite of tests, we should try to leverage typescript. We don't currently test the interoperability of components + typescript from within cauldron and that's a huge gap and I wonder if this is a reasonable opportunity to do so.

Are you saying that when I want to rewrite tests for a component to RTL I also need to add TypeScript types for that component or components?

scurker commented 1 year ago

Related, if a component test file has 5 tests and we update functionality for that component, should the new test be written using RTL in the same file or should the entire file be converted to RTL in addition to the new test? This could allow someone to only focus on the new code and leave other test rewrites for a separate PR.

I think my vote would be to convert the test to use RTL. I do realize that will tack on additional work for someone making a small change to a component, but without fulltime dedicated resources to Cauldron, I think we need to try to make gains wherever we can.

Are you saying that when I want to rewrite tests for a component to RTL I also need to add TypeScript types for that component or components?

The test itself should be written in typescript, hopefully this wouldn't require changing any component typescript types and might expose issues where something was typed incorrectly.

scurker commented 1 year ago

Thinking out loud: we did a conversion of all the documentation from custom React documentation to MDX starting in mid-March, with the final conversion completed mid-August, so roughly ~5 months. Assuming that converting tests takes roughly the same amount of time as documentation, it is possible that we could have all tests converted over to RTL by end of Q1 2024 if we were to start in October.

If we could borrow some resources from teams, this could go faster - just trying to get a rough time frame out there of what kind of scope we're looking at.

anastasialanz commented 1 year ago

Assuming that converting tests takes roughly the same amount of time as documentation

I'm not sure if this is a fair comparison but I looked at some of the largest test files and compared it to the lines of code in the docs file. Looking through all of the test files, there are more test files under 100 lines that would be quicker to migrate, but I can imagine the ones below taking more effort.

Component Name Lines of Tests Lines of Docs
OptionsMenu 550 99
Pagination 327 161
Accordion 292 99
Toast 272 191
ExpandCollapsePanel 267 114
TwoColumnPanel 234 317
scurker commented 1 year ago

I'm not sure if this is a fair comparison

Maybe not, I'm just trying to put some ballpark figure out there to work with. We can pad it accordingly once I can figure out if there's additional resources we can lean on.

scurker commented 11 months ago

👍 from me!

JoshMK commented 11 months ago

👍

DelishusCake commented 11 months ago

👍

anastasialanz commented 9 months ago

Closing this issue since the initial setup has been completed. We'll track migrating each component in https://github.com/dequelabs/cauldron/issues/1159.