Update CSR to yield() and update tests #1294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

steve-the-edwards merged 1 commit into main from sedwards/conflate-timeout-1

Apr 22, 2025

Contributor

steve-the-edwards commented Apr 16, 2025 •

edited

Loading

As I was testing DRAIN_EXCLUSIVE_ACTIONS and ensuring that our tests would mirror the way that runtime is usually run (that is, with Dispatchers.Main.immediate - which is an unconfined dispatcher), I realized that we should always be testing with UnconfinedTestDispatcher in the runtime in order to accurately represent that behavior vis-a-vis these optimizations.

That testing change caused me to recognize that if we do not yield() before processing extra actions then depending on where the coroutines for processing the updated signals are dispatched, they may not be able to actually process them and send their actions to the sink. (like multiple Workers listening to the same signal).

So here we add yield() before processing extra actions.

There was also a bug where we did not output if we were short-circuiting after an action processed as part of CSR. I fixed this bug and updated the test to ensure it was tested.

I also updated RenderWorkflowInTest and WorkflowLifecycleTests tests to use the UnconfinedTestDispatcher. There was a confusing @Ignored test there that I updated the kdoc on to show why it was ignored.

UPDATE: I made the dispatcher one of the parameters for the test and use both UnconfinedTestDispatcher and StandardTestDispatcher.

More discussion of this and alternatives: https://square.slack.com/archives/C07UHTZKAPJ/p1744896778454249

steve-the-edwards requested review from zach-klippenstein and a team as code owners

April 16, 2025 19:19

steve-the-edwards commented

View reviewed changes

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt Outdated Show resolved Hide resolved

steve-the-edwards force-pushed the sedwards/conflate-timeout-1 branch from 9f218b2 to dbd02dc Compare

April 16, 2025 19:52

zach-klippenstein reviewed

View reviewed changes

Collaborator

zach-klippenstein left a comment

So this is great in that it now actually runs the tests the way we run the code in Register. However, it removes test coverage for normal dispatchers. Are we officially dropping support for non-immediate dispatchers? I thought we had some headless ones in Register, so we probably can't do that. It looks like we already have some kind of parameterized testing infra, can we use that to test on both types of dispatchers?

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt Show resolved Hide resolved

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt Outdated

                         // Skip rendering if we had unchanged state, keep draining actions.
-                        if (shouldShortCircuitForUnchangedState(actionResult)) continue
+                        if (shouldShortCircuitForUnchangedState(actionResult)) {
+                          // Emit the Output

Collaborator

zach-klippenstein Apr 17, 2025

Nit: This comment is pretty redundant, "emit" is just another word for "send" and that's literally the function name.

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt Outdated

                         // Skip rendering if we had unchanged state, keep draining actions.
-                        if (shouldShortCircuitForUnchangedState(actionResult)) continue
+                        if (shouldShortCircuitForUnchangedState(actionResult)) {
+                          // Emit the Output

Collaborator

zach-klippenstein Apr 17, 2025

Nit: This comment is pretty redundant, "emit" is just another word for "send" and that's literally the function name.

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt Outdated

                         // Skip rendering if we had unchanged state, keep draining actions.
-                        if (shouldShortCircuitForUnchangedState(actionResult)) continue
+                        if (shouldShortCircuitForUnchangedState(actionResult)) {
+                          // Emit the Output

Collaborator

zach-klippenstein Apr 17, 2025

Nit: This comment is pretty redundant, "emit" is just another word for "send" and that's literally the function name.

Contributor Author

steve-the-edwards Apr 17, 2025

Was this some kind of inception?
.... Was this some kind of inception?
..... Was this some kind of inception?

Collaborator

zach-klippenstein Apr 17, 2025

Wtf is going on lol 🫥

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt

		}
		val collectionJob = launch(UnconfinedTestDispatcher(testScheduler)) {

Collaborator

zach-klippenstein Apr 17, 2025

Yay for consistency within the test!

Contributor Author

steve-the-edwards commented Apr 17, 2025 •

edited

Loading

So this is great in that it now actually runs the tests the way we run the code in Register. However, it removes test coverage for normal dispatchers. Are we officially dropping support for non-immediate dispatchers? I thought we had some headless ones in Register, so we probably can't do that. It looks like we already have some kind of parameterized testing infra, can we use that to test on both types of dispatchers?

The parameterized testing is my own hand-rolled one here due to RuntimeConfig 😅 .
We could extend it for dispatchers yes.

However, i don't think it would be that simple as the test shape will have to change slightly for a StandardTestDispatcher (in the least it would include runCurrent() etc.).

I think your point that "We should test with non-immediate dispatchers" is a good one.
I disagree that this 'takes away' that testing as the way it was currently was by no means robust or comprehensive. I think I would take your suggestion and make it into a separate PR that would add the same behavior testing for RenderWorkflow with non-immediate dispatchers (likely just as a separate suite of tests).

Update: Nevermind, I made it work pretty straightforwardly and now test with both types of dispatchers.


          Update CSR to yield() and update tests w/ multiple dispatchers

934826b

steve-the-edwards force-pushed the sedwards/conflate-timeout-1 branch from dbd02dc to 934826b Compare

April 17, 2025 20:50

steve-the-edwards requested a review from zach-klippenstein

April 17, 2025 20:52

zach-klippenstein approved these changes

View reviewed changes

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt

Comment on lines +67 to +68

		UnconfinedTestDispatcher(),
		myStandardTestDispatcher

Collaborator

zach-klippenstein Apr 17, 2025

Sweet!

steve-the-edwards merged commit 9b8077c into main

43 checks passed

steve-the-edwards deleted the sedwards/conflate-timeout-1 branch

April 22, 2025 14:32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet