Init Migration from Enzyme to React Testing Library #1643

anshgoyalevil · 2023-08-08T04:14:31Z

Which problem is this PR solving?

This PR migrates a test ScoreCard.test.js from Enzyme to React Testing Library
Enzyme is no longer supported in React 18

Description of the changes

It uses the custom shallow function to render the components

How was this change tested?

It was tested by running the command yarn test -- -u to re-generate snapshots of the changed tests
After that, yarn test-ci ScoreCard.test.js --coverage was used to check if all tests pass and coverage is 100%

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality

Signed-off-by: Ansh Goyal <[email protected]>

codecov · 2023-08-08T04:27:46Z

Codecov Report

Patch coverage: 72.50% and project coverage change: -0.13% ⚠️

Comparison is base (425fe08) 96.01% compared to head (b2ee316) 95.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1643      +/-   ##
==========================================
- Coverage   96.01%   95.89%   -0.13%     
==========================================
  Files         242      243       +1     
  Lines        7559     7599      +40     
  Branches     1984     2004      +20     
==========================================
+ Hits         7258     7287      +29     
- Misses        301      311      +10     
- Partials        0        1       +1

Files Changed	Coverage Δ
...aeger-ui/src/utils/helpers/ReactShallowRenderer.js	`72.50% <72.50%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2023-08-08T05:08:39Z

packages/jaeger-ui/src/components/QualityMetrics/ScoreCard.test.js

-        />
-      )
-    ).toMatchSnapshot();
+    const { container } = render(


couldn't you just change shallow->render and keep the rest the same, to minimize the changes?

As I have discovered till now, the render function is not a direct replacement for shallow. As per docs,

Enzyme's shallow renderer doesn't render sub-components, so React Testing Library's render method is more similar to Enzyme's mount method.

This is also the reason why snapshot file is modified with a lot of code additions.

It can be inlined, but we need to use container out of render function to match the snapshot, as it doesn't provide that utility itself.

packages/jaeger-ui/package.json

yurishkuro · 2023-08-08T05:11:42Z

packages/jaeger-ui/src/components/QualityMetrics/__snapshots__/ScoreCard.test.js.snap

+    <div
+      class="ScoreCard--CircularProgressbarWrapper"
+    >
+      <svg


the previous snapshot was a lot more compact, this one contains a lot of junk which is difficult to grok for a human, making it much less valuable as a tool to detect regressions. Perhaps this was the difference in using the shallow function - is there an equivalent in the new libs?

Actually, the reason for that is, previously, the snapshots were containing the react-dom, but in the case of RTL, they contain HTML dom

According to official migration guide https://testing-library.com/docs/react-testing-library/migrate-from-enzyme/
The shallow function needs to be replaced with render, which seems like the only solution.

There is a solution on the internet that suggests changing the jest snapshot serializer to output smaller snapshots.

That could break all of the tests, and thus tests would need a huge code change.

Signed-off-by: Ansh Goyal <[email protected]>

anshgoyalevil · 2023-08-09T01:16:32Z

@yurishkuro Which path should we go for in migration?

Create a helper function shallowRender using the library react-shallow-renderer and modify each test to use that instead of mount from enzyme. It would not affect snapshots much.
Use render from RTL, which would then make the snapshots bulky, but tests won't have to be modified much.

yurishkuro · 2023-08-09T02:10:56Z

Using render from RTL is unacceptabl. I see that it creates huge snapshots which are very difficult to use for detecting regressions, and based on other information I read are not even stable.

Ideal solution would be to avoid using snapshot testing altogether, given many reasonable arguments against snapshots. But that means we have to migrate 175 call sites, a pretty large lift. So using react-shallow-renderer seems like a reasonable compromise, and we can try to enforce the policy of no new snapshot testing going forward (and perhaps eventually phase out the existing ones in favor of proper user-facing behavior testing).

yurishkuro · 2023-08-09T02:11:50Z

NB: please do not create a single PR that updates all 175 call sites. We should do it module by module, so that the changes to the snapshots can be actually reviewed.

anshgoyalevil · 2023-08-09T03:10:07Z

Thanks for the inputs 🚀
I would try to keep the scope of this PR to a single test file only.

Signed-off-by: Ansh Goyal <[email protected]>

anshgoyalevil · 2023-08-09T03:29:53Z

@yurishkuro I have made the changes, and as I am seeing, the new helper function just reverted the snapshot changes. That's amazing 🚀
That means we can safely migrate from enzyme, without much input in code reviewing.

Signed-off-by: Ansh Goyal <[email protected]>

packages/jaeger-ui/src/components/QualityMetrics/ScoreCard.test.js

Signed-off-by: Ansh Goyal <[email protected]>

anshgoyalevil · 2023-08-09T18:12:30Z

@yurishkuro I think, this PR can be merged safely.

The codecov tests are failing because the new helper function doesn't have tests written for it. Should I add it to codecov ignore list?

yurishkuro · 2023-08-10T00:34:02Z

packages/jaeger-ui/package.json

@@ -17,6 +17,8 @@
    "@babel/preset-typescript": "^7.21.0",
    "@svgr/babel-plugin-transform-svg-component": "^8.0.0",
    "@svgr/babel-preset": "^8.0.0",
+    "@testing-library/jest-dom": "^5.17.0",


why is this explicitly added to deps? It's not used in the code, looks like it's a transitive dep that will be included in the lock file

This would need to be installed when migrating from mount function (in Enzyme) to RTL, according to the official docs.

yurishkuro · 2023-08-10T00:36:08Z

packages/jaeger-ui/package.json

@@ -36,6 +38,8 @@
    "jest-environment-jsdom": "^29.5.0",
    "jest-junit": "^16.0.0",
    "less": "3.13.1",
+    "react-is": "^18.2.0",
+    "react-shallow-renderer": "^16.15.0",


is this used?

These are the dependencies for React Test Renderer, which is already installed inside jaeger-ui

yurishkuro · 2023-08-10T00:37:04Z

packages/jaeger-ui/src/utils/helpers/ReactShallowRenderer.js

@@ -0,0 +1,94 @@
+// Copyright (c) 2020 Uber Technologies, Inc.


Suggested change

// Copyright (c) 2020 Uber Technologies, Inc.

// Copyright (c) 2023 The Jaeger Authors

yurishkuro · 2023-08-10T00:37:48Z

packages/jaeger-ui/src/utils/helpers/ReactShallowRenderer.js

+import ShallowRenderer from 'react-test-renderer/shallow';
+import { isFragment, isLazy, isPortal, isMemo, isSuspense, isForwardRef } from 'react-is';
+
+const shallow = (element, options) => {


so I am not clear - why are we adding this code, isn't it what react-shallow-renderer supposed to solve?

react-test-renderer already comes with react-shallow-renderer and react-is, and we have this installed already.
https://www.npmjs.com/package/react-test-renderer?activeTab=dependencies

anshgoyalevil · 2023-08-10T01:50:31Z

Closing due to merge conflicts

## Which problem is this PR solving? - This PR migrates a test `ScoreCard.test.js` from Enzyme to React Testing Library - Enzyme is no longer supported in React 18 - Replacement for #1643 ## Description of the changes - It uses the custom shallow function to render the components - Code adopted from https://thesametech.com/snapshot-testing-in-rtl/, with author's permission: #1653 (comment) ## How was this change tested? - It was tested by running the command `yarn test -- -u` to re-generate snapshots of the changed tests - After that, `yarn test-ci ScoreCard.test.js --coverage` was used to check if all tests pass and coverage is 100% ## 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 --------- Signed-off-by: Ansh Goyal <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

anshgoyalevil added 4 commits August 8, 2023 09:29

migrate from enzyme to rtl

27572ac

Signed-off-by: Ansh Goyal <[email protected]>

change ut

2ed8d11

Signed-off-by: Ansh Goyal <[email protected]>

change ut

ca5e6ed

Signed-off-by: Ansh Goyal <[email protected]>

lint test

295d52c

Signed-off-by: Ansh Goyal <[email protected]>

anshgoyalevil requested review from everett980 and rubenvp8510 as code owners August 8, 2023 04:14

yurishkuro reviewed Aug 8, 2023

View reviewed changes

anshgoyalevil added 4 commits August 8, 2023 11:13

move to devDeps

227313c

Signed-off-by: Ansh Goyal <[email protected]>

modify ScoreCard.test

9142452

Signed-off-by: Ansh Goyal <[email protected]>

modify ScoreCard.test

0220fee

Signed-off-by: Ansh Goyal <[email protected]>

modify ScoreCard.test

cf2c175

Signed-off-by: Ansh Goyal <[email protected]>

add ReactShallowRenderer

b0b94de

Signed-off-by: Ansh Goyal <[email protected]>

anshgoyalevil added 2 commits August 9, 2023 09:13

fix lint error

36a9674

Signed-off-by: Ansh Goyal <[email protected]>

fix lint error

a982b6b

Signed-off-by: Ansh Goyal <[email protected]>

yurishkuro reviewed Aug 9, 2023

View reviewed changes

packages/jaeger-ui/src/components/QualityMetrics/ScoreCard.test.js Outdated Show resolved Hide resolved

modify shallow function to match enzyme usage

b2ee316

Signed-off-by: Ansh Goyal <[email protected]>

anshgoyalevil changed the title ~~[WIP] Migrate from Enzyme to React Testing Library~~ Init Migration from Enzyme to React Testing Library Aug 9, 2023

yurishkuro reviewed Aug 10, 2023

View reviewed changes

anshgoyalevil closed this Aug 10, 2023

yurishkuro mentioned this pull request Aug 10, 2023

Migrate ScoreCard.test.js from Enzyme to React Shallow Renderer #1653

Merged

3 tasks

anshgoyalevil mentioned this pull request Aug 13, 2023

Remove dependency on enzyme #1668

Open

anshgoyalevil deleted the dom branch August 16, 2023 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init Migration from Enzyme to React Testing Library #1643

Init Migration from Enzyme to React Testing Library #1643

anshgoyalevil commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

yurishkuro Aug 8, 2023

anshgoyalevil Aug 8, 2023

anshgoyalevil Aug 8, 2023

yurishkuro Aug 8, 2023

anshgoyalevil Aug 8, 2023

anshgoyalevil Aug 8, 2023

anshgoyalevil commented Aug 9, 2023

yurishkuro commented Aug 9, 2023

yurishkuro commented Aug 9, 2023

anshgoyalevil commented Aug 9, 2023

anshgoyalevil commented Aug 9, 2023

anshgoyalevil commented Aug 9, 2023

yurishkuro Aug 10, 2023

anshgoyalevil Aug 10, 2023

yurishkuro Aug 10, 2023

anshgoyalevil Aug 10, 2023 •

edited

Loading

yurishkuro Aug 10, 2023

yurishkuro Aug 10, 2023

anshgoyalevil Aug 10, 2023

anshgoyalevil commented Aug 10, 2023

		@@ -0,0 +1,94 @@
		// Copyright (c) 2020 Uber Technologies, Inc.

	// Copyright (c) 2020 Uber Technologies, Inc.
	// Copyright (c) 2023 The Jaeger Authors

Init Migration from Enzyme to React Testing Library #1643

Init Migration from Enzyme to React Testing Library #1643

Conversation

anshgoyalevil commented Aug 8, 2023 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil commented Aug 9, 2023

yurishkuro commented Aug 9, 2023

yurishkuro commented Aug 9, 2023

anshgoyalevil commented Aug 9, 2023

anshgoyalevil commented Aug 9, 2023

anshgoyalevil commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshgoyalevil commented Aug 10, 2023

anshgoyalevil commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

anshgoyalevil Aug 10, 2023 •

edited

Loading