You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/11/13 04:25:55 UTC

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

ktmud opened a new issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688

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

### Motivation

In Sept 2020, [React Testing Library](https://github.com/testing-library/react-testing-library) (RTL) has officially overtaken Enzyme as the most popular React testing library according to [NPM downloads](https://npmcharts.com/compare/@testing-library/react,enzyme?interval=7&log=false). While still wildly popular, Enzyme has been running behind in catching up with all the latest React trends. It still [doesn’t support](https://github.com/enzymejs/enzyme/issues/2011) the `useEffect` hook properly and has yet to fully support [React 17](https://github.com/enzymejs/enzyme/issues/2429).

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 rendered. So instead of checking component props and states with fine-grained internal APIs, you check component outputs based on the expected user interactions. The official doc has more [explanation](https://testing-library.com/docs/react-testing-library/migrate-from-enzyme/).

We believe this is the right way to go and will help us write stabler and more efficient tests with more confidence. With better support for simulating [user events](https://github.com/testing-library/user-event), we may even replace some Cypress tests with smaller scoped (and faster) RTL tests.

### Proposed Change

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

### New dependencies

`@testing-library/react` and `@testing-library/jest-dom` 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 should 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.

----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

ktmud closed issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688


   


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   +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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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!


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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. 


----------------------------------------------------------------
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

[GitHub] [incubator-superset] nytai edited a comment on issue #11688: [SIP-56] Adopt React Testing Library for testing React components

Posted by GitBox <gi...@apache.org>.

nytai edited a comment on issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688#issuecomment-733939266

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](https://github.com/apache/incubator-superset/blob/9c69679e14c2fa2b031c5c22b5cb95b640925176/superset-frontend/spec/helpers/waitForComponentToPaint.ts#L29) 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.

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

[GitHub] [incubator-superset] eschutho edited a comment on issue #11688: [SIP-56] Adopt React Testing Library for testing React components

Posted by GitBox <gi...@apache.org>.

eschutho edited a comment on issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688#issuecomment-733949694


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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   @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. 


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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. 


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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. 


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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

@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 #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 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](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.

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

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

Posted by GitBox <gi...@apache.org>.

graceguo-supercat commented on issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688#issuecomment-730164042


   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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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

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](https://github.com/apache/incubator-superset/blob/9c69679e14c2fa2b031c5c22b5cb95b640925176/superset-frontend/spec/helpers/waitForComponentToPaint.ts#L29) is 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.

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

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

Posted by 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

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

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

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

Posted by GitBox <gi...@apache.org>.

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


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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   The sip has passed.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   +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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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. 


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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

@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.

#### Migration costs

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

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

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

Posted by GitBox <gi...@apache.org>.

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


   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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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


   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?


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

issue-label-bot[bot] commented on issue #11688:
URL: https://github.com/apache/incubator-superset/issues/11688#issuecomment-726499600


   Issue-Label Bot is automatically applying the label `#enhancement` to this issue, with a confidence of 0.65. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback! 
   
    Links: [app homepage](https://github.com/marketplace/issue-label-bot), [dashboard](https://mlbot.net/data/apache/incubator-superset) and [code](https://github.com/hamelsmu/MLapp) for this bot.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.

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

@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

#### Migration costs

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

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