Skip to content
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.

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

Use test-specific unconfined when test scheduler is in use #151

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

JakeWharton
Copy link
Collaborator

@JakeWharton JakeWharton commented Sep 15, 2022

This retains the test scheduler virtual time.

Unfortunately this requires taking a dependency on the test library and also relying on unstable API.

Closes #124. Closes #118.

This retains the test scheduler virtual time.

Unfortunately this requires taking a dependency on the test library and also relying on unstable API.
Copy link
Collaborator

@jingibus jingibus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of Unconfined. But given that we're using it, this is the right thing to do.

@JakeWharton
Copy link
Collaborator Author

Yeah no one should use it... except this library. Consuming a Flow without dispatch and without needing Unconfined would be very welcome.

@JakeWharton JakeWharton merged commit b4f99fa into trunk Sep 15, 2022
@JakeWharton JakeWharton deleted the jw/grrrrrrr/2022-09-15 branch September 15, 2022 17:14
@jingibus
Copy link
Collaborator

Consuming a Flow without dispatch and without needing Unconfined would be very welcome.

Is UNDISPATCHED insufficient?

My mental model for the tool has always been something like (pseudocode):

val channel
val job = launch { flow => channel }

validate(channel) 
cleanup(channel, job)

Which is just two coroutines running in parallel.

When I'm lazy and want to sketch out a flow validation without adding the Turbine dependency, I tend to just do something like this by hand. And that works just fine. UNDISPATCHED makes certain startup scenarios easier to wire up, but I'm not aware of anything made easier by Unconfined.

@JakeWharton
Copy link
Collaborator Author

UNDISPATCHED only controls the first entry into the coroutine. Unconfined is UNDISPATCHED but for every re-entry after that first one.

Without Unconfined we can miss values while waiting on our coroutine to be resumed only to put them into a channel. This is most visible in things which conflate emissions and can receive two updates in the same stackframe.

The test to ensure Unconfined is doing its job for us is like

val flow = MutableStateFlow(0)
flow.test {
  assertEquals(0, awaitItem())
  flow.value = 1
  flow.value = 2
  assertEquals(1, awaitItem())
  assertEquals(2, awaitItem())
}

Remove Unconfined and the second awaitItem() sees only 2 and never 1.

@@ -108,10 +111,16 @@ public fun <T> Flow<T>.testIn(scope: CoroutineScope): ReceiveTurbine<T> {
return turbine
}

@OptIn(ExperimentalCoroutinesApi::class) // New kotlinx.coroutines test APIs are not stable 😬
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to #132 😭

@jingibus
Copy link
Collaborator

Without Unconfined we can miss values while waiting on our coroutine to be resumed only to put them into a channel. This is most visible in things which conflate emissions and can receive two updates in the same stackframe.

Yep, I'm aware. :(

My POV is that walking down this path leads to places like this. And I don't really like those places.

I have this wonderful set of tools in my coroutines toolbelt, and those places say "Okay, write your real code with your coroutines toolbelt, and then switch to this totally different testing toolbelt to validate it. Your testing toolbelt will let you forget about all that coroutines stuff, it just works!" And I go "Hmm, okay..." and use the testing toolbelt. But then when my code gets bold and exciting (e.g. molecule), the underlying coroutines-iness rears its head and I have to deal with it again.

@JakeWharton
Copy link
Collaborator Author

That issue is precisely what we want!

@jingibus
Copy link
Collaborator

It's not what I want. See discussion here. See talk here. If it is in fact what I want, I need some convincing.

The bedrock that has served me is this idea: tooling that encourages engineers to write code with an incorrect mental model will lead to confusion and errors. When I see advice like, "Oh, you just need to use UnconfinedTestDispatcher and then your test will do what you expect it do," it's hard not to see that as whitewash over easily fixed data races.

And of course, whether that tooling can even work 100% of the time is still an open question.

@JakeWharton
Copy link
Collaborator Author

It's what this library wants. Completely dropping Unconfined and the conditional we have to make to support runTest sounds great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not cooperate well with runTest
2 participants