How to test all intermediate emissions despite conflation #3120
Comments
|
btw the |
|
I fidgeted a bit with it and from what I understand: As UnconfinedTestDispatcher doesn't ensure any order of performing tasks, keeping everything under this one Unconfined dispatcher is expected to give you bad results here.
But it also is supposed to resume its coroutines immediately without going through dispatch, you actually also have an example of using it for StateFlow testing in documentation of UnconfinedTestDispatcher
So I suppose what you want to do is run the collection of the flow with an UnconfinedTestDispatcher, but the rest of the test, the code that is emitting, under a StandardTestDispatcher that lets you precisely control the order of execution That would leave you with something like this: I'm not awfully familiar with StateFlow and new testing mechanics yet, so somebody stronger in this topic could elaborate on the importance of sharing the testScheduler in that scenario (I'll see more about it tomorrow too :) ) |
|
Ok I understand your suggestion, but if that's the case, I simply don't want to stop using I'm sure there are some confusing scenarios with the Right now I'd have to redo almost 200 test cases in our code base to fit the new APIs somewhat, making tests unreadable (there would have to be way more And what do I get back for the rework? Nothing. And I'd be rewriting it into something that uses experimental APIs, so that I get the joy of rewriting those 200 test cases again, if the team decides to change the API again. |
|
I understand your frustration, I'm somewhat guilty of using experimental api in production too, but we were explicitly warned about it being experimental If you re satisfied with old api, I'd recommend you just stick to it despite being deprecated. Regarding multiple calls of |
|
We also encounter the same issue with 1.6.0. when you collect, you receive only |
|
If you don't need to handle delays, check out the implementation of This is not documented anywhere and it is not the official recommended way to do this (I don't know what is the official way if there even is any in the first place), but it definitely works. Unfortunately this will not work if you have delays in your coroutines, because this dispatcher does not handle delays. One could try to implement delays by themselves, but the IDE will warn you that you're trying to use internal coroutine API's which is discouraged. My view is, that either the Alternatively you have to change your tests to account for conflation (documentation of I think something like |
|
Also one more thing. While the example by @Bezkarpie works, it does not reflect reality of at least our code base, and probably also not other codebases. In @Bezkarpie example the state flow needs to be collected by I couldn't get it to work any other way (this could be my own failing entirely). Often that's not what we want. Since in at least in context of android development, we rely on If we're also using things, such as It would mean that every single coroutine that is launched in view model using But that's not how we want to have our code structured, it's a lot of boilerplate and it shouldn't be necessary. It wasn't necessary with the old test APIs. |
|
Is the 1.6.0 implementation of EDIT : I also have come to the same conclusion about the test of viewmodel in Android, with this new API (which I had nothing against) |
@lukas1 I don't quite understand the disappointment here, nor the need for the old behaviour. The tests as written here don't seem deterministic. You are launching 2 coroutines, not waiting for anything, and then hoping to have the results in a list that you update concurrently. Making this test work by dispatcher magic (unconfined, immediate, and the likes) really doesn't seem like the right way to approach the problem IMO. The test just doesn't read well in the first place. If I see If you want to force other coroutines execution, you should use facilities like
Relying on special dispatcher implementations is just as brittle as that. Adding suspension points in production code could change the behaviour too and break those tests. I'm not arguing that you should account for skipped events, I'm saying that you should make sure all events happen by inserting |
This does not work however; If you have example such as the one provided by @ReginFell , using
Again, this is not appropriate. Again the example by @ReginFell shows precisely why we don't want to just test for the last state; We want to make sure that the state update happens. That it is not accidentally skipped by wrong implementation. It's not possible/practical to add suspension points before EVERY update to the state flow.
I disagree. Relying on documented dispatcher behaviour is precisely what we should do. There should be a specific dispatcher that guarantees order of execution as if the code was completely blocking. If that were the case, there'd be no confusion about order of execution or anything.
Actually it reads much better than suggested alternatives with using |
|
@NitroG42 I also had some such tests, that stopped working even with the deprecated I plan to study these cases more closely, as I do not know what exactly is going on there, why it fails, but at least for now I was able to fix those cases by using the |
I'm talking about running coroutines between state updates, not doing a single
This really depends. Most of the time doing this is like testing
It's exactly what should be done in a test that is self-explanatory. If you want to test updates at every step, the test should read as such. I guess it would help to see the real life application of those tests, because these hypothetical tests seem strange (at least to me). For instance this part to drive the test case seem unnecessarily complicated: flowOf(Unit)
.onStart {
testedFlow.update { it + 1 }
}
.onCompletion { testedFlow.update { it + 1 } }
.onEach {
testedFlow.update { it + 1 }
}
.launchIn(this)And I'm pretty sure it's written like this here because in your real test you can't just call |
|
@lukas1 @ReginFell Anyway, I ended up with something like this, I also checked some simple delays and it works ok: And it works just fine. If you re experiencing any issues that can not be solved with it, please, provide a full minimal example including the test and tested code |
|
@Bezkarpie this works because you added Also, add another property |
|
Please, put this in your IDE and check it first before making such statements. |
|
Just did a sample project about the Android ViewModel issue : https://github.com/NitroG42/Coroutine160Test |
|
@Bezkarpie I apologize, I should have tested it before making the comment. I responded in a haste. That said, I have created an example where the approach using It's a very simplified example of something that happens in our application a lot. Again, it is possible, that I'm using the new APIs wrong. Here's the example: class DummyTest {
@Test
fun dummyTest() {
ArchTaskExecutor.getInstance()
.setDelegate(object : TaskExecutor() {
override fun executeOnDiskIO(runnable: Runnable) = runnable.run()
override fun postToMainThread(runnable: Runnable) = runnable.run()
override fun isMainThread(): Boolean = true
})
val testScheduler = TestCoroutineScheduler()
Dispatchers.setMain(StandardTestDispatcher(testScheduler))
//Dispatchers.setMain(UnconfinedTestDispatcher(testScheduler))
//Dispatchers.setMain(TestCoroutineDispatcher())
val collectedStates = mutableListOf<DummyViewModel.State>()
val viewModel = DummyViewModel(flowOf(3))
viewModel.stateAsLiveData.observeForever {
collectedStates.add(it)
}
viewModel.runTest()
testScheduler.runCurrent()
assertEquals(
listOf(
DummyViewModel.State(),
DummyViewModel.State(
isLoading = true,
data = null
),
DummyViewModel.State(
isLoading = true,
data = 3
),
DummyViewModel.State(
isLoading = false,
data = 3
),
),
collectedStates
)
Dispatchers.resetMain()
}
class DummyViewModel(
private val dataFlow: Flow<Int>
) : ViewModel() {
data class State(
val isLoading: Boolean = false,
val data: Int? = null
)
private val stateFlow = MutableStateFlow(State())
val stateAsLiveData = stateFlow.asLiveData()
fun runTest() {
dataFlow
.onStart { stateFlow.update { it.copy(isLoading = true) } }
.onCompletion { stateFlow.update { it.copy(isLoading = false) } }
.onEach { current -> stateFlow.update { it.copy(data = current) } }
.launchIn(viewModelScope)
}
}
}This test works fine if you comment out the line with Let me know if you have some ideas how to make my example work, please. In the meantime I will try to also come up with some solution. Thank you for your time. |
|
@Bezkarpie btw indeed you approach works if testing the |
Please take a look at my example code above. Maybe you have some idea how to structure this test better with |
|
after some trial and error, I was able to make this work: class DummyTest {
@Test
fun dummyTest() {
ArchTaskExecutor.getInstance()
.setDelegate(object : TaskExecutor() {
override fun executeOnDiskIO(runnable: Runnable) = runnable.run()
override fun postToMainThread(runnable: Runnable) = runnable.run()
override fun isMainThread(): Boolean = true
})
val testScheduler = TestCoroutineScheduler()
Dispatchers.setMain(StandardTestDispatcher(testScheduler))
//Dispatchers.setMain(UnconfinedTestDispatcher(testScheduler))
//Dispatchers.setMain(TestCoroutineDispatcher())
val collectedStates = mutableListOf<DummyViewModel.State>()
val viewModel = DummyViewModel(
flow {
delay(100)
emit(3)
delay(100)
}
)
viewModel.stateAsLiveData.observeForever {
collectedStates.add(it)
}
viewModel.runTest()
testScheduler.advanceUntilIdle()
assertEquals(
listOf(
DummyViewModel.State(),
DummyViewModel.State(
isLoading = true,
data = null
),
DummyViewModel.State(
isLoading = true,
data = 3
),
DummyViewModel.State(
isLoading = false,
data = 3
),
),
collectedStates
)
Dispatchers.resetMain()
}
class DummyViewModel(
private val dataFlow: Flow<Int>
) : ViewModel() {
data class State(
val isLoading: Boolean = false,
val data: Int? = null
)
private val stateFlow = MutableStateFlow(State())
val stateAsLiveData = stateFlow.asLiveData()
fun runTest() {
dataFlow
.onStart { stateFlow.update { it.copy(isLoading = true) } }
.onCompletion { stateFlow.update { it.copy(isLoading = false) } }
.onEach { current -> stateFlow.update { it.copy(data = current) } }
.launchIn(viewModelScope)
}
}
}The relevant changed part is actually this: val viewModel = DummyViewModel(
flow {
delay(100)
emit(3)
delay(100)
}
)Adding a I find it ugly though, so whenever possible, I'd probably stick with my |
|
Thanks for sharing actual code, I guess I would use the test flow provided as For instance, use a channel to control when the source flow emits and completes: class MockSourceFlow<T> {
private val channel = Channel<T>()
val flow = channel.consumeAsFlow()
suspend fun simulateData(value: T) {
channel.send(value)
}
fun simulateCompletion(throwable: Throwable? = null) {
channel.close(throwable)
}
}And then force other coroutines to run before checking intermediate state updates: Click to expand the test implementationclass DummyTest {
@OptIn(ExperimentalCoroutinesApi::class)
@Test
fun dummyTest() = runTest {
ArchTaskExecutor.getInstance().setDelegate(object : TaskExecutor() {
override fun executeOnDiskIO(runnable: Runnable) = runnable.run()
override fun postToMainThread(runnable: Runnable) = runnable.run()
override fun isMainThread(): Boolean = true
})
Dispatchers.setMain(StandardTestDispatcher(testScheduler))
val testDataSource = MockSourceFlow<Int>()
val viewModel = DummyViewModel(testDataSource.flow)
viewModel.stateAsLiveData.observeForever {} // ensures the livedata actually runs
advanceUntilIdle()
assertEquals(DummyViewModel.State(), viewModel.stateAsLiveData.value)
viewModel.runTest()
advanceUntilIdle()
val expectedLoadingStateNoValue = DummyViewModel.State(
isLoading = true,
data = null
)
assertEquals(expectedLoadingStateNoValue, viewModel.stateAsLiveData.value)
testDataSource.simulateData(3)
advanceUntilIdle()
val expectedLoadingStateWithValue = DummyViewModel.State(
isLoading = true,
data = 3
)
assertEquals(expectedLoadingStateWithValue, viewModel.stateAsLiveData.value)
testDataSource.simulateCompletion()
advanceUntilIdle()
val expectedCompletedStateWithValue = DummyViewModel.State(
isLoading = false,
data = 3
)
assertEquals(expectedCompletedStateWithValue, viewModel.stateAsLiveData.value)
Dispatchers.resetMain()
}
class DummyViewModel(
private val dataFlow: Flow<Int>
) : ViewModel() {
data class State(
val isLoading: Boolean = false,
val data: Int? = null
)
private val stateFlow = MutableStateFlow(State())
val stateAsLiveData = stateFlow.asLiveData()
fun runTest() {
dataFlow
.onStart { stateFlow.update { it.copy(isLoading = true) } }
.onCompletion { stateFlow.update { it.copy(isLoading = false) } }
.onEach { current -> stateFlow.update { it.copy(data = current) } }
.launchIn(viewModelScope)
}
}
}This is what I mean by not brittle. I don't believe you should use immediate dispatchers to work around the expected behaviour of state flows, but rather make it explicit in the test that what you want is to assume other coroutines have progressed at this point in the test, and then assert. This also allows you to verify that the value you're expecting in the live data is actually there when you expect it to be. |
|
Indeed this approach does seem to make sense, although it is quite verbose. |
|
It is slightly longer indeed, but it also is more accurate. It will assert the proper things at the proper time, and when it fails you'll see exactly at which point, which can't be said with the single-list-assert approach. |
|
Hi! I'm the maintainer of the test module, so direct the criticisms here. Thank you @joffrey-bion for handling this! Everything's pretty much as you say. An official way to test channels, Adding delays to emulate context switches between different coroutines is also a fine approach, as is using A guiding principle for the new API was that tests should somehow reflect how the code would behave in production. In contrast, no actual dispatchers (I think?) behave exactly like The code in the opening post now fails, and this is intentional, because, if we look at it with fresh eyes, it should fail: one wouldn't observe this behavior in real life consistently, and one wouldn't observe it at all unless in a multithreaded dispatcher. All that said, we're open to introducing something with the behavior equivalent to |
dkhalanskyjb
changed the title
|
The master issue for this topic is now #3939, closing this one in its favor. |
Hi, I'm trying to test events posted to
MutableStateFlow. I'd like to see that order of posting of those events is correct. However, documentation of StateFlow states:https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-state-flow/
And so, this test:
@Test fun mutableStateFlowTest() = runTest(UnconfinedTestDispatcher()) { val testedFlow = MutableStateFlow(0) val testedValues = mutableListOf<Int>() testedFlow .onEach { testedValues.add(it) } .launchIn(this) flowOf(Unit) .onStart { testedFlow.update { it + 1 } } .onCompletion { testedFlow.update { it + 1 } } .onEach { testedFlow.update { it + 1 } } .launchIn(this) assertEquals( listOf(0, 1, 2, 3), testedValues ) }fails.
testedValuesis[0, 3]. That is, it contains only two values first value and the last.While this is understandable in the light of what the documentation says, I would still like to be able to check that all events were sent to the state flow in the order that I sent them.
But in the real application, I do like the conflation, because indeed it doesn't make sense to redraw UI after every single update to the state flow, if updates are faster than redrawing is.
The reason I'd like to check for the order of emitted events in tests is, if for nothing else, then at least for the fact, that if conflation can occur during tests, it makes tests unstable. A little change in code could mean, that some event will get conflated and the test will be broken, even if the change itself should not change which events are being emitted.
Is there any way to make the test above work? It works with the deprecated
TestCoroutineDispatcherandrunBlockingTest.I have tried this, which works:
But in practice, this is not very practical approach.
As a background, the example above is a very simplified example of something, that occurs in tests of our application. With the old
TestCoroutineDispatcherthose tests work correctly, collecting all the events, but with the new test dispatchers from1.6.0, tests are broken.I don't want to rewrite every single one of the test cases to account for these skipped values (it's almost 200 test cases that are broken) and as I said, I do think that in context of tests, all events should be recorded anyway, because if I account for the skipped events now, maybe later I might make a change, that will cause some of those skipped events appear again. That would be very brittle.
Thanks.
The text was updated successfully, but these errors were encountered: