Migrate TraceDiffs/Header from enzyme to RTL #1962

EshaanAgg · 2023-11-10T05:25:50Z

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

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
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

EshaanAgg · 2023-11-10T05:34:24Z

@yurishkuro Can you please re-run the testing workflow and review this PR?
The failing test in the run seems to be a flaky one, as it gives the following warnings on running locally:

yurishkuro · 2023-11-10T17:22:25Z

I don't think it's flaky, I am seeing this error:

FAIL  src/components/TracePage/TracePageHeader/TracePageHeader.test.js
  ● Test suite failed to run

    TypeError: Cannot read properties of undefined (reading 'spanID')

      49 | /* this simulates the hierarchy created by CHILD_OF tags */
      50 | function attachReferences(spans, depth, spansPerLevel) {
    > 51 |   let levels = [[spans[0].spanID]];
         |                           ^
      52 |
      53 |   const duplicateLevelFilter = currentLevels => span =>
      54 |     !currentLevels.find(level => level.indexOf(span.spanID) >= 0);

      at spanID (src/demo/trace-generators.js:51:27)
      at Chance.attachReferences [as trace] (src/demo/trace-generators.js:112:13)
      at trace (src/components/TracePage/TracePageHeader/TracePageHeader.test.js:29:51)
      at Object.describe (src/components/TracePage/TracePageHeader/TracePageHeader.test.js:28:1)

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js

EshaanAgg · 2023-11-10T17:25:32Z

@yurishkuro But I have only made changes to the tests in TraceHeader component, so how can it affect TracePageHeader.test.js?

codecov · 2023-11-10T17:36:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
...mponents/TraceDiff/TraceDiffHeader/TraceHeader.tsx	`100.00% <100.00%> (ø)`
...kages/jaeger-ui/src/components/TracePage/index.tsx	`99.43% <100.00%> (+<0.01%)`	⬆️
...ackages/jaeger-ui/src/constants/default-config.tsx	`75.00% <ø> (ø)`
packages/jaeger-ui/src/utils/config/get-config.tsx	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!

## 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 <96648934+EshaanAgg@users.noreply.github.com>

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js

yurishkuro · 2023-11-11T19:21:21Z

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js

-      expect(shallow(<EmptyAttrs />)).toMatchSnapshot();
+      render(<EmptyAttrs />);
+      expect(screen.getAllByTestId('TraceDiffHeader--traceAttr').length).toBe(1);
+      expect(screen.getByTestId('TraceDiffHeader--traceAttr').textContent.trim()).toBe('');


looking for textContext is a bit weird - what if it is legitimately empty? Could we not assert that getByTestId returns an empty list?

In the current JSX for EmptyAttr, we return the following:

export function EmptyAttrs() { return ( <ul className="TraceDiffHeader--traceAttributes"> <li className="TraceDiffHeader--traceAttr" data-testid="TraceDiffHeader--traceAttr">   </li> </ul> ); }

which is a list with one item, and blank text content. I feel that the current implementation is the best way to test it. What you're suggesting would be more appropriate if we return just an empty ul with no li.

I think I have handled the same in a more graceful manner in the latest changes. Please let me know what you think about it.

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js

yurishkuro · 2023-11-11T19:24:17Z

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js

-      expect(shallow(<Attrs />)).toMatchSnapshot();
+      render(<Attrs />);
+
+      // Test that the default values are correctly


interesting - SHOULD they actually be rendered like this? What is the point of "default values" provided by the UI when it displays externally loaded data?

It probably shouldn't be. I feel that the same arises from this props definition:

type Props = { duration: number | TNil; error?: ApiError; startTime: number | TNil; state: FetchedState | TNil; traceID: string | TNil; traceName: string | TNil; totalSpans: number | TNil; }; type AttrsProps = { startTime: number | TNil; duration: number | TNil; totalSpans: number | TNil; };

The props should use conditional type which require startTime, duration and totalSpans to be present if the status is DONE, and otherwise they should be undefined. And thus the TNil should be removed from the AttrsProps IMO. Should I work on applying these fixes?

I agree, the types are unnecessarily lax, it should be either you have an error or pending state, or you have everything else non-optional. But I think fixing this is a lower priority compared to fixing other enzyme tests

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>

yurishkuro · 2023-11-12T03:51:37Z

Thanks!

EshaanAgg requested a review from a team as a code owner November 10, 2023 05:25

EshaanAgg requested review from joe-elliott and removed request for a team November 10, 2023 05:25

EshaanAgg force-pushed the mig3 branch from 6b2dc84 to b58a405 Compare November 10, 2023 05:26

yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Nov 10, 2023

yurishkuro reviewed Nov 10, 2023

View reviewed changes

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js Outdated Show resolved Hide resolved

EshaanAgg added 3 commits November 11, 2023 15:57

refac: migrate TraceHeader from enzyme to RTL

a3abd00

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>

refac: remove the nesting of TraceHeader

8446e73

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>

EshaanAgg force-pushed the mig3 branch from ae63f77 to 8446e73 Compare November 11, 2023 10:28

Merge branch 'main' into mig3

0b0b906

EshaanAgg requested a review from yurishkuro November 11, 2023 10:29

yurishkuro reviewed Nov 11, 2023

View reviewed changes

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 11, 2023

View reviewed changes

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js Show resolved Hide resolved

yurishkuro reviewed Nov 11, 2023

View reviewed changes

packages/jaeger-ui/src/components/TraceDiff/TraceDiffHeader/TraceHeader.test.js Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 11, 2023

View reviewed changes

EshaanAgg added 2 commits November 12, 2023 01:35

fix: Add explicit checks for traceAttr in tests

0ad9de7

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>

fix: update tests according to review

b816a1f

Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>

yurishkuro approved these changes Nov 12, 2023

View reviewed changes

Merge branch 'main' into mig3

ff4a374

yurishkuro changed the title ~~refac: migrate TraceHeader from enzyme to RTL~~ Migrate TraceDiffs/Header from enzyme to RTL Nov 12, 2023

yurishkuro enabled auto-merge (squash) November 12, 2023 03:51

yurishkuro merged commit 8b0f82a into jaegertracing:main Nov 12, 2023
10 checks passed

EshaanAgg deleted the mig3 branch December 24, 2023 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate TraceDiffs/Header from enzyme to RTL #1962

Migrate TraceDiffs/Header from enzyme to RTL #1962

EshaanAgg commented Nov 10, 2023

EshaanAgg commented Nov 10, 2023

yurishkuro commented Nov 10, 2023

EshaanAgg commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

yurishkuro Nov 11, 2023

EshaanAgg Nov 11, 2023

EshaanAgg Nov 12, 2023

yurishkuro Nov 11, 2023

EshaanAgg Nov 11, 2023

yurishkuro Nov 12, 2023

yurishkuro commented Nov 12, 2023

Migrate TraceDiffs/Header from enzyme to RTL #1962

Migrate TraceDiffs/Header from enzyme to RTL #1962

Conversation

EshaanAgg commented Nov 10, 2023

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

EshaanAgg commented Nov 10, 2023

yurishkuro commented Nov 10, 2023

EshaanAgg commented Nov 10, 2023 • edited Loading

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

yurishkuro Nov 11, 2023

Choose a reason for hiding this comment

EshaanAgg Nov 11, 2023

Choose a reason for hiding this comment

EshaanAgg Nov 12, 2023

Choose a reason for hiding this comment

yurishkuro Nov 11, 2023

Choose a reason for hiding this comment

EshaanAgg Nov 11, 2023

Choose a reason for hiding this comment

yurishkuro Nov 12, 2023

Choose a reason for hiding this comment

yurishkuro commented Nov 12, 2023

EshaanAgg commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading