[SIP-56] Adopt React Testing Library for testing React components

[ktmud]

[SIP] Proposal for adopting React Testing Library to test React components

Motivation

In Sept 2020, React Testing Library (RTL) has officially overtaken Enzyme as the most popular React testing library according to NPM downloads. While still widely popular, Enzyme has been running behind in catching up with all the latest React trends. It still doesn’t support the useEffect hook properly and has yet to officially support React 17.

Besides having a more promising future and a more active community, RTL also introduces a new way of thinking in writing tests. It encourages developers to focus on what users see, as opposed to how the components are implemented. Instead of checking component props and states with fine-grained internal APIs, you check the expected component outputs that are visible to the users. The official doc has more explanation.

We believe this is the right way to go and will help us write stabler tests more efficiently and with more confidence. With better support for simulated user events, we may even replace some Cypress tests with smaller scoped (and faster) React component tests.

Proposed Change

We propose to introduce React Testing Library to Superset’s frontend testing toolkit and gradually migrate Enzyme tests to it when possible.

New dependencies

@testing-library/react and @testing-library/jest-dom and a couple of related ESLint plugins will be added to superset-frontend and superset-ui’s dependencies.

Migration Plan and Compatibility

1. Add RTL to the codebase, try converting a couple of existing components to RTL. Hammer out global setups, etc.
2. Since there is no urgent need to migrate existing Enzyme tests, RTL and Enzyme will live side-by-side for as long as needed.
3. However, all new React component tests is recommended to be written with RTL to take advantage of its benefits described above.

Rejected Alternatives

• Keep status quo: the growing needs of better testing requires a better testing library.

[issue-label-bot]

Issue-Label Bot is automatically applying the label #enhancement to this issue, with a confidence of 0.65. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[rusackas]

While I don't have RTL experience (yet!), this seems promising, so I certainly support the experiment(s) noted in your migration plan.

I'd also like to take a moment to advocate for putting components in directories, with their test files alongside them. Having the component, the styles, the tests, and the storybook stories all in one folder will make things MUCH easier to maintain, I believe.

[mistercrunch]

+1, looked at the examples and it seemed sane to me. I'd say we don't enforce it as much as simply favoring it for a phase ahead in a transitional period. Maybe in a few months we evaluate whether we want to more actively enforce / migrate.

[suddjian]

I have not used RTL but have looked at their docs and agree with their testing philosophy.

[villebro]

I agree with everything said here, +1 for going ahead with implementing RTL and seeing where it takes us.

[junlincc]

i can't speak technical, but i know @ktmud is proposing what he thinks that's the best for Superset, so +1 :)

[zhaoyongjie]

With the use of React hook components, it seems that we need to find a test framework that is more friendly to effectHook, so +1

[graceguo-supercat]

If you want to test a component which nested with many sub-components, using Enzyme is really hassle.
But enzyme offer some shortcut to set state/ set props, it can also make many test scenarios easier.
So +1 for adding RTL in the code base, so that everyone can play and evaluate.

[eschutho]

While I think that RTL has a lot of merit, I don't think that we should completely throw out the idea of unit tests. I believe that RTL is great for integration tests and larger pieces of functionality, but that we still need to test utils and functions that take an argument and return the proper value. Whether it's RTL or enzyme that handles this, I think it would be beneficial to have a set of unit tests, integration test (RTL) and end to end (Cypress) for full test coverage. Not everything can be handled or should be handled from the perspective of the user.

[robdiciuccio]

Agree with @eschutho's comments. I'm also concerned that we're embarking on another long-term migration without a clear definition of the problem we're trying to solve here. What exactly are the problems with current Enzyme testing that necessitate migrating to a new testing library?

[etr2460]

I don't think anything here was mentioning getting rid of unit tests, right? My assumption was that RTL would live as a way to test components and be a possible, even preferred alternative to the current use of Enzyme. That, along with the fact that Enzyme is falling behind in terms of support, made it seem reasonable to start using RTL over Enzyme for new tests, especially ones involving hooks.

That's also ignoring the accessibly centric design of RTL, where as you write RTL tests, you also improve the accessibly of the application for folks with screen readers and who rely on semantic html attributes.

Finally, to Rob's question in the VOTE thread: React hooks have been around for over 2 years now, and there's been significant adoption of hooks as a preferred alternative to HOCs for code reuse and extensibility. Additionally, although it hasn't been explicitly recommended to use hooks within superset-frontend, they're being used increasingly more often, and therefore we need a testing solution for them. I'm biased as I prefer Functional Components + hooks when building my UIs, but I don't think i'm alone there.

In conclusion, I voted +1 because:

• RTL has increased functionality for parts of our code that Enzyme doesn't cover
• RTL helps improve a11y throughout our codebase
• This SIP does not reduce focus on unit or e2e tests, but instead adds additional focus to React component testing

Hope this clears some stuff up!

[ktmud]

@eschutho As noted by Erik, I don't think anyone here proposed to "completely throwing out the idea of unit tests". Of course there will be unit tests. In fact, most RTL tests would still be considered unit tests (for React components). The only thing RTL will replace is Enzyme and some integration tests in Cypress. In case of JavaScript utils or pure data processing functions, vanilla unit tests in jest assertions are still preferred (I thought this was obvious).

@robdiciuccio Erik has articulated the reasoning much better than I did in this SIP, but let me try to rephrase/expand things a little further to make things even more clear:

Problems with Enzyme

1. Lack of good support for React hooks: you have to add a custom act wrapper to test certain things. Some tests are not even possible. For example, I still haven't found a way in Enzyme to do the new test cases I added for the <Timer > component in test: add React Testing Library #11771 .
   • The React team encourages developers to try out hooks for new components. As Erik noted, we've been using it in Superset extensively. It's unrealistic to get rid of it or halt the trend in any way.
   • If we can't get rid of it, why not give it better support?
2. Slow pacing in supporting new React features: E.g., issues with React 17.
   • This one could become critical if there were significant improvement in React that we wanted to try.
3. Testing implementation details vs testing user expectations:
   • This one is a philosophical debate but it doesn't harm to give the new philosophy a try.

Overall, Enzyme is "losing steam"---with proofs like NPM downloads and the fact that more and more companies/projects have embarked on the RTL train, including Airbnb---where Enzyme was created.

Migration costs

If your concern is about the migration costs, I agree with the sentiments in this thread that we do not need to initiate a full migration of all existing components. Test migrations are different than other frontend migrations we have taken on (TypeScript and AntD), because tests do not affect code shipped to the end users and each test files are relatively independent, therefore there would be less need to migrate and less risk if we don't migrate.

Unless things break (e.g. Enzyme completely dead), we don't have to spend time on migrating existing tests.

[nytai]

I think it's quite normal for a codebase that's >5 years old to have a lot of accumulated patterns, especially one with as many contributors as superset. Sometimes introducing a new pattern is conflated with refactoring out all the old patterns. I don't think anyone is suggesting migrating all enzyme tests to use RTL (or whatever the latest flavor is at the moment). React has been making quite a few changes to their API, I don't think we should limiting our use there. Similarly, enzyme has fallen behind the curve, I don't think we should continue to fight with the lib to get it to work with newer portions of the react api.

I have found myself fighting with enzyme, to get what should be simple tests to pass, more often than not (see how ubiquitous this util has become in test files). Therefore, consider this a big

+1

from me as I suspect writing tests will become far more enjoyable with RTL in the toolbox.

[eschutho]

Thanks for the clarification @ktmud and @etr2460. I do not have any opposition to adding a new library, but with this library, as you mentioned, comes a methodology of front end testing, which is to test from the perspective of the user. In this strategy, KCD expresses that you shouldn't test "implementation details", or look at state, or what a function returns. "Those things aren't important to users. You should only focus on what the user sees, etc."

I think that these are fine principles for integration tests, although I understand that people still call these "unit tests", and apologies if my use of wording there was vague. I still believe that there is value in testing that a function receives an argument and reliably outputs what you expect, specifically in smaller react components, and leave the larger "view" components that are composed of smaller components to use this "integration testing" approach from the user's perspective.

I believe this tool brings a heavier weight with it in terms of how you test in general, and I am aiming to raise the concern that in some cases, you should assert on the return value of a function, and I believe that we should see this "user-focused approach" as just 1/3 of the total testing picture.

So to separate the two concerns, if RTL fixes problems with enzyme, then +1 on that, but I believe that with adoption of this library, we should have perhaps a larger discussions about how to test our code in general rather than assume that in using this library, we need to adopt all the principles behind it.

[eschutho]

I'm happy to start a SIP on this three-legged testing approach, if we want to explore it more.

[ktmud]

Thanks for the clarification, @eschutho . I think those are valid concerns. But I also feel these are more like engineering principles that people may feel differently and somewhat hard to enforce. Plus it's not only about tests, but how to abstract and extract utility/data processing functions for your React components. Some may prefer inline functions, some may prefer put as much things outside of the React component as possible, it's quite challenging to draw the line here.

As noted by Max and others in this thread, we can always add the library first and re-evaluate the recommendation later. I think the "larger discussions about how to test our code in general" could happen after people have given RTL a try and had more experience with it to assess which approach is sustainable in the long term.

[nytai]

@eschutho I personally view this as an adoption of a tool (which, admittedly, is built with a philosophy in mind) rather than an entire testing philosophy. I very much view testing as something that is very subjective to personal option and philosophy. Ultimately it's up the the author of a PR and their reviewer to decide what's an acceptable level of testing for a given change. I would much rather see us work on increasing testing is general rather than trying to standardize on a specific testing style/approach.

[nytai]

That being said, I think some literature around current testing patterns in superset with good examples of each could help encourage authors to write more tests and make writing tests more approachable to newcomers.

[robdiciuccio]

Thanks for the additional info on the goals of this SIP @ktmud and @etr2460 . Superset currently suffers from a large number of patterns and approaches which could benefit from a higher degree of standardization, in my opinion. That said, I'm definitely not looking to start a philosophical discussion about how we should approach this in this SIP, but I do think that there would be value in having these discussions at some point rather than deferring these decisions to each new PR.

[ktmud]

+1 on standardizing and not making ad-hoc decisions on adding new patterns, which I think was also why we decided to create a SIP for this in the first place.

If we want a more comprehensive guideline on how to do testing (and/or other things), I'm all for it.

@eschutho would you like to take the lead on writing a SIP for establishing a code/test style guide? The SIP itself doesn't have to be about all aspects of the style guide, but rather the process around creating, updating, and enforcing a guideline when we need to. Or we can discuss whether this is necessary in the next Superset meetup.

[eschutho]

Makes sense to bring this up in the next meetup. I'm hearing a need to perhaps discuss standardization (cleaning up old code), testing approaches, and/or establishing a process around style guide. Let's get some feedback around what people are feeling would be needed at this point to address any concerns left over.

[ktmud]

The sip has passed.

[justinpark]

Track here: https://docs.google.com/spreadsheets/d/12a1zTXdCrEgn3wLDUo6_a_KRs5EgFFCYI8DewcQdy0s/edit?gid=0#gid=0