[GitHub] [incubator-superset] ktmud commented on issue #11688: [SIP-56] Adopt React Testing Library for testing React components

[GitBox <gi...@apache.org>]

ktmud commented on issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688#issuecomment-733931588


   @eschutho As noted my Erik, I don't think anyone was proposing "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 #11771 .
      - The React team [encourages developers to try out hooks for new components](https://reactjs.org/docs/hooks-faq.html#should-i-use-hooks-classes-or-a-mix-of-both). 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](https://github.com/enzymejs/enzyme/issues/2429).
       - 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 but it doesn't harm to give the new philosophy a try.
   
   Overall, Enzyme is "losing esteem"---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](https://airbnb.io/projects/enzyme/).
   
   #### 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.
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org