Conversation
So `UpdateEngagementIntervalCalculator` can be injected with constant value of "now" in tests. Otherwise, the test will be time-sensitive.
So they can be used in unit tests
UpdateEngagementIntervalCalculator class
Refactor part: interval calculator now uses `kotlin.time.Duration` Thanks to this change we: - fix the bugs of unwanted rounding of `double` values to `long` - makes class easier to work, as we don't have to think about units with all the operation - makes implementation more similar to iOS, which uses `TimeInterval` (see: https://github.com/Parsely/AnalyticsSDK-iOS/blob/b1db4b05dcd5af6feb4710ec30d13fa704305692/Sources/Sampler.swift#L136)
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 42.06% 44.23% +2.16%
==========================================
Files 7 8 +1
Lines 359 364 +5
Branches 56 56
==========================================
+ Hits 151 161 +10
+ Misses 194 189 -5
Partials 14 14
☔ View full report in Codecov by Sentry. |
| import kotlin.time.Duration.Companion.minutes | ||
| import kotlin.time.Duration.Companion.seconds | ||
|
|
||
| internal open class HeartbeatIntervalCalculator(private val clock: Clock) { |
There was a problem hiding this comment.
- Praise (❤️): Great work on this, I love how this class is now using duration and the
x.seconds/minutes/hoursapproach, much more readable. Also, kudos for fixing the bugs of unwanted rounding (f273ee1)! 🥇
It also covers the calculator with unit tests and changes the calculator implementation to use less error-prone kotlin.time.Duration class to make arithmetical operations between different times.
- Suggestion (💡): When converting a Java class to Kotlin (9823815), consider using the
Extra commit for .java > .kt renamesoption while committing this change. This will make it easier for anyone reviewing this change later-on as it will show an actual diffing of files instead of showing the Java file being deleted and a new Kotlin file being added.
FYI: For more context on check this discussion I had with an external contributor a while back. Let me know your thoughts. 🙏
There was a problem hiding this comment.
Thanks for sharing about Extra commit option, TIL!
| companion object { | ||
| const val BACKOFF_PROPORTION = 0.3 | ||
| val OFFSET_MATCHING_BASE_INTERVAL = 35.seconds | ||
| val MAX_TIME_BETWEEN_HEARTBEATS = 15.minutes |
There was a problem hiding this comment.
This PR fixes the minor off difference when calculating heartbeat intervals (magnitude of up to 900ms). I won't change anything drastically, yet it's better to be in sync with iOS and have a clear implementation, without "hidden casting" issues.
👍
| } | ||
|
|
||
| class FakeClock : Clock() { | ||
| var fakeNow = Duration.ZERO |
| // surpass MAX_TIME_BETWEEN_HEARTBEATS | ||
| // (currentTime + offset) * backoff = max | ||
| // currentTime = (max / backoff) - offset, so | ||
| // (15 minutes / 0.3) - 35 seconds = 2965 seconds. Add 1 second to be over the limit |
There was a problem hiding this comment.
Praise (❤️): Thanks for adding these comments and making it clear how these numbers are calculated. 🙇
2 participants
Description
This PR fixes the minor off difference when calculating heartbeat intervals (magnitude of up to 900ms). I won't change anything drastically, yet it's better to be in sync with iOS and have a clear implementation, without "hidden casting" issues.
It also covers the calculator with unit tests and changes the calculator implementation to use less error-prone
kotlin.time.Durationclass to make arithmetical operations between different times.iOS implementation of this behavior: https://github.com/Parsely/AnalyticsSDK-iOS/blob/b1db4b05dcd5af6feb4710ec30d13fa704305692/Sources/Sampler.swift#L136