-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add Tests for SubtestsResultsView Component #799
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #799 +/- ##
==========================================
+ Coverage 91.26% 92.65% +1.38%
==========================================
Files 92 92
Lines 2450 2450
Branches 463 463
==========================================
+ Hits 2236 2270 +34
+ Misses 191 157 -34
Partials 23 23 ☔ View full report in Codecov by Sentry. |
0be03b3
to
1287c45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Sumair,
First thanks a ton for these new tests, this will be extremely useful!
I left a few comments to make them more solid and faster to run as well.
I noticed you were using the test ids a lot, but in reality they should be used as a last ressort. See the "Priority" paragraph at https://testing-library.com/docs/queries/about/#priority for more information about that.
Otherwise after these small changes it should be ready to go!
expect(homeBreadcrumb).toBeInTheDocument(); | ||
expect(resultsBreadcrumb).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move these expectations to just before the snapshot check of the first test.
Also you'll be able to use expect(screen.getByText(...))
there because we already awaited for something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You copied them above but you didn't remove this test block, am I missing something?
@@ -26,7 +26,10 @@ function SubtestsResultsView(props: SubtestsResultsViewProps) { | |||
}, [title]); | |||
|
|||
return ( | |||
<div className={styles.container}> | |||
<div | |||
data-testid='beta-version-subtests-compare-over-time-results' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above, this is probably not necessary, you can revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still there
1464030
to
6fa8a05
Compare
139043e
to
94e22f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, but I think you forgot to handle the review comments fully :) can you have another look please?
setup({ | ||
Component: SubtestsResultsMain, | ||
componentProps: { view: 'subtests-results' }, | ||
route: '/subtestsCompareWithBase', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't that be /subtests-compare-results ?
Component, | ||
componentProps = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be easier to pass the react element directly:
const setup = ({
element,
...
}: { element: React.ReactElement, ... )) {
...
renderWithRouter(element, {
route,
search,
loader,
});
}
// and then
setup({ element: <SubtestsResultsView title=""/>, ... });
@@ -26,7 +26,10 @@ function SubtestsResultsView(props: SubtestsResultsViewProps) { | |||
}, [title]); | |||
|
|||
return ( | |||
<div className={styles.container}> | |||
<div | |||
data-testid='beta-version-subtests-compare-over-time-results' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still there
expect(homeBreadcrumb).toBeInTheDocument(); | ||
expect(resultsBreadcrumb).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You copied them above but you didn't remove this test block, am I missing something?
a6972b2
to
20d5257
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me now, we can merge once the snapshots are updated and the tests are green!
Head branch was pushed to by a user without write access
Highlights: [Julien Wajsberg] Add the 'No value' value to the Confidence column also in the subtest… (#807) [Julien Wajsberg] Persist column filters as URL parameters (#810) [beatrice-acasandrei] PCF-501 Search by email should be using substrings (#805) [esanuandra] PCF-549 Add the name of the device used instead of just "Android" (#803) [esanuandra] PCF-323 Expanded row doesn't match the figma design (#800) [Carla Severe] Added the checked item count to the filter header name (#806) [esanuandra] PCF-524 Display the mean instead of the median in the results table rows + update the column title + display median in the expanded row as well (#798) Internal changes: [Julien Wajsberg] Extract the table filters into a common hook (#808) [Julien Wajsberg] Use keys for the column filters (#809) [Carla Severe] Update perfcompare doc link in ReadME (#811) [Sumair Qaisar] Add Tests for SubtestsResultsView Component (#799)
This PR introduces a set of tests for the
SubtestsResultsView
component to validate its rendering and key functionalities. The main areas tested include:Since the
SubtestsOverTimeResultsView
is identical toSubtestsResultsView
only snapshot testing has been done forSubtestsOverTimeResultsView