Skip to content
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

[tests] Remove enzyme from QualityMetrics tests #1684

Merged
merged 10 commits into from
Aug 16, 2023

Conversation

Freedisch
Copy link
Contributor

@Freedisch Freedisch commented Aug 15, 2023

Which problem is this PR solving?

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

Signed-off-by: Freedisch <freeproduc@gmail.com>
@Freedisch
Copy link
Contributor Author

I Will keep migrating other files

@anshgoyalevil
Copy link
Member

@Freedisch Are tests working fine locally? I think you might haven't generated the snapshots.

@Freedisch
Copy link
Contributor Author

@anshgoyalevil, I just noticed, I will update it with the shallow func u created, previously

Freedisch added 2 commits August 15, 2023 15:13
Signed-off-by: Freedisch <freeproduc@gamil.com>
Signed-off-by: Freedisch <freeproduc@gamil.com>
@Freedisch
Copy link
Contributor Author

Mirgation to RLT with QualityMetrics/BannerText.test.js
QualityMetrics/CountCard.test.js pass test

There is an expected behavior with snapshots with QualityMetrics/MetricCard.test.js and the shallow func does solve it. I will keep looking into that.


import CountCard from './CountCard';
import shallow from '../../utils/ReactShallowRenderer.test';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the last two tests be modified to assert specific things on the output instead of using shallow? This seems like a simply component to test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm on it

Freedisch added 2 commits August 16, 2023 01:26
Signed-off-by: Freedisch <freeproduc@gamil.com>
Signed-off-by: Freedisch <freeproduc@gamil.com>
@Freedisch
Copy link
Contributor Author

@yurishkuro, I have updated both tests and currently, they no longer snapshots

const { getByText } = render(<CountCard count={count} title={title} examples={examples} />);

const countElement = getByText('108');
const titleElement = getByText('Test Title');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplify

Suggested change
const titleElement = getByText('Test Title');
expect(getByText(title)).toBeInTheDocument();

this makes the test easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

@yurishkuro
Copy link
Member

@anshgoyalevil do you mind reviewing this?

@anshgoyalevil
Copy link
Member

@yurishkuro Sure. Would be happy to review 🚀

});

it('renders header when props.bannerText is a string', () => {
expect(shallow(<BannerText bannerText="foo text" />)).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since toMatchSnapshot() is no longer used, the snapshots created for this test needs to be purged.
You can use the command yarn test --updateSnapshot for that.

}}
/>
)
).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snapshots need to be purged for this too. Otherwise, the tests would pass, but CI will fail.

});

it('renders header when props.bannerText is a string', () => {
expect(shallow(<BannerText bannerText="foo text" />)).toMatchSnapshot();
const { getByText } = render(<BannerText bannerText="foo text" />);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use most of these functions using the screen object from @testing-library/react.
Example,

render(<BannerText bannerText="foo text" />);
expect(screen.getByText('foo text')).toBeInTheDocument();

Same goes for other such usages as well

Signed-off-by: Freedisch <freeproduc@gmail.com>
Signed-off-by: Freedisch <freeproduc@gmail.com>
@Freedisch
Copy link
Contributor Author

@anshgoyalevil, updated changes

render(<CountCard count={count} title={title} />);

expect(screen.getByText('108')).toBeInTheDocument();
expect(screen.getByText('Test Title')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect(screen.getByText('Test Title')).toBeInTheDocument();
expect(screen.getByText(title)).toBeInTheDocument();

Comment on lines 42 to 44
expect(screen.getByText('108')).toBeInTheDocument();
expect(screen.getByText('Test Title')).toBeInTheDocument();
expect(screen.getByText('Examples')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the same constants, not literal strings

Freedisch and others added 2 commits August 16, 2023 20:32
Signed-off-by: Freedisch <freeproduc@gmail.com>
@yurishkuro yurishkuro changed the title migrating from Enzym to React Testing Library [tests] Remove enzyme from QualityMetrics tests Aug 16, 2023
@yurishkuro
Copy link
Member

Note: this description is inaccurate, this issue does not resolve the issue completely, it's a partial solution.

This PR resolve #1668 on my migrating the existing Enzym library to React Testing Libary

I am changing it to "part of ..."

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (1a01faa) 96.01% compared to head (f3be9d9) 96.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1684   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files         241      241           
  Lines        7560     7560           
  Branches     1985     1985           
=======================================
  Hits         7259     7259           
  Misses        301      301           

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

@yurishkuro yurishkuro merged commit 602cfe3 into jaegertracing:main Aug 16, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants