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

Add Tests for SubtestsResultsView Component #799

Merged
merged 11 commits into from
Dec 3, 2024

Conversation

sumairq
Copy link
Collaborator

@sumairq sumairq commented Nov 12, 2024

This PR introduces a set of tests for the SubtestsResultsView component to validate its rendering and key functionalities. The main areas tested include:

  • Rendering and Snapshot: Verifies that the component renders correctly and matches the expected snapshot.
  • Main Content Display: Confirms that the main content in the view is present.
  • Breadcrumbs Navigation: Ensures breadcrumbs are displayed for easier navigation within the component.
  • Subtests Results Table: Checks that the subtests results table is rendered in the view.
  • Retrigger Button Functionality: Tests the retrigger functionality, confirming the correct alerts and authorization code requests are triggered.
  • Download JSON Button: Validates that clicking the "Download JSON" button correctly triggers the creation of a blob URL.

Since the SubtestsOverTimeResultsView is identical to SubtestsResultsView only snapshot testing has been done for SubtestsOverTimeResultsView

Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for mozilla-perfcompare ready!

Name Link
🔨 Latest commit 5b3ef94
🔍 Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/674f26d8e9a6940008f20826
😎 Deploy Preview https://deploy-preview-799--mozilla-perfcompare.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sumairq sumairq changed the title Add Tests for SubtestsResultsView Component Add Tests for SubtestsResultsView Component [WIP] 🚧 Nov 12, 2024
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.65%. Comparing base (b059f94) to head (5b3ef94).
Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@sumairq sumairq changed the title Add Tests for SubtestsResultsView Component [WIP] 🚧 Add Tests for SubtestsResultsView Component Nov 19, 2024
Copy link
Contributor

@julienw julienw left a 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!

Comment on lines 93 to 75
expect(homeBreadcrumb).toBeInTheDocument();
expect(resultsBreadcrumb).toBeInTheDocument();
Copy link
Contributor

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.

Copy link
Contributor

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'
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still there

@sumairq sumairq force-pushed the subtests-tests branch 2 times, most recently from 1464030 to 6fa8a05 Compare November 27, 2024 16:06
Copy link
Contributor

@julienw julienw left a 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',
Copy link
Contributor

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 ?

Comment on lines 22 to 23
Component,
componentProps = {},
Copy link
Contributor

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'
Copy link
Contributor

Choose a reason for hiding this comment

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

It's still there

Comment on lines 93 to 75
expect(homeBreadcrumb).toBeInTheDocument();
expect(resultsBreadcrumb).toBeInTheDocument();
Copy link
Contributor

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?

@sumairq sumairq requested a review from julienw December 3, 2024 15:25
Copy link
Contributor

@julienw julienw left a 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!

@julienw julienw enabled auto-merge (squash) December 3, 2024 15:30
auto-merge was automatically disabled December 3, 2024 15:42

Head branch was pushed to by a user without write access

@julienw julienw merged commit 3af866d into mozilla:main Dec 3, 2024
10 checks passed
@julienw julienw mentioned this pull request Dec 6, 2024
julienw added a commit that referenced this pull request Dec 6, 2024
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)
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.

2 participants