Refactor `FlushManager` to Kotlin Coroutines #86

wzieba · 2023-10-31T16:20:18Z

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 👍

This class is relatively simple and difficult to test because of `java.utils.Timer`. That's why I decided to make migration to Kotlin right away, without unit tests coverage first.

The returned `Boolean` wasn't used anywhere.

Added a new test to validate the behavior of the FlushManager's queue when two timers are initiated sequentially. The test asserts that the queue flushes correctly as per the time of the first timer's start.

codecov · 2023-10-31T16:28:16Z

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

@@              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	Coverage Δ
...om/parsely/parselyandroid/ParselyCoroutineScope.kt	`100.00% <100.00%> (ø)`
...ava/com/parsely/parselyandroid/ParselyTracker.java	`11.62% <100.00%> (-0.27%)`	⬇️
...in/java/com/parsely/parselyandroid/FlushManager.kt	`63.63% <63.63%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

To give SDK time to prepare and trigger the HTTP request. 100ms is to small timeout for CI emulator. It's enough for local builds and Firebase Test Lab tests. There's no business logic mistake - just the CI emulator is extremly slow.

wzieba · 2023-11-01T13:04:14Z

parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt

+
+            Thread.sleep((flushInterval / 2).inWholeMilliseconds)
+
+            val firstRequestPayload = server.takeRequest(2000, TimeUnit.MILLISECONDS)?.toMap()


Such a long waiting time is needed because of poor performance on GitHub Actions emulator. Also, the tests are executed differently, comparing to local execution (both device and same-settings emulator) and Firebase Test Lab. You can compare the logs, that just after [Parsely] Sending request with 2 events log, GitHub Actions emulator sends test activity to Paused (Lifecycle status change: com.parsely.parselyandroid.FunctionalTests$SampleActivity@316b36c in: PAUSED), when it doesn't happen anywhere else...

All of this makes me think about pivoting to:

using FIrebase Test Lab instead CI emulator

moving from emulator-based functional test to Robolectric

using FIrebase Test Lab instead CI emulator

Hmmm, maybe, not sure? 🤔

moving from emulator-based functional test to Robolectric

Hmmm, if I could vote, I would say to NOT making that move to Robolectric. 😊

ParaskP7

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, everything works as expected, great job, one more test suite live! 🌟 🧪 🌟

I have left few questions (❓) and a minor (🔍) comment for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

EXTRAS

Suggestion (💡): For this PR, and although it seems safe to merge by just depending on CI, 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. Just for us to be on the safe side, wdyt? 🤷

ParaskP7 · 2023-11-02T13:35:09Z

parsely/build.gradle

@@ -63,12 +64,14 @@ dependencies {
    implementation 'com.fasterxml.jackson.core:jackson-databind:2.13.3'
    implementation 'com.google.android.gms:play-services-ads-identifier:18.0.1'
    implementation 'androidx.lifecycle:lifecycle-process:2.6.2'
+    implementation "org.jetbrains.kotlinx:kotlinx-coroutines-android:$coroutinesVersion"


Praise (🎉): Woohoo! 🥇

parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt

parsely/src/main/java/com/parsely/parselyandroid/ParselyCoroutineScope.kt

ParaskP7 · 2023-11-02T13:45:17Z

parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt

+
+            Thread.sleep((flushInterval / 2).inWholeMilliseconds)
+
+            val firstRequestPayload = server.takeRequest(2000, TimeUnit.MILLISECONDS)?.toMap()


using FIrebase Test Lab instead CI emulator

Hmmm, maybe, not sure? 🤔

moving from emulator-based functional test to Robolectric

Hmmm, if I could vote, I would say to NOT making that move to Robolectric. 😊

wzieba · 2023-11-03T11:06:13Z

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 · 2023-11-03T11:10:12Z

@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 · 2023-11-03T11:26:36Z

👋 @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 · 2023-11-03T11:27:24Z

@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.

👍

wzieba added 11 commits October 31, 2023 17:12

tests: add functional test for validating flushing queue

0395b31

refactor: extract FlushManager to separate class

4d5ca5d

Rename .java to .kt

8b9a247

refactor: move the class to Kotlin

a5511de

This class is relatively simple and difficult to test because of `java.utils.Timer`. That's why I decided to make migration to Kotlin right away, without unit tests coverage first.

refactor: make stop return void

3fc205f

The returned `Boolean` wasn't used anywhere.

build: add coroutines dependency

171ed10

feat: rewrite FlushManager to coroutines

7f3e28b

tests: add unit tests for FlushManager

ae2bac5

tests: "double start" FlushManager test case

9b9e527

Added a new test to validate the behavior of the FlushManager's queue when two timers are initiated sequentially. The test asserts that the queue flushes correctly as per the time of the first timer's start.

refactor: remove unnecessary null check on job

4f3f4c1

tests: reduce time of default interval in tests to 5sec

b5bc33e

wzieba marked this pull request as draft October 31, 2023 18:02

tests: increase request waiting timeout to 2s

1326644

To give SDK time to prepare and trigger the HTTP request. 100ms is to small timeout for CI emulator. It's enough for local builds and Firebase Test Lab tests. There's no business logic mistake - just the CI emulator is extremly slow.

wzieba force-pushed the extract_flush_manager branch from 86d811f to 1326644 Compare November 1, 2023 12:30

wzieba commented Nov 1, 2023

View reviewed changes

wzieba marked this pull request as ready for review November 1, 2023 13:04

wzieba requested a review from ParaskP7 November 1, 2023 13:04

ParaskP7 approved these changes Nov 2, 2023

View reviewed changes

wzieba changed the base branch from main to coroutines November 3, 2023 11:08

style: simplify FlushManager to have one-liners

aad4629

wzieba merged commit 86bac35 into coroutines Nov 3, 2023

wzieba mentioned this pull request Nov 7, 2023

Migrate threading model to Coroutines - long running branch #89

Merged

wzieba mentioned this pull request Nov 29, 2023

Engagement manager to coroutines #95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `FlushManager` to Kotlin Coroutines #86

Refactor `FlushManager` to Kotlin Coroutines #86

wzieba commented Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

Loading

wzieba Nov 1, 2023

ParaskP7 Nov 2, 2023

ParaskP7 left a comment

ParaskP7 Nov 2, 2023

ParaskP7 Nov 2, 2023

wzieba commented Nov 3, 2023

wzieba commented Nov 3, 2023

ParaskP7 commented Nov 3, 2023

ParaskP7 commented Nov 3, 2023


		Thread.sleep((flushInterval / 2).inWholeMilliseconds)

		val firstRequestPayload = server.takeRequest(2000, TimeUnit.MILLISECONDS)?.toMap()

Refactor FlushManager to Kotlin Coroutines #86

Refactor FlushManager to Kotlin Coroutines #86

Conversation

wzieba commented Oct 31, 2023

Description

To test

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

wzieba Nov 1, 2023

Choose a reason for hiding this comment

ParaskP7 Nov 2, 2023

Choose a reason for hiding this comment

ParaskP7 left a comment

Choose a reason for hiding this comment

ParaskP7 Nov 2, 2023

Choose a reason for hiding this comment

ParaskP7 Nov 2, 2023

Choose a reason for hiding this comment

wzieba commented Nov 3, 2023

wzieba commented Nov 3, 2023

ParaskP7 commented Nov 3, 2023

ParaskP7 commented Nov 3, 2023

Refactor `FlushManager` to Kotlin Coroutines #86

Refactor `FlushManager` to Kotlin Coroutines #86

codecov bot commented Oct 31, 2023 •

edited

Loading