fix(ta): seconds to duration function #3891
Conversation
Bundle ReportChanges will decrease total bundle size by 2.56kB (-0.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-production-systemAssets Changed:
Files in
view changes for bundle: gazebo-production-esmAssets Changed:
Files in
|
Bundle ReportChanges will decrease total bundle size by 2.56kB (-0.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-staging-systemAssets Changed:
Files in
view changes for bundle: gazebo-staging-esmAssets Changed:
Files in
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3891 +/- ##
=======================================
Coverage 98.72% 98.73%
=======================================
Files 827 827
Lines 15029 15040 +11
Branches 4297 4306 +9
=======================================
+ Hits 14838 14849 +11
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3891 +/- ##
=======================================
Coverage 98.72% 98.73%
=======================================
Files 827 827
Lines 15029 15040 +11
Branches 4297 4298 +1
=======================================
+ Hits 14838 14849 +11
Misses 184 184
Partials 7 7
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 @@
## main #3891 +/- ##
=======================================
Coverage 98.72% 98.73%
=======================================
Files 827 827
Lines 15029 15040 +11
Branches 4297 4306 +9
=======================================
+ Hits 14838 14849 +11
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3891 +/- ##
=======================================
Coverage 98.72% 98.73%
=======================================
Files 827 827
Lines 15029 15040 +11
Branches 4305 4306 +1
=======================================
+ Hits 14838 14849 +11
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
✅ Sentry found no issues in your recent changes ✅ |
|
@sentry-ai-review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request improves the Click to see moreKey Technical ChangesThe core technical change is the replacement of Architecture DecisionsThe architectural decision is to move away from the Dependencies and InteractionsThis change removes the dependency on Risk ConsiderationsThe main risk is the potential for introducing errors in the manual calculation logic. Thorough testing is crucial to ensure the correctness of the new implementation, especially for edge cases and float values. Another consideration is the change in output for durations less than 1 second (from '0s' to '<1s'), which might affect existing components that rely on the previous output format. This change should be carefully reviewed to ensure compatibility. Notable Implementation DetailsThe implementation uses |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
src/shared/utils/dates.ts
Outdated
| const days = Math.floor(totalSeconds / 86400) | ||
| const hours = Math.floor((totalSeconds % 86400) / 3600) | ||
| const minutes = Math.floor((totalSeconds % 3600) / 60) | ||
| const seconds = Math.floor(totalSeconds % 60) |
There was a problem hiding this comment.
The manual calculation approach is good, but consider adding comments to explain the magic numbers for better maintainability. 86400 (seconds in a day), 3600 (seconds in an hour), and 60 (seconds in a minute) should be documented.
| const days = Math.floor(totalSeconds / 86400) | |
| const hours = Math.floor((totalSeconds % 86400) / 3600) | |
| const minutes = Math.floor((totalSeconds % 3600) / 60) | |
| const seconds = Math.floor(totalSeconds % 60) | |
| // Calculate time components | |
| const SECONDS_PER_DAY = 86400 | |
| const SECONDS_PER_HOUR = 3600 | |
| const SECONDS_PER_MINUTE = 60 | |
| const days = Math.floor(totalSeconds / SECONDS_PER_DAY) | |
| const hours = Math.floor((totalSeconds % SECONDS_PER_DAY) / SECONDS_PER_HOUR) | |
| const minutes = Math.floor((totalSeconds % SECONDS_PER_HOUR) / SECONDS_PER_MINUTE) | |
| const seconds = Math.floor(totalSeconds % SECONDS_PER_MINUTE) |
src/shared/utils/dates.ts
Outdated
| if (timeParts.length === 0 && totalSeconds > 0) { | ||
| return '<1s' | ||
| } |
There was a problem hiding this comment.
The condition timeParts.length === 0 && totalSeconds > 0 could be simplified since we already know totalSeconds > 0 from the earlier checks. Also, consider if this edge case is actually needed - if all time parts are 0 but totalSeconds > 0, it means totalSeconds is between 0 and 1, which might be better represented as '0s' for consistency.
| if (timeParts.length === 0 && totalSeconds > 0) { | |
| return '<1s' | |
| } | |
| if (timeParts.length === 0) { | |
| return '0s' | |
| } |
|
|
||
| it('returns the correct time format when totalSeconds is a float', () => { | ||
| expect(formatTimeFromSeconds(12901948.144373389)).toBe('149d 7h 52m 28s') |
There was a problem hiding this comment.
The test case for float values is good, but the specific large float value 12901948.144373389 seems arbitrary. Consider using a more readable test case or adding a comment explaining why this specific value was chosen. Also, consider adding more edge cases for floats, such as values between 0 and 1.
| it('returns the correct time format when totalSeconds is a float', () => { | |
| expect(formatTimeFromSeconds(12901948.144373389)).toBe('149d 7h 52m 28s') | |
| it('returns the correct time format when totalSeconds is a float', () => { | |
| // Test large float value | |
| expect(formatTimeFromSeconds(12901948.144373389)).toBe('149d 7h 52m 28s') | |
| }) | |
| it('returns "0s" when totalSeconds is a small float less than 1', () => { | |
| expect(formatTimeFromSeconds(0.5)).toBe('0s') | |
| }) |
src/shared/utils/dates.ts
Outdated
| const SECONDS_PER_DAY = 86400 | ||
| const SECONDS_PER_HOUR = 3600 | ||
| const SECONDS_PER_MINUTE = 60 |
There was a problem hiding this comment.
these can be defined outside of the function
src/shared/utils/dates.ts
Outdated
| if (days > 0) { | ||
| timeParts.push(`${days}d`) | ||
| } | ||
| if (hours > 0) { | ||
| timeParts.push(`${hours}h`) | ||
| } | ||
| if (minutes > 0) { | ||
| timeParts.push(`${minutes}m`) | ||
| } | ||
| if (seconds > 0) { | ||
| timeParts.push(`${seconds}s`) | ||
| } | ||
|
|
||
| if (timeParts.length === 0) { | ||
| return '<1s' | ||
| } |
There was a problem hiding this comment.
Thoughts on keeping the original code here if we just moved the <1s check before?
if (totalSeconds < 1) {
return '<1s'
}
return [
days ? `${days}d` : '',
hours ? `${hours}h` : '',
minutes ? `${minutes}m` : '',
seconds ? `${seconds}s` : '',
]
.filter(Boolean)
.join(' ')
This would exit earlier and skip all of lines 34-49 if we're between 0 and 1 second
The total test run time on https://app.codecov.io/github/getsentry/sentry/tests/master is bugged, this fixes that.