-
Notifications
You must be signed in to change notification settings - Fork 503
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
Remove dependency on enzyme #1668
Comments
I want to work on this @yurishkuro. Can you assign it to me please? Will be a very good start for me |
We do not assign issues. There are multiple packages that are affected, don't try to change all in one PR. |
@yurishkuro Any previous discussion that I should not miss, execpt the mentioned ones. |
I also want to work on this. There are several test files, so I think we can split the task between us, @Sangam-ghimire. Please let me know if you want to handle all the files alone because if we both work on same files, it will waste time for both of us. |
Sure @AbhishekTiwari23 that's a great spirit. Let's split and work together. I might need some help too. |
@Sangam-ghimire @AbhishekTiwari23 Please have a look at the discussion among the PRs: #1659 #1653 #1643 |
Thanks, working on it |
## Which problem is this PR solving? - part of #1668 - migrating the existing Enzyme based tests to React Testing Libary ## Description of the changes Currently I migrated 3 files - `QualityMetrics/BannerText.test.js` - `QualityMetrics/CountCard.test.js` ## How was this change tested? - replaced the shallow module from enzyme to render from RTL ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Freedisch <[email protected]> Signed-off-by: Freedisch <[email protected]> Co-authored-by: Freedisch <[email protected]>
## Which problem is this PR solving? - This PR migrates the `TraceIDSearchInput.test.js` from Enzyme to RTL - Snapshot testing is replaced with specific expect statement - Part of: #1668 ## How was this change tested? - Using yarn lint && yarn test-ci TraceIDSearchInput --coverage ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <[email protected]>
## Which problem is this PR solving? - This PR migrates the `DAG.test.js` from Enzyme to RTL - part of: #1668 ## How was this change tested? - Using `yarn test-ci` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <[email protected]>
## Which problem is this PR solving? - This PR Migrates `SpanBar.test.js` from Enzyme to RTL - part of: #1668 ## How was this change tested? - using `yarn test-ci` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <[email protected]>
## Which problem is this PR solving? - This PR migrates the test `NewWindowIcon.test.js` from Enzyme to RTL - part of: #1668 ## How was this change tested? - using `yarn test-ci` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <[email protected]>
Have you started? |
I am working over here https://github.com/jaegertracing/jaeger-ui/tree/main/packages/jaeger-ui/src/components/App is that fine |
Now i am thinking to work on components/DeepDependencies |
## Which problem is this PR solving? - Part of #1668 - Using React Testing Library to remove dependency on enzyme for index.test.js and TopNav.test.js ## How was this change tested? - Tested my changes inside packages/jaeger-ui/ using command `npx jest --testPathPattern=src/components/App/.*\.test\.js` - Also performed yarn test ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: suhail34 <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes Migrated the `TraceTimelineLink.test.js` to `RTL` using the idiomatic RTL practices ## How was this change tested? Ran the test suite to ensure all changes passed. ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Eshaan Aggarwal <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes Migrated one component to RTL ## How was this change tested? Ran the test suite and ensured all of them pass ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Eshaan Aggarwal <[email protected]>
Hi! I have been migrating some files to RTL, and one common pattern that I keep stumbling upon is using snapshots in testing. Have any tests been migrated from The reason I'm saying this is due to the bulky snapshots generated by RTL and the fact that it is built to test for user interactions, and not necessarily the rendered DOM. |
@EshaanAgg in many cases snapshot testing is just lazy tests. Ideally we want to move away from those to more explicit tests like the two you fixed today. But we do have a shortcut that still allows to use snapshots without enzyme, as in #1653. |
## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes Migrates the test for `ChevronDown.tsx` to RTL ## How was this change tested? Ran the test suite locally ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Eshaan Aggarwal <[email protected]>
Ahh, I got it. I'll try to experiment with the tests and see if we can move away from them in some components. |
## Which problem is this PR solving? Fixes part of jaegertracing#1668 ## Description of the changes Migrates the test for `ChevronDown.tsx` to RTL ## How was this change tested? Ran the test suite locally ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Eshaan Aggarwal <[email protected]>
## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes Migrates `TraceHeader.tsx` from enzyme to RTL. Removes the snapshot tests to relevant assertions with the help of `test-ids`. ## How was this change tested? Running the test suite locally. ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Eshaan Aggarwal <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
…zyme to RTL (#2066) ## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes - Migrates `ExamplesLink` and `EmphasizedNode` components from enzyme to RTL - Replaces snapshot testing with actual test for the URLs ## How was this change tested? Running the tests locally ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Eshaan Aggarwal <[email protected]>
## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes Migrate `ExternalLinks` component from enzyme to RTL ## How was this change tested? Ran the tests locally ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Eshaan Aggarwal <[email protected]>
## Which problem is this PR solving? Fixes part of #1668 ## Description of the changes Migrates the following components from Enzyme to RTL 1. `CircularProgressbar` 2. `ErrorMessage` 3. `TraceName` - Removed the instances of snapshot testing from the same and migrated to tests checking the functionality associated with them - Refactored some of the original code of the components to make them more readable ## How was this change tested? - Running the tests locally ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Eshaan Aggarwal <[email protected]>
Enzyme is not compatible with React 18, so we need to phase it out.
Preferred approach is to migrate to
react-testing-library
. In some cases we may use a replacementshallow
function #1653, but only when absolutely necessary, it's better to move away from snapshot testing.For examples of this being partially done, see merged PRs in this ticket below (e.g. #1659 #1653 #1643).
The scope of this is sizeable, so the recommendation is to limit the changes to specific modules, a couple test files at a time.
Current enzyme references (Nov 9):
The text was updated successfully, but these errors were encountered: