Track batch execution via inspector. #361
Conversation
JaCoCo Code Coverage 83.65% ✅
Generated by 🚫 Danger |
|
|
||
| private val actionInput = ActionDelegateFormula.Input( |
There was a problem hiding this comment.
Simplifying this test class, no need to delegate to another formula for this.
Laimiux
force-pushed
the
laimonas/log-batch-execution
branch
from
5859e5a to
d4a0dc3
Compare
| // Inspect! | ||
| for (inspector in listOf(localInspector, globalInspector)) { | ||
| // Filtering out logs before "batch-started" | ||
| val events = inspector.events.dropWhile { !it.contains("batch-started") } |
There was a problem hiding this comment.
nit: I wonder if we should pass batchStartedEventPrefix as TestInspector field - to decipher this hidden "batch-started" dependency?
There was a problem hiding this comment.
While for this use case, it's a small lift, generally, this pattern would be hard to manage when verifying multiple events such as evaluate-started, state-change, etc. In my opinion, it's just easier to have a single TestInspector` with all the cases managed there (with no customizations across tests).
| "evaluate-finished: com.instacart.formula.types.IncrementActionFormula", | ||
| "evaluate-finished: null", | ||
| "formula-run-finished", | ||
| "batch-finished", |
There was a problem hiding this comment.
Same here with "batch-finished"
| ) : StatelessFormula<Unit, Int>() { | ||
| ) : Formula<Unit, Int, Int>() { |
There was a problem hiding this comment.
Does it need to be stateful now?
There was a problem hiding this comment.
Yeah, we increment integer state as part of the batched event.
What
I'm adding
onBatchStartedandonBatchFinishedto theInspectorinterface. This will enable us to debug how well event batching works.