Stop running test

Fix stop. Show test run errors.
ooni · Aug 20, 2024 · 68c8a24 · 68c8a24
1 parent a727bc2
commit 68c8a24
Show file tree

Hide file tree

Showing 11 changed files with 266 additions and 95 deletions.
diff --git a/composeApp/src/commonMain/composeResources/values/strings-common.xml b/composeApp/src/commonMain/composeResources/values/strings-common.xml
@@ -7,6 +7,11 @@
     <string name="Dashboard_RunV2_Ooni_Title">OONI Tests</string>
     <string name="Dashboard_RunV2_Title">OONI Run Links</string>
 
+    <string name="Dashboard_Running_Stopping_Title">Stopping test…</string>
+    <string name="Dashboard_Running_Stopping_Notice">Finishing the currently pending tests, please wait…</string>
+
+    <string name="Notification_StopTest">Stop test</string>
+
     <string name="Test_Websites_Fullname">Websites</string>
     <string name="Test_InstantMessaging_Fullname">Instant Messaging</string>
     <string name="Test_Middleboxes_Fullname">Middleboxes</string>
@@ -126,4 +131,5 @@
     <string name="CategoryCode_IGO_Description">Intergovernmental organizations including The United Nations</string>
     <string name="CategoryCode_MISC_Description">Sites that haven\'t been categorized yet</string>
 
+    <string name="Modal_Error_CantDownloadURLs">Unable to download URL list. Please try again.</string>
 </resources>
diff --git a/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt b/composeApp/src/commonMain/kotlin/org/ooni/engine/Engine.kt
@@ -6,6 +6,7 @@ import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.channelFlow
 import kotlinx.coroutines.flow.flowOn
+import kotlinx.coroutines.isActive
 import kotlinx.serialization.encodeToString
 import kotlinx.serialization.json.Json
 import org.ooni.engine.OonimkallBridge.SubmitMeasurementResults
@@ -48,20 +49,19 @@ class Engine(
             try {
                 task = bridge.startTask(settingsSerialized)
 
-                while (!task.isDone()) {
+                while (!task.isDone() && isActive) {
                     val eventJson = task.waitForNextEvent()
                     val taskEventResult = json.decodeFromString<TaskEventResult>(eventJson)
                     taskEventMapper(taskEventResult)?.let { send(it) }
                 }
+            } catch (e: CancellationException) {
+                Logger.d("Test cancelled")
+                throw e
             } catch (e: Exception) {
-                task?.interrupt()
+                Logger.d("Error while running task", e)
                 throw MkException(e)
-            }
-
-            invokeOnClose {
-                if (it is CancellationException) {
-                    task.interrupt()
-                }
+            } finally {
+                task?.interrupt()
             }
         }.flowOn(backgroundDispatcher)
 

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/App.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/App.kt
@@ -3,9 +3,14 @@ package org.ooni.probe
 import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Scaffold
+import androidx.compose.material3.SnackbarHost
+import androidx.compose.material3.SnackbarHostState
 import androidx.compose.material3.Surface
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.CompositionLocalProvider
 import androidx.compose.runtime.LaunchedEffect
+import androidx.compose.runtime.compositionLocalOf
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import androidx.navigation.compose.rememberNavController
 import co.touchlab.kermit.Logger
@@ -19,21 +24,25 @@ import org.ooni.probe.ui.theme.AppTheme
 @Preview
 fun App(dependencies: Dependencies) {
     val navController = rememberNavController()
+    val snackbarHostState = remember { SnackbarHostState() }
 
-    AppTheme {
-        Surface(
-            modifier = Modifier.fillMaxSize(),
-            color = MaterialTheme.colorScheme.background,
-        ) {
-            Scaffold(
-                bottomBar = {
-                    BottomNavigationBar(navController)
-                },
+    CompositionLocalProvider(
+        values = arrayOf(LocalSnackbarHostState provides snackbarHostState),
+    ) {
+        AppTheme {
+            Surface(
+                modifier = Modifier.fillMaxSize(),
+                color = MaterialTheme.colorScheme.background,
             ) {
-                Navigation(
-                    navController = navController,
-                    dependencies = dependencies,
-                )
+                Scaffold(
+                    snackbarHost = { SnackbarHost(snackbarHostState) },
+                    bottomBar = { BottomNavigationBar(navController) },
+                ) {
+                    Navigation(
+                        navController = navController,
+                        dependencies = dependencies,
+                    )
+                }
             }
         }
     }
@@ -56,3 +65,5 @@ private fun logAppStart(dependencies: Dependencies) {
         )
     }
 }
+
+val LocalSnackbarHostState = compositionLocalOf<SnackbarHostState?> { null }
diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/TestRunError.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/TestRunError.kt
@@ -0,0 +1,5 @@
+package org.ooni.probe.data.models
+
+sealed interface TestRunError {
+    data object DownloadUrlsFailed : TestRunError
+}
diff --git a/...n/org/ooni/probe/data/models/TestState.kt → ...rg/ooni/probe/data/models/TestRunState.kt b/...n/org/ooni/probe/data/models/TestState.kt → ...rg/ooni/probe/data/models/TestRunState.kt
@@ -4,8 +4,8 @@ import org.ooni.engine.models.TestType
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.seconds
 
-sealed interface TestState {
-    data object Idle : TestState
+sealed interface TestRunState {
+    data object Idle : TestRunState
 
     data class Running(
         val descriptorName: String? = null,
@@ -14,7 +14,7 @@ sealed interface TestState {
         val testProgress: Double = 0.0,
         val testIndex: Int = 0,
         val log: String? = "",
-    ) : TestState {
+    ) : TestRunState {
         val estimatedTimeLeft: Duration?
             get() {
                 if (estimatedRuntime == null) return null
@@ -26,4 +26,6 @@ sealed interface TestState {
                 return remainingTests + remainingFromCurrentTest
             }
     }
+
+    data object Stopping : TestRunState
 }
diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt
@@ -8,9 +8,6 @@ import androidx.datastore.preferences.core.Preferences
 import app.cash.sqldelight.db.SqlDriver
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.IO
-import kotlinx.coroutines.flow.MutableStateFlow
-import kotlinx.coroutines.flow.asStateFlow
-import kotlinx.coroutines.flow.update
 import kotlinx.serialization.json.Json
 import okio.FileSystem
 import okio.Path.Companion.toPath
@@ -27,7 +24,6 @@ import org.ooni.probe.data.disk.WriteFileOkio
 import org.ooni.probe.data.models.PreferenceCategoryKey
 import org.ooni.probe.data.models.ResultModel
 import org.ooni.probe.data.models.SettingsCategoryItem
-import org.ooni.probe.data.models.TestState
 import org.ooni.probe.data.repositories.MeasurementRepository
 import org.ooni.probe.data.repositories.NetworkRepository
 import org.ooni.probe.data.repositories.PreferenceRepository
@@ -44,6 +40,7 @@ import org.ooni.probe.domain.GetTestDescriptors
 import org.ooni.probe.domain.GetTestDescriptorsBySpec
 import org.ooni.probe.domain.RunDescriptors
 import org.ooni.probe.domain.RunNetTest
+import org.ooni.probe.domain.TestRunStateManager
 import org.ooni.probe.shared.PlatformInfo
 import org.ooni.probe.ui.dashboard.DashboardViewModel
 import org.ooni.probe.ui.result.ResultViewModel
@@ -70,7 +67,6 @@ class Dependencies(
 
     private val json by lazy { buildJson() }
     private val database by lazy { buildDatabase(databaseDriverFactory) }
-    private val currentTestState by lazy { MutableStateFlow<TestState>(TestState.Idle) }
 
     private val measurementRepository by lazy {
         MeasurementRepository(database, backgroundDispatcher)
@@ -112,7 +108,12 @@ class Dependencies(
             createOrIgnoreTestDescriptors = testDescriptorRepository::createOrIgnore,
         )
     }
-    val downloadUrls by lazy { DownloadUrls(engine::checkIn, urlRepository::createOrUpdateByUrl) }
+    private val downloadUrls by lazy {
+        DownloadUrls(
+            engine::checkIn,
+            urlRepository::createOrUpdateByUrl,
+        )
+    }
     private val getBootstrapTestDescriptors by lazy {
         GetBootstrapTestDescriptors(readAssetFile, json, backgroundDispatcher)
     }
@@ -134,7 +135,7 @@ class Dependencies(
         RunNetTest(
             startTest = engine::startTask,
             storeResult = resultRepository::createOrUpdate,
-            setCurrentTestState = currentTestState::update,
+            setCurrentTestState = testStateManager::updateState,
             getUrlByUrl = urlRepository::getByUrl,
             storeMeasurement = measurementRepository::createOrUpdate,
             storeNetwork = networkRepository::createIfNew,
@@ -149,20 +150,26 @@ class Dependencies(
             getTestDescriptorsBySpec = getTestDescriptorsBySpec::invoke,
             downloadUrls = downloadUrls::invoke,
             storeResult = resultRepository::createOrUpdate,
-            getCurrentTestState = currentTestState.asStateFlow(),
-            setCurrentTestState = currentTestState::update,
+            getCurrentTestRunState = testStateManager.observeState(),
+            setCurrentTestState = testStateManager::updateState,
+            observeCancelTestRun = testStateManager.observeTestRunCancels(),
+            reportTestRunError = testStateManager::reportError,
             runNetTest = { runNetTest(it)() },
         )
     }
 
+    private val testStateManager by lazy { TestRunStateManager() }
+
     // ViewModels
 
     val dashboardViewModel
         get() =
             DashboardViewModel(
                 getTestDescriptors = getTestDescriptors::invoke,
                 runDescriptors = runDescriptors::invoke,
-                getCurrentTestState = currentTestState::asStateFlow,
+                cancelTestRun = testStateManager::cancelTestRun,
+                observeTestRunState = testStateManager.observeState(),
+                observeTestRunErrors = testStateManager.observeError(),
             )
 
     fun resultsViewModel(goToResult: (ResultModel.Id) -> Unit) = ResultsViewModel(goToResult, getResults::invoke)

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunDescriptors.kt
@@ -1,8 +1,11 @@
 package org.ooni.probe.domain
 
 import co.touchlab.kermit.Logger
+import kotlinx.coroutines.async
+import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.flow.take
 import org.ooni.engine.Engine.MkException
 import org.ooni.engine.models.Result
 import org.ooni.engine.models.TaskOrigin
@@ -11,35 +14,68 @@ import org.ooni.probe.data.models.Descriptor
 import org.ooni.probe.data.models.NetTest
 import org.ooni.probe.data.models.ResultModel
 import org.ooni.probe.data.models.RunSpecification
-import org.ooni.probe.data.models.TestState
+import org.ooni.probe.data.models.TestRunError
+import org.ooni.probe.data.models.TestRunState
 import org.ooni.probe.data.models.UrlModel
 import kotlin.time.Duration.Companion.seconds
 
 class RunDescriptors(
     private val getTestDescriptorsBySpec: suspend (RunSpecification) -> List<Descriptor>,
     private val downloadUrls: suspend (TaskOrigin) -> Result<List<UrlModel>, MkException>,
     private val storeResult: suspend (ResultModel) -> ResultModel.Id,
-    private val getCurrentTestState: Flow<TestState>,
-    private val setCurrentTestState: ((TestState) -> TestState) -> Unit,
+    private val getCurrentTestRunState: Flow<TestRunState>,
+    private val setCurrentTestState: ((TestRunState) -> TestRunState) -> Unit,
+    private val observeCancelTestRun: Flow<Unit>,
+    private val reportTestRunError: (TestRunError) -> Unit,
     private val runNetTest: suspend (RunNetTest.Specification) -> Unit,
 ) {
     suspend operator fun invoke(spec: RunSpecification) {
         val descriptors = getTestDescriptorsBySpec(spec)
         val descriptorsWithFinalInputs = descriptors.prepareInputs(spec.taskOrigin)
 
-        if (getCurrentTestState.first() is TestState.Running) {
+        if (getCurrentTestRunState.first() is TestRunState.Running) {
             Logger.i("Tests are already running, so we won't run other tests")
             return
         }
         setCurrentTestState {
-            TestState.Running(estimatedRuntime = descriptorsWithFinalInputs.estimatedRuntime)
+            TestRunState.Running(estimatedRuntime = descriptorsWithFinalInputs.estimatedRuntime)
         }
 
-        descriptorsWithFinalInputs.forEach { descriptor ->
-            runDescriptor(descriptor, spec.taskOrigin, spec.isRerun)
-        }
+        runDescriptorsCancellable(descriptorsWithFinalInputs, spec)
+
+        setCurrentTestState { TestRunState.Idle }
+    }
 
-        setCurrentTestState { TestState.Idle }
+    private suspend fun runDescriptorsCancellable(
+        descriptors: List<Descriptor>,
+        spec: RunSpecification,
+    ) {
+        coroutineScope {
+            val runJob = async {
+                // Actually running the descriptors
+                descriptors.forEach { descriptor ->
+                    runDescriptor(descriptor, spec.taskOrigin, spec.isRerun)
+                }
+            }
+            // Observe if a cancel request has been made
+            val cancelJob = async {
+                observeCancelTestRun
+                    .take(1)
+                    .collect {
+                        setCurrentTestState { TestRunState.Stopping }
+                        runJob.cancel()
+                    }
+            }
+
+            try {
+                runJob.await()
+            } catch (e: Exception) {
+                // Exceptions were logged in the Engine
+            }
+            if (cancelJob.isActive) {
+                cancelJob.cancel()
+            }
+        }
     }
 
     private suspend fun List<Descriptor>.prepareInputs(taskOrigin: TaskOrigin) =
@@ -63,7 +99,7 @@ class RunDescriptors(
             ?: emptyList()
 
         if (urls.isEmpty()) {
-            // TODO: Add error to state
+            reportTestRunError(TestRunError.DownloadUrlsFailed)
         }
 
         return urls

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt
@@ -16,7 +16,7 @@ import org.ooni.probe.data.models.MeasurementModel
 import org.ooni.probe.data.models.NetTest
 import org.ooni.probe.data.models.NetworkModel
 import org.ooni.probe.data.models.ResultModel
-import org.ooni.probe.data.models.TestState
+import org.ooni.probe.data.models.TestRunState
 import org.ooni.probe.data.models.UrlModel
 import org.ooni.probe.shared.toLocalDateTime
 import kotlin.time.Duration
@@ -27,7 +27,7 @@ class RunNetTest(
     private val storeMeasurement: suspend (MeasurementModel) -> MeasurementModel.Id,
     private val storeNetwork: suspend (NetworkModel) -> NetworkModel.Id,
     private val storeResult: suspend (ResultModel) -> ResultModel.Id,
-    private val setCurrentTestState: ((TestState) -> TestState) -> Unit,
+    private val setCurrentTestState: ((TestRunState) -> TestRunState) -> Unit,
     private val writeFile: WriteFile,
     private val deleteFile: DeleteFile,
     private val json: Json,
@@ -51,7 +51,7 @@ class RunNetTest(
 
     suspend operator fun invoke() {
         setCurrentTestState {
-            if (it !is TestState.Running) return@setCurrentTestState it
+            if (it !is TestRunState.Running) return@setCurrentTestState it
             it.copy(
                 testType = spec.netTest.test,
                 testProgress = spec.testIndex * progressStep,
@@ -116,7 +116,7 @@ class RunNetTest(
                 )
 
                 setCurrentTestState {
-                    if (it !is TestState.Running) return@setCurrentTestState it
+                    if (it !is TestRunState.Running) return@setCurrentTestState it
                     it.copy(log = event.message)
                 }
 
@@ -125,7 +125,7 @@ class RunNetTest(
 
             is TaskEvent.Progress -> {
                 setCurrentTestState {
-                    if (it !is TestState.Running) return@setCurrentTestState it
+                    if (it !is TestRunState.Running) return@setCurrentTestState it
                     it.copy(
                         testProgress = (spec.testIndex + event.progress) * progressStep,
                         log = event.message,