From 448c1bd9ce6e74d316cee8a1d2411f0225dc5a42 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 21:16:53 +0100 Subject: [PATCH 01/13] tests: add functional test for engagement session --- .../parsely/parselyandroid/FunctionalTests.kt | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt index ead1058c..8a4f591c 100644 --- a/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt +++ b/parsely/src/androidTest/java/com/parsely/parselyandroid/FunctionalTests.kt @@ -28,6 +28,7 @@ import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.mockwebserver.RecordedRequest import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.within import org.junit.Test import org.junit.runner.RunWith @@ -190,6 +191,96 @@ class FunctionalTests { } } + /** + * In this scenario consumer app starts an engagement session and after 27150 ms, + * it stops the session. + * + * Intervals: + * With current implementation of `HeartbeatIntervalCalculator`, the next intervals are: + * - 10500ms for the first interval + * - 13650ms for the second interval + * + * So after ~27,2s we should observe + * - 2 `heartbeat` events from `startEngagement` + 1 `heartbeat` event caused by `stopEngagement` which is triggered during engagement interval + * + * Time off-differences in assertions are acceptable, because it's a time-sensitive test + */ + @Test + fun engagementManagerTest() { + val engagementUrl = "engagementUrl" + var startTimestamp = Duration.ZERO + val firstInterval = 10500.milliseconds + val secondInterval = 13650.milliseconds + val pauseInterval = 3.seconds + ActivityScenario.launch(SampleActivity::class.java).use { scenario -> + // given + scenario.onActivity { activity: Activity -> + beforeEach(activity) + server.enqueue(MockResponse().setResponseCode(200)) + parselyTracker = initializeTracker(activity, flushInterval = 30.seconds) + + // when + startTimestamp = System.currentTimeMillis().milliseconds + parselyTracker.startEngagement(engagementUrl, null) + } + + Thread.sleep((firstInterval + secondInterval + pauseInterval).inWholeMilliseconds) + parselyTracker.stopEngagement() + + // then + val request = server.takeRequest(35, TimeUnit.SECONDS)!!.toMap()["events"]!! + + assertThat( + request.sortedBy { it.data.timestamp } + .filter { it.action == "heartbeat" } + ).hasSize(3) + .satisfies({ + val firstEvent = it[0] + val secondEvent = it[1] + val thirdEvent = it[2] + + assertThat(firstEvent.data.timestamp).isCloseTo( + (startTimestamp + firstInterval).inWholeMilliseconds, + within(1.seconds.inWholeMilliseconds) + ) + assertThat(firstEvent.totalTime).isCloseTo( + firstInterval.inWholeMilliseconds, + within(100L) + ) + assertThat(firstEvent.incremental).isCloseTo( + firstInterval.inWholeSeconds, + within(1L) + ) + + assertThat(secondEvent.data.timestamp).isCloseTo( + (startTimestamp + firstInterval + secondInterval).inWholeMilliseconds, + within(1.seconds.inWholeMilliseconds) + ) + assertThat(secondEvent.totalTime).isCloseTo( + (firstInterval + secondInterval).inWholeMilliseconds, + within(100L) + ) + assertThat(secondEvent.incremental).isCloseTo( + secondInterval.inWholeSeconds, + within(1L) + ) + + assertThat(thirdEvent.data.timestamp).isCloseTo( + (startTimestamp + firstInterval + secondInterval + pauseInterval).inWholeMilliseconds, + within(1.seconds.inWholeMilliseconds) + ) + assertThat(thirdEvent.totalTime).isCloseTo( + (firstInterval + secondInterval + pauseInterval).inWholeMilliseconds, + within(100L) + ) + assertThat(thirdEvent.incremental).isCloseTo( + (pauseInterval).inWholeSeconds, + within(1L) + ) + }) + } + } + private fun RecordedRequest.toMap(): Map> { val listType: TypeReference>> = object : TypeReference>>() {} @@ -200,6 +291,15 @@ class FunctionalTests { @JsonIgnoreProperties(ignoreUnknown = true) data class Event( @JsonProperty("idsite") var idsite: String, + @JsonProperty("action") var action: String, + @JsonProperty("data") var data: ExtraData, + @JsonProperty("tt") var totalTime: Long, + @JsonProperty("inc") var incremental: Long, + ) + + @JsonIgnoreProperties(ignoreUnknown = true) + data class ExtraData( + @JsonProperty("ts") var timestamp: Long, ) private val locallyStoredEvents From 868a74dd839f3f3db23ff07c9317ef23359a38fe Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 13:46:57 +0100 Subject: [PATCH 02/13] Rename .java to .kt --- .../{EngagementManager.java => EngagementManager.kt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename parsely/src/main/java/com/parsely/parselyandroid/{EngagementManager.java => EngagementManager.kt} (100%) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.java b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt similarity index 100% rename from parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.java rename to parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt From 6dfbf42b56acae41481a90c3cc801b49ba288d0f Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 13:46:57 +0100 Subject: [PATCH 03/13] refactor: rewrite `EngagementManager` to Kotlin --- .../parselyandroid/EngagementManager.kt | 152 ++++++++---------- 1 file changed, 66 insertions(+), 86 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index 182dc407..9b697736 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -1,120 +1,100 @@ -package com.parsely.parselyandroid; +package com.parsely.parselyandroid -import java.util.Calendar; -import java.util.HashMap; -import java.util.Map; -import java.util.TimeZone; -import java.util.Timer; -import java.util.TimerTask; +import java.util.Calendar +import java.util.TimeZone +import java.util.Timer +import java.util.TimerTask /** * Engagement manager for article and video engagement. - *

+ * + * * Implemented to handle its own queuing of future executions to accomplish * two things: - *

+ * + * * 1. Flushing any engaged time before canceling. * 2. Progressive backoff for long engagements to save data. */ -class EngagementManager { - - private final ParselyTracker parselyTracker; - public Map baseEvent; - private boolean started; - private final Timer parentTimer; - private TimerTask waitingTimerTask; - private long latestDelayMillis, totalTime; - private Calendar startTime; - private final HeartbeatIntervalCalculator intervalCalculator; - - public EngagementManager( - ParselyTracker parselyTracker, - Timer parentTimer, - long intervalMillis, - Map baseEvent, - HeartbeatIntervalCalculator intervalCalculator - ) { - this.parselyTracker = parselyTracker; - this.baseEvent = baseEvent; - this.parentTimer = parentTimer; - this.intervalCalculator = intervalCalculator; - latestDelayMillis = intervalMillis; - totalTime = 0; - startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")); - } - - public boolean isRunning() { - return started; +internal class EngagementManager( + private val parselyTracker: ParselyTracker, + private val parentTimer: Timer, + private var latestDelayMillis: Long, + var baseEvent: Map, + private val intervalCalculator: HeartbeatIntervalCalculator +) { + var isRunning = false + private set + private var waitingTimerTask: TimerTask? = null + private var totalTime: Long = 0 + private var startTime: Calendar + + init { + startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")) } - public void start() { - scheduleNextExecution(latestDelayMillis); - started = true; - startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + fun start() { + scheduleNextExecution(latestDelayMillis) + isRunning = true + startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")) } - public void stop() { - waitingTimerTask.cancel(); - started = false; + fun stop() { + waitingTimerTask!!.cancel() + isRunning = false } - public boolean isSameVideo(String url, String urlRef, ParselyVideoMetadata metadata) { - Map baseMetadata = (Map) baseEvent.get("metadata"); - return (baseEvent.get("url").equals(url) && - baseEvent.get("urlref").equals(urlRef) && - baseMetadata.get("link").equals(metadata.link) && - (int) (baseMetadata.get("duration")) == metadata.durationSeconds); + fun isSameVideo(url: String, urlRef: String, metadata: ParselyVideoMetadata): Boolean { + val baseMetadata = baseEvent["metadata"] as Map? + return baseEvent["url"] == url && baseEvent["urlref"] == urlRef && baseMetadata!!["link"] == metadata.link && baseMetadata["duration"] as Int == metadata.durationSeconds } - private void scheduleNextExecution(long delay) { - TimerTask task = new TimerTask() { - public void run() { - doEnqueue(scheduledExecutionTime()); - latestDelayMillis = intervalCalculator.calculate(startTime); - scheduleNextExecution(latestDelayMillis); + private fun scheduleNextExecution(delay: Long) { + val task: TimerTask = object : TimerTask() { + override fun run() { + doEnqueue(scheduledExecutionTime()) + latestDelayMillis = intervalCalculator.calculate(startTime) + scheduleNextExecution(latestDelayMillis) } - public boolean cancel() { - boolean output = super.cancel(); + override fun cancel(): Boolean { + val output = super.cancel() // Only enqueue when we actually canceled something. If output is false then // this has already been canceled. if (output) { - doEnqueue(scheduledExecutionTime()); + doEnqueue(scheduledExecutionTime()) } - return output; + return output } - }; - latestDelayMillis = delay; - parentTimer.schedule(task, delay); - waitingTimerTask = task; + } + latestDelayMillis = delay + parentTimer.schedule(task, delay) + waitingTimerTask = task } - private void doEnqueue(long scheduledExecutionTime) { + private fun doEnqueue(scheduledExecutionTime: Long) { // Create a copy of the base event to enqueue - Map event = new HashMap<>(baseEvent); - ParselyTracker.PLog(String.format("Enqueuing %s event.", event.get("action"))); + val event: MutableMap = HashMap( + baseEvent + ) + ParselyTracker.PLog(String.format("Enqueuing %s event.", event["action"])) // Update `ts` for the event since it's happening right now. - Calendar now = Calendar.getInstance(TimeZone.getTimeZone("UTC")); - @SuppressWarnings("unchecked") - Map baseEventData = (Map) event.get("data"); - assert baseEventData != null; - Map data = new HashMap<>(baseEventData); - data.put("ts", now.getTimeInMillis()); - event.put("data", data); + val now = Calendar.getInstance(TimeZone.getTimeZone("UTC")) + val baseEventData = (event["data"] as Map?)!! + val data: MutableMap = HashMap(baseEventData) + data["ts"] = now.timeInMillis + event["data"] = data // Adjust inc by execution time in case we're late or early. - long executionDiff = (System.currentTimeMillis() - scheduledExecutionTime); - long inc = (latestDelayMillis + executionDiff); - totalTime += inc; - event.put("inc", inc / 1000); - event.put("tt", totalTime); - - parselyTracker.enqueueEvent(event); + val executionDiff = System.currentTimeMillis() - scheduledExecutionTime + val inc = latestDelayMillis + executionDiff + totalTime += inc + event["inc"] = inc / 1000 + event["tt"] = totalTime + parselyTracker.enqueueEvent(event) } - - public double getIntervalMillis() { - return latestDelayMillis; - } + val intervalMillis: Double + get() = latestDelayMillis.toDouble() } From 8279392d81da4b10aa39f4b97c1e60bf8368398c Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 15:40:44 +0100 Subject: [PATCH 04/13] tests: add test for `inc` parameter and `EngagementManager#stop` behavior It has to have longer delays as `inc` is calculated in seconds and test is time-sensitive. --- .../parselyandroid/EngagementManagerTest.kt | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index 6b5448c1..30af2730 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -4,6 +4,7 @@ import androidx.test.core.app.ApplicationProvider import java.util.Calendar import java.util.TimeZone import java.util.Timer +import kotlin.time.Duration.Companion.seconds import org.assertj.core.api.AbstractLongAssert import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.within @@ -92,6 +93,37 @@ internal class EngagementManagerTest { ) } + @Test + fun `given started manager, when stopping manager before interval ticks, then schedule an event`() { + // given + sut = EngagementManager( + tracker, + parentTimer, + 5.seconds.inWholeMilliseconds, + baseEvent, + object : FakeIntervalCalculator() { + override fun calculate(startTime: Calendar): Long { + return 5.seconds.inWholeMilliseconds + } + } + ) + sut.start() + + // when + sleep(12.seconds.inWholeMilliseconds) + sut.stop() + + // then + // first tick: after initial delay 5s, incremental addition 5s + // second tick: after regular delay 5s, incremental addition 5s + // third tick: after cancellation after 2s, incremental addition 2s + assertThat(tracker.events).hasSize(3).satisfies({ + assertThat(it[0]).containsEntry("inc", 5L) + assertThat(it[1]).containsEntry("inc", 5L) + assertThat(it[2]).containsEntry("inc", 2L) + }) + } + private fun sleep(millis: Long) = Thread.sleep(millis + THREAD_SLEEPING_THRESHOLD) private fun MapAssert.isCorrectEvent( @@ -130,7 +162,7 @@ internal class EngagementManagerTest { } } - class FakeIntervalCalculator : HeartbeatIntervalCalculator(Clock()) { + open class FakeIntervalCalculator : HeartbeatIntervalCalculator(Clock()) { override fun calculate(startTime: Calendar): Long { return DEFAULT_INTERVAL_MILLIS } @@ -138,6 +170,7 @@ internal class EngagementManagerTest { private companion object { const val DEFAULT_INTERVAL_MILLIS = 100L + // Additional time to wait to ensure that the timer has fired const val THREAD_SLEEPING_THRESHOLD = 50L val testData = mutableMapOf( From 728fd26f6c4c979d5ca3dea8a614efb0ccbdfd7d Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 15:09:30 +0100 Subject: [PATCH 05/13] feat: use Coroutines to enqueue events in `EngagementManager` 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. --- .../java/com/parsely/parselyandroid/Clock.kt | 3 +- .../parselyandroid/EngagementManager.kt | 34 ++++++++-- .../parselyandroid/ParselyTracker.java | 10 ++- .../parselyandroid/EngagementManagerTest.kt | 68 +++++++++++++------ 4 files changed, 82 insertions(+), 33 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/Clock.kt b/parsely/src/main/java/com/parsely/parselyandroid/Clock.kt index 2db30db8..42d937b3 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/Clock.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/Clock.kt @@ -2,9 +2,10 @@ package com.parsely.parselyandroid import java.util.Calendar import java.util.TimeZone +import kotlin.time.Duration import kotlin.time.Duration.Companion.milliseconds open class Clock { - open val now + open val now: Duration get() = Calendar.getInstance(TimeZone.getTimeZone("UTC")).timeInMillis.milliseconds } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index 9b697736..ab69cc7e 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -4,6 +4,11 @@ import java.util.Calendar import java.util.TimeZone import java.util.Timer import java.util.TimerTask +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.delay +import kotlinx.coroutines.isActive +import kotlinx.coroutines.launch /** * Engagement manager for article and video engagement. @@ -21,26 +26,42 @@ internal class EngagementManager( private val parentTimer: Timer, private var latestDelayMillis: Long, var baseEvent: Map, - private val intervalCalculator: HeartbeatIntervalCalculator + private val intervalCalculator: HeartbeatIntervalCalculator, + private val coroutineScope: CoroutineScope, + private val clock: Clock, ) { var isRunning = false private set private var waitingTimerTask: TimerTask? = null + private var job: Job? = null private var totalTime: Long = 0 private var startTime: Calendar + private var nextScheduledExecution: Long = 0 init { startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")) } fun start() { - scheduleNextExecution(latestDelayMillis) isRunning = true - startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")) + startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")).apply { + timeInMillis = clock.now.inWholeMilliseconds + } + job = coroutineScope.launch { + while (isActive) { + latestDelayMillis = intervalCalculator.calculate(startTime) + nextScheduledExecution = clock.now.inWholeMilliseconds + latestDelayMillis + delay(latestDelayMillis) + doEnqueue(clock.now.inWholeMilliseconds) + } + } } fun stop() { - waitingTimerTask!!.cancel() + job?.let { + it.cancel() + doEnqueue(nextScheduledExecution) + } isRunning = false } @@ -80,14 +101,13 @@ internal class EngagementManager( ParselyTracker.PLog(String.format("Enqueuing %s event.", event["action"])) // Update `ts` for the event since it's happening right now. - val now = Calendar.getInstance(TimeZone.getTimeZone("UTC")) val baseEventData = (event["data"] as Map?)!! val data: MutableMap = HashMap(baseEventData) - data["ts"] = now.timeInMillis + data["ts"] = clock.now.inWholeMilliseconds event["data"] = data // Adjust inc by execution time in case we're late or early. - val executionDiff = System.currentTimeMillis() - scheduledExecutionTime + val executionDiff = clock.now.inWholeMilliseconds - scheduledExecutionTime val inc = latestDelayMillis + executionDiff totalTime += inc event["inc"] = inc / 1000 diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 7ce0ebc0..c87d6d48 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -56,7 +56,9 @@ public class ParselyTracker { @NonNull private final EventsBuilder eventsBuilder; @NonNull - private final HeartbeatIntervalCalculator intervalCalculator = new HeartbeatIntervalCalculator(new Clock()); + private final Clock clock; + @NonNull + private final HeartbeatIntervalCalculator intervalCalculator; @NonNull private final LocalStorageRepository localStorageRepository; @NonNull @@ -81,6 +83,8 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { return Unit.INSTANCE; }); sendEvents = new SendEvents(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); + clock = new Clock(); + intervalCalculator = new HeartbeatIntervalCalculator(clock); // get the adkey straight away on instantiation timer = new Timer(); @@ -284,7 +288,7 @@ public void startEngagement( // Start a new EngagementTask Map event = eventsBuilder.buildEvent(url, urlRef, "heartbeat", null, extraData, lastPageviewUuid); - engagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, event, intervalCalculator); + engagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, event, intervalCalculator, ParselyCoroutineScopeKt.getSdkScope(), clock ); engagementManager.start(); } @@ -360,7 +364,7 @@ public void trackPlay( // Start a new engagement manager for the video. @NonNull final Map hbEvent = eventsBuilder.buildEvent(url, urlRef, "vheartbeat", videoMetadata, extraData, uuid); // TODO: Can we remove some metadata fields from this request? - videoEngagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, hbEvent, intervalCalculator); + videoEngagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, hbEvent, intervalCalculator, ParselyCoroutineScopeKt.getSdkScope(), clock); videoEngagementManager.start(); } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index 30af2730..f2135222 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -5,6 +5,14 @@ import java.util.Calendar import java.util.TimeZone import java.util.Timer import kotlin.time.Duration.Companion.seconds +import kotlin.time.Duration +import kotlin.time.Duration.Companion.milliseconds +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.currentTime +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest import org.assertj.core.api.AbstractLongAssert import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.within @@ -17,6 +25,7 @@ import org.robolectric.RobolectricTestRunner private typealias Event = MutableMap +@OptIn(ExperimentalCoroutinesApi::class) @RunWith(RobolectricTestRunner::class) internal class EngagementManagerTest { @@ -28,23 +37,23 @@ internal class EngagementManagerTest { "data" to testData ) - @Before - fun setUp() { + @Test + fun `when starting manager, then record the correct event after interval millis`() = runTest { + // when sut = EngagementManager( tracker, parentTimer, DEFAULT_INTERVAL_MILLIS, baseEvent, - FakeIntervalCalculator() + FakeIntervalCalculator(), + backgroundScope, + FakeClock(testScheduler), ) - } - @Test - fun `when starting manager, then record the correct event after interval millis`() { - // when sut.start() - sleep(DEFAULT_INTERVAL_MILLIS) - val timestamp = now - THREAD_SLEEPING_THRESHOLD + advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + runCurrent() + val timestamp = currentTime // then assertThat(tracker.events[0]).isCorrectEvent( @@ -56,19 +65,29 @@ internal class EngagementManagerTest { } @Test - fun `when starting manager, then schedule task each interval period`() { + fun `when starting manager, then schedule task each interval period`() = runTest { + // when + sut = EngagementManager( + tracker, + parentTimer, + DEFAULT_INTERVAL_MILLIS, + baseEvent, + FakeIntervalCalculator(), + backgroundScope, + FakeClock(testScheduler), + ) sut.start() - sleep(DEFAULT_INTERVAL_MILLIS) - val firstTimestamp = now - THREAD_SLEEPING_THRESHOLD + advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + val firstTimestamp = currentTime - sleep(DEFAULT_INTERVAL_MILLIS) - val secondTimestamp = now - 2 * THREAD_SLEEPING_THRESHOLD + advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + val secondTimestamp = currentTime - sleep(DEFAULT_INTERVAL_MILLIS) - val thirdTimestamp = now - 3 * THREAD_SLEEPING_THRESHOLD + advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + val thirdTimestamp = currentTime - sleep(THREAD_SLEEPING_THRESHOLD) + runCurrent() val firstEvent = tracker.events[0] assertThat(firstEvent).isCorrectEvent( @@ -94,7 +113,7 @@ internal class EngagementManagerTest { } @Test - fun `given started manager, when stopping manager before interval ticks, then schedule an event`() { + fun `given started manager, when stopping manager before interval ticks, then schedule an event`() = runTest { // given sut = EngagementManager( tracker, @@ -105,12 +124,14 @@ internal class EngagementManagerTest { override fun calculate(startTime: Calendar): Long { return 5.seconds.inWholeMilliseconds } - } + }, + this, + FakeClock(testScheduler) ) sut.start() // when - sleep(12.seconds.inWholeMilliseconds) + advanceTimeBy(12.seconds.inWholeMilliseconds) sut.stop() // then @@ -124,8 +145,6 @@ internal class EngagementManagerTest { }) } - private fun sleep(millis: Long) = Thread.sleep(millis + THREAD_SLEEPING_THRESHOLD) - private fun MapAssert.isCorrectEvent( withTotalTime: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, withTimestamp: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, @@ -168,6 +187,11 @@ internal class EngagementManagerTest { } } + class FakeClock(private val scheduler: TestCoroutineScheduler) : Clock() { + override val now: Duration + get() = scheduler.currentTime.milliseconds + } + private companion object { const val DEFAULT_INTERVAL_MILLIS = 100L From 56f1ea62f2e6aa5864bb00625932cc68461c79c8 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 18:49:51 +0100 Subject: [PATCH 06/13] tests: narrow down assertions for `EngagementManager` Now, as tests are no longer time-sensitive, it's possible to narrow down unit tests to check exact expected values. --- .../parselyandroid/EngagementManagerTest.kt | 43 +++++++------------ 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index f2135222..8b724ef9 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -4,9 +4,9 @@ import androidx.test.core.app.ApplicationProvider import java.util.Calendar import java.util.TimeZone import java.util.Timer -import kotlin.time.Duration.Companion.seconds import kotlin.time.Duration import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineScheduler import kotlinx.coroutines.test.advanceTimeBy @@ -15,10 +15,7 @@ import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.assertj.core.api.AbstractLongAssert import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.within -import org.assertj.core.api.Assertions.withinPercentage import org.assertj.core.api.MapAssert -import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -39,7 +36,7 @@ internal class EngagementManagerTest { @Test fun `when starting manager, then record the correct event after interval millis`() = runTest { - // when + // given sut = EngagementManager( tracker, parentTimer, @@ -50,23 +47,21 @@ internal class EngagementManagerTest { FakeClock(testScheduler), ) + // when sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS) runCurrent() - val timestamp = currentTime // then assertThat(tracker.events[0]).isCorrectEvent( - // Ideally: totalTime should be equal to DEFAULT_INTERVAL_MILLIS - withTotalTime = { isCloseTo(DEFAULT_INTERVAL_MILLIS, withinPercentage(10)) }, - // Ideally: timestamp should be equal to System.currentTimeMillis() at the time of recording the event - withTimestamp = { isCloseTo(timestamp, within(20L)) } + withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS) }, + withTimestamp = { isEqualTo(currentTime) } ) } @Test fun `when starting manager, then schedule task each interval period`() = runTest { - // when + // given sut = EngagementManager( tracker, parentTimer, @@ -78,6 +73,7 @@ internal class EngagementManagerTest { ) sut.start() + // when advanceTimeBy(DEFAULT_INTERVAL_MILLIS) val firstTimestamp = currentTime @@ -85,30 +81,24 @@ internal class EngagementManagerTest { val secondTimestamp = currentTime advanceTimeBy(DEFAULT_INTERVAL_MILLIS) - val thirdTimestamp = currentTime - runCurrent() + val thirdTimestamp = currentTime + // then val firstEvent = tracker.events[0] assertThat(firstEvent).isCorrectEvent( - // Ideally: totalTime should be equal to DEFAULT_INTERVAL_MILLIS - withTotalTime = { isCloseTo(DEFAULT_INTERVAL_MILLIS, withinPercentage(10)) }, - // Ideally: timestamp should be equal to `now` at the time of recording the event - withTimestamp = { isCloseTo(firstTimestamp, within(20L)) } + withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS) }, + withTimestamp = { isEqualTo(firstTimestamp) } ) val secondEvent = tracker.events[1] assertThat(secondEvent).isCorrectEvent( - // Ideally: totalTime should be equal to DEFAULT_INTERVAL_MILLIS * 2 - withTotalTime = { isCloseTo(DEFAULT_INTERVAL_MILLIS * 2, withinPercentage(10)) }, - // Ideally: timestamp should be equal to `now` at the time of recording the event - withTimestamp = { isCloseTo(secondTimestamp, within(20L)) } + withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS * 2) }, + withTimestamp = { isEqualTo(secondTimestamp) } ) val thirdEvent = tracker.events[2] assertThat(thirdEvent).isCorrectEvent( - // Ideally: totalTime should be equal to DEFAULT_INTERVAL_MILLIS * 3 - withTotalTime = { isCloseTo(DEFAULT_INTERVAL_MILLIS * 3, withinPercentage(10)) }, - // Ideally: timestamp should be equal to `now` at the time of recording the event - withTimestamp = { isCloseTo(thirdTimestamp, within(20L)) } + withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS * 3) }, + withTimestamp = { isEqualTo(thirdTimestamp) } ) } @@ -194,9 +184,6 @@ internal class EngagementManagerTest { private companion object { const val DEFAULT_INTERVAL_MILLIS = 100L - - // Additional time to wait to ensure that the timer has fired - const val THREAD_SLEEPING_THRESHOLD = 50L val testData = mutableMapOf( "os" to "android", "parsely_site_uuid" to "e8857cbe-5ace-44f4-a85e-7e7475f675c5", From 68ba19ed7e3b2f159e0a173544cec548e9c59709 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 18:59:20 +0100 Subject: [PATCH 07/13] tests: add tests for checking `inc` parameter As tests are now not time-sensitive, there's possibility to verify `inc` parameter as well without making test suite taking a long time. --- .../parselyandroid/EngagementManagerTest.kt | 63 +++++++++---------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index 8b724ef9..323fbec3 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -2,7 +2,6 @@ package com.parsely.parselyandroid import androidx.test.core.app.ApplicationProvider import java.util.Calendar -import java.util.TimeZone import java.util.Timer import kotlin.time.Duration import kotlin.time.Duration.Companion.milliseconds @@ -40,7 +39,7 @@ internal class EngagementManagerTest { sut = EngagementManager( tracker, parentTimer, - DEFAULT_INTERVAL_MILLIS, + DEFAULT_INTERVAL.inWholeMilliseconds, baseEvent, FakeIntervalCalculator(), backgroundScope, @@ -49,12 +48,13 @@ internal class EngagementManagerTest { // when sut.start() - advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + advanceTimeBy(DEFAULT_INTERVAL) runCurrent() // then assertThat(tracker.events[0]).isCorrectEvent( - withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS) }, + withIncrementalTime = { isEqualTo(DEFAULT_INTERVAL.inWholeSeconds)}, + withTotalTime = { isEqualTo(DEFAULT_INTERVAL.inWholeMilliseconds) }, withTimestamp = { isEqualTo(currentTime) } ) } @@ -65,7 +65,7 @@ internal class EngagementManagerTest { sut = EngagementManager( tracker, parentTimer, - DEFAULT_INTERVAL_MILLIS, + DEFAULT_INTERVAL.inWholeMilliseconds, baseEvent, FakeIntervalCalculator(), backgroundScope, @@ -74,30 +74,33 @@ internal class EngagementManagerTest { sut.start() // when - advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + advanceTimeBy(DEFAULT_INTERVAL) val firstTimestamp = currentTime - advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + advanceTimeBy(DEFAULT_INTERVAL) val secondTimestamp = currentTime - advanceTimeBy(DEFAULT_INTERVAL_MILLIS) + advanceTimeBy(DEFAULT_INTERVAL) runCurrent() val thirdTimestamp = currentTime // then val firstEvent = tracker.events[0] assertThat(firstEvent).isCorrectEvent( - withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS) }, + withIncrementalTime = { isEqualTo(DEFAULT_INTERVAL.inWholeSeconds) }, + withTotalTime = { isEqualTo(DEFAULT_INTERVAL.inWholeMilliseconds) }, withTimestamp = { isEqualTo(firstTimestamp) } ) val secondEvent = tracker.events[1] assertThat(secondEvent).isCorrectEvent( - withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS * 2) }, + withIncrementalTime = { isEqualTo(DEFAULT_INTERVAL.inWholeSeconds) }, + withTotalTime = { isEqualTo((DEFAULT_INTERVAL * 2).inWholeMilliseconds) }, withTimestamp = { isEqualTo(secondTimestamp) } ) val thirdEvent = tracker.events[2] assertThat(thirdEvent).isCorrectEvent( - withTotalTime = { isEqualTo(DEFAULT_INTERVAL_MILLIS * 3) }, + withIncrementalTime = { isEqualTo(DEFAULT_INTERVAL.inWholeSeconds) }, + withTotalTime = { isEqualTo((DEFAULT_INTERVAL * 3).inWholeMilliseconds) }, withTimestamp = { isEqualTo(thirdTimestamp) } ) } @@ -108,40 +111,39 @@ internal class EngagementManagerTest { sut = EngagementManager( tracker, parentTimer, - 5.seconds.inWholeMilliseconds, + DEFAULT_INTERVAL.inWholeMilliseconds, baseEvent, - object : FakeIntervalCalculator() { - override fun calculate(startTime: Calendar): Long { - return 5.seconds.inWholeMilliseconds - } - }, + FakeIntervalCalculator(), this, FakeClock(testScheduler) ) sut.start() // when - advanceTimeBy(12.seconds.inWholeMilliseconds) + advanceTimeBy(70.seconds.inWholeMilliseconds) sut.stop() // then - // first tick: after initial delay 5s, incremental addition 5s - // second tick: after regular delay 5s, incremental addition 5s - // third tick: after cancellation after 2s, incremental addition 2s + // first tick: after initial delay 30s, incremental addition 30s + // second tick: after regular delay 30s, incremental addition 30s + // third tick: after cancellation after 10s, incremental addition 10s assertThat(tracker.events).hasSize(3).satisfies({ - assertThat(it[0]).containsEntry("inc", 5L) - assertThat(it[1]).containsEntry("inc", 5L) - assertThat(it[2]).containsEntry("inc", 2L) + assertThat(it[0]).containsEntry("inc", 30L) + assertThat(it[1]).containsEntry("inc", 30L) + assertThat(it[2]).containsEntry("inc", 10L) }) } private fun MapAssert.isCorrectEvent( + withIncrementalTime: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, withTotalTime: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, withTimestamp: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, ): MapAssert { return containsEntry("action", "heartbeat") - // Incremental will be always 0 because the interval is lower than 1s - .containsEntry("inc", 0L) + .hasEntrySatisfying("inc") { incrementalTime -> + incrementalTime as Long + assertThat(incrementalTime).withIncrementalTime() + } .hasEntrySatisfying("tt") { totalTime -> totalTime as Long assertThat(totalTime).withTotalTime() @@ -156,9 +158,6 @@ internal class EngagementManagerTest { } } - private val now: Long - get() = Calendar.getInstance(TimeZone.getTimeZone("UTC")).timeInMillis - class FakeTracker : ParselyTracker( "", 0, @@ -171,9 +170,9 @@ internal class EngagementManagerTest { } } - open class FakeIntervalCalculator : HeartbeatIntervalCalculator(Clock()) { + class FakeIntervalCalculator : HeartbeatIntervalCalculator(Clock()) { override fun calculate(startTime: Calendar): Long { - return DEFAULT_INTERVAL_MILLIS + return DEFAULT_INTERVAL.inWholeMilliseconds } } @@ -183,7 +182,7 @@ internal class EngagementManagerTest { } private companion object { - const val DEFAULT_INTERVAL_MILLIS = 100L + val DEFAULT_INTERVAL = 30.seconds val testData = mutableMapOf( "os" to "android", "parsely_site_uuid" to "e8857cbe-5ace-44f4-a85e-7e7475f675c5", From 7e1d5915e22012babe4d9a503c0d1c3f38d6df5d Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 19:03:31 +0100 Subject: [PATCH 08/13] style: remove unused code --- .../parselyandroid/EngagementManager.kt | 27 ------------------- .../parselyandroid/ParselyTracker.java | 7 ++--- .../parselyandroid/EngagementManagerTest.kt | 5 ---- 3 files changed, 2 insertions(+), 37 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index ab69cc7e..7bed9672 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -2,8 +2,6 @@ package com.parsely.parselyandroid import java.util.Calendar import java.util.TimeZone -import java.util.Timer -import java.util.TimerTask import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay @@ -23,7 +21,6 @@ import kotlinx.coroutines.launch */ internal class EngagementManager( private val parselyTracker: ParselyTracker, - private val parentTimer: Timer, private var latestDelayMillis: Long, var baseEvent: Map, private val intervalCalculator: HeartbeatIntervalCalculator, @@ -32,7 +29,6 @@ internal class EngagementManager( ) { var isRunning = false private set - private var waitingTimerTask: TimerTask? = null private var job: Job? = null private var totalTime: Long = 0 private var startTime: Calendar @@ -70,29 +66,6 @@ internal class EngagementManager( return baseEvent["url"] == url && baseEvent["urlref"] == urlRef && baseMetadata!!["link"] == metadata.link && baseMetadata["duration"] as Int == metadata.durationSeconds } - private fun scheduleNextExecution(delay: Long) { - val task: TimerTask = object : TimerTask() { - override fun run() { - doEnqueue(scheduledExecutionTime()) - latestDelayMillis = intervalCalculator.calculate(startTime) - scheduleNextExecution(latestDelayMillis) - } - - override fun cancel(): Boolean { - val output = super.cancel() - // Only enqueue when we actually canceled something. If output is false then - // this has already been canceled. - if (output) { - doEnqueue(scheduledExecutionTime()) - } - return output - } - } - latestDelayMillis = delay - parentTimer.schedule(task, delay) - waitingTimerTask = task - } - private fun doEnqueue(scheduledExecutionTime: Long) { // Create a copy of the base event to enqueue val event: MutableMap = HashMap( diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index c87d6d48..8c44f8a5 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -28,7 +28,6 @@ import java.util.Formatter; import java.util.Map; -import java.util.Timer; import java.util.UUID; import kotlin.Unit; @@ -48,7 +47,6 @@ public class ParselyTracker { static final String ROOT_URL = "https://p1.parsely.com/".intern(); private boolean isDebug; private final Context context; - private final Timer timer; private final FlushManager flushManager; private EngagementManager engagementManager, videoEngagementManager; @Nullable @@ -87,7 +85,6 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { intervalCalculator = new HeartbeatIntervalCalculator(clock); // get the adkey straight away on instantiation - timer = new Timer(); isDebug = false; final SdkInit sdkInit = new SdkInit(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, flushManager); @@ -288,7 +285,7 @@ public void startEngagement( // Start a new EngagementTask Map event = eventsBuilder.buildEvent(url, urlRef, "heartbeat", null, extraData, lastPageviewUuid); - engagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, event, intervalCalculator, ParselyCoroutineScopeKt.getSdkScope(), clock ); + engagementManager = new EngagementManager(this, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, event, intervalCalculator, ParselyCoroutineScopeKt.getSdkScope(), clock ); engagementManager.start(); } @@ -364,7 +361,7 @@ public void trackPlay( // Start a new engagement manager for the video. @NonNull final Map hbEvent = eventsBuilder.buildEvent(url, urlRef, "vheartbeat", videoMetadata, extraData, uuid); // TODO: Can we remove some metadata fields from this request? - videoEngagementManager = new EngagementManager(this, timer, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, hbEvent, intervalCalculator, ParselyCoroutineScopeKt.getSdkScope(), clock); + videoEngagementManager = new EngagementManager(this, DEFAULT_ENGAGEMENT_INTERVAL_MILLIS, hbEvent, intervalCalculator, ParselyCoroutineScopeKt.getSdkScope(), clock); videoEngagementManager.start(); } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index 323fbec3..34374fcf 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -2,7 +2,6 @@ package com.parsely.parselyandroid import androidx.test.core.app.ApplicationProvider import java.util.Calendar -import java.util.Timer import kotlin.time.Duration import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds @@ -27,7 +26,6 @@ internal class EngagementManagerTest { private lateinit var sut: EngagementManager private val tracker = FakeTracker() - private val parentTimer = Timer() private val baseEvent: Event = mutableMapOf( "action" to "heartbeat", "data" to testData @@ -38,7 +36,6 @@ internal class EngagementManagerTest { // given sut = EngagementManager( tracker, - parentTimer, DEFAULT_INTERVAL.inWholeMilliseconds, baseEvent, FakeIntervalCalculator(), @@ -64,7 +61,6 @@ internal class EngagementManagerTest { // given sut = EngagementManager( tracker, - parentTimer, DEFAULT_INTERVAL.inWholeMilliseconds, baseEvent, FakeIntervalCalculator(), @@ -110,7 +106,6 @@ internal class EngagementManagerTest { // given sut = EngagementManager( tracker, - parentTimer, DEFAULT_INTERVAL.inWholeMilliseconds, baseEvent, FakeIntervalCalculator(), From 38ef0bba8f950da9c230853c01260f795c321927 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 19:06:48 +0100 Subject: [PATCH 09/13] tests: add unit tests for `EngagementManager#isRunning` --- .../parselyandroid/EngagementManagerTest.kt | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index 34374fcf..526cb28b 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -129,6 +129,45 @@ internal class EngagementManagerTest { }) } + @Test + fun `when starting manager, then it should return true for isRunning`() = runTest { + // given + sut = EngagementManager( + tracker, + DEFAULT_INTERVAL.inWholeMilliseconds, + baseEvent, + FakeIntervalCalculator(), + backgroundScope, + FakeClock(testScheduler) + ) + + // when + sut.start() + + // then + assertThat(sut.isRunning).isTrue + } + + @Test + fun `given started manager, when stoping manager, then it should return false for isRunning`() = runTest { + // given + sut = EngagementManager( + tracker, + DEFAULT_INTERVAL.inWholeMilliseconds, + baseEvent, + FakeIntervalCalculator(), + backgroundScope, + FakeClock(testScheduler) + ) + sut.start() + + // when + sut.stop() + + // then + assertThat(sut.isRunning).isFalse + } + private fun MapAssert.isCorrectEvent( withIncrementalTime: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, withTotalTime: AbstractLongAssert<*>.() -> AbstractLongAssert<*>, From d137dc48a2a82b6dadaaa393f1b5727adaa8d0d9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 19:10:00 +0100 Subject: [PATCH 10/13] refactor: use `Job#isActive` to determine `EngagementManager` state instead of private flag --- .../java/com/parsely/parselyandroid/EngagementManager.kt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index 7bed9672..80b694c4 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -27,8 +27,6 @@ internal class EngagementManager( private val coroutineScope: CoroutineScope, private val clock: Clock, ) { - var isRunning = false - private set private var job: Job? = null private var totalTime: Long = 0 private var startTime: Calendar @@ -38,8 +36,10 @@ internal class EngagementManager( startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")) } + val isRunning: Boolean + get() = job?.isActive ?: false + fun start() { - isRunning = true startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")).apply { timeInMillis = clock.now.inWholeMilliseconds } @@ -58,7 +58,6 @@ internal class EngagementManager( it.cancel() doEnqueue(nextScheduledExecution) } - isRunning = false } fun isSameVideo(url: String, urlRef: String, metadata: ParselyVideoMetadata): Boolean { From 47d230360fe7bd0e59846637480a50a5ecf46ba9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 19:18:17 +0100 Subject: [PATCH 11/13] style: make `baseEvent` private and immutable --- .../main/java/com/parsely/parselyandroid/EngagementManager.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index 80b694c4..24f0b780 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -22,7 +22,7 @@ import kotlinx.coroutines.launch internal class EngagementManager( private val parselyTracker: ParselyTracker, private var latestDelayMillis: Long, - var baseEvent: Map, + private val baseEvent: Map, private val intervalCalculator: HeartbeatIntervalCalculator, private val coroutineScope: CoroutineScope, private val clock: Clock, From 5cbea9f90fdb53a954d2a64131a00429d0ade9db Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Mon, 13 Nov 2023 19:23:29 +0100 Subject: [PATCH 12/13] refactor: drop usages of `java.util.Calendar` In favor of timezone agnostic `kotlin.time.Duration`. It simplifies implementation and removes a need to declare a timezone, which might be confusing. --- .../com/parsely/parselyandroid/EngagementManager.kt | 11 ++++------- .../parselyandroid/HeartbeatIntervalCalculator.kt | 9 +++------ .../parsely/parselyandroid/EngagementManagerTest.kt | 3 +-- .../HeartbeatIntervalCalculatorTest.kt | 12 +++--------- 4 files changed, 11 insertions(+), 24 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index 24f0b780..aae2d419 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -1,7 +1,6 @@ package com.parsely.parselyandroid -import java.util.Calendar -import java.util.TimeZone +import kotlin.time.Duration import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay @@ -29,20 +28,18 @@ internal class EngagementManager( ) { private var job: Job? = null private var totalTime: Long = 0 - private var startTime: Calendar + private var startTime: Duration private var nextScheduledExecution: Long = 0 init { - startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")) + startTime = clock.now } val isRunning: Boolean get() = job?.isActive ?: false fun start() { - startTime = Calendar.getInstance(TimeZone.getTimeZone("UTC")).apply { - timeInMillis = clock.now.inWholeMilliseconds - } + startTime = clock.now job = coroutineScope.launch { while (isActive) { latestDelayMillis = intervalCalculator.calculate(startTime) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/HeartbeatIntervalCalculator.kt b/parsely/src/main/java/com/parsely/parselyandroid/HeartbeatIntervalCalculator.kt index 7e1312f7..d50223ff 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/HeartbeatIntervalCalculator.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/HeartbeatIntervalCalculator.kt @@ -1,18 +1,15 @@ package com.parsely.parselyandroid -import java.util.Calendar -import kotlin.time.Duration.Companion.hours -import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration import kotlin.time.Duration.Companion.minutes import kotlin.time.Duration.Companion.seconds internal open class HeartbeatIntervalCalculator(private val clock: Clock) { - open fun calculate(startTime: Calendar): Long { - val startTimeDuration = startTime.time.time.milliseconds + open fun calculate(startTime: Duration): Long { val nowDuration = clock.now - val totalTrackedTime = nowDuration - startTimeDuration + val totalTrackedTime = nowDuration - startTime val totalWithOffset = totalTrackedTime + OFFSET_MATCHING_BASE_INTERVAL val newInterval = totalWithOffset * BACKOFF_PROPORTION val clampedNewInterval = minOf(MAX_TIME_BETWEEN_HEARTBEATS, newInterval) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt index 526cb28b..a7e5df9e 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/EngagementManagerTest.kt @@ -1,7 +1,6 @@ package com.parsely.parselyandroid import androidx.test.core.app.ApplicationProvider -import java.util.Calendar import kotlin.time.Duration import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.seconds @@ -205,7 +204,7 @@ internal class EngagementManagerTest { } class FakeIntervalCalculator : HeartbeatIntervalCalculator(Clock()) { - override fun calculate(startTime: Calendar): Long { + override fun calculate(startTime: Duration): Long { return DEFAULT_INTERVAL.inWholeMilliseconds } } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/HeartbeatIntervalCalculatorTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/HeartbeatIntervalCalculatorTest.kt index e0f3ffd7..eb5c420e 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/HeartbeatIntervalCalculatorTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/HeartbeatIntervalCalculatorTest.kt @@ -22,9 +22,7 @@ internal class HeartbeatIntervalCalculatorTest { fun `given the same time of start and current time, when calculating interval, return offset times backoff proportion`() { // given fakeClock.fakeNow = Duration.ZERO - val startTime = Calendar.getInstance().apply { - timeInMillis = 0 - } + val startTime = Duration.ZERO // when val result = sut.calculate(startTime) @@ -45,9 +43,7 @@ internal class HeartbeatIntervalCalculatorTest { // (15 minutes / 0.3) - 35 seconds = 2965 seconds. Add 1 second to be over the limit val excessiveTime = 2965.seconds + 1.seconds fakeClock.fakeNow = excessiveTime - val startTime = Calendar.getInstance().apply { - timeInMillis = 0 - } + val startTime = Duration.ZERO // when val result = sut.calculate(startTime) @@ -59,9 +55,7 @@ internal class HeartbeatIntervalCalculatorTest { @Test fun `given a specific time point, when updating latest interval, it correctly calculates the interval`() { // given - val startTime = Calendar.getInstance().apply { - timeInMillis = 0 - } + val startTime = Duration.ZERO fakeClock.fakeNow = 2.seconds // when From d282f8b1117103d251ce3dc2a213b3a27c72f3b3 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 1 Dec 2023 18:54:09 +0100 Subject: [PATCH 13/13] refactor: make `startTime` a local variable --- .../java/com/parsely/parselyandroid/EngagementManager.kt | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt index aae2d419..2f1dc620 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/EngagementManager.kt @@ -28,18 +28,13 @@ internal class EngagementManager( ) { private var job: Job? = null private var totalTime: Long = 0 - private var startTime: Duration private var nextScheduledExecution: Long = 0 - init { - startTime = clock.now - } - val isRunning: Boolean get() = job?.isActive ?: false fun start() { - startTime = clock.now + val startTime = clock.now job = coroutineScope.launch { while (isActive) { latestDelayMillis = intervalCalculator.calculate(startTime)