1311: Dispatcher Tests for RenderWorkflowIn on Android #1350
Conversation
steve-the-edwards
force-pushed
the
sedwards/move-render-android
branch
2 times, most recently
from
b116d56 to
2deda9c
Compare
steve-the-edwards
force-pushed
the
sedwards/move-render-android
branch
from
e00bd02 to
21e5929
Compare
steve-the-edwards
requested review from
zach-klippenstein and
a team
as code owners
| @OptIn(WorkflowExperimentalRuntime::class) | ||
| @Burst | ||
| class AndroidDispatchersRenderWorkflowInTest( | ||
| private val runtime: RuntimeOptions = RuntimeOptions.DEFAULT |
There was a problem hiding this comment.
Note that burst will create a separate test class for each runtime config combination and run them separately. (i'll need to migrate my hand rolled parameterization in the other tests for runtime config to this soon).
| Choreographers.postOnFrameRendered { | ||
| // We are expecting this to happen last, after we get the rendering! | ||
| expectInOrder(2) | ||
| } |
There was a problem hiding this comment.
Right before the event that triggers the change I add a callback for the end of the frame (through papa) that expects to be done at order position 2 (the last position for this test, critically after we've received the updated rendering we want from the change).
@pyricau - does this look like a reasonable way to test this guarantee?
There was a problem hiding this comment.
Can we add an expectInOrder to the end of the test, maybe after the runTest block, to ensure that this callback is also invoked?
There was a problem hiding this comment.
You mean as a verification that we actually have a choreographer on this thread we are running on? Ya that's probably good.
| Choreographers.postOnFrameRendered { | ||
| // This should be happening last! | ||
| expectInOrder(2) | ||
| } |
There was a problem hiding this comment.
Note same thing here, right before the callback event we add the listener to the end of the next frame.
|
|
||
| testScheduler.advanceUntilIdle() | ||
|
|
||
| renderedMutex.lock() |
There was a problem hiding this comment.
Nit: Could we use Turbine to write the assertions on the renderings flow?
There was a problem hiding this comment.
I tried and this version was more readable/intuitive I thought.
| private class SimpleScreen( | ||
| val name: String = "Empty", | ||
| val callback: () -> Unit, | ||
| ) | ||
|
|
||
| private val orderIndex = AtomicInteger(0) | ||
|
|
||
| private fun resetOrderCounter() { | ||
| orderIndex.set(0) | ||
| } | ||
|
|
||
| @Before | ||
| fun setup() { | ||
| resetOrderCounter() | ||
| } | ||
|
|
||
| private fun expectInOrder( | ||
| expected: Int, | ||
| prefix: String = "" | ||
| ) { | ||
| val localActual = orderIndex.getAndIncrement() | ||
| assertEquals( | ||
| expected, | ||
| localActual, | ||
| "$prefix: This should have happened" + | ||
| " in a different order position:" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Nit: Shouldn't these be at the top or bottom of the class, not in the middle of test methods?
| workflow = workflow, | ||
| scope = backgroundScope + | ||
| Dispatchers.Main.immediate, | ||
| props = MutableStateFlow(Unit).asStateFlow(), |
There was a problem hiding this comment.
Oof, I can't believe there's no stateFlowOf().
| Choreographers.postOnFrameRendered { | ||
| // We are expecting this to happen last, after we get the rendering! | ||
| expectInOrder(2) | ||
| } |
There was a problem hiding this comment.
Can we add an expectInOrder to the end of the test, maybe after the runTest block, to ensure that this callback is also invoked?
| DEFAULT(RENDER_PER_ACTION), | ||
| RENDER_ONLY(setOf(RENDER_ONLY_WHEN_STATE_CHANGES)), | ||
| CONFLATE(setOf(CONFLATE_STALE_RENDERINGS)), | ||
| STABLE(setOf(STABLE_EVENT_HANDLERS)), | ||
| RENDER_ONLY_CONFLATE(setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES)), | ||
| RENDER_ONLY_PARTIAL(setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING)), | ||
| RENDER_ONLY_CONFLATE_STABLE( | ||
| setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES, STABLE_EVENT_HANDLERS) | ||
| ), | ||
| RENDER_ONLY_PARTIAL_STABLE( | ||
| setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING, STABLE_EVENT_HANDLERS) | ||
| ), | ||
| RENDER_ONLY_CONFLATE_PARTIAL( | ||
| setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING) | ||
| ), | ||
| RENDER_ONLY_CONFLATE_PARTIAL_STABLE( | ||
| setOf( | ||
| CONFLATE_STALE_RENDERINGS, | ||
| RENDER_ONLY_WHEN_STATE_CHANGES, | ||
| PARTIAL_TREE_RENDERING, | ||
| STABLE_EVENT_HANDLERS | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Holy crap, do we really need to test all these combinations? Are we running (or planning to) all of these in production??
There was a problem hiding this comment.
no, but I wanted not to have restrictions (or unknown bugs) on what combinations could be used.
| val renderings = renderWorkflowIn( | ||
| workflow = workflow, | ||
| scope = backgroundScope + | ||
| Dispatchers.Main.immediate, |
There was a problem hiding this comment.
Consider pulling this out into a file constant or something so that when you change it later it's easier to ensure that all usages are changed. And could probably pull some other common values out of the tests too.
A set of tests for using
renderWorkflowInwith particular dispatchers on Android. These are used in two ways:Main.immediatedispatcher). Now, this 2nd category is currently failing! That's part of the fix that needs to happen for On Android, process all updates from actions from an event before the next frame (Choreographer.doFrame) #1311 . So we merge these red tests first.