Parsely / parsely-android

The official Parse.ly Android toolkit -- for instrumenting your apps with Parse.ly
https://docs.parse.ly/android-sdk/
Other
6 stars 6 forks source link

Refactor `FlushManager` to Kotlin Coroutines #86

Closed wzieba closed 1 year ago

wzieba commented 1 year ago

Description

This PR moves FlushTimer class to separate class, rewrites it in Kotlin Coroutines and adds unit tests coverage. Additionally, it adds a functional test that asserts the correct flushing queue logic.

To test

Green light from CI should be fine πŸ‘

codecov[bot] commented 1 year ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (fcb6d7c) 44.38% compared to head (aad4629) 46.15%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## coroutines #86 +/- ## ============================================== + Coverage 44.38% 46.15% +1.77% ============================================== Files 8 10 +2 Lines 365 364 -1 Branches 56 57 +1 ============================================== + Hits 162 168 +6 + Misses 189 179 -10 - Partials 14 17 +3 ``` | [Files](https://app.codecov.io/gh/Parsely/parsely-android/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Parsely) | Coverage Ξ” | | |---|---|---| | [...om/parsely/parselyandroid/ParselyCoroutineScope.kt](https://app.codecov.io/gh/Parsely/parsely-android/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Parsely#diff-cGFyc2VseS9zcmMvbWFpbi9qYXZhL2NvbS9wYXJzZWx5L3BhcnNlbHlhbmRyb2lkL1BhcnNlbHlDb3JvdXRpbmVTY29wZS5rdA==) | `100.00% <100.00%> (ΓΈ)` | | | [...ava/com/parsely/parselyandroid/ParselyTracker.java](https://app.codecov.io/gh/Parsely/parsely-android/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Parsely#diff-cGFyc2VseS9zcmMvbWFpbi9qYXZhL2NvbS9wYXJzZWx5L3BhcnNlbHlhbmRyb2lkL1BhcnNlbHlUcmFja2VyLmphdmE=) | `11.62% <100.00%> (-0.27%)` | :arrow_down: | | [...in/java/com/parsely/parselyandroid/FlushManager.kt](https://app.codecov.io/gh/Parsely/parsely-android/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Parsely#diff-cGFyc2VseS9zcmMvbWFpbi9qYXZhL2NvbS9wYXJzZWx5L3BhcnNlbHlhbmRyb2lkL0ZsdXNoTWFuYWdlci5rdA==) | `63.63% <63.63%> (ΓΈ)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wzieba commented 1 year ago

I would still recommend having some actual manual testing instructions, to verify them and only then to proceed with merging this, even if that's just for documentation purposes. I am recommending that because FlushManager has been completely re-written to Kotlin + Coroutines, plus, the test suite for it was added after the fact, not before.

I agree that it's quite a big change (first of many to the SDK), but one of the reasons for adding functional tests, was to eliminate a need for manual testing. They are as close to the real scenario as possible - only instead of a person tapping buttons, we run SDK functions, and instead of a person validating logs - we have assertions. Do you have some additional scenario in mind that we should test here manually?

wzieba commented 1 year ago

@ParaskP7 heads up - in case of some changes/bugfixes that can be needed for current main, I've decided to change target of this PR to a new, long-running feature branch coroutines.

As soon as all code will be migrated to use Coroutines, I'll merge the long-running branch to main and prepare a version of SDK.

ParaskP7 commented 1 year ago

πŸ‘‹ @wzieba !

I agree that it's quite a big change (first of many to the SDK), but one of the reasons for adding functional tests, was to eliminate a need for manual testing.

πŸ’― but also, IMHO, elevating manual testing after merging this, not before, thus me suggested we still do manual testing on this PR. Still, I do understand were you are coming from. πŸ’―

They are as close to the real scenario as possible - only instead of a person tapping buttons, we run SDK functions, and instead of a person validating logs - we have assertions.

πŸ‘

Do you have some additional scenario in mind that we should test here manually?

Nothing comes to mind as I am not having all testing scenarios in my head, I just did some basic manual testing myself and wondered if I covered everything needed. Thus me trying to be on the safe side and asking you for an explicit testing scenario list so that I can be a bit more relaxed with this merge. 😊

ParaskP7 commented 1 year ago

@ParaskP7 heads up - in case of some changes/bugfixes that can be needed for current main, I've decided to change target of this PR to a new, long-running feature branch coroutines.

Oh nice, this kind of makes sense, thanks for the heads-up on that @wzieba ! πŸ‘

As soon as all code will be migrated to use Coroutines, I'll merge the long-running branch to main and prepare a version of SDK.

πŸ‘