From 3bd75d2e8bad78c2487246e89591fc36e1c00270 Mon Sep 17 00:00:00 2001 From: Laimonas Turauskas Date: Fri, 13 Sep 2024 09:48:13 -0400 Subject: [PATCH] Increase code coverage (pt2). (#389) --- .../java/com/instacart/formula/Renderer.kt | 14 +- .../formula/test/RxJavaFormulaTestDelegate.kt | 12 +- .../formula/test/TestRuntimeExtensions.kt | 9 +- .../formula/internal/ChildrenManager.kt | 43 +++-- .../formula/internal/ListenerImpl.kt | 25 +-- .../instacart/formula/internal/Listeners.kt | 28 ++-- .../formula/internal/SingleRequestHolder.kt | 9 -- .../formula/internal/SnapshotImpl.kt | 2 +- .../instacart/formula/FormulaRuntimeTest.kt | 152 ++++++++++++++++++ .../com/instacart/formula/SnapshotTest.kt | 27 ++++ .../DuplicateListenerKeysHandledByIndexing.kt | 2 +- .../formula/test/CoroutineTestableRuntime.kt | 16 +- .../formula/test/RxJavaTestableRuntime.kt | 9 +- .../instacart/formula/test/TestableRuntime.kt | 1 + 14 files changed, 250 insertions(+), 99 deletions(-) create mode 100644 formula/src/test/java/com/instacart/formula/SnapshotTest.kt diff --git a/formula-android/src/main/java/com/instacart/formula/Renderer.kt b/formula-android/src/main/java/com/instacart/formula/Renderer.kt index 43333e90d..44c37498f 100644 --- a/formula-android/src/main/java/com/instacart/formula/Renderer.kt +++ b/formula-android/src/main/java/com/instacart/formula/Renderer.kt @@ -12,24 +12,12 @@ package com.instacart.formula * renderText("three") * ``` */ -class Renderer private constructor( +class Renderer( private val renderFunction: (RenderModel) -> Unit ) : (RenderModel) -> Unit { companion object { - /** - * Creates a render function that does nothing. - */ - fun empty() = create { } - - /** - * Creates a render function. - */ - operator fun invoke(render: (RenderModel) -> Unit): Renderer { - return Renderer(renderFunction = render) - } - /** * Creates a render function. */ diff --git a/formula-test/src/main/java/com/instacart/formula/test/RxJavaFormulaTestDelegate.kt b/formula-test/src/main/java/com/instacart/formula/test/RxJavaFormulaTestDelegate.kt index b4fdc4112..714f2d997 100644 --- a/formula-test/src/main/java/com/instacart/formula/test/RxJavaFormulaTestDelegate.kt +++ b/formula-test/src/main/java/com/instacart/formula/test/RxJavaFormulaTestDelegate.kt @@ -2,8 +2,6 @@ package com.instacart.formula.test import com.instacart.formula.IFormula import com.instacart.formula.RuntimeConfig -import com.instacart.formula.plugin.Dispatcher -import com.instacart.formula.plugin.Inspector import com.instacart.formula.rxjava3.toObservable import io.reactivex.rxjava3.subjects.BehaviorSubject @@ -12,16 +10,8 @@ import io.reactivex.rxjava3.subjects.BehaviorSubject */ class RxJavaFormulaTestDelegate>( override val formula: FormulaT, - isValidationEnabled: Boolean = true, - inspector: Inspector?, - dispatcher: Dispatcher?, + runtimeConfig: RuntimeConfig, ) : FormulaTestDelegate { - private val runtimeConfig = RuntimeConfig( - isValidationEnabled = isValidationEnabled, - inspector = inspector, - defaultDispatcher = dispatcher, - ) - private val inputRelay = BehaviorSubject.create() private val observer = formula .toObservable(inputRelay, runtimeConfig) diff --git a/formula-test/src/main/java/com/instacart/formula/test/TestRuntimeExtensions.kt b/formula-test/src/main/java/com/instacart/formula/test/TestRuntimeExtensions.kt index 44222f5c2..59c4f2ca1 100644 --- a/formula-test/src/main/java/com/instacart/formula/test/TestRuntimeExtensions.kt +++ b/formula-test/src/main/java/com/instacart/formula/test/TestRuntimeExtensions.kt @@ -2,6 +2,7 @@ package com.instacart.formula.test import com.instacart.formula.Action import com.instacart.formula.IFormula +import com.instacart.formula.RuntimeConfig import com.instacart.formula.plugin.Dispatcher import com.instacart.formula.plugin.Inspector @@ -15,7 +16,13 @@ fun > F.test( inspector: Inspector? = null, dispatcher: Dispatcher? = null, ): TestFormulaObserver { - val delegate = RxJavaFormulaTestDelegate(this, isValidationEnabled, inspector, dispatcher) + val runtimeConfig = RuntimeConfig( + isValidationEnabled = isValidationEnabled, + inspector = inspector, + defaultDispatcher = dispatcher, + ) + + val delegate = RxJavaFormulaTestDelegate(this, runtimeConfig) return TestFormulaObserver(delegate) } diff --git a/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt b/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt index 200796517..8d5e954b5 100644 --- a/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt +++ b/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt @@ -3,7 +3,6 @@ package com.instacart.formula.internal import com.instacart.formula.FormulaPlugins import com.instacart.formula.IFormula import com.instacart.formula.plugin.Inspector -import java.lang.IllegalStateException /** * Keeps track of child formula managers. @@ -12,7 +11,7 @@ internal class ChildrenManager( private val delegate: FormulaManagerImpl<*, *, *>, private val inspector: Inspector?, ) { - private var children: SingleRequestMap>? = null + private val children: SingleRequestMap> = LinkedHashMap() private var indexes: MutableMap? = null private var pendingRemoval: MutableList>? = null @@ -26,7 +25,7 @@ internal class ChildrenManager( fun prepareForPostEvaluation() { indexes?.clear() - children?.clearUnrequested(this::prepareForTermination) + children.clearUnrequested(this::prepareForTermination) } fun terminateChildren(evaluationId: Long): Boolean { @@ -42,11 +41,11 @@ internal class ChildrenManager( } fun markAsTerminated() { - children?.forEachValue { it.markAsTerminated() } + children.forEachValue { it.markAsTerminated() } } fun performTerminationSideEffects(executeTransitionQueue: Boolean) { - children?.forEachValue { it.performTerminationSideEffects(executeTransitionQueue) } + children.forEachValue { it.performTerminationSideEffects(executeTransitionQueue) } } fun findOrInitChild( @@ -54,8 +53,8 @@ internal class ChildrenManager( formula: IFormula, input: ChildInput, ): FormulaManager { - val childHolder = childFormulaHolder(key, formula, input) - return if (childHolder.requested) { + val childManagerEntry = getOrInitChildManager(key, formula, input) + return if (childManagerEntry.requested) { val logs = duplicateKeyLogs ?: run { val newSet = mutableSetOf() duplicateKeyLogs = newSet @@ -76,38 +75,27 @@ internal class ChildrenManager( ) } - if (key is IndexedKey) { - // This should never happen, but added as safety - throw IllegalStateException("Key already indexed (and still duplicate).") - } - val index = nextIndex(key) val indexedKey = IndexedKey(key, index) findOrInitChild(indexedKey, formula, input) } else { - childHolder.requestAccess { - "There already is a child with same key: $key. Override [Formula.key] function." - } + childManagerEntry.requested = true + childManagerEntry.value } } private fun prepareForTermination(it: FormulaManager<*, *>) { - pendingRemoval = pendingRemoval ?: mutableListOf() + val list = pendingRemoval ?: mutableListOf() + pendingRemoval = list it.markAsTerminated() - pendingRemoval?.add(it) + list.add(it) } - private fun childFormulaHolder( + private fun getOrInitChildManager( key: Any, formula: IFormula, input: ChildInput, ): SingleRequestHolder> { - val children = children ?: run { - val initialized: SingleRequestMap> = LinkedHashMap() - this.children = initialized - initialized - } - val childFormulaHolder = children.findOrInit(key) { val implementation = formula.implementation FormulaManagerImpl( @@ -136,7 +124,12 @@ internal class ChildrenManager( initialized } - val index = indexes.getOrElse(key) { 0 } + 1 + val previousIndex = indexes[key] + val index = if (previousIndex == null) { + 0 + } else { + previousIndex + 1 + } indexes[key] = index return index } diff --git a/formula/src/main/java/com/instacart/formula/internal/ListenerImpl.kt b/formula/src/main/java/com/instacart/formula/internal/ListenerImpl.kt index 01f433ff6..8bd545d40 100644 --- a/formula/src/main/java/com/instacart/formula/internal/ListenerImpl.kt +++ b/formula/src/main/java/com/instacart/formula/internal/ListenerImpl.kt @@ -8,7 +8,7 @@ import com.instacart.formula.plugin.Dispatcher * Note: this class is not a data class because equality is based on instance and not [key]. */ @PublishedApi -internal class ListenerImpl(val key: Any) : Listener { +internal class ListenerImpl(private val key: Any) : Listener { @Volatile private var manager: FormulaManagerImpl? = null @Volatile private var snapshotImpl: SnapshotImpl? = null @@ -21,11 +21,11 @@ internal class ListenerImpl(val key: Any) : Listener handleBatched(type, event) - Transition.Immediate -> executeWithDispatcher(Dispatcher.None, event) - Transition.Background -> executeWithDispatcher(Dispatcher.Background, event) + is Transition.Batched -> handleBatched(manager, type, event) + Transition.Immediate -> executeWithDispatcher(manager, Dispatcher.None, event) + Transition.Background -> executeWithDispatcher(manager, Dispatcher.Background, event) // If transition does not specify dispatcher, we use the default one. - else -> executeWithDispatcher(manager.defaultDispatcher, event) + else -> executeWithDispatcher(manager, manager.defaultDispatcher, event) } } @@ -50,17 +50,22 @@ internal class ListenerImpl(val key: Any) : Listener, + type: Transition.Batched, + event: EventT, + ) { manager.batchManager.add(type, event) { val deferredTransition = DeferredTransition(this, event) manager.onPendingTransition(deferredTransition) } } - private fun executeWithDispatcher(dispatcher: Dispatcher, event: EventT) { - val manager = manager ?: return + private fun executeWithDispatcher( + manager: FormulaManagerImpl, + dispatcher: Dispatcher, + event: EventT, + ) { manager.queue.postUpdate(dispatcher) { val deferredTransition = DeferredTransition(this, event) manager.onPendingTransition(deferredTransition) diff --git a/formula/src/main/java/com/instacart/formula/internal/Listeners.kt b/formula/src/main/java/com/instacart/formula/internal/Listeners.kt index 21b3d4080..679bbf9b7 100644 --- a/formula/src/main/java/com/instacart/formula/internal/Listeners.kt +++ b/formula/src/main/java/com/instacart/formula/internal/Listeners.kt @@ -1,31 +1,20 @@ package com.instacart.formula.internal -import java.lang.IllegalStateException - internal class Listeners { private var listeners: SingleRequestMap>? = null private var indexes: MutableMap? = null - private fun duplicateKeyErrorMessage(key: Any): String { - return "Listener $key is already defined. Unexpected issue." - } - fun initOrFindListener(key: Any, useIndex: Boolean): ListenerImpl { val currentHolder = listenerHolder(key) - return if (currentHolder.requested && useIndex) { - if (key is IndexedKey) { - // This should never happen, but added as safety - throw IllegalStateException("Key already indexed (and still duplicate).") - } - + return if (!currentHolder.requested) { + currentHolder.requested = true + currentHolder.value as ListenerImpl + } else if (useIndex) { val index = nextIndex(key) val indexedKey = IndexedKey(key, index) initOrFindListener(indexedKey, useIndex) } else { - currentHolder - .requestAccess { - duplicateKeyErrorMessage(currentHolder.value.key) - } as ListenerImpl + throw IllegalStateException("Listener $key is already defined. Unexpected issue.") } } @@ -61,7 +50,12 @@ internal class Listeners { initialized } - val index = indexes.getOrElse(key) { 0 } + 1 + val previousIndex = indexes[key] + val index = if (previousIndex == null) { + 0 + } else { + previousIndex + 1 + } indexes[key] = index return index } diff --git a/formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt b/formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt index 13db8166b..dd0472bc0 100644 --- a/formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt +++ b/formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt @@ -7,15 +7,6 @@ package com.instacart.formula.internal internal class SingleRequestHolder(val value: T) { var requested: Boolean = false - inline fun requestAccess(errorMessage: () -> String): T { - if (requested) { - throw IllegalStateException(errorMessage()) - } - - requested = true - return value - } - fun reset() { requested = false } diff --git a/formula/src/main/java/com/instacart/formula/internal/SnapshotImpl.kt b/formula/src/main/java/com/instacart/formula/internal/SnapshotImpl.kt index f57f49448..4db49662c 100644 --- a/formula/src/main/java/com/instacart/formula/internal/SnapshotImpl.kt +++ b/formula/src/main/java/com/instacart/formula/internal/SnapshotImpl.kt @@ -11,7 +11,7 @@ import com.instacart.formula.TransitionContext import java.lang.IllegalStateException import kotlin.reflect.KClass -internal class SnapshotImpl internal constructor( +internal class SnapshotImpl( override val input: Input, override val state: State, listeners: Listeners, diff --git a/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt b/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt index 867980ee3..f8a8d07c3 100644 --- a/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt +++ b/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt @@ -81,6 +81,7 @@ import com.instacart.formula.test.TestCallback import com.instacart.formula.test.TestEventCallback import com.instacart.formula.test.TestFormulaObserver import com.instacart.formula.test.TestableRuntime +import com.instacart.formula.test.test import com.instacart.formula.types.ActionDelegateFormula import com.instacart.formula.types.IncrementActionFormula import com.instacart.formula.types.IncrementFormula @@ -745,6 +746,32 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) { subject.output { assertThat(listeners).containsNoDuplicates() } } + @Test + fun `duplicate child formulas are handled by indexing`() { + val childFormula = object : StatelessFormula() { + override fun key(input: Int): Any = input + + override fun Snapshot.evaluate(): Evaluation { + return Evaluation( + output = 1 + ) + } + } + + val parentFormula = object : StatelessFormula() { + override fun Snapshot.evaluate(): Evaluation { + val childCount = listOf(1, 1, 2, 1, 1).sumOf { key -> + context.child(childFormula, key) + } + return Evaluation(childCount) + } + } + + runtime.test(parentFormula).input(Unit).output { + assertThat(this).isEqualTo(5) + } + } + @Test fun `using key to scope listeners within another function`() { val formula = UsingKeyToScopeCallbacksWithinAnotherFunction.TestFormula() @@ -1313,6 +1340,87 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) { } } + @Test + fun `child formula termination triggers parent state transition`() { + val relay = runtime.newRelay() + val childFormula = object : StatelessFormula() { + override fun Snapshot.evaluate(): Evaluation { + return Evaluation( + output = Unit, + actions = context.actions { + Action.onTerminate().onEvent { + relay.triggerEvent() + none() + } + } + ) + } + } + + val parentFormula = object : Formula() { + override fun initialState(input: Boolean): Int = 0 + + override fun Snapshot.evaluate(): Evaluation { + if (input) { + context.child(childFormula) + } + return Evaluation( + output = state, + actions = context.actions { + relay.action().onEvent { + transition(state + 1) + } + } + ) + } + } + + val observer = runtime.test(parentFormula) + observer.input(true) + observer.output { assertThat(this).isEqualTo(0) } + observer.input(false) + observer.output { assertThat(this).isEqualTo(1) } + observer.input(true) + observer.input(false) + observer.output { assertThat(this).isEqualTo(2) } + } + + @Test + fun `child formula termination triggers parent termination`() { + var terminate = {} + + val childFormula = object : StatelessFormula() { + override fun Snapshot.evaluate(): Evaluation { + return Evaluation( + output = Unit, + actions = context.actions { + Action.onTerminate().onEvent { + terminate() + none() + } + } + ) + } + } + + val parentFormula = object : StatelessFormula() { + override fun Snapshot.evaluate(): Evaluation { + return if (input) { + context.child(childFormula) + Evaluation(1) + } else { + Evaluation(0) + } + } + } + + val observer = runtime.test(parentFormula) + terminate = { observer.dispose() } + observer.input(true) + observer.assertOutputCount(1) + observer.input(false) + observer.assertOutputCount(1) // Terminated, should not have any more outputs + } // End of child specific test cases @@ -1469,6 +1577,50 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) { observer.output { assertThat(this).isEqualTo(0) } } + @Test fun `action events after full-termination are ignored`() { + var sendCallback: (Unit) -> Unit = {} + + val formula = object : Formula() { + override fun initialState(input: Boolean): Int = 0 + + override fun Snapshot.evaluate(): Evaluation { + return Evaluation( + output = state, + actions = context.actions { + if (input) { + val action = object : Action { + override fun key(): Any? = null + override fun start(send: (Unit) -> Unit): Cancelable? { + sendCallback = send + return null + } + } + action.onEvent { + transition(state + 1) + } + } + } + ) + } + } + + val observer = runtime.test(formula) + observer.input(true) + + // Check that action runs + sendCallback(Unit) + observer.output { assertThat(this).isEqualTo(1) } + + // Cancel action + observer.input(false) + + // Check that send actions are ignored + sendCallback(Unit) + sendCallback(Unit) + sendCallback(Unit) + observer.output { assertThat(this).isEqualTo(1) } + } + @Test fun `using from observable with input`() { val onItem = TestEventCallback() diff --git a/formula/src/test/java/com/instacart/formula/SnapshotTest.kt b/formula/src/test/java/com/instacart/formula/SnapshotTest.kt new file mode 100644 index 000000000..63f9eca8e --- /dev/null +++ b/formula/src/test/java/com/instacart/formula/SnapshotTest.kt @@ -0,0 +1,27 @@ +package com.instacart.formula + +import com.google.common.truth.Truth.assertThat +import com.instacart.formula.test.test +import org.junit.Test + +class SnapshotTest { + + @Test + fun `using snapshot outside of evaluation will throw an exception`() { + // Note: you should never expose snapshot like this! This is only to simplify testing + val formula = object : StatelessFormula>() { + override fun Snapshot.evaluate(): Evaluation> { + return Evaluation(this) + } + } + + val observer = formula.test(isValidationEnabled = false) + observer.input(Unit) + val result = runCatching { + observer.output { this.context.callback { none() } } + } + assertThat(result.exceptionOrNull()).hasMessageThat().contains( + "Cannot call this transition after evaluation finished. See https://instacart.github.io/formula/faq/#after-evaluation-finished" + ) + } +} \ No newline at end of file diff --git a/formula/src/test/java/com/instacart/formula/subjects/DuplicateListenerKeysHandledByIndexing.kt b/formula/src/test/java/com/instacart/formula/subjects/DuplicateListenerKeysHandledByIndexing.kt index e3ab7b676..e47c6fdf7 100644 --- a/formula/src/test/java/com/instacart/formula/subjects/DuplicateListenerKeysHandledByIndexing.kt +++ b/formula/src/test/java/com/instacart/formula/subjects/DuplicateListenerKeysHandledByIndexing.kt @@ -16,7 +16,7 @@ object DuplicateListenerKeysHandledByIndexing { class TestFormula : StatelessFormula() { override fun Snapshot.evaluate(): Evaluation { - val keys = listOf(1, 2, 1) + val keys = listOf(1, 2, 1, 1, 1) return Evaluation( output = TestOutput( listeners = keys.map { key -> diff --git a/formula/src/test/java/com/instacart/formula/test/CoroutineTestableRuntime.kt b/formula/src/test/java/com/instacart/formula/test/CoroutineTestableRuntime.kt index 9ba74fb03..50c10f3bd 100644 --- a/formula/src/test/java/com/instacart/formula/test/CoroutineTestableRuntime.kt +++ b/formula/src/test/java/com/instacart/formula/test/CoroutineTestableRuntime.kt @@ -44,9 +44,15 @@ object CoroutinesTestableRuntime : TestableRuntime { formula: F, inspector: Inspector?, defaultDispatcher: Dispatcher?, + isValidationEnabled: Boolean, ): TestFormulaObserver { val scope = coroutineTestRule.testCoroutineScope - val delegate = CoroutineTestDelegate(scope, formula, inspector, defaultDispatcher) + val runtimeConfig = RuntimeConfig( + isValidationEnabled = isValidationEnabled, + inspector = inspector, + defaultDispatcher = defaultDispatcher + ) + val delegate = CoroutineTestDelegate(scope, formula, runtimeConfig) return TestFormulaObserver(delegate) } @@ -112,17 +118,11 @@ private class FlowStreamFormulaSubject : FlowFormula(), StreamFormu private class CoroutineTestDelegate>( private val scope: CoroutineScope, override val formula: FormulaT, - private val inspector: Inspector?, - private val dispatcher: Dispatcher?, + private val runtimeConfig: RuntimeConfig, ): FormulaTestDelegate { private val values = mutableListOf() private val errors = mutableListOf() - private val runtimeConfig = RuntimeConfig( - isValidationEnabled = true, - inspector = inspector, - defaultDispatcher = dispatcher, - ) private val inputFlow = MutableSharedFlow(1, onBufferOverflow = BufferOverflow.DROP_OLDEST) private val formulaFlow = formula.toFlow(inputFlow, runtimeConfig) .onEach { values.add(it) } diff --git a/formula/src/test/java/com/instacart/formula/test/RxJavaTestableRuntime.kt b/formula/src/test/java/com/instacart/formula/test/RxJavaTestableRuntime.kt index dabcf5396..03005c363 100644 --- a/formula/src/test/java/com/instacart/formula/test/RxJavaTestableRuntime.kt +++ b/formula/src/test/java/com/instacart/formula/test/RxJavaTestableRuntime.kt @@ -2,6 +2,7 @@ package com.instacart.formula.test import com.instacart.formula.Action import com.instacart.formula.IFormula +import com.instacart.formula.RuntimeConfig import com.instacart.formula.plugin.Dispatcher import com.instacart.formula.plugin.Inspector import com.instacart.formula.rxjava3.ObservableFormula @@ -18,12 +19,14 @@ object RxJavaTestableRuntime : TestableRuntime { formula: F, inspector: Inspector?, defaultDispatcher: Dispatcher?, + isValidationEnabled: Boolean, ): TestFormulaObserver { - val delegate = RxJavaFormulaTestDelegate( - formula = formula, + val runtimeConfig = RuntimeConfig( + isValidationEnabled = isValidationEnabled, inspector = inspector, - dispatcher = defaultDispatcher, + defaultDispatcher = defaultDispatcher ) + val delegate = RxJavaFormulaTestDelegate(formula, runtimeConfig) return TestFormulaObserver(delegate) } diff --git a/formula/src/test/java/com/instacart/formula/test/TestableRuntime.kt b/formula/src/test/java/com/instacart/formula/test/TestableRuntime.kt index b0dd7b637..5eba62937 100644 --- a/formula/src/test/java/com/instacart/formula/test/TestableRuntime.kt +++ b/formula/src/test/java/com/instacart/formula/test/TestableRuntime.kt @@ -19,6 +19,7 @@ interface TestableRuntime { formula: F, inspector: Inspector? = null, defaultDispatcher: Dispatcher? = null, + isValidationEnabled: Boolean = true, ): TestFormulaObserver /**