fix: Pass in interval to aggregate functions, dynamic copy updates for intervals #3425
Conversation
| @@ -4,7 +4,7 @@ export const MEASUREMENT_INTERVAL = { | |||
| INTERVAL_1_DAY: 'INTERVAL_1_DAY', | |||
| } as const | |||
|
|
|||
| export type MEASUREMENT_INTERVAL_TYPE = keyof typeof MEASUREMENT_INTERVAL | |||
| export type MeasurementInterval = keyof typeof MEASUREMENT_INTERVAL | |||
There was a problem hiding this comment.
This was just a lot cleaner looking to me, changed all the other instances to use this as well
There was a problem hiding this comment.
Nice. Not sure what pattern we go for elsewhere but looks like at least in this file we sometimes have the enum options have a pluralized name to help distinguish it
There was a problem hiding this comment.
Yeah, this is something we'll need to hash out as a team and make a norm around. I see all sorts of patterns throughout the codebase but I think having a shared consts file or directory with labeled files for each const makes the most sense to me
| @@ -56,12 +57,6 @@ const query = ` | |||
| } | |||
| ` | |||
|
|
|||
| export enum MeasurementInterval { | |||
There was a problem hiding this comment.
Removed the enum here so we could have a single source of truth
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Bundle ReportChanges will decrease total bundle size by 6.14MB (-35.6%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 6.14MB (-35.6%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## tests-analytics-v2 #3425 +/- ##
===================================================
Coverage 99.07% 99.07%
===================================================
Files 816 816
Lines 14518 14526 +8
Branches 4023 4034 +11
===================================================
+ Hits 14384 14392 +8
Misses 125 125
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## tests-analytics-v2 #3425 +/- ##
===================================================
Coverage 99.07% 99.07%
===================================================
Files 816 816
Lines 14518 14526 +8
Branches 4023 4034 +11
===================================================
+ Hits 14384 14392 +8
Misses 125 125
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## tests-analytics-v2 #3425 +/- ##
===================================================
Coverage 99.07% 99.07%
===================================================
Files 816 816
Lines 14518 14527 +9
Branches 4030 4034 +4
===================================================
+ Hits 14384 14393 +9
Misses 125 125
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## tests-analytics-v2 #3425 +/- ##
===================================================
Coverage 99.07% 99.07%
===================================================
Files 816 816
Lines 14518 14526 +8
Branches 4023 4034 +11
===================================================
+ Hits 14384 14392 +8
Misses 125 125
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
| @@ -91,7 +91,7 @@ describe('useFlakeAggregates', () => { | |||
| const { result } = renderHook( | |||
| () => | |||
| useFlakeAggregates({ | |||
| interval: MeasurementInterval.INTERVAL_1_DAY, | |||
| interval: 'INTERVAL_1_DAY', | |||
There was a problem hiding this comment.
It was so we could have a single source of truth/type coming from that shared consts file
There was a problem hiding this comment.
Can you import it from there maybe?
There was a problem hiding this comment.
The type is persistent so there's no import required :O
Or what did you have in mind?
There was a problem hiding this comment.
Made a change in 0fc4a00
| const columns = useMemo( | ||
| () => | ||
| getColumns({ | ||
| hideFlakeRate: !!hideFlakeRate, |
There was a problem hiding this comment.
the !! seems unnecessary. This could just be hideFlakeRate, right?
There was a problem hiding this comment.
Good point, I didn't really think about it but yep the !! is unnecessary
| @@ -4,7 +4,7 @@ export const MEASUREMENT_INTERVAL = { | |||
| INTERVAL_1_DAY: 'INTERVAL_1_DAY', | |||
| } as const | |||
|
|
|||
| export type MEASUREMENT_INTERVAL_TYPE = keyof typeof MEASUREMENT_INTERVAL | |||
| export type MeasurementInterval = keyof typeof MEASUREMENT_INTERVAL | |||
There was a problem hiding this comment.
Nice. Not sure what pattern we go for elsewhere but looks like at least in this file we sometimes have the enum options have a pluralized name to help distinguish it
| @@ -83,6 +86,9 @@ const TotalTestsRunTimeCard = ({ | |||
| <PercentBadge value={totalDurationPercentChange} /> | |||
| ) : null} | |||
| </MetricCard.Content> | |||
| <MetricCard.Description> | |||
| The cumulative CI time spent running tests over the last {intervalCopy} | |||
There was a problem hiding this comment.
period at the end here to be consistent with the other descriptions?
There was a problem hiding this comment.
Nice catch! Will fix
| case 'INTERVAL_7_DAY': | ||
| return '7 days' | ||
| case 'INTERVAL_30_DAY': | ||
| return '30 days' |
There was a problem hiding this comment.
nit: we could remove this line since these last two return the same
There was a problem hiding this comment.
trueee, I prefer the explicit nature though personally, in the event someone comes back and changes the default case for whatever reason
There was a problem hiding this comment.
oh i mean just line 333 but leave 332
There was a problem hiding this comment.
ohhhh big facts, I like that
There was a problem hiding this comment.
Updated in 2fbc6ac
ajay-sentry
deleted the
Ajay/pass-in-measurement-interval-and-dynamic-copy
branch
Description
This PR does a small feature add passing in the historicalTrend parameter to the "interval" filter for both the testResultsAggregates and flakeAggregates queries, and does some minor dynamic copy updates taking into account that filter if it exists.
I chose not to use this thing because it was hard coded to have the capitals at the beginning of the label and I needed something a little more precise for my use case.
Closes codecov/engineering-team#2691
gazebo/src/pages/RepoPage/shared/constants.ts
Lines 9 to 13 in 88d2ec9
Screenshots
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.