Increase code coverage (pt2).

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<Input : Any, Output : Any, FormulaT : IFormula<Input, Output>>(
     override val formula: FormulaT,
-    isValidationEnabled: Boolean = true,
-    inspector: Inspector?,
-    dispatcher: Dispatcher?,
+    runtimeConfig: RuntimeConfig,
 ) : FormulaTestDelegate<Input, Output, FormulaT> {
-    private val runtimeConfig = RuntimeConfig(
-        isValidationEnabled = isValidationEnabled,
-        inspector = inspector,
-        defaultDispatcher = dispatcher,
-    )
-
     private val inputRelay = BehaviorSubject.create<Input>()
     private val observer = formula
         .toObservable(inputRelay, runtimeConfig)

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 <Input : Any, Output : Any, F: IFormula<Input, Output>> F.test(
     inspector: Inspector? = null,
     dispatcher: Dispatcher? = null,
 ): TestFormulaObserver<Input, Output, F> {
-    val delegate = RxJavaFormulaTestDelegate(this, isValidationEnabled, inspector, dispatcher)
+    val runtimeConfig = RuntimeConfig(
+        isValidationEnabled = isValidationEnabled,
+        inspector = inspector,
+        defaultDispatcher = dispatcher,
+    )
+
+    val delegate = RxJavaFormulaTestDelegate(this, runtimeConfig)
     return TestFormulaObserver(delegate)
 }
 

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<Any, FormulaManager<*, *>>? = null
+    private val children: SingleRequestMap<Any, FormulaManager<*, *>> = LinkedHashMap()
     private var indexes: MutableMap<Any, Int>? = null
     private var pendingRemoval: MutableList<FormulaManager<*, *>>? = 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,20 +41,20 @@ 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 <ChildInput, ChildOutput> findOrInitChild(
         key: Any,
         formula: IFormula<ChildInput, ChildOutput>,
         input: ChildInput,
     ): FormulaManager<ChildInput, ChildOutput> {
-        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<Any>()
                 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 <ChildInput, ChildOutput>  childFormulaHolder(
+    private fun <ChildInput, ChildOutput> getOrInitChildManager(
         key: Any,
         formula: IFormula<ChildInput, ChildOutput>,
         input: ChildInput,
     ): SingleRequestHolder<FormulaManager<ChildInput, ChildOutput>> {
-        val children = children ?: run {
-            val initialized: SingleRequestMap<Any, FormulaManager<*, *>> = 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
     }

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<Input, State, EventT>(val key: Any) : Listener<EventT> {
+internal class ListenerImpl<Input, State, EventT>(private val key: Any) : Listener<EventT> {
 
     @Volatile private var manager: FormulaManagerImpl<Input, State, *>? = null
     @Volatile private var snapshotImpl: SnapshotImpl<Input, State>? = null

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<Any, ListenerImpl<*, *, *>>? = null
     private var indexes: MutableMap<Any, Int>? = null
 
-    private fun duplicateKeyErrorMessage(key: Any): String {
-        return "Listener $key is already defined. Unexpected issue."
-    }
-
     fun <Input, State, Event> initOrFindListener(key: Any, useIndex: Boolean): ListenerImpl<Input, State, Event> {
         val currentHolder = listenerHolder<Input, State, Event>(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<Input, State, Event>
+        } else if (useIndex) {
             val index = nextIndex(key)
             val indexedKey = IndexedKey(key, index)
             initOrFindListener(indexedKey, useIndex)
         } else {
-            currentHolder
-                .requestAccess {
-                    duplicateKeyErrorMessage(currentHolder.value.key)
-                } as ListenerImpl<Input, State, Event>
+            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
     }

formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt

@@ -7,15 +7,6 @@ package com.instacart.formula.internal
 internal class SingleRequestHolder<T>(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
     }

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<out Input, State> internal constructor(
+internal class SnapshotImpl<out Input, State>(
     override val input: Input,
     override val state: State,
     listeners: Listeners,

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<Int, Int>() {
+            override fun key(input: Int): Any = input
+
+            override fun Snapshot<Int, Unit>.evaluate(): Evaluation<Int> {
+                return Evaluation(
+                    output = 1
+                )
+            }
+        }
+
+        val parentFormula = object : StatelessFormula<Unit, Int>() {
+            override fun Snapshot<Unit, Unit>.evaluate(): Evaluation<Int> {
+                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<Unit, Unit>() {
+            override fun Snapshot<Unit, Unit>.evaluate(): Evaluation<Unit> {
+                return Evaluation(
+                    output = Unit,
+                    actions = context.actions {
+                        Action.onTerminate().onEvent {
+                            relay.triggerEvent()
+                            none()
+                        }
+                    }
+                )
+            }
+        }
+
+        val parentFormula = object : Formula<Boolean, Int, Int>() {
+            override fun initialState(input: Boolean): Int = 0
+
+            override fun Snapshot<Boolean, Int>.evaluate(): Evaluation<Int> {
+                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<Unit, Unit>() {
+            override fun Snapshot<Unit, Unit>.evaluate(): Evaluation<Unit> {
+                return Evaluation(
+                    output = Unit,
+                    actions = context.actions {
+                        Action.onTerminate().onEvent {
+                            terminate()
+                            none()
+                        }
+                    }
+                )
+            }
+        }
+
+        val parentFormula = object : StatelessFormula<Boolean, Int>() {
+            override fun Snapshot<Boolean, Unit>.evaluate(): Evaluation<Int> {
+                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