From 0fcfaf9d7326fa7ad6051bd6d636eaa7f8ece741 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Wed, 8 Jan 2025 11:14:50 +0100 Subject: [PATCH 1/8] Small adjustments on the sort functionality (#837) * Filter by the absolute value of delta instead of the signed value * Make the sort direction at the first click 'desc' instead of 'asc' --- .../SubtestsResultsView.test.tsx | 66 +++++++++---------- .../SubtestsResultsView.test.tsx.snap | 38 +++++------ src/__tests__/utils/fixtures.ts | 4 +- .../SubtestsResults/SubtestsResultsTable.tsx | 4 +- src/components/CompareResults/TableHeader.tsx | 8 +-- src/hooks/useTableSort.ts | 2 +- 6 files changed, 62 insertions(+), 60 deletions(-) diff --git a/src/__tests__/CompareResults/SubtestsResultsView.test.tsx b/src/__tests__/CompareResults/SubtestsResultsView.test.tsx index a7f822bdc..33d25a3a8 100644 --- a/src/__tests__/CompareResults/SubtestsResultsView.test.tsx +++ b/src/__tests__/CompareResults/SubtestsResultsView.test.tsx @@ -209,7 +209,7 @@ describe('SubtestsResultsView Component Tests', () => { // Initial view (alphabetical ordered, even if "sort by subtests" isn't specified expect(summarizeVisibleRows()).toEqual([ 'dhtml.html: 1.14 %, Low', - 'improvement.html: 1.44 %, Low', + 'improvement.html: -1.44 %, Low', 'regression.html: 1.04 %, High', 'tablemutation.html: 0.98 %, Low', ]); @@ -219,65 +219,65 @@ describe('SubtestsResultsView Component Tests', () => { const deltaButton = screen.getByRole('button', { name: /Delta/ }); expect(deltaButton).toMatchSnapshot(); expect(window.location.search).not.toContain('sort='); - // Sort ascending + // Sort descending await user.click(deltaButton); expect(summarizeVisibleRows()).toEqual([ - 'tablemutation.html: 0.98 %, Low', - 'regression.html: 1.04 %, High', + 'improvement.html: -1.44 %, Low', 'dhtml.html: 1.14 %, Low', - 'improvement.html: 1.44 %, Low', + 'regression.html: 1.04 %, High', + 'tablemutation.html: 0.98 %, Low', ]); - // It should have the "ascending" SVG. + // It should have the "descending" SVG. expect(deltaButton).toMatchSnapshot(); // It should be persisted in the URL - expectParameterToHaveValue('sort', 'delta|asc'); + expectParameterToHaveValue('sort', 'delta|desc'); - // Sort descending + // Sort ascending await user.click(deltaButton); expect(summarizeVisibleRows()).toEqual([ - 'improvement.html: 1.44 %, Low', - 'dhtml.html: 1.14 %, Low', - 'regression.html: 1.04 %, High', 'tablemutation.html: 0.98 %, Low', + 'regression.html: 1.04 %, High', + 'dhtml.html: 1.14 %, Low', + 'improvement.html: -1.44 %, Low', ]); - // It should have the "descending" SVG. + // It should have the "ascending" SVG. expect(deltaButton).toMatchSnapshot(); // It should be persisted in the URL - expectParameterToHaveValue('sort', 'delta|desc'); + expectParameterToHaveValue('sort', 'delta|asc'); - // Sort by Confidence ascending + // Sort by Confidence descending const confidenceButton = screen.getByRole('button', { name: /Confidence.*sort/, }); await user.click(confidenceButton); expect(summarizeVisibleRows()).toEqual([ - 'tablemutation.html: 0.98 %, Low', - 'dhtml.html: 1.14 %, Low', - 'improvement.html: 1.44 %, Low', 'regression.html: 1.04 %, High', + 'improvement.html: -1.44 %, Low', + 'dhtml.html: 1.14 %, Low', + 'tablemutation.html: 0.98 %, Low', ]); // It should have the "no sort" SVG. expect(deltaButton).toMatchSnapshot(); - // It should have the "ascending" SVG. + // It should have the "descending" SVG. expect(confidenceButton).toMatchSnapshot(); // It should be persisted in the URL - expectParameterToHaveValue('sort', 'confidence|asc'); + expectParameterToHaveValue('sort', 'confidence|desc'); - // Sort by subtest name ascending + // Sort by subtest name descending const subtestsButton = screen.getByRole('button', { name: /Subtests/ }); await user.click(subtestsButton); expect(summarizeVisibleRows()).toEqual([ - 'dhtml.html: 1.14 %, Low', - 'improvement.html: 1.44 %, Low', - 'regression.html: 1.04 %, High', 'tablemutation.html: 0.98 %, Low', + 'regression.html: 1.04 %, High', + 'improvement.html: -1.44 %, Low', + 'dhtml.html: 1.14 %, Low', ]); // It should have the "no sort" SVG. expect(confidenceButton).toMatchSnapshot(); - // It should have the "ascending" SVG. + // It should have the "descending" SVG. expect(subtestsButton).toMatchSnapshot(); // It should be persisted in the URL - expectParameterToHaveValue('sort', 'subtests|asc'); + expectParameterToHaveValue('sort', 'subtests|desc'); // Clickince twice more should reset the URL. await user.click(subtestsButton); @@ -285,36 +285,36 @@ describe('SubtestsResultsView Component Tests', () => { expect(window.location.search).not.toContain('sort='); }); - it('initializes the sort from the URL at load time for an ascending sort', async () => { + it('initializes the sort from the URL at load time for a ascending sort', async () => { await setupForSorting({ extraParameters: 'sort=delta|asc' }); await screen.findByText('dhtml.html'); expect(summarizeVisibleRows()).toEqual([ 'tablemutation.html: 0.98 %, Low', 'regression.html: 1.04 %, High', 'dhtml.html: 1.14 %, Low', - 'improvement.html: 1.44 %, Low', + 'improvement.html: -1.44 %, Low', ]); // It should have the "ascending" SVG. expect(screen.getByRole('button', { name: /Delta/ })).toMatchSnapshot(); }); - it('initializes the sort from the URL at load time for an implicit ascending sort', async () => { + it('initializes the sort from the URL at load time for an implicit descending sort', async () => { await setupForSorting({ extraParameters: 'sort=delta' }); await screen.findByText('dhtml.html'); expect(summarizeVisibleRows()).toEqual([ - 'tablemutation.html: 0.98 %, Low', - 'regression.html: 1.04 %, High', + 'improvement.html: -1.44 %, Low', 'dhtml.html: 1.14 %, Low', - 'improvement.html: 1.44 %, Low', + 'regression.html: 1.04 %, High', + 'tablemutation.html: 0.98 %, Low', ]); - // It should have the "ascending" SVG. + // It should have the "descending" SVG. expect(screen.getByRole('button', { name: /Delta/ })).toMatchSnapshot(); }); it('initializes the sort from the URL at load time for a descending sort', async () => { await setupForSorting({ extraParameters: 'sort=delta|desc' }); expect(summarizeVisibleRows()).toEqual([ - 'improvement.html: 1.44 %, Low', + 'improvement.html: -1.44 %, Low', 'dhtml.html: 1.14 %, Low', 'regression.html: 1.04 %, High', 'tablemutation.html: 0.98 %, Low', diff --git a/src/__tests__/CompareResults/__snapshots__/SubtestsResultsView.test.tsx.snap b/src/__tests__/CompareResults/__snapshots__/SubtestsResultsView.test.tsx.snap index 1da670691..1c18d8bf1 100644 --- a/src/__tests__/CompareResults/__snapshots__/SubtestsResultsView.test.tsx.snap +++ b/src/__tests__/CompareResults/__snapshots__/SubtestsResultsView.test.tsx.snap @@ -818,7 +818,7 @@ exports[`SubtestsResultsView Component Tests should render the subtests results role="cell" > - 1.44 + -1.44 % @@ -1298,7 +1298,7 @@ exports[`SubtestsResultsView Component Tests table sorting can sort the table an type="button" > - Sorted by Delta in ascending order + Sorted by Delta in descending order Delta @@ -1335,7 +1335,7 @@ exports[`SubtestsResultsView Component Tests table sorting can sort the table an type="button" > - Sorted by Delta in descending order + Sorted by Delta in ascending order Delta @@ -1425,7 +1425,7 @@ exports[`SubtestsResultsView Component Tests table sorting can sort the table an type="button" > - Sorted by Confidence in ascending order + Sorted by Confidence in descending order - Sorted by Subtests in ascending order + Sorted by Subtests in descending order Subtests @@ -1526,7 +1526,7 @@ exports[`SubtestsResultsView Component Tests table sorting can sort the table an `; -exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for a descending sort 1`] = ` +exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for a ascending sort 1`] = ` `; -exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for an ascending sort 1`] = ` +exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for a descending sort 1`] = ` `; -exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for an implicit ascending sort 1`] = ` +exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for an implicit descending sort 1`] = ` +
+
+ + +
+
- OS X 10.15 + Linux 18.04
@@ -515,7 +567,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 704.84 + 776.97 ms @@ -531,7 +583,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 712.44 + 791.34 ms @@ -540,20 +592,20 @@ exports[`Results View The table should match snapshot and other elements should role="cell" >
- Improvement + Regression
- 1.08 + 1.85 %
@@ -571,16 +623,16 @@ exports[`Results View The table should match snapshot and other elements should > - Low + Medium @@ -738,7 +791,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 776.97 + 704.84 ms @@ -754,7 +807,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 791.34 + 712.44 ms @@ -763,20 +816,20 @@ exports[`Results View The table should match snapshot and other elements should role="cell" >
- Regression + Improvement
- 1.85 + 1.08 %
@@ -794,16 +847,16 @@ exports[`Results View The table should match snapshot and other elements should > - Medium + Low
@@ -1218,7 +1260,18 @@ exports[`Results View The table should match snapshot and other elements should class="confidence cell" role="cell" > - - + + High
- Delta -
-
+
+
+ + +
+
- OS X 10.15 + Linux 18.04
@@ -1239,7 +1291,7 @@ exports[`Results Table Should match snapshot 1`] = ` role="cell" > - 704.84 + 776.97 ms @@ -1255,7 +1307,7 @@ exports[`Results Table Should match snapshot 1`] = ` role="cell" > - 712.44 + 791.34 ms
@@ -1264,20 +1316,20 @@ exports[`Results Table Should match snapshot 1`] = ` role="cell" >
- Improvement + Regression
- 1.08 + 1.85 %
@@ -1295,16 +1347,16 @@ exports[`Results Table Should match snapshot 1`] = ` > - Low + Medium
@@ -1462,7 +1515,7 @@ exports[`Results Table Should match snapshot 1`] = ` role="cell" > - 776.97 + 704.84 ms @@ -1478,7 +1531,7 @@ exports[`Results Table Should match snapshot 1`] = ` role="cell" > - 791.34 + 712.44 ms @@ -1487,20 +1540,20 @@ exports[`Results Table Should match snapshot 1`] = ` role="cell" >
- Regression + Improvement
- 1.85 + 1.08 %
@@ -1518,16 +1571,16 @@ exports[`Results Table Should match snapshot 1`] = ` > - Medium + Low
@@ -2135,6 +2188,253 @@ exports[`Results Table Should match snapshot 1`] = ` `; +exports[`Results Table can load the sort parameters from the URL for a descending sort 1`] = ` + +`; + +exports[`Results Table can load the sort parameters from the URL for an ascending sort 1`] = ` + +`; + +exports[`Results Table can sort the table and persist the sort parameters to the URL 1`] = ` + +`; + +exports[`Results Table can sort the table and persist the sort parameters to the URL 2`] = ` + +`; + +exports[`Results Table can sort the table and persist the sort parameters to the URL 3`] = ` + +`; + +exports[`Results Table can sort the table and persist the sort parameters to the URL 4`] = ` + +`; + +exports[`Results Table can sort the table and persist the sort parameters to the URL 5`] = ` + +`; + exports[`Results Table should render different blocks when rendering several revisions 1`] = `
- Delta -
-
+
+
+ + +
+
- OS X 10.15 + Linux 18.04
@@ -1325,7 +1377,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 704.84 + 776.97 ms @@ -1341,7 +1393,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 712.44 + 791.34 ms @@ -1350,20 +1402,20 @@ exports[`Results View The table should match snapshot and other elements should role="cell" >
- Improvement + Regression
- 1.08 + 1.85 %
@@ -1381,16 +1433,16 @@ exports[`Results View The table should match snapshot and other elements should > - Low + Medium
@@ -1548,7 +1601,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 776.97 + 704.84 ms @@ -1564,7 +1617,7 @@ exports[`Results View The table should match snapshot and other elements should role="cell" > - 791.34 + 712.44 ms @@ -1573,20 +1626,20 @@ exports[`Results View The table should match snapshot and other elements should role="cell" >
- Regression + Improvement
- 1.85 + 1.08 %
@@ -1604,16 +1657,16 @@ exports[`Results View The table should match snapshot and other elements should > - Medium + Low
@@ -2028,7 +2070,18 @@ exports[`Results View The table should match snapshot and other elements should class="confidence cell" role="cell" > - - + + High
`; -exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for a ascending sort 1`] = ` +exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for a descending sort 1`] = ` `; -exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for a descending sort 1`] = ` +exports[`SubtestsResultsView Component Tests table sorting initializes the sort from the URL at load time for an ascending sort 1`] = `
- New + + New +
- -
-
-
-
+
+ +
- Confidence -
- ( - 4 - ) -
- + + +
+ + - - - -
+ +
+
- Total Runs + + Total Runs +
a11yr diff --git a/src/__tests__/CompareResults/__snapshots__/ResultsTable.test.tsx.snap b/src/__tests__/CompareResults/__snapshots__/ResultsTable.test.tsx.snap index cc47b9377..d3250a182 100644 --- a/src/__tests__/CompareResults/__snapshots__/ResultsTable.test.tsx.snap +++ b/src/__tests__/CompareResults/__snapshots__/ResultsTable.test.tsx.snap @@ -1017,7 +1017,13 @@ exports[`Results Table Should match snapshot 1`] = ` class="cell base-header" role="columnheader" > - Base + + Base +
- New + + New +
- -
-
-
-
+
+ +
- Confidence -
- ( - 4 - ) -
- + + +
+ + - - - -
+ +
+
- Total Runs + + Total Runs +
a11yr @@ -1943,6 +1974,7 @@ exports[`Results Table Should match snapshot 1`] = ` class="MuiTypography-root MuiTypography-inherit MuiLink-root MuiLink-underlineHover css-g8z9fn-MuiTypography-root-MuiLink-root" href="https://firefox-source-docs.mozilla.org/testing/perfdocs/talos.html#a11yr" target="_blank" + title="Link to suite documentation " > a11yr @@ -2452,6 +2484,7 @@ exports[`Results Table should render different blocks when rendering several rev class="MuiTypography-root MuiTypography-inherit MuiLink-root MuiLink-underlineHover css-g8z9fn-MuiTypography-root-MuiLink-root" href="https://firefox-source-docs.mozilla.org/testing/perfdocs/talos.html#a11yr" target="_blank" + title="Link to suite documentation " > a11yr diff --git a/src/__tests__/CompareResults/__snapshots__/ResultsView.test.tsx.snap b/src/__tests__/CompareResults/__snapshots__/ResultsView.test.tsx.snap index 6638f3f49..5de21cb2e 100644 --- a/src/__tests__/CompareResults/__snapshots__/ResultsView.test.tsx.snap +++ b/src/__tests__/CompareResults/__snapshots__/ResultsView.test.tsx.snap @@ -1103,7 +1103,13 @@ exports[`Results View The table should match snapshot and other elements should class="cell base-header" role="columnheader" > - Base + + Base +
- New + + New +
- -
-
-
-
+
+ +
- Confidence -
- ( - 4 - ) -
- + + +
+ + - - - -
+ +
+
- Total Runs + + Total Runs +
a11yr diff --git a/src/components/CompareResults/ResultsTable.tsx b/src/components/CompareResults/ResultsTable.tsx index 5b84d66f3..5198f6a57 100644 --- a/src/components/CompareResults/ResultsTable.tsx +++ b/src/components/CompareResults/ResultsTable.tsx @@ -42,6 +42,7 @@ const columnsConfiguration: CompareResultsTableConfig = [ name: 'Base', key: 'base', gridWidth: '1fr', + tooltip: 'A summary of all values from Base runs using a mean.', }, { key: 'comparisonSign', @@ -51,8 +52,8 @@ const columnsConfiguration: CompareResultsTableConfig = [ { name: 'New', key: 'new', - gridWidth: '1fr', + tooltip: 'A summary of all values from New runs using a mean.', }, { name: 'Status', @@ -84,12 +85,15 @@ const columnsConfiguration: CompareResultsTableConfig = [ Math.abs(resultA.delta_percentage) - Math.abs(resultB.delta_percentage) ); }, + tooltip: 'The percentage difference between the Base and New values', }, { name: 'Confidence', filter: true, key: 'confidence', gridWidth: '1.5fr', + tooltip: + "Calculated using a Student's T-test comparison. Low is anything under a T value of 3, Medium is between 3 and 5, and High is anything higher than 5.", possibleValues: [ { label: 'No value', key: 'none' }, { label: 'Low', key: 'low' }, @@ -123,8 +127,8 @@ const columnsConfiguration: CompareResultsTableConfig = [ { name: 'Total Runs', key: 'runs', - gridWidth: '1fr', + tooltip: 'The total number of tasks/jobs that ran for this metric.', }, // We use the real pixel value for the buttons, so that everything is better aligned. { key: 'buttons', gridWidth: `calc(3.5 * 34px)` }, // 2 or 3 buttons, so at least 3*34px, but give more so that it can "breathe" diff --git a/src/components/CompareResults/TableHeader.tsx b/src/components/CompareResults/TableHeader.tsx index d80118094..65ff00000 100644 --- a/src/components/CompareResults/TableHeader.tsx +++ b/src/components/CompareResults/TableHeader.tsx @@ -1,7 +1,9 @@ +import { ReactNode } from 'react'; + import KeyboardArrowDownIcon from '@mui/icons-material/KeyboardArrowDown'; import StraightIcon from '@mui/icons-material/Straight'; import SwapVert from '@mui/icons-material/SwapVert'; -import { ListItemIcon, ListItemText } from '@mui/material'; +import { ListItemIcon, ListItemText, Tooltip, Box } from '@mui/material'; import Button from '@mui/material/Button'; import ButtonGroup from '@mui/material/ButtonGroup'; import Checkbox from '@mui/material/Checkbox'; @@ -9,7 +11,6 @@ import Divider from '@mui/material/Divider'; import Menu from '@mui/material/Menu'; import MenuItem from '@mui/material/MenuItem'; import { SxProps, Theme } from '@mui/material/styles'; -import { Box } from '@mui/system'; import { usePopupState, bindTrigger, @@ -311,8 +312,10 @@ function TableHeader({ }; function renderColumnHeader(header: CompareResultsTableColumn) { + let content: ReactNode = header.name; + if ('sortFunction' in header && 'filter' in header) { - return ( + content = ( ); - } - - if ('sortFunction' in header) { - return ( + } else if ('sortFunction' in header) { + content = ( ); - } - - if ('filter' in header) { - return ( + } else if ('filter' in header) { + content = ( + {/* Box is used because tooltip expects a single valid element but sometimes content is a string */} + + {content} + + + ); + } + + return content; } return ( diff --git a/src/components/CompareResults/TestHeader.tsx b/src/components/CompareResults/TestHeader.tsx index f60b3b39d..552deafc4 100644 --- a/src/components/CompareResults/TestHeader.tsx +++ b/src/components/CompareResults/TestHeader.tsx @@ -2,6 +2,7 @@ import { Link } from '@mui/material'; import { style } from 'typestyle'; import LinkToRevision from './LinkToRevision'; +import { Strings } from '../../resources/Strings'; import { Colors, Spacing } from '../../styles'; import type { CompareResultsItem } from '../../types/state'; import { getDocsURL } from '../../utils/helpers'; @@ -71,6 +72,7 @@ function createTitle( isLinkSupported: boolean, ) { const isTestUnavailable = result.test === '' || result.suite === result.test; + const suiteLink = Strings.components.revisionRow.title.suiteLink; if (isLinkSupported) { return ( <> @@ -79,6 +81,7 @@ function createTitle( underline='hover' target='_blank' href={docsURL} + title={suiteLink} > {result.suite} diff --git a/src/resources/Strings.tsx b/src/resources/Strings.tsx index 20ec274f6..aa26cf71c 100644 --- a/src/resources/Strings.tsx +++ b/src/resources/Strings.tsx @@ -101,6 +101,7 @@ export const Strings = { jobLink: 'open treeherder view for', retriggerJobs: 'retrigger jobs', compareViewLink: 'open perfherder compare view for', + suiteLink: 'Link to suite documentation ', }, }, expandableRow: { diff --git a/src/types/types.ts b/src/types/types.ts index bf49af347..0c2cf1675 100644 --- a/src/types/types.ts +++ b/src/types/types.ts @@ -10,6 +10,7 @@ export interface BasicColumn { key: string; // Used in the grid CSS property to configure the width of the column. gridWidth: string; + tooltip?: string; } // This interface is used for a column that can be filtered. From a2917651017d6ef72c9935bf1c38acdb9127d3a4 Mon Sep 17 00:00:00 2001 From: Chineta Adinnu <52791481+Netacci@users.noreply.github.com> Date: Wed, 15 Jan 2025 06:18:13 -0800 Subject: [PATCH 8/8] Add aria expanded to revision row expand button (#826) --- .../OverTimeResultsView.test.tsx.snap | 8 ++++++ .../__snapshots__/ResultsTable.test.tsx.snap | 12 +++++++++ .../__snapshots__/ResultsView.test.tsx.snap | 26 ++++++++++++++----- .../SubtestsResultsView.test.tsx.snap | 18 ++++++++++++- .../SubtestsRevisionRow.test.tsx.snap | 2 ++ src/components/CompareResults/RevisionRow.tsx | 7 +++-- .../CompareResults/RevisionRowExpandable.tsx | 12 ++++++--- .../SubtestsResults/SubtestsRevisionRow.tsx | 7 +++-- 8 files changed, 78 insertions(+), 14 deletions(-) diff --git a/src/__tests__/CompareResults/__snapshots__/OverTimeResultsView.test.tsx.snap b/src/__tests__/CompareResults/__snapshots__/OverTimeResultsView.test.tsx.snap index 25e2288fc..5670f61f2 100644 --- a/src/__tests__/CompareResults/__snapshots__/OverTimeResultsView.test.tsx.snap +++ b/src/__tests__/CompareResults/__snapshots__/OverTimeResultsView.test.tsx.snap @@ -765,6 +765,8 @@ exports[`Results View The table should match snapshot and other elements should data-testid="expand-revision-button" >
- + `; exports[`Results View Should display Base, New and Common graphs with replicates 1`] = ` -
- + `; exports[`Results View Should display Base, New and Common graphs with tooltips 1`] = ` -
- + `; exports[`Results View The table should match snapshot and other elements should be present in the page 1`] = ` @@ -1575,6 +1581,8 @@ exports[`Results View The table should match snapshot and other elements should data-testid="expand-revision-button" >