-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Description
Describe the bug
When replacing the Main Dispatcher with a TestDispatcher, tests are not failing as expected.
I've provided an example below which showcases the issue.
When the Main Dispatcher is replaced, and I don't use runTest
, as it doesn't seem necessary, exceptions during that test get swallowed, and the test completes successfully.
There's some inconsistent behaviour though, and I'm not sure why:
- Sometimes the exception seems to be swallowed completely, and even a subsequent test does not fail. (1 & 2)
- Sometimes a subsequent test which does use
runTest
fails with anUncaughtExceptionsBeforeTest
(4 & 5)
I'm not sure if this is intended behaviour.
If it is, I suppose the main issue I have, is that it's not clear that runTest
should be used for all tests when the Main Dispatcher is replaced with a TestDispatcher;
Even if we're not actively interacting with Coroutines.
(no time control / not launching any jobs / not calling suspending functions ...)
One way this can become an issue without it being noticed:
Given a class with corresponding tests which already replaces the Main Scheduler via setup/teardown or a test rule
if a method of this class, which was not launching any coroutine job before, is changed to launch a coroutine,
the existing test(s) won't be using runTest
.
This may result in exceptions being thrown, but the test not failing.
Best case scenario, a later test fails, giving enough information to fix the actual cause.
Worst case scenario, this exception is not thrown anywhere at all, potentially leaving a bug in the code which should have gotten caught by the tests.
Provide a Reproducer
interface Dependency {
fun getSomething(): Boolean
}
private class Example(
private val dependency: Dependency
) {
private val coroutineScope by lazy {
CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate)
}
fun doSomething(fail: Boolean) {
coroutineScope.launch {
if (fail) throw Exception("fail")
}
}
fun doSomethingWithDependency() {
coroutineScope.launch {
dependency.getSomething()
}
}
}
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
class ExampleTest {
@get:Rule
val mockkRule = MockKRule(this)
private val dependencyMock: Dependency = mockk()
private val sut = Example(dependencyMock)
@Before
fun setUp() {
Dispatchers.setMain(UnconfinedTestDispatcher())
}
@After
fun tearDown() {
Dispatchers.resetMain()
}
@Test
fun `1 should fail but does not`() {
sut.doSomething(fail = true)
}
@Test
fun `2 succeeds - so above error is fully swallowed`() = runTest {
sut.doSomething(fail = false)
}
@Test
fun `3 fails as expected`() = runTest {
sut.doSomething(fail = true)
}
@Test
fun `4 should fail but does not`() {
sut.doSomethingWithDependency()
verify { dependencyMock.getSomething() }
}
@Test
fun `5 should succeed but does not - due to above error`() = runTest {
// kotlinx.coroutines.test.UncaughtExceptionsBeforeTest: There were uncaught exceptions before the test started. Please avoid this, as such exceptions are also reported in a platform-dependent manner so that they are not lost.
every { dependencyMock.getSomething() } returns true
sut.doSomethingWithDependency()
verify { dependencyMock.getSomething() }
}
}
Activity
dkhalanskyjb commentedon May 29, 2024
There's a potential for the documentation to be improved, but yes, it's not enough to replace the
Main
dispatcher with aTestDispatcher
: someone has to actually execute the tasks on that dispatcher (that is, something should emulate the event loop of the main thread), andrunTest
is the one doing it. It's certainly not enough to just override the main dispatcher, even if your coroutines don't throw exceptions: some tasks spawned on the main thread just won't run.In theory, you can use a
TestDispatcher
with aTestCoroutineScheduler
separately fromrunTest
, but this is a very advanced use case that requires a deep understanding, so their docs certainly should mentionrunTest
.SnyersK commentedon May 29, 2024
Would there be a way to detect if a test is not using
runTest
when required, and throw an exception, so it's immediately clear that this is missing?(Or an existing test needs to be updated, after a change)
I don't know if the
TestDispatcher
can perhaps check if it's running in aTestScope
, for example?dkhalanskyjb commentedon May 29, 2024
Unfortunately, no: injecting a
TestDispatcher
before aTestScope
exists orrunTest
is run is an important pattern. Dependency injection is typically initialized before the test is started.SnyersK commentedon May 30, 2024
Ah yes, I understand now.
Classes create their own / use another CoroutineScope.
There's no link between the TestScope and the coroutines launched from it's scope.
I've found that apparently there's a Detekt rule to check if tests which interact with coroutines make use of
runTest
.Will need to try this out.
https://github.com/detekt/detekt/blob/f8bf79b24baebd10fda01ffe2ef586cf8bc1fff1/detekt-core/src/main/resources/default-detekt-config.yml#L175-L178
SnyersK commentedon May 30, 2024
It's safe to assume that, as a general rule of thumb:
all of the tests in a test class which replaces the Main scheduler; and injects the TestDispatcher for all of the other dispatchers; should use
runTest
, to avoid running into potential (future) issues, even if the test itself is not directly interacting with a coroutine / using the time methods, right?So even if a test just calls a method which is not launching any coroutine at all, it would be safer to just use
runTest
regardless?dkhalanskyjb commentedon Aug 14, 2024
Yes, if you replace the
Main
dispatcher,runTest
should be used to ensure that tasks on that dispatcher get processed.