(test) Fix Duration #3900
(test) Fix Duration #3900
Conversation
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request refactors the Click to see moreKey Technical ChangesThe primary technical change is the replacement of the Architecture DecisionsThe architectural decision was to move away from a library-dependent approach to a more self-contained and optimized implementation. This reduces the bundle size and improves performance. The use of constants for time units promotes readability and maintainability. The decision to handle edge cases directly within the function ensures consistent behavior without relying on external library quirks. Dependencies and InteractionsThis pull request removes the dependency on Risk ConsiderationsThe primary risk is the potential for subtle errors in the manual calculation logic. Thorough testing, including edge cases and boundary conditions, is crucial to ensure the new implementation is functionally equivalent to the original and handles all inputs correctly. There is also a risk of introducing performance regressions if the manual calculation is not optimized effectively, although this is unlikely given the simplicity of the operations. The change in null/undefined handling should be carefully reviewed to ensure it aligns with the intended behavior of the application. Notable Implementation DetailsThe implementation uses |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| 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' |
There was a problem hiding this comment.
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.
| 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 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 |
There was a problem hiding this comment.
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.
| 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`) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| 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' |
|
|
||
| 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 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?
| 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') |
There was a problem hiding this comment.
Consider adding a test for the boundary case of exactly 1 second to ensure the '<1s' logic works correctly.
| 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') | |
| }) |
| expect(formatTimeFromSeconds(59.999)).toBe('59s') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
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.
| 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 | |
| }) |
Description
Fixes a duration issue for test analytics. Provides opportunity to test Seer.