More coverage improvements.

instacart · Mar 15, 2024 · 0e9595a · 0e9595a
1 parent 33a3748
commit 0e9595a
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 30 deletions.
diff --git a/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt b/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt
@@ -26,11 +26,7 @@ internal class ChildrenManager(
     fun prepareForPostEvaluation() {
         indexes?.clear()
 
-        children?.clearUnrequested {
-            pendingRemoval = pendingRemoval ?: mutableListOf()
-            it.markAsTerminated()
-            pendingRemoval?.add(it)
-        }
+        children?.clearUnrequested(this::prepareForTermination)
     }
 
     fun terminateChildren(evaluationId: Long): Boolean {
@@ -95,6 +91,12 @@ internal class ChildrenManager(
         }
     }
 
+    private fun prepareForTermination(it: FormulaManager<*, *>) {
+        pendingRemoval = pendingRemoval ?: mutableListOf()
+        it.markAsTerminated()
+        pendingRemoval?.add(it)
+    }
+
     private fun <ChildInput, ChildOutput>  childFormulaHolder(
         key: Any,
         formula: IFormula<ChildInput, ChildOutput>,

diff --git a/formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt b/formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt
@@ -57,7 +57,7 @@ internal class FormulaManagerImpl<Input, State, Output>(
      * each formula output with an identifier value and compare it for validity with
      * the global value.
      */
-    var globalEvaluationId: Long = 0
+    private var globalEvaluationId: Long = 0
 
     /**
      * Determines if we are executing within [run] block. Enables optimizations
@@ -194,7 +194,6 @@ internal class FormulaManagerImpl<Input, State, Output>(
         val snapshot = SnapshotImpl(
             input = input,
             state = state,
-            associatedEvaluationId = evaluationId,
             listeners = listeners,
             delegate = this,
         )
@@ -220,7 +219,7 @@ internal class FormulaManagerImpl<Input, State, Output>(
         listeners.prepareForPostEvaluation()
         childrenManager?.prepareForPostEvaluation()
 
-        snapshot.running = true
+        snapshot.markRunning()
         if (!isValidationEnabled) {
             inspector?.onEvaluateFinished(loggingType, newFrame.evaluation.output, evaluated = true)
         }

diff --git a/formula/src/main/java/com/instacart/formula/internal/Listeners.kt b/formula/src/main/java/com/instacart/formula/internal/Listeners.kt
@@ -35,11 +35,7 @@ internal class Listeners {
      */
     fun prepareForPostEvaluation() {
         indexes?.clear()
-
-        listeners?.clearUnrequested {
-            // TODO log that disabled listener was invoked.
-            it.disable()
-        }
+        listeners?.clearUnrequested(this::disableListener)
     }
 
     fun disableAll() {
@@ -50,6 +46,10 @@ internal class Listeners {
         listeners?.clear()
     }
 
+    private fun disableListener(listener: ListenerImpl<*, *, *>) {
+        listener.disable()
+    }
+
     /**
      * Function which returns next index for a given key. It will
      * mutate the [indexes] map.

diff --git a/formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt b/formula/src/main/java/com/instacart/formula/internal/SingleRequestHolder.kt
@@ -23,7 +23,7 @@ internal class SingleRequestHolder<T>(val value: T) {
 
 internal typealias SingleRequestMap<Key, Value> = MutableMap<Key, SingleRequestHolder<Value>>
 
-internal inline fun <Value> SingleRequestMap<*, Value>.clearUnrequested(onUnrequested: (Value) -> Unit) {
+internal fun <Value> SingleRequestMap<*, Value>.clearUnrequested(onUnrequested: (Value) -> Unit) {
     val callbackIterator = this.iterator()
     while (callbackIterator.hasNext()) {
         val callback = callbackIterator.next()

diff --git a/formula/src/main/java/com/instacart/formula/internal/SnapshotImpl.kt b/formula/src/main/java/com/instacart/formula/internal/SnapshotImpl.kt
@@ -8,20 +8,18 @@ import com.instacart.formula.Listener
 import com.instacart.formula.Snapshot
 import com.instacart.formula.Transition
 import com.instacart.formula.TransitionContext
-import com.instacart.formula.plugin.Dispatcher
 import java.lang.IllegalStateException
 import kotlin.reflect.KClass
 
 internal class SnapshotImpl<out Input, State> internal constructor(
     override val input: Input,
     override val state: State,
-    private val associatedEvaluationId: Long,
     listeners: Listeners,
     private val delegate: FormulaManagerImpl<Input, State, *>,
 ) : FormulaContext<Input, State>(listeners), Snapshot<Input, State>, TransitionContext<Input, State> {
 
     private var scopeKey: Any? = null
-    var running = false
+    private var running = false
 
     override val context: FormulaContext<Input, State> = this
 
@@ -88,15 +86,6 @@ internal class SnapshotImpl<out Input, State> internal constructor(
     }
 
     fun <Event> dispatch(transition: Transition<Input, State, Event>, event: Event) {
-        if (!running) {
-            throw IllegalStateException("Transitions are not allowed during evaluation")
-        }
-
-        if (!delegate.isTerminated() && delegate.isEvaluationNeeded(associatedEvaluationId)) {
-            // We have already transitioned, this should not happen.
-            throw IllegalStateException("Transition already happened. This is using old event listener: $transition & $event. Transition: $associatedEvaluationId != ${delegate.globalEvaluationId}")
-        }
-
         val result = transition.toResult(this, event)
         if (TransitionUtils.isEmpty(result)) {
             return
@@ -105,6 +94,10 @@ internal class SnapshotImpl<out Input, State> internal constructor(
         delegate.handleTransitionResult(event, result)
     }
 
+    fun markRunning() {
+        running = true
+    }
+
     private fun ensureNotRunning() {
         if (running) {
             throw IllegalStateException("Cannot call this transition after evaluation finished. See https://instacart.github.io/formula/faq/#after-evaluation-finished")

diff --git a/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt b/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt
@@ -8,6 +8,7 @@ import com.instacart.formula.internal.ClearPluginsRule
 import com.instacart.formula.internal.FormulaKey
 import com.instacart.formula.internal.TestInspector
 import com.instacart.formula.internal.Try
+import com.instacart.formula.plugin.Dispatcher
 import com.instacart.formula.plugin.Inspector
 import com.instacart.formula.plugin.Plugin
 import com.instacart.formula.rxjava3.RxAction
@@ -653,6 +654,28 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) {
             }
     }
 
+    @Test fun `listener removed while dispatching an event will drop the event`() {
+        var observer: TestFormulaObserver<Unit, OptionalCallbackFormula.Output, OptionalCallbackFormula>? = null
+        FormulaPlugins.setPlugin(object : Plugin {
+            override fun backgroundThreadDispatcher(): Dispatcher {
+                return object : Dispatcher {
+                    override fun dispatch(executable: () -> Unit) {
+                        // We disable callback before executing increment
+                        observer?.output { toggleCallback() }
+                        executable()
+                    }
+                }
+            }
+        })
+
+        val root = OptionalCallbackFormula(
+            incrementExecutionType = Transition.Background
+        )
+        observer = runtime.test(root, Unit)
+        observer.output { listener?.invoke() }
+        observer.output { assertThat(state).isEqualTo(0) }
+    }
+
     @Test
     fun `listeners are not the same after removing then adding it again`() {
         runtime.test(OptionalCallbackFormula(), Unit)

diff --git a/formula/src/test/java/com/instacart/formula/subjects/OptionalCallbackFormula.kt b/formula/src/test/java/com/instacart/formula/subjects/OptionalCallbackFormula.kt
@@ -4,9 +4,12 @@ import com.instacart.formula.Evaluation
 import com.instacart.formula.Formula
 import com.instacart.formula.Listener
 import com.instacart.formula.Snapshot
+import com.instacart.formula.Transition
 
-class OptionalCallbackFormula :
-    Formula<Unit, OptionalCallbackFormula.State, OptionalCallbackFormula.Output>() {
+class OptionalCallbackFormula(
+    private val toggleExecutionType: Transition.ExecutionType? = null,
+    private val incrementExecutionType: Transition.ExecutionType? = null,
+) : Formula<Unit, OptionalCallbackFormula.State, OptionalCallbackFormula.Output>() {
     data class State(
         val callbackEnabled: Boolean = true,
         val state: Int = 0
@@ -22,7 +25,9 @@ class OptionalCallbackFormula :
 
     override fun Snapshot<Unit, State>.evaluate(): Evaluation<Output> {
         val callback = if (state.callbackEnabled) {
-            context.onEvent<Unit> { transition(state.copy(state = state.state + 1)) }
+            context.onEventWithExecutionType<Unit>(incrementExecutionType) {
+                transition(state.copy(state = state.state + 1))
+            }
         } else {
             null
         }
@@ -31,7 +36,7 @@ class OptionalCallbackFormula :
             output = Output(
                 state = state.state,
                 listener = callback,
-                toggleCallback = context.onEvent {
+                toggleCallback = context.onEventWithExecutionType(toggleExecutionType) {
                     transition(state.copy(callbackEnabled = !state.callbackEnabled))
                 }
             )