Refactoring + adding new tests to increase code coverage. #351

Laimiux · 2024-03-15T17:33:21Z

What

Miscellaneous non-behavior changes to increase coverage.

carrotkite · 2024-03-15T17:37:30Z

JaCoCo Code Coverage 83.63% ✅

Class	Covered	Meta	Status
`com/instacart/formula/coroutines/FlowRuntime`	100%	0%	✅
`com/instacart/formula/internal/Listeners`	95%	0%	✅
`com/instacart/formula/internal/SnapshotImpl`	87%	0%	✅
`com/instacart/formula/internal/FormulaManagerImpl`	87%	0%	✅
`com/instacart/formula/internal/ChildrenManager`	75%	0%	✅
`com/instacart/formula/internal/SingleRequestHolder`	0%	0%	✅
`com/instacart/formula/rxjava3/RxJavaRuntime`	100%	0%	✅
`com/instacart/formula/FormulaRuntime`	94%	0%	✅
`com/instacart/formula/ActionBuilder`	100%	0%	✅

Generated by 🚫 Danger

Laimiux · 2024-03-15T18:46:05Z

formula/src/main/java/com/instacart/formula/Cancelable.kt

@@ -3,16 +3,6 @@ package com.instacart.formula
 /**
 * Used within [Action] to receive a cancel event and perform clean up.
 */
-interface Cancelable {
-    companion object {
-        inline operator fun invoke(crossinline cancel: () -> Unit): Cancelable {


Not needed with fun interface

Laimiux · 2024-03-15T18:46:24Z

formula/src/main/java/com/instacart/formula/DeferredAction.kt

@@ -5,13 +5,13 @@ package com.instacart.formula
 */
 class DeferredAction<Event>(
    val key: Any,
-    val action: Action<Event>,


Make it private to reduce coverage requirements

Laimiux · 2024-03-15T18:46:36Z

formula/src/main/java/com/instacart/formula/DeferredAction.kt

    override fun equals(other: Any?): Boolean {
        if (this === other) return true
-        if (javaClass != other?.javaClass) return false


Simplify this as add tests

Laimiux · 2024-03-15T18:46:39Z

formula/src/main/java/com/instacart/formula/DeferredAction.kt

    }

    override fun hashCode(): Int {
        return initial.hashCode()
    }
-
-    fun keyAsString(): String {


Laimiux · 2024-03-15T18:46:48Z

formula/src/main/java/com/instacart/formula/FormulaContext.kt

@@ -123,12 +123,12 @@ abstract class FormulaContext<out Input, State> internal constructor(
    internal abstract fun <Event> eventListener(
        key: Any,
        useIndex: Boolean = true,
-        executionType: Transition.ExecutionType? = null,


Default parameter not needed

Laimiux · 2024-03-15T18:46:55Z

formula/src/main/java/com/instacart/formula/FormulaContext.kt

        transition: Transition<Input, State, Event>
    ): Listener<Event>

    // Internal key scope management
    @PublishedApi internal abstract fun enterScope(key: Any)
    @PublishedApi internal abstract fun endScope()
-    @PublishedApi internal abstract fun createScopedKey(type: KClass<*>, key: Any? = null): Any


Default parameter not needed

Laimiux · 2024-03-15T18:47:13Z

formula/src/main/java/com/instacart/formula/FormulaRuntime.kt

@@ -17,14 +17,14 @@ class FormulaRuntime<Input : Any, Output : Any>(
    private val formula: IFormula<Input, Output>,
    private val onOutput: (Output) -> Unit,
    private val onError: (Throwable) -> Unit,
-    config: RuntimeConfig?,
+    config: RuntimeConfig,


Instead of having default values here as well, lets just use non-null config

Laimiux · 2024-03-15T18:48:23Z

formula/src/main/java/com/instacart/formula/FormulaRuntime.kt

-            } else {
-                pendingEvaluation = pendingEvaluation || evaluate
-            }
+        pendingEvaluation = pendingEvaluation || evaluate


Added runIfNeeded that checks effects and pending evaluation (shared with executeBatch)

Laimiux · 2024-03-15T18:50:48Z

formula/src/main/java/com/instacart/formula/FormulaRuntime.kt

@@ -255,9 +253,10 @@ class FormulaRuntime<Input : Any, Output : Any>(
        } catch (e: Throwable) {
            isRunning = false

-            manager?.markAsTerminated()
+            val manager = requireManager()


Not possible to enter this block with null manager

Laimiux · 2024-03-15T19:46:51Z

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

@@ -91,15 +86,6 @@ internal class SnapshotImpl<out Input, State> internal constructor(
    }

    fun <Event> dispatch(transition: Transition<Input, State, Event>, event: Event) {
-        if (!running) {


We don't need this check as transitions are controlled with a queue mechanism (lives in FormulaManager).

Laimiux · 2024-03-15T19:46:54Z

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

-            throw IllegalStateException("Transitions are not allowed during evaluation")
-        }
-
-        if (!delegate.isTerminated() && delegate.isEvaluationNeeded(associatedEvaluationId)) {


We don't need this check as transitions are controlled with a queue mechanism (lives in FormulaManager).

Laimiux · 2024-03-15T19:47:38Z

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

-    @Volatile internal var manager: FormulaManagerImpl<Input, State, *>? = null
-    @Volatile internal var snapshotImpl: SnapshotImpl<Input, State>? = null
-    @Volatile internal var executionType: Transition.ExecutionType? = null
+    @Volatile private var manager: FormulaManagerImpl<Input, State, *>? = null


Making private since getters are not needed

Laimiux · 2024-03-15T19:50:07Z

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

@@ -11,11 +9,10 @@ import com.instacart.formula.Transition
 */
 class DeferredTransition<Input, State, EventT> internal constructor(
    private val listener: ListenerImpl<Input, State, EventT>,
-    private val transition: Transition<Input, State, EventT>,


This is more of a potential bug fix: we should use mutable transition that lives in ListenerImpl instead relying on fixed value. With dispatching, this value could be out of date by the time we call execute

Laimiux · 2024-03-15T19:50:20Z

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

@@ -41,7 +41,7 @@ internal class ActionBuilderImpl<out Input, State> internal constructor(

    private fun <Event> toBoundStream(
        stream: Action<Event>,
-        executionType: Transition.ExecutionType? = null,


No need for default value

Laimiux · 2024-03-15T19:53:41Z

formula/src/main/java/com/instacart/formula/FormulaRuntime.kt


            if (evaluate) {
                var shouldRun = true
                while (shouldRun) {
                    val localInputId = inputId
-                    if (!manager.isTerminated()) {


It's not possible to hit this case because evaluate will be false if the manager is terminated

Laimiux · 2024-03-15T19:54:08Z

formula/src/main/java/com/instacart/formula/FormulaRuntime.kt

-                                // Terminated manager with input change indicates that formula
-                                // key changed and we are resetting formula state. We need to
-                                // start a new formula manager.
-                                if (localInputId != inputId) {


No need to check if localInputId != inputId

jasonostrander

Looks good! A few questions I have, but no issues.

jasonostrander · 2024-03-15T20:31:44Z

formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt

+        observer.output { assertThat(this).isEqualTo(1) }
+
+        // Check that termination was called
+        assertThat(terminationCallback.values()).containsExactly(0).inOrder()


This is interesting behavior. Took me a few read-throughs to understand why this happens. I wonder if we should alert devs somehow when this occurs?

Could you elaborate on the interesting part?

Triggering onTerminate was surprising to me. I thought it would just discard the current evaluation and re-evaluate.

Also I've seen a lot of people trigger side-effects in an onEvent block like this and I'm thinking if that side-effect triggers a new input this would similarly surprise people.

It terminates because I have key defined for the formula

override fun key(input: Int): Any { return input }

Ah, I see now, that makes more sense.

jasonostrander · 2024-03-15T20:33:44Z

formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt

+        }
+
+        observer = runtime.test(root)
+        observer.input(0)


Should we assert the output value here? Should be 0, right?

There should be no output given formula exited before producing one.

Hmm, I thought there would be a single evaluation, with the transition running afterward?

There was, but since formula was terminated during transition, we don't emit an output.

jasonostrander · 2024-03-15T20:34:28Z

formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt

+        }
+
+        observer = runtime.test(root)
+        observer.input(0)


This wouldn't have a valid output after terminating, right?

Good point, will validate

Laimiux force-pushed the laimonas/increase-coverage branch from 673c99d to a8669da Compare March 15, 2024 18:45

Laimiux requested a review from jasonostrander March 15, 2024 18:45

Laimiux commented Mar 15, 2024

View reviewed changes

Laimiux force-pushed the laimonas/increase-coverage branch from a8669da to a76f428 Compare March 15, 2024 18:50

Laimiux commented Mar 15, 2024

View reviewed changes

Laimiux force-pushed the laimonas/increase-coverage branch 4 times, most recently from 8ecdc4a to 63c552c Compare March 15, 2024 19:41

Laimiux commented Mar 15, 2024

View reviewed changes

Laimiux force-pushed the laimonas/increase-coverage branch from 63c552c to a901953 Compare March 15, 2024 20:04

Laimiux changed the base branch from master to laimonas/increase-coverage-pt1 March 15, 2024 20:04

Base automatically changed from laimonas/increase-coverage-pt1 to master March 15, 2024 20:32

Laimiux force-pushed the laimonas/increase-coverage branch from a901953 to 9b7fb8c Compare March 15, 2024 20:32

jasonostrander approved these changes Mar 15, 2024

View reviewed changes

Laimiux force-pushed the laimonas/increase-coverage branch from 9b7fb8c to 0040fff Compare March 15, 2024 20:52

Laimiux added 5 commits March 15, 2024 17:01

Add coverage for RuntimeConfig.

3d7cd4c

Formula plugin coverage.

0332159

Miscellaneous coverage improvements.

e083692

Increase runtime coverage.

33a3748

More coverage improvements.

0e9595a

Laimiux force-pushed the laimonas/increase-coverage branch from 0040fff to 0e9595a Compare March 15, 2024 21:01

PR feedback.

ca05cb1

Laimiux merged commit fe5cce8 into master Mar 15, 2024
3 checks passed

Laimiux deleted the laimonas/increase-coverage branch March 15, 2024 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring + adding new tests to increase code coverage. #351

Refactoring + adding new tests to increase code coverage. #351

Laimiux commented Mar 15, 2024

carrotkite commented Mar 15, 2024 •

edited

Loading

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

Laimiux Mar 15, 2024

jasonostrander left a comment

jasonostrander Mar 15, 2024

Laimiux Mar 15, 2024

jasonostrander Mar 15, 2024

Laimiux Mar 15, 2024

jasonostrander Mar 15, 2024

jasonostrander Mar 15, 2024

Laimiux Mar 15, 2024

jasonostrander Mar 15, 2024

Laimiux Mar 15, 2024

jasonostrander Mar 15, 2024

Laimiux Mar 15, 2024

Refactoring + adding new tests to increase code coverage. #351

Refactoring + adding new tests to increase code coverage. #351

Conversation

Laimiux commented Mar 15, 2024

What

carrotkite commented Mar 15, 2024 • edited Loading

JaCoCo Code Coverage 83.63% ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonostrander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carrotkite commented Mar 15, 2024 •

edited

Loading