Engagement manager to coroutines #95
Conversation
…vior It has to have longer delays as `inc` is calculated in seconds and test is time-sensitive.
This commits has minimal set of changes that moves implementation towards usage of Kotlin Coroutines with passing unit test. It also makes tests not time-sensitive.
Now, as tests are no longer time-sensitive, it's possible to narrow down unit tests to check exact expected values.
As tests are now not time-sensitive, there's possibility to verify `inc` parameter as well without making test suite taking a long time.
instead of private flag
In favor of timezone agnostic `kotlin.time.Duration`. It simplifies implementation and removes a need to declare a timezone, which might be confusing.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## coroutines #95 +/- ##
==============================================
+ Coverage 66.31% 67.67% +1.35%
==============================================
Files 14 14
Lines 377 365 -12
Branches 54 52 -2
==============================================
- Hits 250 247 -3
+ Misses 109 99 -10
- Partials 18 19 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looks good! I've left one minor suggestion and a question which might prompt another suggestion, but I don't think any of it is blocker, so I am going to approve the PR.
For full transparency, I had some other feedback that I was working on, but then I realized the feedback is about the original implementation and not this conversion - which wouldn't be fair. In my opinion it's a good thing to keep the conversion PRs as close to the original as possible and separate any major improvements to its own PRs. So, I decided to hold off on anything that's unrelated to this specific PR.
parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt
Outdated
Show resolved
Hide resolved
| fun stop() { | ||
| job?.let { | ||
| it.cancel() | ||
| doEnqueue(nextScheduledExecution) |
There was a problem hiding this comment.
Could you provide me some context about what this line is accomplishing, so I can provide better feedback?
There was a problem hiding this comment.
As soon as user calls EngagementManager#stop, the job is canceled. As soon as it happens, the SDK will enqueue the next event with scheduledExecutionTime as when it would be executed "normally" (without cancelation).
I don't know the business use case justification of such logic. I can only guess, that the scheduledExecutionTime is specific (hence HeartbeatIntervalCalculator) and it can't be any value (e.g. value of exact timestamp when EngagementManager was cancelled).
The given started manager, when stopping manager before interval ticks, then schedule an event test case should precisely address such scenario.
Here's the part of the previous implementation:
https://github.com/Parsely/parsely-android/pull/95/files/0a526b5ff05f6a9c20c97eb1c40dad66b6800217#diff-da80d9d42a1766ff69d56aace614ecbb105ef6a0b9e002a56758823a6580d135L77-L85
There's also a possibility, that the original implementation was enqueuing this event unintended - do you think it's what happened here?
Is this explanation sufficient, or do you think of some other information?
There was a problem hiding this comment.
Thanks a lot for the context!
There's also a possibility, that the original implementation was enqueuing this event unintended - do you think it's what happened here?
I thought that might be possible - but the previous implementation looks pretty intentional.
This logic feels a little fragile and out of place, but as we discussed previously, it makes sense for this PR to closely match the previous logic. So, I don't think we need to do anything about that at the moment.
Adding a comment about what's happening could be useful, but I'll admit that I still don't full get it even with your explanation. I'd need to dive deeper into what each component is doing and how it's utilized, but that's not really worth it at the moment.
@wzieba I don't have any other comments that needs addressing, so please feel free to proceed with merging the PR.
|
Thank you @oguzkocer !
Yes, I agree - that's exactly my intention for those conversion PRs. Thank you for thinking about this long term! I'm sure any comments and observations will be very useful when it comes to internal refactor PRs. I addressed the suggestion and added more context to the other comment. |
Description
This PR changes internal implementation of
EngagementManagerfromjava.timerto Kotlin Coroutines. This class is similar toFlushManager, which also usedjava.timerin the past and was migrated in #86 .How to test
Unit test, and functional test added at the beginning of migration should be just fine. Both are executed on CI.