Clean up RenderWorkflowIn Tests

[steve-the-edwards]

In the course of #1263 I encountered some issues with these tests.

1. We had two tests - onOutput_called_after_rendering_emitted and output_is_emitted_before_next_render_pass which were stating that they were testing the opposite things. In fact, there was a bug in onOutput_called_after_rendering_emitted where the assertion that should have failed the test was uncaught and only killed the coroutine TestScope rather than bubbling up to fail the test. So that test was 'failing' silently.
2. The stated intention of the API however, is to call onOutput after the rendering is emitted.
3. How do I know that? We tried to switch them to be more inline with the internal behaviour of a single Workflow's output - Root runWorkflowLoop has inconsistent output behavior compared to its WorkflowNode children  #54 , and then Emit output before next render pass. #68 - and that created a problem in the internal square usage with snapshot ordering - Revert "Emit output before next render pass." #151. However, when that was reverted, the name of the test did not change, so its name did not match the actual test assertion, until we updated the coroutine usage in the KMM migration (Convert :workflow-runtime to KMM #799) where the assertion changed to be output then rendering (in line with the name of the test, but changed from the previous behaviour).
4. Now, the reality is more nuanced. We do update the rendering before we call output, but we do so by assigning the .value on the StateFlow of RenderingAndSnapshots. This is a non-suspending operation, whereas calling onOutput is suspending. So really, onOutput will be called before any collectors of the renderings StateFlow can be dispatched to handle the update, but if the .value of that StateFlow of renderings was fetched in onOutput then it would be "up-to-date".
5. Turns out this is likely good enough behaviour for both coherence/consistency of the renderings wrt handling of the output.

Anyway, as a result of discovering this I have gone through and audited the RenderWorklfowInTest tests, updating them to:

1. Use runTest so that by default anything cancelling the CoroutineScope provided will also fail the test.
2. Remove the incorrect test.
3. Remove the testScope and pausedTestScope in favour of using the scope provided by runTest (and the backgroundScope when we have an operation - like the runtime that we don't care if its stopped before the test is done).

[rjrjr]

LGTM, but it would be wise to get @zach-klippenstein or someone else with stronger Coroutine chops to LG.

[rjrjr]

Neat

[zach-klippenstein]

Glad we're finally using the official test scopes for this!

[zach-klippenstein]

the [onOutput] handler will not be immediately dispatched (it is waiting for dispatch from the scheduler)

The workflow runtime calls onOutput directly, it's just a function, so the dispatcher is not involved at all. The reason this timing is the way it is is because the runtime sets the MSF's value property before calling onOutput. Or am I misreading this explanation?

[steve-the-edwards]

onOutput is a suspend fun, so it will go through dispatch. It's a good point though that perhaps we don't need it to be a suspend fun

[steve-the-edwards]

I'm thinking we made onOutput a suspend fun because we wanted it to be able to cancel the runtime's scope? cc @rjrjr if you remember.

[rjrjr]

No idea. I doubt we did it by accident.

[zach-klippenstein]

Maybe I'm missing something but it looks like what this test is really testing is the behavior of Unconfined dispatchers. The behavior it validates is implied by the previous test when the renderings flow is collected with an unconfined dispatcher. Doesn't seem worth writing our own tests for core coroutine features.

[steve-the-edwards]

heh, ya you're point is valid. I was trying to make a point with the test more than actually test anything.

[zach-klippenstein]

Having expressed my doubt that this test is really worth having at all, if we do want it, it might make the test more clear and explicit to use a pattern that is often used to validate synchronization expectations in concurrent code. I can't remember if we use this elsewhere in workflow tests, but basically you just do this (in the test method, the class, or an external helper class, doesn't matter):

val actual = AtomicInteger(0)
fun expect(expected: Int) {
  val actual = actual.getAndIncrement()
  assertEquals(expected, actual, "Expected $expected, was $actual")
}

And then you pepper the concurrent code with calls something like this:

onOutput = { it: String ->
  expect(1)
}
…
launch {
  renderings.collect {
    expect(0)
  }
}

It's easy to see what you're testing because the code says explicitly what order the expect calls are expected to occur in.

[steve-the-edwards]

That's a nice pattern! I will see if I can make use of in future refactors.