From 366dae5669de3be03dbb73dd4b84180aabe1cf87 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:05:48 +0100 Subject: [PATCH 01/14] refactor: rewrite SdkInit to simple function --- .../parsely/parselyandroid/ParselyTracker.java | 7 +++++-- .../java/com/parsely/parselyandroid/SdkInit.kt | 16 +++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index 7ce0ebc0..f8226291 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -86,8 +86,11 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { timer = new Timer(); isDebug = false; - final SdkInit sdkInit = new SdkInit(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, flushManager); - sdkInit.initialize(); + SdkInitKt.initialize( + ParselyCoroutineScopeKt.getSdkScope(), + localStorageRepository, + flushManager + ); ProcessLifecycleOwner.get().getLifecycle().addObserver( (LifecycleEventObserver) (lifecycleOwner, event) -> { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt index 6e5de8c1..07e00cc5 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt @@ -3,16 +3,14 @@ package com.parsely.parselyandroid import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch -internal class SdkInit( - private val scope: CoroutineScope, - private val localStorageRepository: LocalStorageRepository, - private val flushManager: FlushManager, +internal fun initialize( + scope: CoroutineScope, + localStorageRepository: LocalStorageRepository, + flushManager: FlushManager, ) { - fun initialize() { - scope.launch { - if (localStorageRepository.getStoredQueue().isNotEmpty()) { - flushManager.start() - } + scope.launch { + if (localStorageRepository.getStoredQueue().isNotEmpty()) { + flushManager.start() } } } From acae5f688b553a94dd21ceb92a73c2a179e17752 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:36:35 +0100 Subject: [PATCH 02/14] refactor: introduce `FlushManager` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../parsely/parselyandroid/FlushManager.kt | 21 ++++++++++---- .../parselyandroid/ParselyTracker.java | 2 +- .../parselyandroid/FlushManagerTest.kt | 10 +++---- .../parsely/parselyandroid/SendEventsTest.kt | 29 ++++++++++--------- 4 files changed, 37 insertions(+), 25 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt index d351a181..1a84e7a6 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt @@ -13,14 +13,21 @@ import kotlinx.coroutines.launch * Handles stopping and starting the flush timer. The flush timer * controls how often we send events to Parse.ly servers. */ -internal open class FlushManager( +internal interface FlushManager { + fun start() + fun stop() + val isRunning: Boolean + val intervalMillis: Long +} + +internal class ParselyFlushManager( private val parselyTracker: ParselyTracker, - val intervalMillis: Long, + override val intervalMillis: Long, private val coroutineScope: CoroutineScope -) { +) : FlushManager { private var job: Job? = null - open fun start() { + override fun start() { if (job?.isActive == true) return job = coroutineScope.launch { @@ -31,8 +38,10 @@ internal open class FlushManager( } } - open fun stop() = job?.cancel() + override fun stop() { + job?.cancel() + } - open val isRunning: Boolean + override val isRunning: Boolean get() = job?.isActive ?: false } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index f8226291..eb309c67 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -71,7 +71,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { context = c.getApplicationContext(); eventsBuilder = new EventsBuilder(context, siteId); localStorageRepository = new LocalStorageRepository(context); - flushManager = new FlushManager(this, flushInterval * 1000L, + flushManager = new ParselyFlushManager(this, flushInterval * 1000L, ParselyCoroutineScopeKt.getSdkScope()); inMemoryBuffer = new InMemoryBuffer(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, () -> { if (!flushTimerIsActive()) { diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt index cf2ef157..25f987c3 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt @@ -20,7 +20,7 @@ class FlushManagerTest { @Test fun `when timer starts and interval time passes, then flush queue`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS) @@ -31,7 +31,7 @@ class FlushManagerTest { @Test fun `when timer starts and three interval time passes, then flush queue 3 times`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(3 * DEFAULT_INTERVAL_MILLIS) @@ -43,7 +43,7 @@ class FlushManagerTest { @Test fun `when timer starts and is stopped after 2 intervals passes, then flush queue 2 times`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(2 * DEFAULT_INTERVAL_MILLIS) @@ -58,7 +58,7 @@ class FlushManagerTest { @Test fun `when timer starts, is stopped before end of interval and then time of interval passes, then do not flush queue`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) @@ -73,7 +73,7 @@ class FlushManagerTest { @Test fun `when timer starts, and another timer starts after some time, then flush queue according to the first start`() = runTest { - sut = FlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index f3a0991f..da5ffdfb 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -21,7 +21,7 @@ class SendEventsTest { runTest { // given sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), FakeLocalStorageRepository(), FakeParselyAPIConnection(), this @@ -46,7 +46,7 @@ class SendEventsTest { nextResult = Result.success(Unit) } sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), repository, parselyAPIConnection, this @@ -68,7 +68,7 @@ class SendEventsTest { insertEvents(listOf(mapOf("test" to 123))) } sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), repository, FakeParselyAPIConnection(), this @@ -93,7 +93,7 @@ class SendEventsTest { nextResult = Result.failure(Exception()) } sut = SendEvents( - FakeFlushManager(this), + FakeFlushManager(), repository, parselyAPIConnection, this @@ -111,7 +111,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when sending events, then flush manager is stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() val repository = FakeLocalStorageRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } @@ -137,7 +137,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when sending events fails, then flush manager is not stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() val repository = FakeLocalStorageRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } @@ -163,7 +163,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when storage is not empty after successful event, then flush manager is not stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() val repository = object : FakeLocalStorageRepository() { override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(listOf(mapOf("test" to 123))) @@ -190,7 +190,7 @@ class SendEventsTest { @Test fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { // given - val flushManager = FakeFlushManager(this) + val flushManager = FakeFlushManager() sut = SendEvents( flushManager, FakeLocalStorageRepository(), @@ -206,17 +206,20 @@ class SendEventsTest { assertThat(flushManager.stopped).isTrue() } - private class FakeFlushManager(scope: CoroutineScope) : FlushManager(FakeTracker(), 10, scope) { + private class FakeFlushManager : FlushManager { var stopped = false + override fun start() { + TODO("Not implemented") + } override fun stop() { stopped = true } - } - private class FakeTracker : ParselyTracker( - "siteId", 10, ApplicationProvider.getApplicationContext() - ) { + override val isRunning + get() = TODO("Not implemented") + override val intervalMillis + get() = TODO("Not implemented") } private open class FakeLocalStorageRepository : From 77303c61969bb1418844763f83be7f0a871b9356 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:44:37 +0100 Subject: [PATCH 03/14] refactor: introduce `QueueManager` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../com/parsely/parselyandroid/InMemoryBuffer.kt | 2 +- .../parselyandroid/LocalStorageRepository.kt | 14 ++++++++++---- .../java/com/parsely/parselyandroid/SendEvents.kt | 2 +- .../parsely/parselyandroid/InMemoryBufferTest.kt | 8 +++++--- .../com/parsely/parselyandroid/SendEventsTest.kt | 3 +-- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt b/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt index c92c0c7a..619e993d 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/InMemoryBuffer.kt @@ -10,7 +10,7 @@ import kotlinx.coroutines.sync.withLock internal class InMemoryBuffer( private val coroutineScope: CoroutineScope, - private val localStorageRepository: LocalStorageRepository, + private val localStorageRepository: QueueRepository, private val onEventAddedListener: () -> Unit, ) { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index 038d38d6..dc7d7134 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -8,7 +8,13 @@ import java.io.ObjectOutputStream import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -internal open class LocalStorageRepository(private val context: Context) { +interface QueueRepository { + suspend fun remove(toRemove: List?>) + suspend fun getStoredQueue(): ArrayList?> + suspend fun insertEvents(toInsert: List?>) +} + +internal class LocalStorageRepository(private val context: Context): QueueRepository { private val mutex = Mutex() @@ -32,7 +38,7 @@ internal open class LocalStorageRepository(private val context: Context) { } } - open suspend fun remove(toRemove: List?>) { + override suspend fun remove(toRemove: List?>) { val storedEvents = getStoredQueue() mutex.withLock { @@ -45,7 +51,7 @@ internal open class LocalStorageRepository(private val context: Context) { * * @return The stored queue of events. */ - open suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { + override suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { var storedQueue: ArrayList?> = ArrayList() try { @@ -71,7 +77,7 @@ internal open class LocalStorageRepository(private val context: Context) { /** * Save the event queue to persistent storage. */ - open suspend fun insertEvents(toInsert: List?>){ + override suspend fun insertEvents(toInsert: List?>){ val storedEvents = getStoredQueue() mutex.withLock { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 4172debb..1be22b5d 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -8,7 +8,7 @@ import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, - private val localStorageRepository: LocalStorageRepository, + private val localStorageRepository: QueueRepository, private val parselyAPIConnection: ParselyAPIConnection, private val scope: CoroutineScope ) { diff --git a/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt index 71266c46..e4f354ff 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/InMemoryBufferTest.kt @@ -1,6 +1,5 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel @@ -78,8 +77,7 @@ internal class InMemoryBufferTest { assertThat(repository.getStoredQueue()).containsOnlyOnceElementsOf(events) } - class FakeLocalStorageRepository : - LocalStorageRepository(ApplicationProvider.getApplicationContext()) { + class FakeLocalStorageRepository : QueueRepository { private val events = mutableListOf?>() @@ -87,6 +85,10 @@ internal class InMemoryBufferTest { events.addAll(toInsert) } + override suspend fun remove(toRemove: List?>) { + TODO("Not implemented") + } + override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(events) } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index da5ffdfb..677d1892 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -222,8 +222,7 @@ class SendEventsTest { get() = TODO("Not implemented") } - private open class FakeLocalStorageRepository : - LocalStorageRepository(ApplicationProvider.getApplicationContext()) { + private class FakeLocalStorageRepository : QueueRepository { private var storage = emptyList?>() override suspend fun insertEvents(toInsert: List?>) { From be74b2898171be16d0d08491a5cd664e2fb4ef79 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:46:15 +0100 Subject: [PATCH 04/14] refactor: introduce `QueueManager` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../parselyandroid/LocalStorageRepository.kt | 2 +- .../com/parsely/parselyandroid/SendEvents.kt | 10 ++++----- .../parsely/parselyandroid/SendEventsTest.kt | 22 +++++++++---------- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index dc7d7134..6bc9bd24 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -8,7 +8,7 @@ import java.io.ObjectOutputStream import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -interface QueueRepository { +internal interface QueueRepository { suspend fun remove(toRemove: List?>) suspend fun getStoredQueue(): ArrayList?> suspend fun insertEvents(toInsert: List?>) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index 1be22b5d..e613f13b 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -8,7 +8,7 @@ import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, - private val localStorageRepository: QueueRepository, + private val repository: QueueRepository, private val parselyAPIConnection: ParselyAPIConnection, private val scope: CoroutineScope ) { @@ -18,7 +18,7 @@ internal class SendEvents( operator fun invoke(isDebug: Boolean) { scope.launch { mutex.withLock { - val eventsToSend = localStorageRepository.getStoredQueue() + val eventsToSend = repository.getStoredQueue() if (eventsToSend.isEmpty()) { flushManager.stop() @@ -32,16 +32,16 @@ internal class SendEvents( if (isDebug) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") - localStorageRepository.remove(eventsToSend) + repository.remove(eventsToSend) } else { ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) parselyAPIConnection.send(jsonPayload) .fold( onSuccess = { ParselyTracker.PLog("Pixel request success") - localStorageRepository.remove(eventsToSend) + repository.remove(eventsToSend) ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (localStorageRepository.getStoredQueue().isEmpty()) { + if (repository.getStoredQueue().isEmpty()) { flushManager.stop() } }, diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index 677d1892..cb3a2524 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -1,7 +1,5 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest @@ -22,7 +20,7 @@ class SendEventsTest { // given sut = SendEvents( FakeFlushManager(), - FakeLocalStorageRepository(), + FakeRepository(), FakeParselyAPIConnection(), this ) @@ -32,14 +30,14 @@ class SendEventsTest { runCurrent() // then - assertThat(FakeLocalStorageRepository().getStoredQueue()).isEmpty() + assertThat(FakeRepository().getStoredQueue()).isEmpty() } @Test fun `given non-empty local storage and debug mode off, when sending events, then events are sent and removed from local storage`() = runTest { // given - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -64,7 +62,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode on, when sending events, then events are not sent and removed from local storage`() = runTest { // given - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } sut = SendEvents( @@ -86,7 +84,7 @@ class SendEventsTest { fun `given non-empty local storage and debug mode off, when sending events fails, then events are not removed from local storage`() = runTest { // given - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -112,7 +110,7 @@ class SendEventsTest { runTest { // given val flushManager = FakeFlushManager() - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -138,7 +136,7 @@ class SendEventsTest { runTest { // given val flushManager = FakeFlushManager() - val repository = FakeLocalStorageRepository().apply { + val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } val parselyAPIConnection = FakeParselyAPIConnection().apply { @@ -164,7 +162,7 @@ class SendEventsTest { runTest { // given val flushManager = FakeFlushManager() - val repository = object : FakeLocalStorageRepository() { + val repository = object : FakeRepository() { override suspend fun getStoredQueue(): ArrayList?> { return ArrayList(listOf(mapOf("test" to 123))) } @@ -193,7 +191,7 @@ class SendEventsTest { val flushManager = FakeFlushManager() sut = SendEvents( flushManager, - FakeLocalStorageRepository(), + FakeRepository(), FakeParselyAPIConnection(), this ) @@ -222,7 +220,7 @@ class SendEventsTest { get() = TODO("Not implemented") } - private class FakeLocalStorageRepository : QueueRepository { + private open class FakeRepository : QueueRepository { private var storage = emptyList?>() override suspend fun insertEvents(toInsert: List?>) { From a13485cdf98815ab9016de458562bd8d8d23dfde Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 12:53:08 +0100 Subject: [PATCH 05/14] refactor: introduce `RestClient` interface Makes creating fake objects less problematic by removing a need to call real object constructor. --- .../parselyandroid/ParselyAPIConnection.kt | 11 ++++++----- .../com/parsely/parselyandroid/SendEvents.kt | 4 ++-- .../parsely/parselyandroid/SendEventsTest.kt | 18 +++++++++--------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt index 91d4aa3e..c1c1e422 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyAPIConnection.kt @@ -17,12 +17,13 @@ package com.parsely.parselyandroid import java.net.HttpURLConnection import java.net.URL -import kotlinx.coroutines.CoroutineDispatcher -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.withContext -internal open class ParselyAPIConnection(private val url: String) { - open suspend fun send(payload: String): Result { +internal interface RestClient { + suspend fun send(payload: String): Result +} + +internal class ParselyAPIConnection(private val url: String) : RestClient { + override suspend fun send(payload: String): Result { var connection: HttpURLConnection? = null try { connection = URL(url).openConnection() as HttpURLConnection diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt index e613f13b..b1f06e20 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt @@ -9,7 +9,7 @@ import kotlinx.coroutines.sync.withLock internal class SendEvents( private val flushManager: FlushManager, private val repository: QueueRepository, - private val parselyAPIConnection: ParselyAPIConnection, + private val restClient: RestClient, private val scope: CoroutineScope ) { @@ -35,7 +35,7 @@ internal class SendEvents( repository.remove(eventsToSend) } else { ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - parselyAPIConnection.send(jsonPayload) + restClient.send(jsonPayload) .fold( onSuccess = { ParselyTracker.PLog("Pixel request success") diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt index cb3a2524..77be99d6 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt @@ -21,7 +21,7 @@ class SendEventsTest { sut = SendEvents( FakeFlushManager(), FakeRepository(), - FakeParselyAPIConnection(), + FakeRestClient(), this ) @@ -40,7 +40,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } sut = SendEvents( @@ -68,7 +68,7 @@ class SendEventsTest { sut = SendEvents( FakeFlushManager(), repository, - FakeParselyAPIConnection(), + FakeRestClient(), this ) @@ -87,7 +87,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } sut = SendEvents( @@ -113,7 +113,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } sut = SendEvents( @@ -139,7 +139,7 @@ class SendEventsTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } sut = SendEvents( @@ -167,7 +167,7 @@ class SendEventsTest { return ArrayList(listOf(mapOf("test" to 123))) } } - val parselyAPIConnection = FakeParselyAPIConnection().apply { + val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } sut = SendEvents( @@ -192,7 +192,7 @@ class SendEventsTest { sut = SendEvents( flushManager, FakeRepository(), - FakeParselyAPIConnection(), + FakeRestClient(), this ) @@ -236,7 +236,7 @@ class SendEventsTest { } } - private class FakeParselyAPIConnection : ParselyAPIConnection("") { + private class FakeRestClient : RestClient { var nextResult: Result? = null From cb5113c8ba298cfedb33813fe0705d5aa8516d2b Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 13:14:04 +0100 Subject: [PATCH 06/14] feat: start `FlushManager` without checking state of stored queue first The `FlushManager` eventually invokes `SendEvents` which checks for the size of stored queue anyways. This change reduces unnecessary complexity and overhead. More context: https://github.com/Parsely/parsely-android/pull/92#discussion_r1396052648 --- .../parsely/parselyandroid/ParselyTracker.java | 6 +----- .../java/com/parsely/parselyandroid/SdkInit.kt | 16 ---------------- 2 files changed, 1 insertion(+), 21 deletions(-) delete mode 100644 parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index eb309c67..d67a91e0 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -86,11 +86,7 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { timer = new Timer(); isDebug = false; - SdkInitKt.initialize( - ParselyCoroutineScopeKt.getSdkScope(), - localStorageRepository, - flushManager - ); + flushManager.start(); ProcessLifecycleOwner.get().getLifecycle().addObserver( (LifecycleEventObserver) (lifecycleOwner, event) -> { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt b/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt deleted file mode 100644 index 07e00cc5..00000000 --- a/parsely/src/main/java/com/parsely/parselyandroid/SdkInit.kt +++ /dev/null @@ -1,16 +0,0 @@ -package com.parsely.parselyandroid - -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.launch - -internal fun initialize( - scope: CoroutineScope, - localStorageRepository: LocalStorageRepository, - flushManager: FlushManager, -) { - scope.launch { - if (localStorageRepository.getStoredQueue().isNotEmpty()) { - flushManager.start() - } - } -} From 4b4e47116c8ea5c9e22fedc4cdd7dafbacc3dee6 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:21:33 +0100 Subject: [PATCH 07/14] refactor: pass `onFlush` lambda to `ParselyFlushManager` This change decouples `ParselyFlushManager` and `ParselyTracker`. It also makes `FlushManagerTest` resistant to `ParselyTracker` implementation changes. --- .../parsely/parselyandroid/FlushManager.kt | 4 +- .../parselyandroid/ParselyTracker.java | 10 ++++- .../parselyandroid/FlushManagerTest.kt | 43 ++++++++----------- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt index 1a84e7a6..5026c8d8 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushManager.kt @@ -21,7 +21,7 @@ internal interface FlushManager { } internal class ParselyFlushManager( - private val parselyTracker: ParselyTracker, + private val onFlush: () -> Unit, override val intervalMillis: Long, private val coroutineScope: CoroutineScope ) : FlushManager { @@ -33,7 +33,7 @@ internal class ParselyFlushManager( job = coroutineScope.launch { while (isActive) { delay(intervalMillis) - parselyTracker.flushEvents() + onFlush.invoke() } } } diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index d67a91e0..cffa9938 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -32,6 +32,7 @@ import java.util.UUID; import kotlin.Unit; +import kotlin.jvm.functions.Function0; /** * Tracks Parse.ly app views in Android apps @@ -71,7 +72,13 @@ protected ParselyTracker(String siteId, int flushInterval, Context c) { context = c.getApplicationContext(); eventsBuilder = new EventsBuilder(context, siteId); localStorageRepository = new LocalStorageRepository(context); - flushManager = new ParselyFlushManager(this, flushInterval * 1000L, + flushManager = new ParselyFlushManager(new Function0() { + @Override + public Unit invoke() { + flushEvents(); + return Unit.INSTANCE; + } + }, flushInterval * 1000L, ParselyCoroutineScopeKt.getSdkScope()); inMemoryBuffer = new InMemoryBuffer(ParselyCoroutineScopeKt.getSdkScope(), localStorageRepository, () -> { if (!flushTimerIsActive()) { @@ -466,5 +473,4 @@ void flushEvents() { } sendEvents.invoke(isDebug); } - } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt index 25f987c3..7621b09a 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt @@ -1,6 +1,5 @@ package com.parsely.parselyandroid -import androidx.test.core.app.ApplicationProvider import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy @@ -8,42 +7,42 @@ import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.assertj.core.api.Assertions.assertThat import org.junit.Test -import org.junit.runner.RunWith -import org.robolectric.RobolectricTestRunner @OptIn(ExperimentalCoroutinesApi::class) -@RunWith(RobolectricTestRunner::class) class FlushManagerTest { private lateinit var sut: FlushManager - private val tracker = FakeTracker() + private var flushEventsCounter = 0 @Test fun `when timer starts and interval time passes, then flush queue`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(1) + assertThat(flushEventsCounter).isEqualTo(1) } @Test fun `when timer starts and three interval time passes, then flush queue 3 times`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(3 * DEFAULT_INTERVAL_MILLIS) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(3) + assertThat(flushEventsCounter).isEqualTo(3) } @Test fun `when timer starts and is stopped after 2 intervals passes, then flush queue 2 times`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(2 * DEFAULT_INTERVAL_MILLIS) @@ -52,13 +51,14 @@ class FlushManagerTest { advanceTimeBy(DEFAULT_INTERVAL_MILLIS) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(2) + assertThat(flushEventsCounter).isEqualTo(2) } @Test fun `when timer starts, is stopped before end of interval and then time of interval passes, then do not flush queue`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) @@ -67,13 +67,14 @@ class FlushManagerTest { advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(0) + assertThat(flushEventsCounter).isEqualTo(0) } @Test fun `when timer starts, and another timer starts after some time, then flush queue according to the first start`() = runTest { - sut = ParselyFlushManager(tracker, DEFAULT_INTERVAL_MILLIS, backgroundScope) + sut = + ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) @@ -82,25 +83,15 @@ class FlushManagerTest { advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(1) + assertThat(flushEventsCounter).isEqualTo(1) advanceTimeBy(DEFAULT_INTERVAL_MILLIS / 2) runCurrent() - assertThat(tracker.flushEventsCounter).isEqualTo(1) + assertThat(flushEventsCounter).isEqualTo(1) } private companion object { val DEFAULT_INTERVAL_MILLIS: Long = 30.seconds.inWholeMilliseconds } - - class FakeTracker : ParselyTracker( - "", 0, ApplicationProvider.getApplicationContext() - ) { - var flushEventsCounter = 0 - - override fun flushEvents() { - flushEventsCounter++ - } - } } From c9872d488a070234975b32b1f605f4f46de2c626 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:33:33 +0100 Subject: [PATCH 08/14] refactor: rename `SendEvents` to `FlushQueue` and `isDebug` to `skipSendingEvents` --- .../{SendEvents.kt => FlushQueue.kt} | 6 ++-- .../parselyandroid/ParselyTracker.java | 6 ++-- .../{SendEventsTest.kt => FlushQueueTest.kt} | 32 +++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) rename parsely/src/main/java/com/parsely/parselyandroid/{SendEvents.kt => FlushQueue.kt} (94%) rename parsely/src/test/java/com/parsely/parselyandroid/{SendEventsTest.kt => FlushQueueTest.kt} (83%) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt similarity index 94% rename from parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt rename to parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index b1f06e20..867c536e 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/SendEvents.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -6,7 +6,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -internal class SendEvents( +internal class FlushQueue( private val flushManager: FlushManager, private val repository: QueueRepository, private val restClient: RestClient, @@ -15,7 +15,7 @@ internal class SendEvents( private val mutex = Mutex() - operator fun invoke(isDebug: Boolean) { + operator fun invoke(skipSendingEvents: Boolean) { scope.launch { mutex.withLock { val eventsToSend = repository.getStoredQueue() @@ -30,7 +30,7 @@ internal class SendEvents( ParselyTracker.PLog("POST Data %s", jsonPayload) - if (isDebug) { + if (skipSendingEvents) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") repository.remove(eventsToSend) } else { diff --git a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java index cffa9938..6b341e87 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java +++ b/parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.java @@ -63,7 +63,7 @@ public class ParselyTracker { @NonNull private final InMemoryBuffer inMemoryBuffer; @NonNull - private final SendEvents sendEvents; + private final FlushQueue flushQueue; /** * Create a new ParselyTracker instance. @@ -87,7 +87,7 @@ public Unit invoke() { } return Unit.INSTANCE; }); - sendEvents = new SendEvents(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); + flushQueue = new FlushQueue(flushManager, localStorageRepository, new ParselyAPIConnection(ROOT_URL + "mobileproxy"), ParselyCoroutineScopeKt.getSdkScope()); // get the adkey straight away on instantiation timer = new Timer(); @@ -471,6 +471,6 @@ void flushEvents() { PLog("Network unreachable. Not flushing."); return; } - sendEvents.invoke(isDebug); + flushQueue.invoke(isDebug); } } diff --git a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt similarity index 83% rename from parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt rename to parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt index 77be99d6..75a8fe9b 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/SendEventsTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt @@ -10,15 +10,15 @@ import org.robolectric.RobolectricTestRunner @OptIn(ExperimentalCoroutinesApi::class) @RunWith(RobolectricTestRunner::class) -class SendEventsTest { +class FlushQueueTest { - private lateinit var sut: SendEvents + private lateinit var sut: FlushQueue @Test fun `given empty local storage, when sending events, then do nothing`() = runTest { // given - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), FakeRepository(), FakeRestClient(), @@ -34,7 +34,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events, then events are sent and removed from local storage`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events, then events are sent and removed from local storage`() = runTest { // given val repository = FakeRepository().apply { @@ -43,7 +43,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -59,13 +59,13 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode on, when sending events, then events are not sent and removed from local storage`() = + fun `given non-empty local storage, when flushing queue with skipping sending events, then events are not sent and removed from local storage`() = runTest { // given val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), repository, FakeRestClient(), @@ -81,7 +81,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events fails, then events are not removed from local storage`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events fails, then events are not removed from local storage`() = runTest { // given val repository = FakeRepository().apply { @@ -90,7 +90,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = SendEvents( + sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -106,7 +106,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events, then flush manager is stopped`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events, then flush manager is stopped`() = runTest { // given val flushManager = FakeFlushManager() @@ -116,7 +116,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = SendEvents( + sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -132,7 +132,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when sending events fails, then flush manager is not stopped`() = + fun `given non-empty local storage, when flushing queue with not skipping sending events fails, then flush manager is not stopped`() = runTest { // given val flushManager = FakeFlushManager() @@ -142,7 +142,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = SendEvents( + sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -158,7 +158,7 @@ class SendEventsTest { } @Test - fun `given non-empty local storage and debug mode off, when storage is not empty after successful event, then flush manager is not stopped`() = + fun `given non-empty local storage, when storage is not empty after successful flushing queue with not skipping sending events, then flush manager is not stopped`() = runTest { // given val flushManager = FakeFlushManager() @@ -170,7 +170,7 @@ class SendEventsTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = SendEvents( + sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -189,7 +189,7 @@ class SendEventsTest { fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { // given val flushManager = FakeFlushManager() - sut = SendEvents( + sut = FlushQueue( flushManager, FakeRepository(), FakeRestClient(), From 269a9d64f3146e23063237b02774916cdee3ce18 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:40:34 +0100 Subject: [PATCH 09/14] style: improve position of logging statements in `FlushQueue` --- .../main/java/com/parsely/parselyandroid/FlushQueue.kt | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 867c536e..2d880159 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -24,16 +24,14 @@ internal class FlushQueue( flushManager.stop() return@launch } - ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) - - val jsonPayload = toParselyEventsPayload(eventsToSend) - - ParselyTracker.PLog("POST Data %s", jsonPayload) if (skipSendingEvents) { - ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly") + ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events") repository.remove(eventsToSend) } else { + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + val jsonPayload = toParselyEventsPayload(eventsToSend) + ParselyTracker.PLog("POST Data %s", jsonPayload) ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) restClient.send(jsonPayload) .fold( From 409c42d89639e1f7139db7487ab3766bff3bf8f9 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 17 Nov 2023 15:44:27 +0100 Subject: [PATCH 10/14] style: return from `FlushQueue` if `skipSendingEvents` To improve readability and align with `eventsToSend.isEmpty()` check --- parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 2d880159..54a4f2b8 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -28,6 +28,7 @@ internal class FlushQueue( if (skipSendingEvents) { ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events") repository.remove(eventsToSend) + return@launch } else { ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) val jsonPayload = toParselyEventsPayload(eventsToSend) From 95f7a65278cd120895bea01df3e15b3e2dde0d5a Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 12:38:03 +0100 Subject: [PATCH 11/14] style: make variables test-local where possible --- .../parsely/parselyandroid/FlushManagerTest.kt | 18 ++++++++++-------- .../parsely/parselyandroid/FlushQueueTest.kt | 18 ++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt index 7621b09a..02842a2c 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushManagerTest.kt @@ -11,12 +11,10 @@ import org.junit.Test @OptIn(ExperimentalCoroutinesApi::class) class FlushManagerTest { - private lateinit var sut: FlushManager - private var flushEventsCounter = 0 - @Test fun `when timer starts and interval time passes, then flush queue`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -28,7 +26,8 @@ class FlushManagerTest { @Test fun `when timer starts and three interval time passes, then flush queue 3 times`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -41,7 +40,8 @@ class FlushManagerTest { @Test fun `when timer starts and is stopped after 2 intervals passes, then flush queue 2 times`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -57,7 +57,8 @@ class FlushManagerTest { @Test fun `when timer starts, is stopped before end of interval and then time of interval passes, then do not flush queue`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() @@ -73,7 +74,8 @@ class FlushManagerTest { @Test fun `when timer starts, and another timer starts after some time, then flush queue according to the first start`() = runTest { - sut = + var flushEventsCounter = 0 + val sut = ParselyFlushManager({ flushEventsCounter++ }, DEFAULT_INTERVAL_MILLIS, backgroundScope) sut.start() diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt index 75a8fe9b..d92c5705 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt @@ -12,13 +12,11 @@ import org.robolectric.RobolectricTestRunner @RunWith(RobolectricTestRunner::class) class FlushQueueTest { - private lateinit var sut: FlushQueue - @Test fun `given empty local storage, when sending events, then do nothing`() = runTest { // given - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), FakeRepository(), FakeRestClient(), @@ -43,7 +41,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -65,7 +63,7 @@ class FlushQueueTest { val repository = FakeRepository().apply { insertEvents(listOf(mapOf("test" to 123))) } - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), repository, FakeRestClient(), @@ -90,7 +88,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = FlushQueue( + val sut = FlushQueue( FakeFlushManager(), repository, parselyAPIConnection, @@ -116,7 +114,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = FlushQueue( + val sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -142,7 +140,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.failure(Exception()) } - sut = FlushQueue( + val sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -170,7 +168,7 @@ class FlushQueueTest { val parselyAPIConnection = FakeRestClient().apply { nextResult = Result.success(Unit) } - sut = FlushQueue( + val sut = FlushQueue( flushManager, repository, parselyAPIConnection, @@ -189,7 +187,7 @@ class FlushQueueTest { fun `given empty local storage, when invoked, then flush manager is stopped`() = runTest { // given val flushManager = FakeFlushManager() - sut = FlushQueue( + val sut = FlushQueue( flushManager, FakeRepository(), FakeRestClient(), From c32302f9094b5eef7342ec065ba3315b5c1a8564 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 13:12:06 +0100 Subject: [PATCH 12/14] feat: update lock logic on local storage repo Now, instead of multiple locks in case of removing or inserting data, we do the same with a single lock. See: https://github.com/Parsely/parsely-android/pull/92#discussion_r1400768099 --- .../parselyandroid/LocalStorageRepository.kt | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt index 6bc9bd24..1f1f28fc 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/LocalStorageRepository.kt @@ -14,7 +14,7 @@ internal interface QueueRepository { suspend fun insertEvents(toInsert: List?>) } -internal class LocalStorageRepository(private val context: Context): QueueRepository { +internal class LocalStorageRepository(private val context: Context) : QueueRepository { private val mutex = Mutex() @@ -38,21 +38,7 @@ internal class LocalStorageRepository(private val context: Context): QueueReposi } } - override suspend fun remove(toRemove: List?>) { - val storedEvents = getStoredQueue() - - mutex.withLock { - persistObject(storedEvents - toRemove.toSet()) - } - } - - /** - * Get the stored event queue from persistent storage. - * - * @return The stored queue of events. - */ - override suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { - + private fun getInternalStoredQueue(): ArrayList?> { var storedQueue: ArrayList?> = ArrayList() try { val fis = context.applicationContext.openFileInput(STORAGE_KEY) @@ -74,15 +60,26 @@ internal class LocalStorageRepository(private val context: Context): QueueReposi return storedQueue } + override suspend fun remove(toRemove: List?>) = mutex.withLock { + val storedEvents = getInternalStoredQueue() + persistObject(storedEvents - toRemove.toSet()) + } + /** - * Save the event queue to persistent storage. + * Get the stored event queue from persistent storage. + * + * @return The stored queue of events. */ - override suspend fun insertEvents(toInsert: List?>){ - val storedEvents = getStoredQueue() + override suspend fun getStoredQueue(): ArrayList?> = mutex.withLock { + getInternalStoredQueue() + } - mutex.withLock { - persistObject(ArrayList((toInsert + storedEvents).distinct())) - } + /** + * Save the event queue to persistent storage. + */ + override suspend fun insertEvents(toInsert: List?>) = mutex.withLock { + val storedEvents = getInternalStoredQueue() + persistObject(ArrayList((toInsert + storedEvents).distinct())) } companion object { From 2c0ce6e5e726cb2afc4185cf1dfa2fceb5be6a4b Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 14:11:58 +0100 Subject: [PATCH 13/14] style: remove unnecessary `else` from `FlushQueue` --- .../com/parsely/parselyandroid/FlushQueue.kt | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index 54a4f2b8..b5864e91 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -29,27 +29,26 @@ internal class FlushQueue( ParselyTracker.PLog("Debug mode on. Not sending to Parse.ly. Otherwise, would sent ${eventsToSend.size} events") repository.remove(eventsToSend) return@launch - } else { - ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) - val jsonPayload = toParselyEventsPayload(eventsToSend) - ParselyTracker.PLog("POST Data %s", jsonPayload) - ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) - restClient.send(jsonPayload) - .fold( - onSuccess = { - ParselyTracker.PLog("Pixel request success") - repository.remove(eventsToSend) - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (repository.getStoredQueue().isEmpty()) { - flushManager.stop() - } - }, - onFailure = { - ParselyTracker.PLog("Pixel request exception") - ParselyTracker.PLog(it.toString()) - } - ) } + ParselyTracker.PLog("Sending request with %d events", eventsToSend.size) + val jsonPayload = toParselyEventsPayload(eventsToSend) + ParselyTracker.PLog("POST Data %s", jsonPayload) + ParselyTracker.PLog("Requested %s", ParselyTracker.ROOT_URL) + restClient.send(jsonPayload) + .fold( + onSuccess = { + ParselyTracker.PLog("Pixel request success") + repository.remove(eventsToSend) + ParselyTracker.PLog("Event queue empty, flush timer cleared.") + if (repository.getStoredQueue().isEmpty()) { + flushManager.stop() + } + }, + onFailure = { + ParselyTracker.PLog("Pixel request exception") + ParselyTracker.PLog(it.toString()) + } + ) } } } From 784a0211aaddccc3ce22287cac3f8927c6463268 Mon Sep 17 00:00:00 2001 From: Wojtek Zieba Date: Fri, 24 Nov 2023 14:52:41 +0100 Subject: [PATCH 14/14] feat: do not stop FlushManager on successful flush If stored queue will be empty, on next execution of `FlushQueue`, the manager will be stopped anyway. https://github.com/Parsely/parsely-android/pull/92#discussion_r1400718192 --- .../com/parsely/parselyandroid/FlushQueue.kt | 4 --- .../parsely/parselyandroid/FlushQueueTest.kt | 26 ------------------- 2 files changed, 30 deletions(-) diff --git a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt index b5864e91..4a989b95 100644 --- a/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt +++ b/parsely/src/main/java/com/parsely/parselyandroid/FlushQueue.kt @@ -39,10 +39,6 @@ internal class FlushQueue( onSuccess = { ParselyTracker.PLog("Pixel request success") repository.remove(eventsToSend) - ParselyTracker.PLog("Event queue empty, flush timer cleared.") - if (repository.getStoredQueue().isEmpty()) { - flushManager.stop() - } }, onFailure = { ParselyTracker.PLog("Pixel request exception") diff --git a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt index d92c5705..e49605b6 100644 --- a/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt +++ b/parsely/src/test/java/com/parsely/parselyandroid/FlushQueueTest.kt @@ -103,32 +103,6 @@ class FlushQueueTest { assertThat(repository.getStoredQueue()).isNotEmpty } - @Test - fun `given non-empty local storage, when flushing queue with not skipping sending events, then flush manager is stopped`() = - runTest { - // given - val flushManager = FakeFlushManager() - val repository = FakeRepository().apply { - insertEvents(listOf(mapOf("test" to 123))) - } - val parselyAPIConnection = FakeRestClient().apply { - nextResult = Result.success(Unit) - } - val sut = FlushQueue( - flushManager, - repository, - parselyAPIConnection, - this - ) - - // when - sut.invoke(false) - runCurrent() - - // then - assertThat(flushManager.stopped).isTrue - } - @Test fun `given non-empty local storage, when flushing queue with not skipping sending events fails, then flush manager is not stopped`() = runTest {