Make RenderTester throw a more realistic exception if the same workflow/side effect is ran twice.

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

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

workflow-core/src/test/java/com/squareup/workflow1/WorkflowIdentifierTest.kt

@@ -36,18 +36,41 @@ 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<Nothing, Nothing, Nothing>, ImpostorWorkflow {
+      override val realIdentifier: WorkflowIdentifier = TestWorkflow1.identifier
+      override fun describeRealIdentifier(): String? =
+        "TestImpostor(${TestWorkflow1::class.simpleName})"
+
+      override fun asStatefulWorkflow(): StatefulWorkflow<Nothing, *, Nothing, Nothing> =
+        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<Nothing, Nothing, Nothing>, ImpostorWorkflow {
+      override val realIdentifier: WorkflowIdentifier = TestWorkflow1.identifier
+      override fun describeRealIdentifier(): String? = null
+
+      override fun asStatefulWorkflow(): StatefulWorkflow<Nothing, *, Nothing, Nothing> =
+        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()
     )
   }
 
   @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`() {

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<PropsT, StateT, OutputT>(
     // 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\""
       }
     }
 

workflow-runtime/src/main/java/com/squareup/workflow1/internal/WorkflowNode.kt

@@ -142,7 +142,7 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
   ) {
     // 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(

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
     )
   }

workflow-runtime/src/test/java/com/squareup/workflow1/internal/WorkflowNodeTest.kt

@@ -449,7 +449,7 @@ class WorkflowNodeTest {
     val error = assertFailsWith<IllegalArgumentException> {
       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`() {

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<PropsT, StateT, OutputT, RenderingT>(
   private val workflow: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>,
   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<Expectation<*>> = 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<Expectation<*>> = 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<PropsT, StateT, OutputT>? = 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<PropsT, StateT, OutputT>? = 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<Pair<WorkflowIdentifier, String>> = 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<String> = mutableListOf()
 ) : RenderTester<PropsT, StateT, OutputT, RenderingT>,
     BaseRenderContext<PropsT, StateT, OutputT>,
     RenderTestResult<PropsT, StateT, OutputT>,
@@ -142,9 +169,15 @@ internal class RealRenderTester<PropsT, StateT, OutputT, RenderingT>(
     key: String,
     handler: (ChildOutputT) -> WorkflowAction<PropsT, StateT, OutputT>
   ): 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<PropsT, StateT, OutputT, RenderingT>(
     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<ExpectedSideEffect>()

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<IllegalStateException> {
       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<Unit, Nothing, Unit> {
+      runningSideEffect("key") {}
+      runningSideEffect("key") {}
+    }
+
+    val tester = workflow.testRender(Unit)
+        .expectSideEffect("key")
+
+    val error = assertFailsWith<IllegalArgumentException> {
+      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<Unit, Nothing, Int> { 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,14 +519,30 @@ 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(),
         error.message
     )
   }
 
+  @Test fun `renderChild throws on duplicate call`() {
+    val child = Workflow.rendering(Unit)
+    val workflow = Workflow.stateless<Unit, Nothing, Unit> {
+      renderChild(child)
+      renderChild(child)
+    }
+
+    val tester = workflow.testRender(Unit)
+        .expectWorkflow<Nothing, Unit>(child.identifier, Unit)
+
+    val error = assertFailsWith<IllegalArgumentException> {
+      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<Nothing> {
       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<Unit, Nothing, Unit> {
+      runningWorker(worker)
+      runningWorker(worker)
+    }
+
+    val tester = workflow.testRender(Unit)
+    val error = assertFailsWith<IllegalArgumentException> {
+      tester.render()
+    }
+    assertEquals(
+        "Expected keys to be unique for worker com.squareup.workflow1.Worker<java.lang.Void>: key=\"\"",
+        error.message
+    )
+  }
+
   @Test fun `render throws when unconsumed workflow`() {
     val workflow = Workflow.stateless<Unit, Nothing, Unit> {
       // 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
     )
   }
 

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,