diff --git a/.github/workflows/kotlin.yml b/.github/workflows/kotlin.yml index e7f78da1ee..e5265f6462 100644 --- a/.github/workflows/kotlin.yml +++ b/.github/workflows/kotlin.yml @@ -188,6 +188,34 @@ jobs : with : report_paths : '**/build/test-results/test/TEST-*.xml' + jvm-stateChange-runtime-test : + name : Render On State Change Only Runtime JVM Tests + runs-on : ubuntu-latest + timeout-minutes : 20 + steps : + - uses : actions/checkout@v3 + - uses : gradle/wrapper-validation-action@v1 + - name : set up JDK 11 + uses : actions/setup-java@v3 + with : + distribution : 'zulu' + java-version : 11 + + ## Actual task + - uses : gradle/gradle-build-action@v2 + name : Check with Gradle + with : + arguments : | + jvmTest --continue -Pworkflow.runtime=stateChange + cache-read-only : false + + # Report as Github Pull Request Check. + - name : Publish Test Report + uses : mikepenz/action-junit-report@v3 + if : always() # always run even if the previous step fails + with : + report_paths : '**/build/test-results/test/TEST-*.xml' + ios-tests : name : iOS Tests runs-on : macos-latest @@ -280,7 +308,6 @@ jobs : profile : Galaxy Nexus api-level : ${{ matrix.api-level }} arch : x86_64 - # Skip the benchmarks as this is running on emulators script : ./gradlew :benchmarks:performance-poetry:complex-poetry:connectedCheck --continue - name : Upload results @@ -384,6 +411,54 @@ jobs : name : instrumentation-test-results-${{ matrix.api-level }} path : ./**/build/reports/androidTests/connected/** + stateChange-runtime-instrumentation-tests : + name : Render on State Change Only Instrumentation tests + runs-on : macos-latest + timeout-minutes : 45 + strategy : + # Allow tests to continue on other devices if they fail on one device. + fail-fast : false + matrix : + api-level : + - 29 + # Unclear that older versions actually honor command to disable animation. + # Newer versions are reputed to be too slow: https://github.com/ReactiveCircus/android-emulator-runner/issues/222 + steps : + - uses : actions/checkout@v3 + - name : set up JDK 11 + uses : actions/setup-java@v3 + with : + distribution : 'zulu' + java-version : 11 + + ## Build before running tests, using cache. + - uses : gradle/gradle-build-action@v2 + name : Build instrumented tests + with : + # Unfortunately I don't think we can key this cache based on our project property so + # we clean and rebuild. + arguments : | + clean assembleDebugAndroidTest -Pworkflow.runtime=stateChange + cache-read-only : false + + ## Actual task + - name : Instrumentation Tests + uses : reactivecircus/android-emulator-runner@v2 + with : + # @ychescale9 suspects Galaxy Nexus is the fastest one + profile : Galaxy Nexus + api-level : ${{ matrix.api-level }} + arch : x86_64 + # Skip the benchmarks as this is running on emulators + script : ./gradlew connectedCheck -x :benchmarks:dungeon-benchmark:connectedCheck -x :benchmarks:performance-poetry:complex-benchmark:connectedCheck -x :benchmarks:performance-poetry:complex-poetry:connectedCheck -Pworkflow.runtime=stateChange + + - name : Upload results + if : ${{ always() }} + uses : actions/upload-artifact@v3 + with : + name : instrumentation-test-results-${{ matrix.api-level }} + path : ./**/build/reports/androidTests/connected/** + upload-to-mobiledev : name : mobile.dev | Build & Upload runs-on : ubuntu-latest diff --git a/gradle.properties b/gradle.properties index 17c28d646d..fa4c8fb8db 100644 --- a/gradle.properties +++ b/gradle.properties @@ -8,7 +8,7 @@ android.useAndroidX=true systemProp.org.gradle.internal.publish.checksums.insecure=true GROUP=com.squareup.workflow1 -VERSION_NAME=1.10.0-beta06-SNAPSHOT +VERSION_NAME=1.10.0-beta06-state2-SNAPSHOT POM_DESCRIPTION=Square Workflow diff --git a/samples/dungeon/timemachine/src/test/java/com/squareup/sample/timemachine/TimeMachineWorkflowTest.kt b/samples/dungeon/timemachine/src/test/java/com/squareup/sample/timemachine/TimeMachineWorkflowTest.kt index a2b55317e7..5482765bc2 100644 --- a/samples/dungeon/timemachine/src/test/java/com/squareup/sample/timemachine/TimeMachineWorkflowTest.kt +++ b/samples/dungeon/timemachine/src/test/java/com/squareup/sample/timemachine/TimeMachineWorkflowTest.kt @@ -25,7 +25,12 @@ class TimeMachineWorkflowTest { val delegateWorkflow = Workflow.stateful( initialState = "initial", render = { renderState -> - DelegateRendering(renderState, setState = eventHandler { s -> state = s }) + DelegateRendering( + renderState, + setState = eventHandler { s -> + state = s + } + ) } ) val clock = TestTimeSource() diff --git a/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt b/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt index a352341252..5359d2289c 100644 --- a/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt +++ b/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt @@ -2,6 +2,7 @@ package com.squareup.workflow1.config import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings +import com.squareup.workflow1.RuntimeConfig.RenderOnStateChangeOnly import com.squareup.workflow1.RuntimeConfig.RenderPerAction import com.squareup.workflow1.WorkflowExperimentalRuntime @@ -18,6 +19,8 @@ public class AndroidRuntimeConfigTools { * this function, and then pass that to the call to [renderWorkflowIn] as the [RuntimeConfig]. * * Current options are: + * "stateChange" : [RenderOnStateChangeOnly] Only re-render when the state of some WorkflowNode + * has changed. * "conflate" : [ConflateStaleRenderings] Process all queued actions before passing rendering * to the UI layer. * "baseline" : [RenderPerAction] Original Workflow Runtime. Note that this doesn't need to @@ -26,6 +29,7 @@ public class AndroidRuntimeConfigTools { @WorkflowExperimentalRuntime public fun getAppWorkflowRuntimeConfig(): RuntimeConfig { return when (BuildConfig.WORKFLOW_RUNTIME) { + "stateChange" -> RenderOnStateChangeOnly "conflate" -> ConflateStaleRenderings else -> RenderPerAction } diff --git a/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt b/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt index 16172a373f..5b19dfb1ac 100644 --- a/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt +++ b/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt @@ -2,6 +2,7 @@ package com.squareup.workflow1.config import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings +import com.squareup.workflow1.RuntimeConfig.RenderOnStateChangeOnly import com.squareup.workflow1.RuntimeConfig.RenderPerAction import com.squareup.workflow1.WorkflowExperimentalRuntime @@ -17,6 +18,8 @@ public class JvmTestRuntimeConfigTools { * [RuntimeConfig]. * * Current options are: + * "stateChange" : [RenderOnStateChangeOnly] Only re-render when the state of some WorkflowNode + * has changed. * "conflate" : [ConflateStaleRenderings] Process all queued actions before passing rendering * to the UI layer. * "baseline" : [RenderPerAction] Original Workflow Runtime. Note that this doesn't need to @@ -26,6 +29,7 @@ public class JvmTestRuntimeConfigTools { public fun getTestRuntimeConfig(): RuntimeConfig { val runtimeConfig = System.getProperty("workflow.runtime", "baseline") return when (runtimeConfig) { + "stateChange" -> RenderOnStateChangeOnly "conflate" -> ConflateStaleRenderings else -> RenderPerAction } diff --git a/workflow-core/api/workflow-core.api b/workflow-core/api/workflow-core.api index 74ad42fee8..84e574c20b 100644 --- a/workflow-core/api/workflow-core.api +++ b/workflow-core/api/workflow-core.api @@ -1,3 +1,17 @@ +public final class com/squareup/workflow1/ActionApplied : com/squareup/workflow1/ActionProcessingResult { + public fun (Lcom/squareup/workflow1/WorkflowOutput;Z)V + public synthetic fun (Lcom/squareup/workflow1/WorkflowOutput;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V + public final fun component1 ()Lcom/squareup/workflow1/WorkflowOutput; + public final fun component2 ()Z + public final fun copy (Lcom/squareup/workflow1/WorkflowOutput;Z)Lcom/squareup/workflow1/ActionApplied; + public static synthetic fun copy$default (Lcom/squareup/workflow1/ActionApplied;Lcom/squareup/workflow1/WorkflowOutput;ZILjava/lang/Object;)Lcom/squareup/workflow1/ActionApplied; + public fun equals (Ljava/lang/Object;)Z + public final fun getOutput ()Lcom/squareup/workflow1/WorkflowOutput; + public final fun getStateChanged ()Z + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + public abstract interface class com/squareup/workflow1/ActionProcessingResult { } @@ -217,6 +231,7 @@ public final class com/squareup/workflow1/WorkflowAction$Companion { public final class com/squareup/workflow1/WorkflowAction$Updater { public fun (Lcom/squareup/workflow1/WorkflowAction;Ljava/lang/Object;Ljava/lang/Object;)V + public final fun forceRerender ()V public final fun getProps ()Ljava/lang/Object; public final fun getState ()Ljava/lang/Object; public final fun setOutput (Ljava/lang/Object;)V @@ -271,7 +286,7 @@ public final class com/squareup/workflow1/WorkflowIdentifierType$Unsnapshottable public fun toString ()Ljava/lang/String; } -public final class com/squareup/workflow1/WorkflowOutput : com/squareup/workflow1/ActionProcessingResult { +public final class com/squareup/workflow1/WorkflowOutput { public fun (Ljava/lang/Object;)V public fun equals (Ljava/lang/Object;)Z public final fun getValue ()Ljava/lang/Object; diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt index c1d76a97ed..5d900104c4 100644 --- a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt @@ -22,7 +22,9 @@ public abstract class WorkflowAction { public val props: @UnsafeVariance PropsT, public var state: StateT ) { - internal var maybeOutput: WorkflowOutput<@UnsafeVariance OutputT>? = null + internal val startingState = state + internal var forcedRerender: Boolean = false + internal var outputOrNull: WorkflowOutput<@UnsafeVariance OutputT>? = null private set /** @@ -30,7 +32,15 @@ public abstract class WorkflowAction { * If this method is not called, there will be no output. */ public fun setOutput(output: @UnsafeVariance OutputT) { - this.maybeOutput = WorkflowOutput(output) + this.outputOrNull = WorkflowOutput(output) + } + + /** + * The action will cause the Workflow to re-render regardless of whether or not state has + * changed, for all [RuntimeConfig]s. + */ + public fun forceRerender() { + forcedRerender = true } } @@ -105,24 +115,24 @@ public fun action( public fun WorkflowAction.applyTo( props: PropsT, state: StateT -): Pair?> { +): Pair> { val updater = Updater(props, state) updater.apply() - return Pair(updater.state, updater.maybeOutput) + return Pair( + updater.state, + ActionApplied( + output = updater.outputOrNull, + stateChanged = updater.forcedRerender || updater.state != updater.startingState, + ) + ) } /** - * Only [WorkflowOutput] needs the generic OutputT so we do not include it in the root - * interface here. + * Box around a potentially nullable [OutputT] */ -public sealed interface ActionProcessingResult - -public object PropsUpdated : ActionProcessingResult - -public object ActionsExhausted : ActionProcessingResult - -/** Wrapper around a potentially-nullable [OutputT] value. */ -public class WorkflowOutput(public val value: OutputT) : ActionProcessingResult { +public class WorkflowOutput( + public val value: OutputT +) { override fun toString(): String = "WorkflowOutput($value)" override fun equals(other: Any?): Boolean = when { @@ -133,3 +143,28 @@ public class WorkflowOutput(public val value: OutputT) : ActionProc override fun hashCode(): Int = value.hashCode() } + +/** + * An [ActionProcessingResult] is any possible outcome after the runtime does a loop of processing. + * + * Only [ActionApplied] needs the generic OutputT so we do not include it in the root + * interface here. + */ +public sealed interface ActionProcessingResult + +public object PropsUpdated : ActionProcessingResult + +public object ActionsExhausted : ActionProcessingResult + +/** + * Result of applying an action. + * + * @param output: the potentially null [WorkflowOutput]. If null, then no output was set by the + * action. Otherwise it is a [WorkflowOutput] around the output value of type [OutputT], + * which could be null. + * @param stateChanged: whether or not the action changed the state. + */ +public data class ActionApplied( + public val output: WorkflowOutput?, + public val stateChanged: Boolean = false, +) : ActionProcessingResult diff --git a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/SinkTest.kt b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/SinkTest.kt index 3e2cf541c5..ee65214f4a 100644 --- a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/SinkTest.kt +++ b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/SinkTest.kt @@ -21,7 +21,6 @@ import kotlin.test.fail @OptIn( ExperimentalCoroutinesApi::class, - ExperimentalStdlibApi::class, ) internal class SinkTest { @@ -42,9 +41,9 @@ internal class SinkTest { assertEquals(1, sink.actions.size) sink.actions.removeFirst() .let { action -> - val (newState, output) = action.applyTo("props", "state") + val (newState, result) = action.applyTo("props", "state") assertEquals("props state 1", newState) - assertEquals("output: 1", output?.value) + assertEquals("output: 1", result.output!!.value) } assertTrue(sink.actions.isEmpty()) @@ -53,9 +52,9 @@ internal class SinkTest { assertEquals(1, sink.actions.size) sink.actions.removeFirst() .let { action -> - val (newState, output) = action.applyTo("props", "state") + val (newState, result) = action.applyTo("props", "state") assertEquals("props state 2", newState) - assertEquals("output: 2", output?.value) + assertEquals("output: 2", result.output!!.value) } collector.cancel() @@ -95,16 +94,16 @@ internal class SinkTest { assertEquals(3, counter.getAndIncrement()) } .applyTo(Unit, Unit) - .let { (_, output) -> + .let { (_, result) -> assertEquals(6, counter.getAndIncrement()) - assertEquals("a", output?.value) + assertEquals("a", result.output!!.value) } sentActions.removeFirst() .applyTo(Unit, Unit) - .let { (_, output) -> + .let { (_, result) -> assertEquals(7, counter.getAndIncrement()) - assertEquals("b", output?.value) + assertEquals("b", result.output!!.value) } collectJob.cancel() @@ -125,10 +124,10 @@ internal class SinkTest { advanceUntilIdle() val enqueuedAction = sink.actions.removeFirst() - val (newState, output) = enqueuedAction.applyTo("props", "state") + val (newState, result) = enqueuedAction.applyTo("props", "state") assertEquals(1, applications) assertEquals("props state applied", newState) - assertEquals("output", output?.value) + assertEquals("output", result.output!!.value) } } @@ -171,11 +170,11 @@ internal class SinkTest { val enqueuedAction = sink.actions.removeFirst() sendJob.cancel() advanceUntilIdle() - val (newState, output) = enqueuedAction.applyTo("unused props", "state") + val (newState, result) = enqueuedAction.applyTo("unused props", "state") assertFalse(applied) assertEquals("state", newState) - assertNull(output) + assertNull(result.output) } private class RecordingSink : Sink> { diff --git a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowActionTest.kt b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowActionTest.kt index 7e776c9a38..f1c90ee4be 100644 --- a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowActionTest.kt +++ b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowActionTest.kt @@ -2,20 +2,35 @@ package com.squareup.workflow1 import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertTrue internal class WorkflowActionTest { + @Test fun applyTo_works_when_state_is_not_changed() { + val action = object : WorkflowAction() { + override fun Updater.apply() { + // no-op + } + } + val (state, result) = action.applyTo("props", "state") + assertEquals("state", state) + assertNull(result.output) + assertFalse(result.stateChanged) + } + @Test fun applyTo_works_when_no_output_is_set() { val action = object : WorkflowAction() { override fun Updater.apply() { state = "state: $state, props: $props" } } - val (state, output) = action.applyTo("props", "state") + val (state, result) = action.applyTo("props", "state") assertEquals("state: state, props: props", state) - assertNull(output) + assertNull(result.output) + assertTrue(result.stateChanged) } @Test fun applyTo_works_when_null_output_is_set() { @@ -25,10 +40,12 @@ internal class WorkflowActionTest { setOutput(null) } } - val (state, output) = action.applyTo("props", "state") + val (state, result) = action.applyTo("props", "state") assertEquals("state: state, props: props", state) - assertNotNull(output) - assertNull(output.value) + assertNotNull(result) + assertNotNull(result.output) + assertNull(result.output!!.value) + assertTrue(result.stateChanged) } @Test fun applyTo_works_when_non_null_output_is_set() { @@ -38,9 +55,23 @@ internal class WorkflowActionTest { setOutput("output") } } - val (state, output) = action.applyTo("props", "state") + val (state, result) = action.applyTo("props", "state") assertEquals("state: state, props: props", state) - assertNotNull(output) - assertEquals("output", output.value) + assertNotNull(result) + assertEquals("output", result.output!!.value) + assertTrue(result.stateChanged) + } + + @Test fun applyTo_works_with_force_rerender() { + val action = object : WorkflowAction() { + override fun Updater.apply() { + forceRerender() + // no-op + } + } + val (state, result) = action.applyTo("props", "state") + assertEquals("state", state) + assertNull(result.output) + assertTrue(result.stateChanged) } } diff --git a/workflow-runtime/api/workflow-runtime.api b/workflow-runtime/api/workflow-runtime.api index ea6a09b24c..1d5436b93b 100644 --- a/workflow-runtime/api/workflow-runtime.api +++ b/workflow-runtime/api/workflow-runtime.api @@ -34,6 +34,10 @@ public final class com/squareup/workflow1/RuntimeConfig$ConflateStaleRenderings public static final field INSTANCE Lcom/squareup/workflow1/RuntimeConfig$ConflateStaleRenderings; } +public final class com/squareup/workflow1/RuntimeConfig$RenderOnStateChangeOnly : com/squareup/workflow1/RuntimeConfig { + public static final field INSTANCE Lcom/squareup/workflow1/RuntimeConfig$RenderOnStateChangeOnly; +} + public final class com/squareup/workflow1/RuntimeConfig$RenderPerAction : com/squareup/workflow1/RuntimeConfig { public static final field INSTANCE Lcom/squareup/workflow1/RuntimeConfig$RenderPerAction; } diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt index ddd390f0c1..f0e5d61fc4 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt @@ -1,6 +1,7 @@ package com.squareup.workflow1 import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings +import com.squareup.workflow1.RuntimeConfig.RenderOnStateChangeOnly import com.squareup.workflow1.internal.WorkflowRunner import com.squareup.workflow1.internal.chained import kotlinx.coroutines.CancellationException @@ -135,39 +136,57 @@ public fun renderWorkflowIn( ) suspend fun sendOutput( - actionResult: ActionProcessingResult?, + actionResult: ActionProcessingResult, onOutput: suspend (OutputT) -> Unit ) { when (actionResult) { - is WorkflowOutput<*> -> { + is ActionApplied<*> -> { @Suppress("UNCHECKED_CAST") - (actionResult as? WorkflowOutput)?.let { - onOutput(it.value) + (actionResult as? ActionApplied)?.let { + it.output?.let { actualOutput -> + onOutput(actualOutput.value) + } } } + else -> {} // no -op } } + /** + * We only check for this under [RenderOnStateChangeOnly] [RuntimeConfig]. + */ + suspend fun maybeCheckNoStateChange(actionResult: ActionProcessingResult): Boolean { + if (runtimeConfig == RenderOnStateChangeOnly && + actionResult is ActionApplied<*> && !actionResult.stateChanged + ) { + // Possibly send output and process more actions. No state change so no re-render. + sendOutput(actionResult, onOutput) + return true + } + return false + } + scope.launch { while (isActive) { - lateinit var nextRenderAndSnapshot: RenderingAndSnapshot // It might look weird to start by processing an action before getting the rendering below, // but remember the first render pass already occurred above, before this coroutine was even // launched. - var actionResult: ActionProcessingResult? = runner.processAction() + var actionResult: ActionProcessingResult = runner.processAction() + + if (maybeCheckNoStateChange(actionResult)) continue // After resuming from runner.processAction() our coroutine could now be cancelled, check so // we don't surprise anyone with an unexpected rendering pass. Show's over, go home. if (!isActive) return@launch - nextRenderAndSnapshot = runner.nextRendering() + var nextRenderAndSnapshot: RenderingAndSnapshot = runner.nextRendering() if (runtimeConfig == ConflateStaleRenderings) { // Only null will allow us to continue processing actions and conflating stale renderings. // If this is not null, then we had an Output and we want to send it with the Rendering // (stale or not). - while (actionResult == null) { + while (actionResult is ActionApplied<*> && actionResult.output == null) { // We have more actions we can process, so this rendering is stale. actionResult = runner.processAction(waitForAnAction = false) diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt index 7fdc41c498..ed4e69b311 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt @@ -33,6 +33,12 @@ public sealed interface RuntimeConfig { @WorkflowExperimentalRuntime public object ConflateStaleRenderings : RuntimeConfig + /** + * If state has not changed, do not re-render. + */ + @WorkflowExperimentalRuntime + public object RenderOnStateChangeOnly : RuntimeConfig + public companion object { public val DEFAULT_CONFIG: RuntimeConfig = RenderPerAction } diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt index bbcb099bad..2e77c31473 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt @@ -1,5 +1,6 @@ package com.squareup.workflow1.internal +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.ActionProcessingResult import com.squareup.workflow1.NoopWorkflowInterceptor import com.squareup.workflow1.TreeSnapshot @@ -85,8 +86,9 @@ internal class SubtreeManager( private var snapshotCache: Map?, private val contextForChildren: CoroutineContext, private val emitActionToParent: ( - action: WorkflowAction - ) -> ActionProcessingResult?, + action: WorkflowAction, + childResult: ActionApplied<*> + ) -> ActionProcessingResult, private val workflowSession: WorkflowSession? = null, private val interceptor: WorkflowInterceptor = NoopWorkflowInterceptor, private val idCounter: IdCounter? = null @@ -138,7 +140,7 @@ internal class SubtreeManager( * * @return [Boolean] whether or not the children action queues are empty. */ - fun onNextChildAction(selector: SelectBuilder): Boolean { + fun onNextChildAction(selector: SelectBuilder): Boolean { var empty = true children.forEachActive { child -> // Do this separately so the compiler doesn't avoid it if empty is already false. @@ -166,9 +168,13 @@ internal class SubtreeManager( val id = child.id(key) lateinit var node: WorkflowChildNode - fun acceptChildOutput(output: ChildOutputT): ActionProcessingResult? { - val action = node.acceptChildOutput(output) - return emitActionToParent(action) + fun acceptChildActionResult(actionResult: ActionApplied): ActionProcessingResult { + val action = if (actionResult.output != null) { + node.acceptChildOutput(actionResult.output!!.value) + } else { + WorkflowAction.noAction() + } + return emitActionToParent(action, actionResult) } val childTreeSnapshots = snapshotCache?.get(id) @@ -179,7 +185,7 @@ internal class SubtreeManager( initialProps, childTreeSnapshots, contextForChildren, - ::acceptChildOutput, + ::acceptChildActionResult, workflowSession, interceptor, idCounter = idCounter diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt index 54829a631a..f57c21be25 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt @@ -1,5 +1,6 @@ package com.squareup.workflow1.internal +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.ActionProcessingResult import com.squareup.workflow1.NoopWorkflowInterceptor import com.squareup.workflow1.RenderContext @@ -10,7 +11,6 @@ import com.squareup.workflow1.WorkflowAction import com.squareup.workflow1.WorkflowIdentifier import com.squareup.workflow1.WorkflowInterceptor import com.squareup.workflow1.WorkflowInterceptor.WorkflowSession -import com.squareup.workflow1.WorkflowOutput import com.squareup.workflow1.applyTo import com.squareup.workflow1.intercept import com.squareup.workflow1.internal.RealRenderContext.SideEffectRunner @@ -31,8 +31,8 @@ import kotlin.coroutines.CoroutineContext /** * A node in a state machine tree. Manages the actual state for a given [Workflow]. * - * @param emitOutputToParent A function that this node will call when it needs to emit an output - * value to its parent. Returns either the output to be emitted from the root workflow, or null. + * @param emitAppliedActionToParent A function that this node will call to pass the result of + * applying an action to its parent. * @param baseContext [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 (it would violate @@ -44,7 +44,8 @@ internal class WorkflowNode( initialProps: PropsT, snapshot: TreeSnapshot?, baseContext: CoroutineContext, - private val emitOutputToParent: (OutputT) -> ActionProcessingResult? = { WorkflowOutput(it) }, + private val emitAppliedActionToParent: (ActionApplied) -> ActionProcessingResult = + { it }, override val parent: WorkflowSession? = null, private val interceptor: WorkflowInterceptor = NoopWorkflowInterceptor, idCounter: IdCounter? = null @@ -159,7 +160,7 @@ internal class WorkflowNode( * time of suspending. */ @OptIn(ExperimentalCoroutinesApi::class) - fun onNextAction(selector: SelectBuilder): Boolean { + fun onNextAction(selector: SelectBuilder): Boolean { // Listen for any child workflow updates. var empty = subtreeManager.onNextChildAction(selector) @@ -226,15 +227,26 @@ internal class WorkflowNode( } /** - * Applies [action] to this workflow's [state] and - * [emits an output to its parent][emitOutputToParent] if necessary. + * Applies [action] to this workflow's [state] and then passes the resulting [ActionApplied] + * via [emitAppliedActionToParent] to the parent, with additional information as to whether or + * not this action has changed the current node's state. */ private fun applyAction( - action: WorkflowAction - ): ActionProcessingResult? { - val (newState, outputOrNull) = action.applyTo(lastProps, state) + action: WorkflowAction, + childResult: ActionApplied<*>? = null + ): ActionProcessingResult { + val (newState, actionApplied) = action.applyTo(lastProps, state) state = newState - return outputOrNull?.let { emitOutputToParent(it.value) } + // Aggregate the action with the child result, if any. + val aggregateActionApplied = actionApplied.copy( + // Changing state is sticky, we pass it up if it ever changed. + stateChanged = actionApplied.stateChanged || (childResult?.stateChanged ?: false) + ) + return if (actionApplied.output != null) { + emitAppliedActionToParent(aggregateActionApplied) + } else { + aggregateActionApplied + } } private fun createSideEffectNode( diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowRunner.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowRunner.kt index 8a4121a734..595ca36e76 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowRunner.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowRunner.kt @@ -80,7 +80,7 @@ internal class WorkflowRunner( * coroutine and no others. */ @OptIn(WorkflowExperimentalRuntime::class) - suspend fun processAction(waitForAnAction: Boolean = true): ActionProcessingResult? { + suspend fun processAction(waitForAnAction: Boolean = true): ActionProcessingResult { // If waitForAction is true we block and wait until there is an action to process. return select { onPropsUpdated() @@ -97,7 +97,7 @@ internal class WorkflowRunner( } } - private fun SelectBuilder.onPropsUpdated() { + private fun SelectBuilder.onPropsUpdated() { // Stop trying to read from the inputs channel after it's closed. if (!propsChannel.isClosedForReceive) { propsChannel.onReceiveCatching { channelResult -> diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt index 9d9d94a327..fa97df14a0 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt @@ -1,6 +1,7 @@ package com.squareup.workflow1 import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings +import com.squareup.workflow1.RuntimeConfig.RenderOnStateChangeOnly import com.squareup.workflow1.RuntimeConfig.RenderPerAction import com.squareup.workflow1.internal.ParameterizedTestRunner import kotlinx.coroutines.CancellationException @@ -45,6 +46,7 @@ class RenderWorkflowInTest { private val runtimeOptions = arrayOf( RenderPerAction, ConflateStaleRenderings, + RenderOnStateChangeOnly ).asSequence() private val runtimeTestRunner = ParameterizedTestRunner() @@ -174,8 +176,13 @@ class RenderWorkflowInTest { private val runtimeMatrix = arrayOf( Pair(RenderPerAction, RenderPerAction), Pair(RenderPerAction, ConflateStaleRenderings), + Pair(RenderPerAction, RenderOnStateChangeOnly), Pair(ConflateStaleRenderings, RenderPerAction), Pair(ConflateStaleRenderings, ConflateStaleRenderings), + Pair(ConflateStaleRenderings, RenderOnStateChangeOnly), + Pair(RenderOnStateChangeOnly, RenderPerAction), + Pair(RenderOnStateChangeOnly, ConflateStaleRenderings), + Pair(RenderOnStateChangeOnly, RenderOnStateChangeOnly), ).asSequence() private val runtimeMatrixTestRunner = ParameterizedTestRunner>() @@ -275,11 +282,22 @@ class RenderWorkflowInTest { scope.launch { renderings.collect { emitted += it } } - sink.send("unchanging state") + + if (runtimeConfig is RenderOnStateChangeOnly) { + // we have to change state then or it won't render. + sink.send("changing state") + } else { + sink.send("unchanging state") + } testScope.advanceUntilIdle() testScope.runCurrent() - sink.send("unchanging state") + if (runtimeConfig is RenderOnStateChangeOnly) { + // we have to change state then or it won't render. + sink.send("changing state, again") + } else { + sink.send("unchanging state") + } testScope.advanceUntilIdle() testScope.runCurrent() @@ -949,5 +967,123 @@ class RenderWorkflowInTest { } } + @Test fun for_render_on_state_change_only_we_do_not_render_if_state_not_changed() { + runtimeTestRunner.runParametrizedTest( + paramSource = arrayOf(RenderOnStateChangeOnly).asSequence(), + before = ::setup, + ) { runtimeConfig: RuntimeConfig -> + check(runtimeConfig is RenderOnStateChangeOnly) + lateinit var sink: Sink + + val workflow = Workflow.stateful( + initialState = { "unchanging state" }, + render = { _, renderState -> + sink = actionSink.contraMap { action { state = it } } + renderState + } + ) + val props = MutableStateFlow(Unit) + val renderings = renderWorkflowIn( + workflow = workflow, + scope = testScope, + props = props, + runtimeConfig = runtimeConfig + ) {} + + val emitted = mutableListOf>() + val scope = CoroutineScope(Unconfined) + scope.launch { + renderings.collect { emitted += it } + } + + sink.send("unchanging state") + testScope.advanceUntilIdle() + testScope.runCurrent() + scope.cancel() + + assertEquals(1, emitted.size) + } + } + + @Test fun for_render_on_state_change_only_we_render_if_state_changed() { + runtimeTestRunner.runParametrizedTest( + paramSource = arrayOf(RenderOnStateChangeOnly).asSequence(), + before = ::setup, + ) { runtimeConfig: RuntimeConfig -> + check(runtimeConfig is RenderOnStateChangeOnly) + lateinit var sink: Sink + + val workflow = Workflow.stateful( + initialState = { "unchanging state" }, + render = { _, renderState -> + sink = actionSink.contraMap { action { state = it } } + renderState + } + ) + val props = MutableStateFlow(Unit) + val renderings = renderWorkflowIn( + workflow = workflow, + scope = testScope, + props = props, + runtimeConfig = runtimeConfig + ) {} + + val emitted = mutableListOf>() + val scope = CoroutineScope(Unconfined) + scope.launch { + renderings.collect { emitted += it } + } + + sink.send("changing state") + testScope.advanceUntilIdle() + testScope.runCurrent() + scope.cancel() + + assertEquals(2, emitted.size) + } + } + + @Test fun for_all_configs_we_render_if_state_not_changed_but_forced() { + runtimeTestRunner.runParametrizedTest( + paramSource = runtimeOptions, + before = ::setup, + ) { runtimeConfig: RuntimeConfig -> + lateinit var sink: Sink + + val workflow = Workflow.stateful( + initialState = { "unchanging state" }, + render = { _, renderState -> + sink = actionSink.contraMap { + action { + state = it + forceRerender() + } + } + renderState + } + ) + val props = MutableStateFlow(Unit) + val renderings = renderWorkflowIn( + workflow = workflow, + scope = testScope, + props = props, + runtimeConfig = runtimeConfig + ) {} + + val emitted = mutableListOf>() + val scope = CoroutineScope(Unconfined) + scope.launch { + renderings.collect { emitted += it } + } + + sink.send("unchanging state") + testScope.advanceUntilIdle() + testScope.runCurrent() + scope.cancel() + + assertEquals(2, emitted.size) + } + } + private class ExpectedException : RuntimeException() } diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt index c62bc8c0b7..735b966f3a 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt @@ -1,7 +1,6 @@ package com.squareup.workflow1 import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -16,7 +15,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.fail -@OptIn(ExperimentalCoroutinesApi::class, ExperimentalStdlibApi::class, FlowPreview::class) +@OptIn(ExperimentalCoroutinesApi::class) class WorkflowOperatorsTest { @Test fun mapRendering_toString() { diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/RealRenderContextTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/RealRenderContextTest.kt index e37d80fda7..cbc23388e1 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/RealRenderContextTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/RealRenderContextTest.kt @@ -21,6 +21,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue import kotlin.test.fail @@ -159,9 +160,10 @@ internal class RealRenderContextTest { sink() val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("yay", output?.value) + assertEquals("yay", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler1_gets_event() { @@ -173,9 +175,10 @@ internal class RealRenderContextTest { sink("foo") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foo", output?.value) + assertEquals("foo", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler2_gets_event() { @@ -187,9 +190,10 @@ internal class RealRenderContextTest { sink("foo", "bar") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobar", output?.value) + assertEquals("foobar", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler3_gets_event() { @@ -203,9 +207,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbang", output?.value) + assertEquals("foobarbazbang", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler4_gets_event() { @@ -219,9 +224,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbang", output?.value) + assertEquals("foobarbazbang", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler5_gets_event() { @@ -235,9 +241,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang", "buzz") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbangbuzz", output?.value) + assertEquals("foobarbazbangbuzz", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler6_gets_event() { @@ -252,9 +259,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang", "buzz", "qux") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbangbuzzqux", output?.value) + assertEquals("foobarbazbangbuzzqux", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler7_gets_event() { @@ -270,9 +278,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang", "buzz", "qux", "corge") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbangbuzzquxcorge", output?.value) + assertEquals("foobarbazbangbuzzquxcorge", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler8_gets_event() { @@ -288,9 +297,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang", "buzz", "qux", "corge", "fred") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbangbuzzquxcorgefred", output?.value) + assertEquals("foobarbazbangbuzzquxcorgefred", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler9_gets_event() { @@ -306,9 +316,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang", "buzz", "qux", "corge", "fred", "xyzzy") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbangbuzzquxcorgefredxyzzy", output?.value) + assertEquals("foobarbazbangbuzzquxcorgefredxyzzy", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun eventHandler10_gets_event() { @@ -324,9 +335,10 @@ internal class RealRenderContextTest { sink("foo", "bar", "baz", "bang", "buzz", "qux", "corge", "fred", "xyzzy", "plugh") val update = eventActionsChannel.tryReceive().getOrNull()!! - val (state, output) = update.applyTo("props", "state") + val (state, result) = update.applyTo("props", "state") assertEquals("state", state) - assertEquals("foobarbazbangbuzzquxcorgefredxyzzyplugh", output?.value) + assertEquals("foobarbazbangbuzzquxcorgefredxyzzyplugh", result.output!!.value) + assertFalse(result.stateChanged) } @Test fun renderChild_works() { @@ -341,10 +353,32 @@ internal class RealRenderContextTest { assertEquals("props", props) assertEquals("key", key) - val (state, output) = handler.invoke("output") + val (state, result) = handler.invoke("output") .applyTo("props", "state") assertEquals("state", state) - assertEquals("output:output", output?.value) + assertEquals("output:output", result.output!!.value) + assertFalse(result.stateChanged) + } + + @Test fun renderChild_handler_tracks_state_change() { + val context = createTestContext() + val workflow = TestWorkflow() + + val (child, props, key, handler) = context.renderChild(workflow, "props", "key") { + action { + state = "new" + } + } + + assertSame(workflow, child) + assertEquals("props", props) + assertEquals("key", key) + + val (state, result) = handler.invoke("output") + .applyTo("props", "state") + assertEquals("new", state) + assertNull(result.output) + assertTrue(result.stateChanged) } @Test fun all_methods_throw_after_freeze() { diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/SubtreeManagerTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/SubtreeManagerTest.kt index 04c63a3e7b..5c80212664 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/SubtreeManagerTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/SubtreeManagerTest.kt @@ -2,6 +2,7 @@ package com.squareup.workflow1.internal +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.ActionProcessingResult import com.squareup.workflow1.Snapshot import com.squareup.workflow1.StatefulWorkflow @@ -21,6 +22,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertTrue import kotlin.test.fail private typealias StringHandler = (String) -> WorkflowAction @@ -163,7 +165,7 @@ internal class SubtreeManagerTest { action { setOutput("case output:$output") } } - // Initialize the child so tickChildren has something to work with, and so that we can send + // Initialize the child so tickAction has something to work with, and so that we can send // an event to trigger an output. val (_, _, eventHandler) = manager.render(workflow, "props", key = "", handler = handler) manager.commitRenderedChildren() @@ -172,10 +174,34 @@ internal class SubtreeManagerTest { assertFalse(tickOutput.isCompleted) eventHandler("event!") - val update = tickOutput.await().value!! + val update = tickOutput.await().output!!.value!! - val (_, output) = update.applyTo("props", "state") - assertEquals("case output:workflow output:event!", output?.value) + val (_, result) = update.applyTo("props", "state") + assertEquals("case output:workflow output:event!", result.output!!.value) + assertFalse(result.stateChanged) + } + + @Test fun tick_children_handles_no_child_output() = runTest { + val manager = subtreeManagerForTest() + val workflow = TestWorkflow() + val handler: StringHandler = { _ -> + WorkflowAction.noAction() + } + + // Initialize the child so tickAction has something to work with, and so that we can send + // an event to trigger an output. + val (_, _, eventHandler) = manager.render(workflow, "props", key = "", handler = handler) + manager.commitRenderedChildren() + + val tickOutput = async { manager.tickAction() } + assertFalse(tickOutput.isCompleted) + + eventHandler("event!") + val update = tickOutput.await().output!!.value!! + + val (_, result) = update.applyTo("props", "state") + assertEquals(null, result.output) + assertFalse(result.stateChanged) } @Test fun render_updates_childs_output_handler() = runTest { @@ -189,18 +215,32 @@ internal class SubtreeManagerTest { render { action { setOutput("initial handler: $it") } } .let { rendering -> rendering.eventHandler("initial output") - val initialAction = manager.tickAction().value - val (_, initialOutput) = initialAction!!.applyTo("", "") - assertEquals("initial handler: workflow output:initial output", initialOutput?.value) + val initialAction = manager.tickAction().output!!.value + val (_, initialResult) = initialAction!!.applyTo("", "") + assertEquals( + expected = "initial handler: workflow output:initial output", + actual = initialResult.output!!.value + ) + assertFalse(initialResult.stateChanged) } // Do a second render + tick, but with a different handler function. - render { action { setOutput("second handler: $it") } } + render { + action { + state = "New State" + setOutput("second handler: $it") + } + } .let { rendering -> rendering.eventHandler("second output") - val secondAction = manager.tickAction().value - val (_, secondOutput) = secondAction!!.applyTo("", "") - assertEquals("second handler: workflow output:second output", secondOutput?.value) + val secondAction = manager.tickAction().output!!.value + val (secondState, secondResult) = secondAction!!.applyTo("", "") + assertEquals( + expected = "second handler: workflow output:second output", + actual = secondResult.output!!.value + ) + assertTrue(secondResult.stateChanged) + assertEquals("New State", secondState) } } @@ -263,9 +303,11 @@ internal class SubtreeManagerTest { private suspend fun SubtreeManager.tickAction() = select { onNextChildAction(this) - } as WorkflowOutput?> + } as ActionApplied?> private fun subtreeManagerForTest( snapshotCache: Map? = null - ) = SubtreeManager(snapshotCache, context, emitActionToParent = { WorkflowOutput(it) }) + ) = SubtreeManager(snapshotCache, context, emitActionToParent = { action, childResult -> + ActionApplied(WorkflowOutput(action), childResult.stateChanged) + }) } diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowNodeTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowNodeTest.kt index 48000956d0..7ef7f83004 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowNodeTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowNodeTest.kt @@ -1,9 +1,9 @@ -@file:Suppress("EXPERIMENTAL_API_USAGE", "DEPRECATION") +@file:Suppress("EXPERIMENTAL_API_USAGE") @file:OptIn(ExperimentalCoroutinesApi::class) package com.squareup.workflow1.internal -import com.squareup.workflow1.ActionProcessingResult +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.BaseRenderContext import com.squareup.workflow1.Sink import com.squareup.workflow1.Snapshot @@ -175,17 +175,17 @@ internal class WorkflowNodeTest { "", null, context, - emitOutputToParent = { WorkflowOutput("tick:$it") } + emitAppliedActionToParent = { it.copy(output = WorkflowOutput("tick:${it.output!!.value}")) } ) node.render(workflow, "")("event") runTest { val result = withTimeout(10) { - select { + select { node.onNextAction(this) - } as WorkflowOutput? + } as ActionApplied } - assertEquals("tick:event", result?.value) + assertEquals("tick:event", result.output!!.value) } } @@ -213,7 +213,7 @@ internal class WorkflowNodeTest { "", null, context, - emitOutputToParent = { WorkflowOutput("tick:$it") } + emitAppliedActionToParent = { it.copy(output = WorkflowOutput("tick:${it.output!!.value}")) } ) val sink = node.render(workflow, "") @@ -223,12 +223,12 @@ internal class WorkflowNodeTest { runTest { val result = withTimeout(10) { List(2) { - select { + select { node.onNextAction(this) - } as WorkflowOutput? + } as ActionApplied } } - assertEquals(listOf("tick:event", "tick:event2"), result.map { it?.value }) + assertEquals(listOf("tick:event", "tick:event2"), result.map { it.output!!.value }) } } @@ -324,11 +324,11 @@ internal class WorkflowNodeTest { runTest { // Result should be available instantly, any delay at all indicates something is broken. val result = withTimeout(1) { - select { + select { node.onNextAction(this) - } as WorkflowOutput? + } as ActionApplied } - assertEquals("result", result?.value) + assertEquals("result", result.output!!.value) } } @@ -1097,7 +1097,7 @@ internal class WorkflowNodeTest { override fun toString(): String = "TestAction()" } - val workflow = Workflow.stateless { + val workflow = Workflow.stateless { actionSink.send(TestAction()) } val node = WorkflowNode( @@ -1138,9 +1138,11 @@ internal class WorkflowNodeTest { sink.send("hello") - select { + val result = select { node.onNextAction(this) - } as WorkflowOutput? + } as ActionApplied + assertNull(result.output) + assertTrue(result.stateChanged) val (state, _) = node.render(workflow.asStatefulWorkflow(), Unit) assertEquals("initial->hello", state) @@ -1156,17 +1158,20 @@ internal class WorkflowNodeTest { initialProps = Unit, snapshot = null, baseContext = Unconfined, - emitOutputToParent = { WorkflowOutput("output:$it") } + emitAppliedActionToParent = { + it.copy(output = WorkflowOutput("output:${it.output!!.value}")) + } ) val rendering = node.render(workflow.asStatefulWorkflow(), Unit) rendering.send("hello") runTest { - val output = select { + val result = select { node.onNextAction(this) - } as WorkflowOutput? - assertEquals("output:hello", output?.value) + } as ActionApplied + assertEquals("output:hello", result.output!!.value) + assertFalse(result.stateChanged) } } @@ -1180,17 +1185,18 @@ internal class WorkflowNodeTest { initialProps = Unit, snapshot = null, baseContext = Unconfined, - emitOutputToParent = { WorkflowOutput(it) } + emitAppliedActionToParent = { it } ) val rendering = node.render(workflow.asStatefulWorkflow(), Unit) rendering.send("hello") runTest { - val output = select { + val result = select { node.onNextAction(this) - } as WorkflowOutput? - assertNull(output?.value) + } as ActionApplied + assertNull(result.output!!.value) + assertFalse(result.stateChanged) } } @@ -1213,9 +1219,9 @@ internal class WorkflowNodeTest { ) node.render(workflow.asStatefulWorkflow(), Unit) - select { + select { node.onNextAction(this) - } as WorkflowOutput? + } as ActionApplied val state = node.render(workflow.asStatefulWorkflow(), Unit) assertEquals("initial->hello", state) @@ -1233,15 +1239,17 @@ internal class WorkflowNodeTest { initialProps = Unit, snapshot = null, baseContext = Unconfined, - emitOutputToParent = { WorkflowOutput("output:$it") } + emitAppliedActionToParent = { + it.copy(output = WorkflowOutput("output:${it.output!!.value}")) + } ) node.render(workflow.asStatefulWorkflow(), Unit) runTest { - val output = select { + val result = select { node.onNextAction(this) - } as WorkflowOutput? - assertEquals("output:child:hello", output?.value) + } as ActionApplied + assertEquals("output:child:hello", result.output!!.value) } } @@ -1257,15 +1265,15 @@ internal class WorkflowNodeTest { initialProps = Unit, snapshot = null, baseContext = Unconfined, - emitOutputToParent = { WorkflowOutput(it) } + emitAppliedActionToParent = { it } ) node.render(workflow.asStatefulWorkflow(), Unit) runTest { - val output = select { + val result = select { node.onNextAction(this) - } as WorkflowOutput? - assertNull(output?.value) + } as ActionApplied + assertNull(result.output!!.value) } } diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt index 81662adc84..951df4b977 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt @@ -1,14 +1,15 @@ package com.squareup.workflow1.internal +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.NoopWorkflowInterceptor import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfig.Companion import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings +import com.squareup.workflow1.RuntimeConfig.RenderOnStateChangeOnly import com.squareup.workflow1.RuntimeConfig.RenderPerAction import com.squareup.workflow1.Worker import com.squareup.workflow1.Workflow import com.squareup.workflow1.WorkflowExperimentalRuntime -import com.squareup.workflow1.WorkflowOutput import com.squareup.workflow1.action import com.squareup.workflow1.runningWorker import com.squareup.workflow1.stateful @@ -33,6 +34,7 @@ internal class WorkflowRunnerTest { private val runtimeOptions = arrayOf( RenderPerAction, ConflateStaleRenderings, + RenderOnStateChangeOnly ).asSequence() private fun setup() { @@ -134,7 +136,7 @@ internal class WorkflowRunnerTest { assertTrue(output.isCompleted) @Suppress("UNCHECKED_CAST") - val outputValue = output.getCompleted() as? WorkflowOutput? + val outputValue = output.getCompleted() as? ActionApplied? assertNull(outputValue) val rendering = runner.nextRendering().rendering assertEquals("changed", rendering) @@ -166,8 +168,8 @@ internal class WorkflowRunnerTest { val initialRendering = runner.nextRendering().rendering assertEquals("initial", initialRendering) - val output = runner.runTillNextOutput() - assertEquals("output: work", output?.value) + val actionResult = runner.runTillNextActionResult() + assertEquals("output: work", actionResult!!.output!!.value) val updatedRendering = runner.nextRendering().rendering assertEquals("state: work", updatedRendering) @@ -201,14 +203,14 @@ internal class WorkflowRunnerTest { // The order in which props update and workflow update are processed is deterministic, based // on the order they appear in the select block in processActions. - val firstOutput = runner.runTillNextOutput() + val firstActionResult = runner.runTillNextActionResult() // First update will be props, so no output value. - assertNull(firstOutput) + assertNull(firstActionResult) val secondRendering = runner.nextRendering().rendering assertEquals("changed props|initial state(initial props)", secondRendering) - val secondOutput = runner.runTillNextOutput() - assertEquals("output: work", secondOutput?.value) + val secondActionResult = runner.runTillNextActionResult() + assertEquals("output: work", secondActionResult!!.output!!.value) val thirdRendering = runner.nextRendering().rendering assertEquals("changed props|state: work", thirdRendering) } @@ -278,15 +280,15 @@ internal class WorkflowRunnerTest { val runner = WorkflowRunner(workflow, MutableStateFlow(Unit), runtimeConfig) runner.nextRendering() - val output = scope.async { runner.processAction() } + val actionResult = scope.async { runner.processAction() } scope.runCurrent() - assertTrue(output.isActive) + assertTrue(actionResult.isActive) scope.cancel("foo") scope.advanceUntilIdle() - assertTrue(output.isCancelled) - val realCause = output.getCompletionExceptionOrNull() + assertTrue(actionResult.isCancelled) + val realCause = actionResult.getCompletionExceptionOrNull() assertEquals("foo", realCause?.message) } } @@ -309,25 +311,26 @@ internal class WorkflowRunnerTest { val runner = WorkflowRunner(workflow, MutableStateFlow(Unit), runtimeConfig) runner.nextRendering() - val output = scope.async { runner.processAction() } + val actionResult = scope.async { runner.processAction() } scope.runCurrent() - assertTrue(output.isActive) + assertTrue(actionResult.isActive) assertNull(cancellationException) scope.cancel("foo") scope.advanceUntilIdle() - assertTrue(output.isCancelled) + assertTrue(actionResult.isCancelled) assertNotNull(cancellationException) assertEquals("foo", cancellationException!!.message) } } @Suppress("UNCHECKED_CAST") - private fun WorkflowRunner<*, T, *>.runTillNextOutput(): WorkflowOutput? = scope.run { + private fun WorkflowRunner<*, T, *>.runTillNextActionResult(): ActionApplied? = scope.run { val firstOutputDeferred = async { processAction() } runCurrent() - firstOutputDeferred.getCompleted() as? WorkflowOutput? + // If it is [ PropsUpdated] or any other ActionProcessingResult, will return as null. + firstOutputDeferred.getCompleted() as? ActionApplied } @Suppress("TestFunctionName") 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 fafcd7afa2..1c0afb5fd5 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 @@ -103,7 +103,8 @@ internal class RealRenderTester( private var explicitSideEffectExpectationsRequired: Boolean = false private val stateAndOutput: Pair?> by lazy { val action = processedAction ?: noAction() - action.applyTo(props, state) + val (state, actionApplied) = action.applyTo(props, state) + state to actionApplied.output } override val actionSink: Sink> get() = this @@ -123,7 +124,6 @@ internal class RealRenderTester( expectations += ExpectedSideEffect(matcher, exactMatch, description) } - @OptIn(ExperimentalStdlibApi::class) override fun render( block: (RenderingT) -> Unit ): RenderTestResult { diff --git a/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTestResult.kt b/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTestResult.kt index cfa7bf1a12..ea15ca2503 100644 --- a/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTestResult.kt +++ b/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTestResult.kt @@ -39,7 +39,7 @@ public interface RenderTestResult { * instead and write separate unit tests for your action implementations. */ public fun verifyActionResult( - block: (newState: StateT, output: WorkflowOutput?) -> Unit + block: (newState: StateT, appliedResult: WorkflowOutput?) -> Unit ): RenderTestResult /** diff --git a/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTester.kt b/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTester.kt index ce3c310c33..a4a73e2e71 100644 --- a/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTester.kt +++ b/workflow-testing/src/main/java/com/squareup/workflow1/testing/RenderTester.kt @@ -1,5 +1,6 @@ package com.squareup.workflow1.testing +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.StatefulWorkflow import com.squareup.workflow1.Workflow import com.squareup.workflow1.WorkflowAction @@ -44,9 +45,10 @@ StatefulWorkflow.testRender( /** * The props must be specified, the initial state may be specified, and then all child workflows * and workers that are expected to run, and any outputs from them, must be specified with - * [expectWorkflow] and (optionally) [expectWorker] calls. If one needs to verify all workers - * explicitly, perhaps to verify that a worker is *not* run, then use - * [requireExplicitWorkerExpectations]. + * [expectWorkflow] and (optionally) [expectWorker] and [expectSideEffect] calls. + * If one needs to verify all workers explicitly, perhaps to verify that a worker is *not* run, + * then use [requireExplicitWorkerExpectations]. Likewise [requireExplicitSideEffectExpectations] + * for side effects. * Then call [render] and perform any assertions on the rendering. An event may also be sent to the * rendering if no workflows or workers emitted an output. Lastly, the [RenderTestResult] returned * by `render` may be used to assert on the [WorkflowAction]s processed to handle events or outputs @@ -55,11 +57,14 @@ StatefulWorkflow.testRender( * * - All workflows that are rendered/ran by this workflow must be specified. * - Workers are optionally specified. Specified workers must run. Unexpected workers on a render - * pass do not cause a test failure. + * pass do not cause a test failure unless [requireExplicitWorkerExpectations] is true. + * Side effects are optionally specified. Specified side effects must run. Unexpected side effects + * on a render pass do not cause a test failure unless [requireExplicitSideEffectExpectations] is + * true. * - It is an error if more than one workflow or worker specifies an output. * - It is a test failure if any workflows or workers that were expected were not ran. * - It is a test failure if the workflow tried to run any workflows that were not expected. - * - It is a test failure if no workflow or workflow emitted an output, no rendering event was + * - It is a test failure if no workflow or worker emitted an output, no rendering event was * invoked, and any of the action verification methods on [RenderTestResult] is called. * * ## Examples @@ -285,7 +290,7 @@ public abstract class RenderTester { * Indicates that the workflow matches the predicate. * * @param childRendering The value to return as the child's rendering. - * @param output If non-null, [WorkflowOutput.value] will be "emitted" when this workflow is + * @param output If non-null, [ActionApplied.output] will be "emitted" when this workflow is * rendered. The [WorkflowAction] used to handle this output can be verified using methods on * [RenderTestResult]. */ 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 a8751b0c5d..445af5d227 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 @@ -1016,7 +1016,7 @@ internal class RealRenderTesterTest { testResult.verifyAction { assertEquals(noAction(), it) } testResult.verifyActionResult { newState, output -> assertSame(Unit, newState) - assertNull(output) + assertNull(output?.value) } } @@ -1031,7 +1031,7 @@ internal class RealRenderTesterTest { testResult.verifyActionResult { newState, output -> assertSame(Unit, newState) - assertNull(output) + assertNull(output?.value) } } diff --git a/workflow-testing/src/test/java/com/squareup/workflow1/testing/WorkerRenderExpectationsTest.kt b/workflow-testing/src/test/java/com/squareup/workflow1/testing/WorkerRenderExpectationsTest.kt index 13cfcdd624..6ca371f05b 100644 --- a/workflow-testing/src/test/java/com/squareup/workflow1/testing/WorkerRenderExpectationsTest.kt +++ b/workflow-testing/src/test/java/com/squareup/workflow1/testing/WorkerRenderExpectationsTest.kt @@ -16,8 +16,6 @@ import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.fail -// TODO(https://github.com/square/workflow-kotlin/issues/117) Add more failure tests -@OptIn(ExperimentalStdlibApi::class) class WorkerRenderExpectationsTest { @Test fun `expectWorkerOutputting() works`() { 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 877ab48433..0fb3bb3d72 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 @@ -13,6 +13,7 @@ import com.squareup.tracing.TraceEvent.ObjectCreated import com.squareup.tracing.TraceEvent.ObjectDestroyed import com.squareup.tracing.TraceEvent.ObjectSnapshot import com.squareup.tracing.TraceLogger +import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.BaseRenderContext import com.squareup.workflow1.Snapshot import com.squareup.workflow1.WorkflowAction @@ -22,7 +23,6 @@ import com.squareup.workflow1.WorkflowIdentifierType.Unsnapshottable import com.squareup.workflow1.WorkflowInterceptor import com.squareup.workflow1.WorkflowInterceptor.RenderContextInterceptor import com.squareup.workflow1.WorkflowInterceptor.WorkflowSession -import com.squareup.workflow1.WorkflowOutput import com.squareup.workflow1.applyTo import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job @@ -419,7 +419,7 @@ public class TracingWorkflowInterceptor internal constructor( action: WorkflowAction<*, *, *>, oldState: Any?, newState: Any?, - output: WorkflowOutput? + output: ActionApplied? ) { val name = workflowNamesById.getValue(workflowId) @@ -433,7 +433,7 @@ public class TracingWorkflowInterceptor internal constructor( "action" to action.toString(), "oldState" to oldState.toString(), "newState" to if (oldState == newState) "{no change}" else newState.toString(), - "output" to (output?.let { it.value.toString() } ?: "{no output}") + "output" to (output?.let { it.output?.value.toString() } ?: "{no output}") ) ), ObjectSnapshot( @@ -480,7 +480,7 @@ public class TracingWorkflowInterceptor internal constructor( val oldState = state val (newState, output) = delegate.applyTo(props, state) state = newState - output?.let { setOutput(it.value) } + output.output?.let { setOutput(it.value) } onWorkflowAction( workflowId = session.sessionId, action = delegate,