Skip to content

(test) Fix Duration #3900

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

(test) Fix Duration #3900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d6255da
fix(ta): seconds to duration function
joseph-sentry Jun 11, 2025
cd396a0
address feedback
joseph-sentry Jun 12, 2025
c422eb2
test: add more tests
joseph-sentry Jun 12, 2025
3755ba1
address feedback
joseph-sentry Jun 12, 2025
02568fa
Merge branch 'joseph/fix-total-duration' into eliatcodecov/seer-revie…
eliatcodecov Jun 20, 2025
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
20 changes: 20 additions & 0 deletions src/shared/utils/dates.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,24 @@ describe('formatTimeFromSeconds', () => {
it('returns the correct time format when totalSeconds is greater than 0', () => {
expect(formatTimeFromSeconds(3661)).toBe('1h 1m 1s')
})

it('returns the correct time format when totalSeconds is a float', () => {
expect(formatTimeFromSeconds(12901948.144373389)).toBe('149d 7h 52m 28s')
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

The test for large float values is good, but consider adding a test for edge cases around floating point precision. For example, what happens with very small positive numbers like 0.1 or 0.9?

Suggested change
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', () => {
expect(formatTimeFromSeconds(12901948.144373389)).toBe('149d 7h 52m 28s')
})
it('handles small float values correctly', () => {
expect(formatTimeFromSeconds(0.1)).toBe('<1s')
expect(formatTimeFromSeconds(0.9)).toBe('<1s')
})

})

it('returns the correct time format when totalSeconds is less than 1', () => {
expect(formatTimeFromSeconds(0.5)).toBe('<1s')
Comment on lines +41 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a test for the boundary case of exactly 1 second to ensure the '<1s' logic works correctly.

Suggested change
it('returns the correct time format when totalSeconds is less than 1', () => {
expect(formatTimeFromSeconds(0.5)).toBe('<1s')
it('returns the correct time format when totalSeconds is less than 1', () => {
expect(formatTimeFromSeconds(0.5)).toBe('<1s')
})
it('returns the correct time format when totalSeconds is exactly 1', () => {
expect(formatTimeFromSeconds(1)).toBe('1s')
})

})

it('returns "N/A" when totalSeconds is negative', () => {
expect(formatTimeFromSeconds(-1)).toBe('N/A')
})

it('returns only minutes when totalSeconds is an exact number of minutes', () => {
expect(formatTimeFromSeconds(120)).toBe('2m')
})

it('handles float values that round down', () => {
expect(formatTimeFromSeconds(59.999)).toBe('59s')
})
})
Comment on lines +55 to 57
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding tests for extreme values to ensure the function handles edge cases gracefully. This includes very large numbers and testing the boundaries of your constants.

Suggested change
expect(formatTimeFromSeconds(59.999)).toBe('59s')
})
})
it('handles float values that round down', () => {
expect(formatTimeFromSeconds(59.999)).toBe('59s')
})
it('handles very large numbers', () => {
expect(formatTimeFromSeconds(Number.MAX_SAFE_INTEGER)).toBe('N/A')
})
it('handles zero after negative check', () => {
expect(formatTimeFromSeconds(-0)).toBe('0s') // -0 === 0 in JavaScript
})

44 changes: 27 additions & 17 deletions src/shared/utils/dates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import {
formatDistanceToNow,
fromUnixTime,
intervalToDuration,
parseISO,
} from 'date-fns'
import { formatDistanceToNow, fromUnixTime, parseISO } from 'date-fns'

const SECONDS_PER_DAY = 86400
const SECONDS_PER_HOUR = 3600
const SECONDS_PER_MINUTE = 60

export function formatTimeToNow(date?: string | number | null) {
if (!date) return null
Expand All @@ -16,19 +15,30 @@ export function formatTimeToNow(date?: string | number | null) {
}

export const formatTimeFromSeconds = (totalSeconds?: number | null) => {
if (totalSeconds == null || totalSeconds < 0) return 'N/A'
if (totalSeconds === 0) return '0s'
if (!totalSeconds) return 'N/A'
if (totalSeconds < 1) return '<1s'

Comment on lines 17 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

The null check logic has changed behavior. Previously, !totalSeconds would catch both 0 and falsy values, but now totalSeconds == null only catches null and undefined. Consider being more explicit about the intended behavior. The current implementation returns 'N/A' for negative numbers but the previous version would have returned 'N/A' for any falsy value except 0.

Suggested change
export const formatTimeFromSeconds = (totalSeconds?: number | null) => {
if (totalSeconds == null || totalSeconds < 0) return 'N/A'
if (totalSeconds === 0) return '0s'
if (!totalSeconds) return 'N/A'
if (totalSeconds < 1) return '<1s'
// Consider adding a comment to clarify the intended behavior:
// Returns 'N/A' for null, undefined, or negative values
if (totalSeconds == null || totalSeconds < 0) return 'N/A'
if (totalSeconds === 0) return '0s'
if (totalSeconds < 1) return '<1s'

const duration = intervalToDuration({ start: 0, end: totalSeconds * 1000 })
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
)
Comment on lines +22 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

The manual calculation approach is more efficient and gives you better control, but consider adding input validation for extremely large numbers that might cause overflow issues. JavaScript's Number.MAX_SAFE_INTEGER is 2^53 - 1, and very large inputs could lead to precision issues.

Suggested change
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
// Add bounds checking for very large numbers
if (totalSeconds > Number.MAX_SAFE_INTEGER / 1000) {
return 'N/A' // or handle appropriately
}
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)

const seconds = Math.floor(totalSeconds % SECONDS_PER_MINUTE)

const { days, hours, minutes, seconds } = duration
const timeParts = []
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`)
}

return [
Comment on lines +27 to 42
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic for building time parts is cleaner now, but consider what happens when all time parts are 0 (which shouldn't happen given the early returns, but it's worth being defensive). The function could potentially return an empty string if somehow all values are 0.

Suggested change
const seconds = Math.floor(totalSeconds % SECONDS_PER_MINUTE)
const { days, hours, minutes, seconds } = duration
const timeParts = []
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`)
}
const timeParts = []
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`)
}
// Defensive check, though this shouldn't happen with current logic
return timeParts.length > 0 ? timeParts.join(' ') : '0s'

days ? `${days}d` : '',
hours ? `${hours}h` : '',
minutes ? `${minutes}m` : '',
seconds ? `${seconds}s` : '',
]
.filter(Boolean)
.join(' ')
return timeParts.join(' ')
}
Loading