From b7f7a16e630ac8874523620889f8a9e52227473b Mon Sep 17 00:00:00 2001 From: Zach Klippenstein Date: Mon, 13 Jul 2020 15:59:12 -0700 Subject: [PATCH] Make RenderTester throw a more realistic exception if the same workflow/side effect is ran twice. It now throws an exception that looks just like the real runtime. This doesn't _fail_ the test, it _errors_ it, which is also better behavior since this case means the workflow-under-test is written incorrectly, not that it doesn't match expected test behavior. Fixes #120. This change also cleans up the way `WorkflowIdentifier`s are described in error and testing strings, unifies the impostor and non-impostor calls, makes the string consistent between runtime and tests, and removes a method from the public API. --- workflow-core/api/workflow-core.api | 1 - .../squareup/workflow1/WorkflowIdentifier.kt | 17 ++--- .../workflow1/WorkflowIdentifierTest.kt | 31 ++++++-- .../workflow1/internal/SubtreeManager.kt | 3 +- .../workflow1/internal/WorkflowNode.kt | 2 +- .../workflow1/internal/SubtreeManagerTest.kt | 4 +- .../workflow1/internal/WorkflowNodeTest.kt | 2 +- .../workflow1/testing/RealRenderTester.kt | 40 ++++++++++- .../workflow1/testing/RealRenderTesterTest.kt | 71 +++++++++++++++---- .../tracing/TracingWorkflowInterceptor.kt | 2 +- 10 files changed, 137 insertions(+), 36 deletions(-) diff --git a/workflow-core/api/workflow-core.api b/workflow-core/api/workflow-core.api index 79ee28de6c..ede66bd23e 100644 --- a/workflow-core/api/workflow-core.api +++ b/workflow-core/api/workflow-core.api @@ -192,7 +192,6 @@ public final class com/squareup/workflow1/WorkflowAction$Updater { public final class com/squareup/workflow1/WorkflowIdentifier { public static final field Companion Lcom/squareup/workflow1/WorkflowIdentifier$Companion; - public final fun describeRealIdentifier ()Ljava/lang/String; public fun equals (Ljava/lang/Object;)Z public final fun getRealIdentifierType ()Lkotlin/reflect/KAnnotatedElement; public fun hashCode ()I diff --git a/workflow-core/src/main/java/com/squareup/workflow1/WorkflowIdentifier.kt b/workflow-core/src/main/java/com/squareup/workflow1/WorkflowIdentifier.kt index 95e7db82c6..b3d6f265b7 100644 --- a/workflow-core/src/main/java/com/squareup/workflow1/WorkflowIdentifier.kt +++ b/workflow-core/src/main/java/com/squareup/workflow1/WorkflowIdentifier.kt @@ -113,20 +113,15 @@ class WorkflowIdentifier internal constructor( /** * If this identifier identifies an [ImpostorWorkflow], returns the result of that workflow's - * [ImpostorWorkflow.describeRealIdentifier] method, otherwise returns null. + * [ImpostorWorkflow.describeRealIdentifier] method, otherwise returns a description of this + * identifier including the name of its workflow type and any [ImpostorWorkflow.realIdentifier]s. * - * Use [toString] to get a complete representation of this identifier. - */ - fun describeRealIdentifier(): String? = description?.invoke() - - /** - * Returns a description of this identifier including the name of its workflow type and any - * [ImpostorWorkflow.realIdentifier]s. */ override fun toString(): String = - proxiedIdentifiers - .joinToString { it.typeName } - .let { "WorkflowIdentifier($it)" } + description?.invoke() + ?: proxiedIdentifiers + .joinToString { it.typeName } + .let { "WorkflowIdentifier($it)" } override fun equals(other: Any?): Boolean = when { this === other -> true diff --git a/workflow-core/src/test/java/com/squareup/workflow1/WorkflowIdentifierTest.kt b/workflow-core/src/test/java/com/squareup/workflow1/WorkflowIdentifierTest.kt index e949e5ca5b..48fa8675b4 100644 --- a/workflow-core/src/test/java/com/squareup/workflow1/WorkflowIdentifierTest.kt +++ b/workflow-core/src/test/java/com/squareup/workflow1/WorkflowIdentifierTest.kt @@ -36,10 +36,33 @@ class WorkflowIdentifierTest { ) } - @Test fun `impostor identifier toString`() { - val id = TestImpostor1(TestWorkflow1).identifier + @Test fun `impostor identifier toString uses describeRealIdentifier when non-null`() { + class TestImpostor : Workflow, ImpostorWorkflow { + override val realIdentifier: WorkflowIdentifier = TestWorkflow1.identifier + override fun describeRealIdentifier(): String? = + "TestImpostor(${TestWorkflow1::class.simpleName})" + + override fun asStatefulWorkflow(): StatefulWorkflow = + throw NotImplementedError() + } + + val id = TestImpostor().identifier + assertEquals("TestImpostor(TestWorkflow1)", id.toString()) + } + + @Test + fun `impostor identifier toString uses full chain when describeRealIdentifier returns null`() { + class TestImpostor : Workflow, ImpostorWorkflow { + override val realIdentifier: WorkflowIdentifier = TestWorkflow1.identifier + override fun describeRealIdentifier(): String? = null + + override fun asStatefulWorkflow(): StatefulWorkflow = + throw NotImplementedError() + } + + val id = TestImpostor().identifier assertEquals( - "WorkflowIdentifier(com.squareup.workflow1.WorkflowIdentifierTest\$TestImpostor1, " + + "WorkflowIdentifier(${TestImpostor::class.java.name}, " + "com.squareup.workflow1.WorkflowIdentifierTest\$TestWorkflow1)", id.toString() ) @@ -47,7 +70,7 @@ class WorkflowIdentifierTest { @Test fun `impostor identifier description`() { val id = TestImpostor1(TestWorkflow1).identifier - assertEquals("TestImpostor1(TestWorkflow1)", id.describeRealIdentifier()) + assertEquals("TestImpostor1(TestWorkflow1)", id.toString()) } @Test fun `restored identifier toString`() { diff --git a/workflow-runtime/src/main/java/com/squareup/workflow1/internal/SubtreeManager.kt b/workflow-runtime/src/main/java/com/squareup/workflow1/internal/SubtreeManager.kt index a9cbac1844..b7b936d78a 100644 --- a/workflow-runtime/src/main/java/com/squareup/workflow1/internal/SubtreeManager.kt +++ b/workflow-runtime/src/main/java/com/squareup/workflow1/internal/SubtreeManager.kt @@ -23,6 +23,7 @@ import com.squareup.workflow1.WorkflowAction import com.squareup.workflow1.WorkflowInterceptor import com.squareup.workflow1.WorkflowInterceptor.WorkflowSession import com.squareup.workflow1.WorkflowOutput +import com.squareup.workflow1.identifier import kotlinx.coroutines.selects.SelectBuilder import kotlin.coroutines.CoroutineContext @@ -140,7 +141,7 @@ internal class SubtreeManager( // Prevent duplicate workflows with the same key. children.forEachStaging { require(!(it.matches(child, key))) { - "Expected keys to be unique for ${child::class.java.name}: key=$key" + "Expected keys to be unique for ${child.identifier}: key=\"$key\"" } } diff --git a/workflow-runtime/src/main/java/com/squareup/workflow1/internal/WorkflowNode.kt b/workflow-runtime/src/main/java/com/squareup/workflow1/internal/WorkflowNode.kt index 2f9ac3b0bd..b019942819 100644 --- a/workflow-runtime/src/main/java/com/squareup/workflow1/internal/WorkflowNode.kt +++ b/workflow-runtime/src/main/java/com/squareup/workflow1/internal/WorkflowNode.kt @@ -142,7 +142,7 @@ internal class WorkflowNode( ) { // Prevent duplicate side effects with the same key. sideEffects.forEachStaging { - require(key != it.key) { "Expected side effect keys to be unique: $key" } + require(key != it.key) { "Expected side effect keys to be unique: \"$key\"" } } sideEffects.retainOrCreate( diff --git a/workflow-runtime/src/test/java/com/squareup/workflow1/internal/SubtreeManagerTest.kt b/workflow-runtime/src/test/java/com/squareup/workflow1/internal/SubtreeManagerTest.kt index 559bd4a1ae..e8b7e32023 100644 --- a/workflow-runtime/src/test/java/com/squareup/workflow1/internal/SubtreeManagerTest.kt +++ b/workflow-runtime/src/test/java/com/squareup/workflow1/internal/SubtreeManagerTest.kt @@ -18,7 +18,6 @@ package com.squareup.workflow1.internal import com.squareup.workflow1.ExperimentalWorkflowApi -import com.squareup.workflow1.RenderContext import com.squareup.workflow1.Sink import com.squareup.workflow1.Snapshot import com.squareup.workflow1.StatefulWorkflow @@ -28,6 +27,7 @@ import com.squareup.workflow1.action import com.squareup.workflow1.applyTo import com.squareup.workflow1.internal.SubtreeManagerTest.TestWorkflow.Rendering import com.squareup.workflow1.makeEventSink +import com.squareup.workflow1.workflowIdentifier import kotlinx.coroutines.Dispatchers.Unconfined import kotlinx.coroutines.async import kotlinx.coroutines.runBlocking @@ -146,7 +146,7 @@ class SubtreeManagerTest { manager.render(workflow, "props", "foo", handler = { fail() }) } assertEquals( - "Expected keys to be unique for ${TestWorkflow::class.java.name}: key=foo", + "Expected keys to be unique for ${TestWorkflow::class.workflowIdentifier}: key=\"foo\"", error.message ) } diff --git a/workflow-runtime/src/test/java/com/squareup/workflow1/internal/WorkflowNodeTest.kt b/workflow-runtime/src/test/java/com/squareup/workflow1/internal/WorkflowNodeTest.kt index d0da1503ff..75ae5b7b66 100644 --- a/workflow-runtime/src/test/java/com/squareup/workflow1/internal/WorkflowNodeTest.kt +++ b/workflow-runtime/src/test/java/com/squareup/workflow1/internal/WorkflowNodeTest.kt @@ -449,7 +449,7 @@ class WorkflowNodeTest { val error = assertFailsWith { node.render(workflow.asStatefulWorkflow(), Unit) } - assertEquals("Expected side effect keys to be unique: same", error.message) + assertEquals("Expected side effect keys to be unique: \"same\"", error.message) } @Test fun `staggered sideEffects`() { diff --git a/workflow-testing/src/main/java/com/squareup/workflow1/testing/RealRenderTester.kt b/workflow-testing/src/main/java/com/squareup/workflow1/testing/RealRenderTester.kt index a95c13867e..03b85f0d2e 100644 --- a/workflow-testing/src/main/java/com/squareup/workflow1/testing/RealRenderTester.kt +++ b/workflow-testing/src/main/java/com/squareup/workflow1/testing/RealRenderTester.kt @@ -28,6 +28,7 @@ import com.squareup.workflow1.WorkflowIdentifier import com.squareup.workflow1.WorkflowOutput import com.squareup.workflow1.applyTo import com.squareup.workflow1.identifier +import com.squareup.workflow1.testing.RealRenderTester.Expectation import com.squareup.workflow1.testing.RealRenderTester.Expectation.ExpectedSideEffect import com.squareup.workflow1.testing.RealRenderTester.Expectation.ExpectedWorker import com.squareup.workflow1.testing.RealRenderTester.Expectation.ExpectedWorkflow @@ -47,10 +48,36 @@ internal class RealRenderTester( private val workflow: StatefulWorkflow, private val props: PropsT, private val state: StateT, + /** + * List of [Expectation]s that are expected when the workflow is rendered. New expectations are + * registered into this list. Once the render pass has started, expectations are moved from this + * list to [consumedExpectations] as soon as they're matched. + */ private val expectations: MutableList> = mutableListOf(), + /** + * Empty until the render pass starts, then every time the workflow matches an expectation that + * has `exactMatch` set to true, it is moved from [expectations] to this list. + */ private val consumedExpectations: MutableList> = mutableListOf(), + /** + * Flag that is set as soon as an expectation is registered that emits an output. + */ private var childWillEmitOutput: Boolean = false, - private var processedAction: WorkflowAction? = null + /** + * If an expectation includes a [WorkflowOutput], then when that expectation is matched, this + * property stores the [WorkflowAction] that was specified to handle that output. + */ + private var processedAction: WorkflowAction? = null, + /** + * Tracks the identifier/key pairs of all calls to [renderChild], so it can emulate the behavior + * of the real runtime and throw if a workflow is rendered twice in the same pass. + */ + private val renderedChildren: MutableList> = mutableListOf(), + /** + * Tracks the keys of all calls to [runningSideEffect], so it can emulate the behavior of the real + * runtime and throw if a side effects is ran twice in the same pass. + */ + private val ranSideEffects: MutableList = mutableListOf() ) : RenderTester, BaseRenderContext, RenderTestResult, @@ -142,9 +169,15 @@ internal class RealRenderTester( key: String, handler: (ChildOutputT) -> WorkflowAction ): ChildRenderingT { + val identifierPair = Pair(child.identifier, key) + require(identifierPair !in renderedChildren) { + "Expected keys to be unique for ${child.identifier}: key=\"$key\"" + } + renderedChildren += identifierPair + val description = buildString { append("child ") - append(child.identifier.describeRealIdentifier() ?: "workflow ${child.identifier}") + append(child.identifier) if (key.isNotEmpty()) { append(" with key \"$key\"") } @@ -195,6 +228,9 @@ internal class RealRenderTester( key: String, sideEffect: suspend () -> Unit ) { + require(key !in ranSideEffects) { "Expected side effect keys to be unique: \"$key\"" } + ranSideEffects += key + val description = "side effect with key \"$key\"" val matches = expectations.filterIsInstance() diff --git a/workflow-testing/src/test/java/com/squareup/workflow1/testing/RealRenderTesterTest.kt b/workflow-testing/src/test/java/com/squareup/workflow1/testing/RealRenderTesterTest.kt index 7621199b5d..49b9c7db15 100644 --- a/workflow-testing/src/test/java/com/squareup/workflow1/testing/RealRenderTesterTest.kt +++ b/workflow-testing/src/test/java/com/squareup/workflow1/testing/RealRenderTesterTest.kt @@ -17,7 +17,6 @@ package com.squareup.workflow1.testing import com.squareup.workflow1.ExperimentalWorkflowApi import com.squareup.workflow1.ImpostorWorkflow -import com.squareup.workflow1.RenderContext import com.squareup.workflow1.Sink import com.squareup.workflow1.Snapshot import com.squareup.workflow1.StatefulWorkflow @@ -32,6 +31,7 @@ import com.squareup.workflow1.WorkflowOutput import com.squareup.workflow1.contraMap import com.squareup.workflow1.identifier import com.squareup.workflow1.renderChild +import com.squareup.workflow1.rendering import com.squareup.workflow1.runningWorker import com.squareup.workflow1.stateful import com.squareup.workflow1.stateless @@ -70,9 +70,8 @@ class RealRenderTesterTest { val failure = assertFailsWith { tester.render() } - assertEquals( - "Expected only one output to be expected: child workflow ${child2.identifier} " + + "Expected only one output to be expected: child ${child2.identifier} " + "expected to emit kotlin.Unit but WorkflowAction.noAction() was already processed.", failure.message ) @@ -241,7 +240,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier}", + "Tried to render unexpected child ${child.identifier}", error.message ) } @@ -379,6 +378,21 @@ class RealRenderTesterTest { ) } + @Test fun `runningSideEffect throws on duplicate call`() { + val workflow = Workflow.stateless { + runningSideEffect("key") {} + runningSideEffect("key") {} + } + + val tester = workflow.testRender(Unit) + .expectSideEffect("key") + + val error = assertFailsWith { + tester.render() + } + assertEquals("Expected side effect keys to be unique: \"key\"", error.message) + } + @Test fun `renderChild rendering non-Unit throws when none expected and unexpected children are allowed`() { val child = Workflow.stateless { 42 } @@ -391,7 +405,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier}", + "Tried to render unexpected child ${child.identifier}", error.message ) } @@ -409,7 +423,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier}", + "Tried to render unexpected child ${child.identifier}", error.message ) } @@ -427,7 +441,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier}", + "Tried to render unexpected child ${child.identifier}", error.message ) } @@ -443,7 +457,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier} with key \"key\"", + "Tried to render unexpected child ${child.identifier} with key \"key\"", error.message ) } @@ -460,7 +474,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier} with key \"key\"", + "Tried to render unexpected child ${child.identifier} with key \"key\"", error.message ) } @@ -478,7 +492,7 @@ class RealRenderTesterTest { tester.render() } assertEquals( - "Tried to render unexpected child workflow ${child.identifier} with key \"key\"", + "Tried to render unexpected child ${child.identifier} with key \"key\"", error.message ) } @@ -505,7 +519,7 @@ class RealRenderTesterTest { } assertEquals( """ - Multiple expectations matched child workflow ${Child::class.workflowIdentifier}: + Multiple expectations matched child ${Child::class.workflowIdentifier}: workflow identifier=${OutputNothingChild::class.workflowIdentifier}, key=, rendering=kotlin.Unit, output=null workflow identifier=${Child::class.workflowIdentifier}, key=, rendering=kotlin.Unit, output=null """.trimIndent(), @@ -513,6 +527,22 @@ class RealRenderTesterTest { ) } + @Test fun `renderChild throws on duplicate call`() { + val child = Workflow.rendering(Unit) + val workflow = Workflow.stateless { + renderChild(child) + renderChild(child) + } + + val tester = workflow.testRender(Unit) + .expectWorkflow(child.identifier, Unit) + + val error = assertFailsWith { + tester.render() + } + assertEquals("Expected keys to be unique for ${child.identifier}: key=\"\"", error.message) + } + @Test fun `runningWorker doesn't throw when none expected`() { val worker = object : Worker { override fun doesSameWorkAs(otherWorker: Worker<*>): Boolean = true @@ -635,6 +665,23 @@ class RealRenderTesterTest { ) } + @Test fun `runningWorker throws on duplicate call`() { + val worker = Worker.createSideEffect {} + val workflow = Workflow.stateless { + runningWorker(worker) + runningWorker(worker) + } + + val tester = workflow.testRender(Unit) + val error = assertFailsWith { + tester.render() + } + assertEquals( + "Expected keys to be unique for worker com.squareup.workflow1.Worker: key=\"\"", + error.message + ) + } + @Test fun `render throws when unconsumed workflow`() { val workflow = Workflow.stateless { // Do nothing. @@ -754,7 +801,7 @@ class RealRenderTesterTest { tester.render {} } assertEquals( - "Tried to render unexpected child workflow $actualId", error.message + "Tried to render unexpected child $actualId", error.message ) } diff --git a/workflow-tracing/src/main/java/com/squareup/workflow1/diagnostic/tracing/TracingWorkflowInterceptor.kt b/workflow-tracing/src/main/java/com/squareup/workflow1/diagnostic/tracing/TracingWorkflowInterceptor.kt index 20a7717658..940fa2cb3a 100644 --- a/workflow-tracing/src/main/java/com/squareup/workflow1/diagnostic/tracing/TracingWorkflowInterceptor.kt +++ b/workflow-tracing/src/main/java/com/squareup/workflow1/diagnostic/tracing/TracingWorkflowInterceptor.kt @@ -169,7 +169,7 @@ class TracingWorkflowInterceptor internal constructor( onWorkflowStarted( workflowId = session.sessionId, parentId = session.parent?.sessionId, - workflowType = session.identifier.let { it.describeRealIdentifier() ?: it.toString() }, + workflowType = session.identifier.toString(), key = session.renderKey, initialProps = props, initialState = initialState,