-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(Codecov): add test analytics table #91832
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
Changes from all commits
3b09875
1a67f0e
e2654ca
978f32f
7cf6a98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import type {ValidSort} from 'sentry/views/codecov/tests/testAnalyticsTable/testAnalyticsTable'; | ||
|
||
export const DEFAULT_SORT: ValidSort = { | ||
field: 'commitsFailed', | ||
kind: 'desc', | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import type {ReactNode} from 'react'; | ||
import styled from '@emotion/styled'; | ||
|
||
import Link from 'sentry/components/links/link'; | ||
import QuestionTooltip from 'sentry/components/questionTooltip'; | ||
import {IconArrow} from 'sentry/icons'; | ||
import {space} from 'sentry/styles/space'; | ||
import type {Sort} from 'sentry/utils/discover/fields'; | ||
import {useLocation} from 'sentry/utils/useLocation'; | ||
|
||
type HeaderParams = { | ||
alignment: string; | ||
fieldName: string; | ||
label: string; | ||
sort: undefined | Sort; | ||
tooltip?: string | ReactNode; | ||
}; | ||
|
||
function SortableHeader({fieldName, label, sort, tooltip, alignment}: HeaderParams) { | ||
const location = useLocation(); | ||
|
||
const arrowDirection = sort?.kind === 'asc' ? 'up' : 'down'; | ||
const sortArrow = <IconArrow size="xs" direction={arrowDirection} />; | ||
|
||
return ( | ||
<HeaderCell alignment={alignment}> | ||
<StyledLink | ||
role="columnheader" | ||
aria-sort={ | ||
sort?.field.endsWith(fieldName) | ||
? sort?.kind === 'asc' | ||
? 'ascending' | ||
: 'descending' | ||
: 'none' | ||
} | ||
to={{ | ||
pathname: location.pathname, | ||
query: { | ||
...location.query, | ||
sort: sort?.field.endsWith(fieldName) | ||
? sort?.kind === 'desc' | ||
? fieldName | ||
: '-' + fieldName | ||
: '-' + fieldName, | ||
}, | ||
}} | ||
> | ||
{label} {sort?.field === fieldName && sortArrow} | ||
</StyledLink> | ||
{tooltip ? ( | ||
<StyledQuestionTooltip size="xs" position="top" title={tooltip} isHoverable /> | ||
) : null} | ||
</HeaderCell> | ||
); | ||
} | ||
|
||
const HeaderCell = styled('div')<{alignment: string}>` | ||
display: block; | ||
width: 100%; | ||
text-align: ${p => (p.alignment === 'left' ? 'left' : 'right')}; | ||
`; | ||
|
||
const StyledLink = styled(Link)` | ||
color: inherit; | ||
|
||
:hover { | ||
color: inherit; | ||
} | ||
|
||
svg { | ||
vertical-align: top; | ||
} | ||
`; | ||
|
||
const StyledQuestionTooltip = styled(QuestionTooltip)` | ||
margin-left: ${space(0.5)}; | ||
`; | ||
|
||
export default SortableHeader; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import styled from '@emotion/styled'; | ||
|
||
import {Tag} from 'sentry/components/core/badge/tag'; | ||
import {DateTime} from 'sentry/components/dateTime'; | ||
import PerformanceDuration from 'sentry/components/performanceDuration'; | ||
import {space} from 'sentry/styles/space'; | ||
import { | ||
type Column, | ||
RIGHT_ALIGNED_FIELDS, | ||
type Row, | ||
} from 'sentry/views/codecov/tests/testAnalyticsTable/testAnalyticsTable'; | ||
|
||
interface TableBodyProps { | ||
column: Column; | ||
row: Row; | ||
} | ||
|
||
export function renderTableBody({column, row}: TableBodyProps) { | ||
const key = column.key; | ||
const value = row[key]; | ||
const alignment = RIGHT_ALIGNED_FIELDS.has(key) ? 'right' : 'left'; | ||
|
||
if (key === 'testName') { | ||
return <Container alignment={alignment}>{value}</Container>; | ||
} | ||
|
||
if (key === 'averageDurationMs') { | ||
return ( | ||
<NumberContainer> | ||
<PerformanceDuration milliseconds={Number(value)} abbreviation /> | ||
</NumberContainer> | ||
); | ||
} | ||
|
||
if (key === 'flakeRate') { | ||
const isBrokenTest = row.isBrokenTest; | ||
return ( | ||
<NumberContainer> | ||
{isBrokenTest && <StyledTag type={'highlight'}>Broken test</StyledTag>} | ||
{value}% | ||
</NumberContainer> | ||
); | ||
} | ||
|
||
if (key === 'commitsFailed') { | ||
return <Container alignment={alignment}>{value}</Container>; | ||
} | ||
|
||
if (key === 'lastRun') { | ||
return ( | ||
<DateContainer> | ||
<DateTime date={value} year seconds timeZone /> | ||
</DateContainer> | ||
); | ||
} | ||
|
||
return <Container alignment={alignment}>{value}</Container>; | ||
} | ||
|
||
export const Container = styled('div')<{alignment: string}>` | ||
${p => p.theme.overflowEllipsis}; | ||
font-family: ${p => p.theme.text.familyMono}; | ||
text-align: ${p => (p.alignment === 'left' ? 'left' : 'right')}; | ||
`; | ||
|
||
const DateContainer = styled('div')` | ||
color: ${p => p.theme.tokens.content.muted}; | ||
text-align: 'left'; | ||
`; | ||
|
||
const NumberContainer = styled('div')` | ||
text-align: right; | ||
font-variant-numeric: tabular-nums; | ||
${p => p.theme.overflowEllipsis}; | ||
`; | ||
|
||
const StyledTag = styled(Tag)` | ||
margin-right: ${space(1.5)}; | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import {tct} from 'sentry/locale'; | ||
import type {Sort} from 'sentry/utils/discover/fields'; | ||
import SortableHeader from 'sentry/views/codecov/tests/testAnalyticsTable/sortableHeader'; | ||
import type {Column} from 'sentry/views/codecov/tests/testAnalyticsTable/testAnalyticsTable'; | ||
import {RIGHT_ALIGNED_FIELDS} from 'sentry/views/codecov/tests/testAnalyticsTable/testAnalyticsTable'; | ||
|
||
type TableHeaderParams = { | ||
column: Column; | ||
sort?: Sort; | ||
}; | ||
|
||
type FlakyTestTooltipProps = { | ||
date: string; | ||
}; | ||
|
||
function FlakyTestsTooltip({date}: FlakyTestTooltipProps) { | ||
return ( | ||
<p> | ||
{tct( | ||
`Shows how often a flake occurs by tracking how many times a test goes from fail to pass or pass to fail on a given branch and commit within the last [date]`, | ||
{date} | ||
)} | ||
. | ||
</p> | ||
); | ||
} | ||
|
||
export const renderTableHeader = ({column, sort}: TableHeaderParams) => { | ||
const {key, name} = column; | ||
// TODO: adjust when the date selector is completed | ||
const date = '30 days'; | ||
|
||
const alignment = RIGHT_ALIGNED_FIELDS.has(key) ? 'right' : 'left'; | ||
|
||
return ( | ||
<SortableHeader | ||
alignment={alignment} | ||
sort={sort} | ||
fieldName={key} | ||
label={name} | ||
{...(key === 'flakeRate' && { | ||
tooltip: <FlakyTestsTooltip date={date} />, | ||
})} | ||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
import type {GridColumnHeader} from 'sentry/components/gridEditable'; | ||
import GridEditable, {COL_WIDTH_UNDEFINED} from 'sentry/components/gridEditable'; | ||
import {t} from 'sentry/locale'; | ||
import type {Sort} from 'sentry/utils/discover/fields'; | ||
import {renderTableBody} from 'sentry/views/codecov/tests/testAnalyticsTable/tableBody'; | ||
import {renderTableHeader} from 'sentry/views/codecov/tests/testAnalyticsTable/tableHeader'; | ||
|
||
type TestAnalyticsTableResponse = { | ||
averageDurationMs: number; | ||
commitsFailed: number; | ||
flakeRate: number; | ||
isBrokenTest: boolean; | ||
lastRun: string; | ||
testName: string; | ||
}; | ||
|
||
export type Row = Pick< | ||
TestAnalyticsTableResponse, | ||
| 'testName' | ||
| 'averageDurationMs' | ||
| 'flakeRate' | ||
| 'commitsFailed' | ||
| 'lastRun' | ||
| 'isBrokenTest' | ||
>; | ||
export type Column = GridColumnHeader< | ||
'testName' | 'averageDurationMs' | 'flakeRate' | 'commitsFailed' | 'lastRun' | ||
>; | ||
|
||
export type ValidSort = Sort & { | ||
field: (typeof SORTABLE_FIELDS)[number]; | ||
}; | ||
|
||
const COLUMNS_ORDER: Column[] = [ | ||
{key: 'testName', name: t('Test Name'), width: COL_WIDTH_UNDEFINED}, | ||
{key: 'averageDurationMs', name: t('Avg. Duration'), width: COL_WIDTH_UNDEFINED}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any idea if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong feel for it, but it might worth bringing up in a design sync as a consideration when thinking on abbreviations. Seems to me like this could be some antipattern design wise? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll make a note to bring up |
||
{key: 'flakeRate', name: t('Flake Rate'), width: COL_WIDTH_UNDEFINED}, | ||
{key: 'commitsFailed', name: t('Commits Failed'), width: COL_WIDTH_UNDEFINED}, | ||
{key: 'lastRun', name: t('Last Run'), width: COL_WIDTH_UNDEFINED}, | ||
]; | ||
|
||
export const RIGHT_ALIGNED_FIELDS = new Set([ | ||
'averageDurationMs', | ||
'flakeRate', | ||
'commitsFailed', | ||
]); | ||
|
||
export const SORTABLE_FIELDS = [ | ||
'testName', | ||
'averageDurationMs', | ||
'flakeRate', | ||
'commitsFailed', | ||
'lastRun', | ||
] as const; | ||
|
||
export function isAValidSort(sort: Sort): sort is ValidSort { | ||
return (SORTABLE_FIELDS as unknown as string[]).includes(sort.field); | ||
} | ||
|
||
interface Props { | ||
response: { | ||
data: Row[]; | ||
isLoading: boolean; | ||
error?: Error | null; | ||
}; | ||
sort: ValidSort; | ||
} | ||
|
||
export default function TestAnalyticsTable({response, sort}: Props) { | ||
const {data, isLoading} = response; | ||
|
||
return ( | ||
<GridEditable | ||
aria-label={t('Test Analytics')} | ||
isLoading={isLoading} | ||
error={response.error} | ||
data={data} | ||
columnOrder={COLUMNS_ORDER} | ||
// TODO: This isn't used as per the docs but is still required. Test if | ||
// it affects sorting when backend is ready. | ||
columnSortBy={[ | ||
{ | ||
key: sort.field, | ||
order: sort.kind, | ||
}, | ||
]} | ||
grid={{ | ||
renderHeadCell: column => | ||
renderTableHeader({ | ||
column, | ||
sort, | ||
}), | ||
renderBodyCell: (column, row) => renderTableBody({column, row}), | ||
}} | ||
/> | ||
); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
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.
Is there a specific reason why we're using Pick here and picking everything instead of using the TestAnalyticsTableResponse type directly or setting
Row = TestAnalyticsTableResponse
? Are we implying that new response fields may not automatically want to be added to Row?Uh oh!
There was an error while loading. Please reload this page.
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.
My read here was that your row values don't need to necessarily follow the backend response. I think makes sense that an api response (or one after it's transformed from the hooks) exclusively contain the values you'll use in the table. We might fetch more data but we only care about the items relevant to the table.
I think this gives room for the api response to expand as needed without coupling it w/ the table/ui. There's an argument to how that shouldn't happen but yeah I think that'd lead to the API being more coupled to the design
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.
That's fair. Cool, I'd say just leave it then for the implication that it can be decoupled.