Skip to content
Open
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
import { cn } from 'shared/utils/cn'
Copy link

Choose a reason for hiding this comment

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

Missing import for React and useMemo hook that are needed for the performance optimizations.

Did we get this right? 👍 / 👎 to inform future reviews.


interface TestResult {
id: string
name: string
duration: number
status: 'passed' | 'failed' | 'skipped'
timestamp: number
branch: string
}

interface ChartDataPoint {
date: string
passed: number
failed: number
skipped: number
totalDuration: number
}

interface TestResultsChartProps {
results: TestResult[]
}

export function TestResultsChart({ results }: TestResultsChartProps) {
Comment on lines +23 to +24
Copy link

Choose a reason for hiding this comment

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

The component should use React.memo for performance optimization since it processes potentially large datasets. The data processing operations are expensive and should be memoized to prevent unnecessary recalculations on re-renders.

Did we get this right? 👍 / 👎 to inform future reviews.

if (!results || results.length === 0) {
return null
}

const chartData = results.reduce((acc: ChartDataPoint[], result) => {
const date = new Date(result.timestamp).toLocaleDateString()
const existing = acc.find((item) => item.date === date)

if (existing) {
existing[result.status]++
existing.totalDuration += result.duration
} else {
acc.push({
date,
passed: result.status === 'passed' ? 1 : 0,
failed: result.status === 'failed' ? 1 : 0,
skipped: result.status === 'skipped' ? 1 : 0,
totalDuration: result.duration,
})
}

return acc
}, [])
Copy link

Choose a reason for hiding this comment

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

Data processing operations should be wrapped in useMemo hooks to prevent expensive recalculations on every render. The chartData, slowestTests, branchStats, and flakinessScores calculations are all expensive operations that depend only on the results prop.

Did we get this right? 👍 / 👎 to inform future reviews.


const slowestTests = results
.filter((result) => result.status !== 'skipped')
.sort((a, b) => b.duration - a.duration)
.slice(0, 10)
Comment on lines 79 to 86
Copy link

Choose a reason for hiding this comment

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

The slowestTests calculation should also be memoized for performance. Additionally, consider adding error handling for edge cases where duration might be undefined or null.

Did we get this right? 👍 / 👎 to inform future reviews.


const branchStats = results.reduce(
(
acc: Record<
string,
{ total: number; failed: number; avgDuration: number }
>,
result
) => {
if (!acc[result.branch]) {
acc[result.branch] = { total: 0, failed: 0, avgDuration: 0 }
}
const branchData = acc[result.branch]
if (branchData) {
branchData.total++
if (result.status === 'failed') {
branchData.failed++
}
branchData.avgDuration =
(branchData.avgDuration * (branchData.total - 1) + result.duration) /
branchData.total
}
return acc
},
{}
Comment on lines 88 to 120
Copy link

Choose a reason for hiding this comment

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

The branchStats calculation should be memoized and has a redundant null check. The branchData variable is guaranteed to exist after the initialization check.

Did we get this right? 👍 / 👎 to inform future reviews.

)

const flakinessScores = results.reduce(
(acc: Record<string, number>, result) => {
if (!acc[result.name]) {
acc[result.name] = 0
}
const testResults = results.filter((r) => r.name === result.name)
let statusChanges = 0
for (let i = 1; i < testResults.length; i++) {
const current = testResults.at(i)
const previous = testResults.at(i - 1)
if (current && previous && current.status !== previous.status) {
statusChanges++
}
}
acc[result.name] = (statusChanges / testResults.length) * 100
return acc
},
{}
Comment on lines 122 to 135
Copy link

Choose a reason for hiding this comment

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

The flakiness calculation has a potential bug when accessing array elements. Using array.at() with negative indices or out-of-bounds indices can return undefined, which will cause the comparison to fail silently. Also, the algorithm assumes the test results are sorted by timestamp, but there's no guarantee of this ordering.

Did we get this right? 👍 / 👎 to inform future reviews.

)

const flakyTests = Object.entries(flakinessScores)
.filter(([, score]) => score > 20)
.sort(([, a], [, b]) => b - a)
.slice(0, 5)
Comment on lines +137 to +162
Copy link

Choose a reason for hiding this comment

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

The flakyTests calculation should be memoized and depends on flakinessScores, which should be calculated first.

Did we get this right? 👍 / 👎 to inform future reviews.


return (
<div className="space-y-6">
<div>
<h2 className="mb-4 text-xl font-semibold">Test Results Over Time</h2>
<div className="space-y-2">
{chartData.map((point) => (
<div
key={point.date}
className="flex items-center gap-4 rounded border border-ds-gray-tertiary p-3"
>
<span className="w-24 text-sm">{point.date}</span>
<div className="flex flex-1 gap-2">
<div className="flex items-center gap-2">
<div className="size-3 rounded-full bg-ds-primary-green" />
<span className="text-sm">{point.passed}</span>
</div>
<div className="flex items-center gap-2">
<div className="size-3 rounded-full bg-ds-primary-red" />
<span className="text-sm">{point.failed}</span>
</div>
<div className="flex items-center gap-2">
<div className="size-3 rounded-full bg-ds-gray-quinary" />
<span className="text-sm">{point.skipped}</span>
</div>
</div>
<span className="text-sm text-ds-gray-senary">
{(point.totalDuration / 1000).toFixed(2)}s
</span>
</div>
))}
</div>
</div>

<div>
<h3 className="mb-3 text-lg font-semibold">Slowest Tests</h3>
<div className="space-y-2">
{slowestTests.map((test) => (
<div
key={test.id}
className="flex items-center justify-between rounded border border-ds-gray-tertiary p-3"
>
<span className="font-mono text-sm">{test.name}</span>
<span className="text-sm font-semibold text-ds-primary-yellow">
{(test.duration / 1000).toFixed(2)}s
</span>
</div>
))}
</div>
</div>

{flakyTests.length > 0 && (
<div>
<h3 className="mb-3 text-lg font-semibold">Flaky Tests</h3>
<div className="space-y-2">
{flakyTests.map(([name, score]) => (
<div
key={name}
className="flex items-center justify-between rounded border border-ds-primary-red p-3"
>
<span className="font-mono text-sm">{name}</span>
<span className="text-sm font-semibold text-ds-primary-red">
{score.toFixed(1)}% flaky
</span>
</div>
))}
</div>
</div>
)}

<div>
<h3 className="mb-3 text-lg font-semibold">Branch Statistics</h3>
<div className="grid grid-cols-3 gap-4">
{Object.entries(branchStats).map(([branch, stats]) => (
<div
key={branch}
className={cn(
'rounded border p-4',
stats.failed > 0
? 'border-ds-primary-red bg-ds-pink-default'
: 'border-ds-gray-tertiary bg-white'
)}
>
<h4 className="font-mono text-sm font-semibold">{branch}</h4>
<div className="mt-2 space-y-1 text-xs">
<div>Total: {stats.total}</div>
<div className="text-ds-primary-red">
Failed: {stats.failed}
</div>
<div>Avg: {stats.avgDuration.toFixed(0)}ms</div>
</div>
</div>
))}
</div>
</div>
</div>
)
}

export default TestResultsChart