From befaf367aa7db9822ed64678f9d61de4353df3e6 Mon Sep 17 00:00:00 2001 From: Zach Klippenstein Date: Thu, 6 Feb 2020 13:14:39 -0800 Subject: [PATCH] Make WorkflowTester tests use the main context as the worker context. This is an enhancement to #940 that gives more reasonable behavior. The context parameter to the test methods are primarily used to pass in test dispatchers, and those dispatchers often really only need to be used for workers. Since I don't know of a use case for specifying context separately just for workers, this also removes the `WorkflowTestParams.workerContext` property. --- CHANGELOG.md | 2 +- .../com/squareup/workflow/LaunchWorkflow.kt | 1 + .../workflow/internal/WorkflowNode.kt | 3 +++ .../workflow/testing/LaunchWorkflow.kt | 4 +++- .../workflow/testing/WorkflowTestParams.kt | 8 +------ .../workflow/internal/LaunchWorkflowTest.kt | 23 +++++++++++++++++++ .../workflow/testing/WorkflowTester.kt | 8 ++++++- .../WorkerCompositionIntegrationTest.kt | 18 +++++++++++++-- 8 files changed, 55 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e281bf2da..bfddb467d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ Change Log ### Kotlin - * Make the context used to start workers configurable for tests. (#940) + * Make the context used to start workers configurable for tests. (#940, #943) ### Swift diff --git a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/LaunchWorkflow.kt b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/LaunchWorkflow.kt index 9aa09f728..17a0b1e37 100644 --- a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/LaunchWorkflow.kt +++ b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/LaunchWorkflow.kt @@ -131,6 +131,7 @@ internal fun launchWorkflow val renderingsAndSnapshots = ConflatedBroadcastChannel>() val outputs = BroadcastChannel(capacity = 1) val workflowScope = scope + Job(parent = scope.coroutineContext[Job]) + require(workerContext[Job] == null) { "Expected workerContext not to have a Job." } // Give the caller a chance to start collecting outputs. val session = WorkflowSession(renderingsAndSnapshots.asFlow(), outputs.asFlow()) diff --git a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt index 774ce0ea3..4c1d4fb07 100644 --- a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt +++ b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/internal/WorkflowNode.kt @@ -44,6 +44,9 @@ import kotlin.coroutines.EmptyCoroutineContext * value to its parent. Returns either the output to be emitted from the root workflow, or null. * @param initialState Allows unit tests to start the node from a given state, instead of calling * [StatefulWorkflow.initialState]. + * @param workerContext [CoroutineContext] that is appended to the end of the context used to launch + * worker coroutines. This context will override anything from the workflow's scope and any other + * hard-coded values added to worker contexts. It must not contain a [Job] element. */ @UseExperimental(VeryExperimentalWorkflow::class) internal class WorkflowNode( diff --git a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/LaunchWorkflow.kt b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/LaunchWorkflow.kt index 359e1eb15..a034edae2 100644 --- a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/LaunchWorkflow.kt +++ b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/LaunchWorkflow.kt @@ -28,6 +28,7 @@ import com.squareup.workflow.testing.WorkflowTestParams.StartMode.StartFromWorkf import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow import org.jetbrains.annotations.TestOnly +import kotlin.coroutines.CoroutineContext /** * Launches the [workflow] in a new coroutine in [scope]. The workflow tree is seeded with @@ -42,6 +43,7 @@ fun launchWorkflowForTestFr workflow: StatefulWorkflow, props: Flow, testParams: WorkflowTestParams, + workerContext: CoroutineContext, beforeStart: CoroutineScope.(session: WorkflowSession) -> RunnerT ): RunnerT { val initialState = (testParams.startFrom as? StartFromState)?.state @@ -63,6 +65,6 @@ fun launchWorkflowForTestFr initialState = initialState, initialSnapshot = initialSnapshot, beforeStart = beforeStart, - workerContext = testParams.workerContext + workerContext = workerContext ) } diff --git a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/WorkflowTestParams.kt b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/WorkflowTestParams.kt index c4f5b9b2c..2c83e9849 100644 --- a/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/WorkflowTestParams.kt +++ b/kotlin/workflow-runtime/src/main/java/com/squareup/workflow/testing/WorkflowTestParams.kt @@ -22,8 +22,6 @@ import com.squareup.workflow.testing.WorkflowTestParams.StartMode.StartFromCompl import com.squareup.workflow.testing.WorkflowTestParams.StartMode.StartFromState import com.squareup.workflow.testing.WorkflowTestParams.StartMode.StartFromWorkflowSnapshot import org.jetbrains.annotations.TestOnly -import kotlin.coroutines.CoroutineContext -import kotlin.coroutines.EmptyCoroutineContext /** * Defines configuration for workflow testing infrastructure such as `testRender`, `testFromStart`. @@ -36,15 +34,11 @@ import kotlin.coroutines.EmptyCoroutineContext * for any given state, so performing side effects in `render` will almost always result in bugs. * It is recommended to leave this on, but if you need to debug a test and don't want to have to * deal with the extra passes, you can temporarily set it to false. - * @param workerContext Used to customize the context in which workers are started for tests. - * Default is [EmptyCoroutineContext], which means the workers will use the context from their - * workflow and use the [Unconfined][kotlinx.coroutines.Dispatchers.Unconfined] dispatcher. */ @TestOnly data class WorkflowTestParams( val startFrom: StartMode = StartFresh, - val checkRenderIdempotence: Boolean = true, - val workerContext: CoroutineContext = EmptyCoroutineContext + val checkRenderIdempotence: Boolean = true ) { /** * Defines how to start the workflow for tests. diff --git a/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/LaunchWorkflowTest.kt b/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/LaunchWorkflowTest.kt index 7c85ddcae..3bac494e9 100644 --- a/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/LaunchWorkflowTest.kt +++ b/kotlin/workflow-runtime/src/test/java/com/squareup/workflow/internal/LaunchWorkflowTest.kt @@ -51,6 +51,7 @@ import kotlin.coroutines.CoroutineContext import kotlin.test.AfterTest import kotlin.test.assertEquals import kotlin.test.assertFails +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNull import kotlin.test.assertTrue @@ -530,6 +531,28 @@ class LaunchWorkflowTest { assertEquals(listOf("onRuntimeStopped"), listener.consumeEventNames()) } } + + @Test fun `throws when workerContext contains Job`() { + val loop = simpleLoop { _, _ -> hang() } + val workflow = Workflow.stateless { } + val workerContext = Job() + + val error = assertFailsWith { + runBlocking { + launchWorkflowImpl( + scope = this, + workflowLoop = loop, + workflow = workflow.asStatefulWorkflow(), + props = emptyFlow(), + initialSnapshot = null, + initialState = null, + beforeStart = {}, + workerContext = workerContext + ) + } + } + assertEquals("Expected workerContext not to have a Job.", error.message) + } } private suspend fun hang(): Nothing = suspendCancellableCoroutine { } diff --git a/kotlin/workflow-testing/src/main/java/com/squareup/workflow/testing/WorkflowTester.kt b/kotlin/workflow-testing/src/main/java/com/squareup/workflow/testing/WorkflowTester.kt index 7afc74711..4ba9074a5 100644 --- a/kotlin/workflow-testing/src/main/java/com/squareup/workflow/testing/WorkflowTester.kt +++ b/kotlin/workflow-testing/src/main/java/com/squareup/workflow/testing/WorkflowTester.kt @@ -27,6 +27,7 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.Unconfined +import kotlinx.coroutines.Job import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.cancel import kotlinx.coroutines.channels.Channel @@ -283,7 +284,12 @@ fun scope = CoroutineScope(Unconfined + context + uncaughtExceptionHandler), workflow = this@test, props = propsChannel.asFlow(), - testParams = testParams + testParams = testParams, + // Use the base context as the starting point for the worker context since it's often used + // to pass in test dispatchers. + // Remove any job present in the base context since worker context aren't allowed to contain + // jobs (it would violate structured concurrency). + workerContext = context.minusKey(Job) ) { session -> WorkflowTester(this, propsChannel, session.renderingsAndSnapshots, session.outputs) .apply { collectFromWorkflow() } diff --git a/kotlin/workflow-testing/src/test/java/com/squareup/workflow/WorkerCompositionIntegrationTest.kt b/kotlin/workflow-testing/src/test/java/com/squareup/workflow/WorkerCompositionIntegrationTest.kt index 5676aaa0a..b78bcbb57 100644 --- a/kotlin/workflow-testing/src/test/java/com/squareup/workflow/WorkerCompositionIntegrationTest.kt +++ b/kotlin/workflow-testing/src/test/java/com/squareup/workflow/WorkerCompositionIntegrationTest.kt @@ -19,10 +19,10 @@ package com.squareup.workflow import com.squareup.workflow.WorkflowAction.Companion.noAction import com.squareup.workflow.testing.WorkerSink -import com.squareup.workflow.testing.WorkflowTestParams import com.squareup.workflow.testing.testFromStart import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineName +import kotlinx.coroutines.Job import kotlinx.coroutines.TimeoutCancellationException import kotlinx.coroutines.channels.Channel import kotlin.coroutines.CoroutineContext @@ -31,6 +31,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertNotSame import kotlin.test.assertTrue import kotlin.test.fail @@ -250,9 +251,22 @@ class WorkerCompositionIntegrationTest { } val workerContext = CoroutineName("worker context") - workflow.testFromStart(testParams = WorkflowTestParams(workerContext = workerContext)) { + workflow.testFromStart(context = workerContext) { val actualWorkerContext = awaitNextOutput() assertEquals("worker context", actualWorkerContext[CoroutineName]!!.name) } } + + @Test fun `worker context job is ignored`() { + val worker = Worker.from { coroutineContext } + val workflow = Workflow.stateless { + runningWorker(worker) { context -> action { setOutput(context) } } + } + val job: Job = Job() + + workflow.testFromStart(context = job) { + val actualWorkerContext = awaitNextOutput() + assertNotSame(job, actualWorkerContext[Job]) + } + } }