Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.simprints.feature.setup

import com.simprints.feature.setup.location.LocationStoreWorkerScheduler
import com.simprints.feature.setup.location.LocationStoreImpl
import dagger.Binds
import dagger.Module
import dagger.hilt.InstallIn
Expand All @@ -10,5 +10,5 @@ import dagger.hilt.components.SingletonComponent
@InstallIn(SingletonComponent::class)
abstract class SetupModule {
@Binds
internal abstract fun provideLocationStore(authManager: LocationStoreWorkerScheduler): LocationStore
internal abstract fun provideLocationStore(locationStoreImpl: LocationStoreImpl): LocationStore
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package com.simprints.feature.setup.location

import com.simprints.infra.events.event.domain.models.scope.Location
import com.simprints.infra.logging.Simber
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.flow.filterNotNull
import javax.inject.Inject

internal class CollectLocationUseCase @Inject constructor(
private val locationManager: LocationManager,
private val updateSessionScopeLocationUseCase: UpdateSessionScopeLocationUseCase,
) {
/**
* Runs directly in the caller's coroutine context/scope so that cancelling the caller
* (e.g. cancelling its scope's children) cancels this collection too, without needing to
* track and cancel a [kotlinx.coroutines.Job] manually.
*/
suspend operator fun invoke() {
val requestStartTimeMs = System.currentTimeMillis()
Simber.i("Started collecting location", tag = TAG)
try {
locationManager
.requestLocation()
.filterNotNull()
.collect { location -> runCatching { saveUserLocation(location, requestStartTimeMs) } }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the runCatching() wrap and hide the cancellation exception, or any other error? Which means that the outer try-catch is useless.

Simber.d("Finished collecting location (took ${elapsedMs(requestStartTimeMs)}ms)", tag = TAG)
} catch (c: CancellationException) {
Simber.d("Stopped collecting location (took ${elapsedMs(requestStartTimeMs)}ms)", tag = TAG)
throw c
} catch (t: Throwable) {
Simber.e("Failed to collect location", t, tag = TAG)
}
}

private suspend fun saveUserLocation(
location: Location,
requestStartTimeMs: Long,
) {
updateSessionScopeLocationUseCase(location)
Simber.d("Saved user's location into the current session (took ${elapsedMs(requestStartTimeMs)}ms)", tag = TAG)
}

private fun elapsedMs(requestStartTimeMs: Long) = System.currentTimeMillis() - requestStartTimeMs

private companion object {
private const val TAG = "CollectLocationUseCase"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.simprints.feature.setup.location

import com.simprints.core.AppScope
import com.simprints.feature.setup.LocationStore
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancelChildren
import kotlinx.coroutines.launch
import javax.inject.Inject
import javax.inject.Singleton

@Singleton
internal class LocationStoreImpl @Inject constructor(
@AppScope appScope: CoroutineScope,
private val collectLocation: CollectLocationUseCase,
) : LocationStore {
// A child scope of the app scope so that collection coroutines can be cancelled
private val scope = CoroutineScope(appScope.coroutineContext + SupervisorJob(appScope.coroutineContext[Job]))

override fun collectLocationInBackground() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole point was to do the collection coroutine in the context/scope of the caller to get the benefit of structured concurrency and avoid tracking this job manually.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My Idea was that the collectLocationInBackground gets started in the setup fragment and gets canceled from the orchestrator. That is why I needed ahigher context than the caller so I used AppScope.
I implemented it in a nicer way now in LocationStoreImpl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the expected result is for location collection to run as long as the session is active. If we switch it to cancel as soon as Setup is done, that's a behaviour change and likely a negative one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BurningAXE The cancellation is happening as before, at the end of the orchestration fragment rather than during the setup.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BurningAXE technically, it would be until location is received or session interrupted. This implementation seems to be slightly simpler to understand, although I am surprised that the time benefits are negligible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was commenting on the suggestion to use the scope of the caller which is the Setup VM. Would have been easier if we had a dedicated session scope. (We do have SessionCoroutineScope but that one is strictly reserved for events and I don't think this single use case merits creating a new one)

scope.coroutineContext.cancelChildren()
scope.launch { collectLocation() }
}

override fun cancelLocationCollection() {
scope.coroutineContext.cancelChildren()
}
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
package com.simprints.feature.setup.location

import android.os.PowerManager
import com.simprints.infra.events.event.domain.models.scope.Location
import com.simprints.testtools.common.coroutines.TestCoroutineRule
import io.mockk.*
import io.mockk.MockKAnnotations
import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.every
import io.mockk.impl.annotations.MockK
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.runTest
import org.junit.Before
import org.junit.Rule
import org.junit.Test

internal class StoreUserLocationIntoCurrentSessionWorkerTest {
internal class CollectLocationUseCaseTest {
@get:Rule
val testCoroutineRule = TestCoroutineRule()

Expand All @@ -21,53 +23,43 @@ internal class StoreUserLocationIntoCurrentSessionWorkerTest {
@MockK
private lateinit var updateSessionScopeLocationUseCase: UpdateSessionScopeLocationUseCase

private lateinit var worker: StoreUserLocationIntoCurrentSessionWorker
private lateinit var collectLocation: CollectLocationUseCase

@Before
fun setUp() {
MockKAnnotations.init(this, relaxed = true)

worker = StoreUserLocationIntoCurrentSessionWorker(
mockk(relaxed = true) {
every { getSystemService<PowerManager>(any()) } returns mockk {
every { isIgnoringBatteryOptimizations(any()) } returns true
}
},
mockk(relaxed = true),
updateSessionScopeLocationUseCase,
locationManager,
testCoroutineRule.testCoroutineDispatcher,
collectLocation = CollectLocationUseCase(
locationManager = locationManager,
updateSessionScopeLocationUseCase = updateSessionScopeLocationUseCase,
)
}

@Test
fun storeUserLocationIntoCurrentSession() = runTest {
fun `invoke saves location into current session`() = runTest {
every { locationManager.requestLocation() } returns flowOf(Location(latitude = 23.0, longitude = 54.0))
worker.doWork()

collectLocation()

coVerify(exactly = 1) { updateSessionScopeLocationUseCase.invoke(any()) }
}

@Test
fun `storeUserLocationIntoCurrentSession requestLocation throw exception`() = runTest {
fun `invoke requestLocation throws exception does not crash`() = runTest {
every { locationManager.requestLocation() } throws Exception("Location collect exception")
worker.doWork()

collectLocation()

coVerify(exactly = 0) { updateSessionScopeLocationUseCase.invoke(any()) }
}

@Test(expected = Test.None::class)
fun `storeUserLocationIntoCurrentSession can't save event should not crash the app`() = runTest {
fun `invoke can't save event should not crash the app`() = runTest {
every { locationManager.requestLocation() } returns flowOf(Location(latitude = 23.0, longitude = 54.0))
coEvery {
updateSessionScopeLocationUseCase.invoke(any())
} throws Exception("No session capture event found")
worker.doWork()
}

@Test
fun `storeUserLocationIntoCurrentSession can't save events if the worker is canceled`() = runTest {
every { locationManager.requestLocation() } returns flowOf(Location(latitude = 23.0, longitude = 54.0))
worker.stop(0)
worker.doWork()
coVerify(exactly = 0) { updateSessionScopeLocationUseCase.invoke(any()) }
collectLocation()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package com.simprints.feature.setup.location

import com.google.common.truth.Truth.assertThat
import com.simprints.testtools.common.coroutines.TestCoroutineRule
import io.mockk.MockKAnnotations
import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.impl.annotations.MockK
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.awaitCancellation
import org.junit.Before
import org.junit.Rule
import org.junit.Test

internal class LocationStoreImplTest {
@get:Rule
val testCoroutineRule = TestCoroutineRule()

@MockK
private lateinit var collectLocation: CollectLocationUseCase

private lateinit var appScope: CoroutineScope
private lateinit var locationStore: LocationStoreImpl

@Before
fun setUp() {
MockKAnnotations.init(this, relaxed = true)
appScope = CoroutineScope(testCoroutineRule.testCoroutineDispatcher + Job())
locationStore = LocationStoreImpl(appScope, collectLocation)
}

@Test
fun `collectLocationInBackground starts a new collection`() {
coEvery { collectLocation() } returns Unit

locationStore.collectLocationInBackground()

coVerify(exactly = 1) { collectLocation() }
}

@Test
fun `collectLocationInBackground cancels a previous collection before starting a new one`() {
val firstCollectionCancelled = CompletableDeferred<Unit>()
coEvery { collectLocation() } coAnswers {
try {
awaitCancellation()
} catch (c: CancellationException) {
firstCollectionCancelled.complete(Unit)
throw c
}
}

locationStore.collectLocationInBackground()
locationStore.collectLocationInBackground()

assertThat(firstCollectionCancelled.isCompleted).isTrue()
}

@Test
fun `cancelLocationCollection cancels the current collection`() {
val collectionCancelled = CompletableDeferred<Unit>()
coEvery { collectLocation() } coAnswers {
try {
awaitCancellation()
} catch (c: CancellationException) {
collectionCancelled.complete(Unit)
throw c
}
}

locationStore.collectLocationInBackground()
locationStore.cancelLocationCollection()

assertThat(collectionCancelled.isCompleted).isTrue()
}
}
Loading