[FE4]: Test Results Chart Component #3942
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3942 +/- ##
==========================================
- Coverage 98.62% 98.04% -0.58%
==========================================
Files 828 829 +1
Lines 15099 15213 +114
Branches 4326 4382 +56
==========================================
+ Hits 14891 14916 +25
- Misses 200 272 +72
- Partials 8 25 +17
... and 11 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3942 +/- ##
==========================================
- Coverage 98.62% 98.04% -0.58%
==========================================
Files 828 829 +1
Lines 15099 15213 +114
Branches 4318 4382 +64
==========================================
+ Hits 14891 14916 +25
- Misses 200 272 +72
- Partials 8 25 +17
... and 11 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3942 +/- ##
==========================================
+ Coverage 96.54% 98.04% +1.49%
==========================================
Files 828 829 +1
Lines 15099 15213 +114
Branches 4318 4374 +56
==========================================
+ Hits 14578 14916 +338
+ Misses 466 272 -194
+ Partials 55 25 -30
... and 53 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 4.24kB (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-staging-esmAssets Changed:
view changes for bundle: gazebo-staging-systemAssets Changed:
|
This comment has been minimized.
This comment has been minimized.
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| export function TestResultsChart({ results }: TestResultsChartProps) { |
There was a problem hiding this comment.
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.
|
|
||
| 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 | ||
| }, []) |
There was a problem hiding this comment.
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 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 | ||
| }, | ||
| {} |
There was a problem hiding this comment.
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 slowestTests = results | ||
| .filter((result) => result.status !== 'skipped') | ||
| .sort((a, b) => b.duration - a.duration) | ||
| .slice(0, 10) |
There was a problem hiding this comment.
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 | ||
| }, | ||
| {} |
There was a problem hiding this comment.
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 flakyTests = Object.entries(flakinessScores) | ||
| .filter(([, score]) => score > 20) | ||
| .sort(([, a], [, b]) => b - a) | ||
| .slice(0, 5) |
There was a problem hiding this comment.
The flakyTests calculation should be memoized and depends on flakinessScores, which should be calculated first.
Did we get this right? 👍 / 👎 to inform future reviews.
| @@ -0,0 +1,203 @@ | |||
| import { cn } from 'shared/utils/cn' | |||
There was a problem hiding this comment.
Missing import for React and useMemo hook that are needed for the performance optimizations.
Did we get this right? 👍 / 👎 to inform future reviews.
This PR includes a new Test Results Chart Component to show some data on the frontend
Link to Sample Entry
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.