TestCoroutineDispatcher swallows exceptions

manuelvicnt commented 5 years ago

@julioyg found an interesting bug(?) regarding the TestCoroutineDispatcher.

The fact that runBlockingTest uses async to run the test body (TestBuilders.runBlockingTest#L49), by the time it tries to throw an exception, the test has already completed.

Problems: 1) Silent exceptions If the coroutine inside a function throws an exception, the test passes and the exception is thrown silently to the console.

2) Tests that expect an exception to be thrown Consider the following scenario. Since we're running this in a blocking way, the expectation is that the exception is thrown, but it is not.

class MyViewModel(): ViewModel() {
   fun throwException() {
        viewModelScope.launch {
            throw Exception("Not found")
        }
    }
}

class MyViewModelTest {

   @get:Rule
   var coroutinesTestRule = CoroutinesTestRule()

   @Test
   fun testExceptionThrown() = coroutinesTestRule.testDispatcher.runBlockingTest {
        val assertionThrown = false
        try {
            val viewModel = MyViewModel()
            viewModel.throwException()
        } catch(e: Exception) {
            assertionThrown = true
        }    
        assertTrue(assertionThrown)
   }
}

manuelvicnt commented 5 years ago

cc @objcode

objcode commented 5 years ago

Ah so this one is a bit weird - since you're not passing the exception handler from the TestCoroutineScope the exception is handled by viewModelScope (which I don't believe will surface it).

This might be more properly a bug against the viewModelScope implementation (allow the injection of parent scope) WDYT?

objcode commented 5 years ago

Two options here:

Add test-injection of uncaught exception handler to viewModelScope for a specific viewModel
Add a global uncaught exception handler on CoroutineScope (CoroutineScope.addGlobalUncaughtExceptionHandler) - this mirrors the thread APIs. However, it's really quite global (and note that it's not normal to set the thread version in test code because it breaks test parallelism)

yigit commented 5 years ago

I've not thought about the whole problem but there is a flaw in that test code.

class MyViewModel(): ViewModel() {
   fun throwException() {
        viewModelScope.launch {
            throw Exception("Not found")
        }
    }
}

You are calling a regular function and launching inside it. It is async, there is no reason for your test to wait for it to finish.

I don't think this has anything to do w/ uncaught exceptions. That test would never really work as intended.

For the view-model though, might be necessary for us to provide some side channels to access / swap the context / scope.

julioyg commented 5 years ago

I guess the problem is that the tests passes (printing the exception in the log) but the code in production crashes because the exception is not handled.

objcode commented 5 years ago

Agreed - this should definitely cause a test (or at least the test runner from a global uncaught exception) to fault via some mechanism.

manuelvicnt commented 5 years ago

I don't completely agree with the fact that the test is flawed @yigit . It's true that the example is taken to extremes but the same way you test the happy path of a coroutine, you should be able to test the sad path.

We've been testing happy paths forever. e.g. using LiveDataTestUtil to wait for an element to be present. The sad path should be as easy to test as the happy path.

objcode commented 5 years ago

Agreed, this does seem to violate surface expectations despite being "working as intended." Given how prevalent non-configurable test exception handlers have been in the current API designs (e.g. viewModelScope and liveData {}) it's likely to assume that this will be a recurring problem both in public APIs and normal usage.

Notably the following two things are true:

Since the TestCoroutineDispatcher ensures that the coroutine completes prior to the test, it is known that the exception happens during test execution.
Uncaught exceptions in a thread are silently logged by this JUnit4 configuration.

I'm starting to lean towards installing a Thread.setDefaultUncaughtExceptionHandler inside of runBlockingTest when executed on the JVM.

Pros:

Doesn't introduce a CoroutineScope.globalUncaughtException handler (I'm hesitant about adding another global without a clear well understood use case that can't be solved by Thread's uncaught exception handler)
Meets developer expectations of what happens when an exception is thrown during test execution.
No dependency on threading CoroutineContext correctly through tests and code under test to get the correct behavior

Cons:

Tests may not run in parallel
This won't solve the async {} and never await issue, though that may be considered a unsupported use case.

Looking at prior work (e.g. https://github.com/ReactiveX/RxJava/issues/5234), this sort of global is a common solution to async code + tests + exceptions on the JVM.

This code would look very similar to the TestCoroutineUncaughtExceptionHandler, but only be installed on the JVM via expect/actual and used inside of runBlockingTest. Should this be easy to disable for a specific invocation of runBlockingTest that prefers to run in parallel?

@Test
fun testWithExceptions() = runBlockingTest {
    // installed by default on the JVM
}

@Test
fun testWithoutExceptions_canBeParallelExecuted() = runBlockingTest(handleThreadExceptions=false) {
   // not installed on the JVM
}

Thought: I do not think it's a good idea to expose this as a public interface since it's not coroutine-specific (and easily recreated). Developers using TestCoroutineDispatcher or TestCoroutineScope to manually manage coroutines without runBlockingTest should also install their own appropriate solution.

Alternative: Provide a public interface to give a way to do this without runBlockingTest, it would look something like this:

@Test
fun testFoo() {
    rethrowAllUncaughtExceptions {
        // do stuff with TCD/TCS
    }
}

ZacSweers commented 5 years ago

I'm a little unfamiliar with coroutines internals, but after eyeballing https://github.com/Kotlin/kotlinx.coroutines/blob/69c26dfbcefc24c66aadc586f9b5129894516bee/kotlinx-coroutines-core/jvm/src/CoroutineExceptionHandlerImpl.kt I think the RxJava solutions in the linked thread would work here too. Hoping I'm understanding the thread above is about uncaught exceptions silently failing, and not race threading race conditions where the test is finishing before the async work does (forgive me if it is!).

The problem here is with JUnit, not any of the solutions itself (though maybe there's cancellation exceptions being bubbled up that are inherent to doing concurrency in tests, I unfortunately wasn't sure if that was happening in the examples above). JUnit's default exception handler will silently accept and disregard exceptions, which is why the test doesn't fail.

There's a few solutions

Test rule

You could install a test rule that installs a global errors hook on test start and removes it on test end. It would record unhandled errors and, in successful tests, check to make sure they're empty.

At Uber we installed an "RxErrorsRule" into the base test class that every test in the Android monorepo used, and it worked extremely well. There was no opt-in or opt-out, just flat enabled for everyone. There was an API on the rule to "take" expected exceptions during the test.

This relies on having the ability to install a global errors hook (not sure if coroutines has a global solution like this)

Example implementation: https://github.com/uber/AutoDispose/blob/master/test-utils/src/main/java/com/uber/autodispose/test/RxErrorsRule.java

Global run listener

This doesn't work in gradle currently (or at least I've been woefully unsuccessful), but JUnit has a higher level API for "run listeners" where you could install a global exception handler. OkHttp had an implementation of this prior to its move to gradle: https://github.com/square/okhttp/blob/9042a2c2436c7acfd384f2726a5c1a84ae1145f8/okhttp-testing-support/src/main/java/okhttp3/testing/InstallUncaughtExceptionHandlerListener.java

I tried making it work in gradle here but never got much progress https://github.com/ZacSweers/okhttp/tree/z/testingJunitFoundation

Localized

Basically @objcode's example above:

@Test
fun testFoo() {
    rethrowAllUncaughtExceptions {
        // do stuff with TCD/TCS
    }
}

Is opt-in, creates a custom uncaught handler for the scope of that execution.

I'd recommend the base test class + rule approach for maximum flexibility (having an API to expect exceptions), and frankly feasibility since the runlistener approach wasn't working in gradle.

fwiw - I think the blanket global handler is a cleaner solution since it doesn't have an API. Having an API to expect them sometimes led to tests misusing it to assert implementation details/control flow rather than just use it sparingly for unavoidable cases. I had started trying to do a general case via adapting the okhttp runlistener into a general test rule at Uber before I left, but never finished. Kinda of the opinion that JUnit should do that in general, but 🤷‍♂.

julioyg commented 5 years ago

Yeah, thanks, I guess then it's nothing to do with coroutines but the way JUnit works, thanks for that.

I believe for my use case I could create that rule to install setDefaultUncaughtExceptionHandler before tests when we need it, as I'm not using async in my code, but I guess we will only do that if we expect that code to error, my concern then is: what if we don't expect it to error?

Don't really know what the best option would it be tho... Dispatchers.setMain() (I know that's part of the coroutines lib) seems to do the job to override the dispatcher... maybe the option of a CoroutineScope.globalUncaughtException is the best idea? 🤷‍♂

manuelvicnt commented 5 years ago

Thread.setDefaultUncaughtExceptionHandler doesn't do anything when installed. You can print the exception in a nicer way if you want but the test will silently pass anyway.

Re CoroutineScope.globalUncaughtException, there will be a lot of thoughts to put into this if it becomes a thing such as: how global is the globalUncaughtException, is this only for tests?, etc.

I like the idea of combining the Global run listener into a Rule as @ZacSweers suggested but seems like it's not an easy task? What's the issue with Gradle?

ZakTaccardi commented 5 years ago

I don't use viewModelScope, but I believe it will not be a child scope of the scope emitted by runBlockingTest { }, which creates this problem?

I avoid using ViewModelScope directly and allow injection of my own CoroutineScope into every ViewModel of mine to avoid issues like this.

audkar commented 4 years ago

Why ViewModel uses SupervisorJob instead of Job here?

yigit commented 4 years ago

@audkar , we debated a lot about whether to use a supervisor or not, at then end, decided to go w/ it because VM cannot destroy itself so in the case where VM is still alive but the scope is cancelled, it would be a very confusing inconsistent situation. also, developer is likely to launch unrelated stuff in that scope, just because one is cancelled usually does not mean desire to cancel the other one.

@ZakTaccardi VM uses the Main dispatcher so i think it would work fine as long as you swap the main dispatcher w/ the test dispatcher. I've not tried though.

audkar commented 4 years ago

VM cannot destroy itself so in the case where VM is still alive but the scope is cancelled

Sorry but I don't follow. In what case this would happen if Job would be used?

also, developer is likely to launch unrelated stuff in that scope, just because one is cancelled usually does not mean desire to cancel the other one.

Hmmm. Developers who uses coroutines should know how scopes works. Silently catching exceptions isn't expected behavior IMO. And this ticket itself shows that it is not expected behavior.

yigit commented 4 years ago

not sure what you mean by that, we do not catch exceptions silently.

class MainActivity : AppCompatActivity() {
    val viewModel by viewModels<MyViewModel>()
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        viewModel.crashForReal()
    }
}

class MyViewModel : ViewModel() {
    fun crashForReal() {
        viewModelScope.launch {
            throw RuntimeException("i fail, app fails")
        }
    }
    fun cancelNormally() {
        viewModelScope.launch {
            throw CancellationException("cancel normally")
        }
    }
}

If you run the code above, app will immediately crash on a real exception vs a cancelling coroutine will not crash the app as you can cancel a coroutine maybe to just start another work because it is not valid anymore.

iRYO400 commented 4 years ago

So what we get here?

premnirmal commented 4 years ago

+1 to the bug found by @julioyg and @manuelvicnt, seeing similar behaviour here. Why is the exception logged but not re-thrown to fail the test?

premnirmal commented 4 years ago

Just an FYI - the viewModelScope doesn't swallow the exceptions, it throws them. It is the runBlockingTest call that is swallowing the exception

audkar commented 4 years ago

Will application crash if you execute such code on JVM (not Android)?

CoroutineScope(SupervisorJob()).launch {
  throw IllegalArgumentException("Error")
}

I don't think that JVM Application will crash. Only error message will be printed out. https://pl.kotl.in/G8VIg_Ys4 So I don't see how this is related to runBlockingTest at all

premnirmal commented 4 years ago

why does

Will application crash if you execute such code on JVM (not Android)?
CoroutineScope(SupervisorJob()).launch {
  throw IllegalArgumentException("Error")
}
I don't think that JVM Application will crash. Only error message will be printed out. https://pl.kotl.in/G8VIg_Ys4 So I don't see how this is related to runBlockingTest at all

Does the coroutines library come with a default exception handler when run in the JVM?

Audhil commented 3 years ago

is this the final way to write a test case, for error case?

@Test fun testExceptionThrown() = coroutinesTestRule.testDispatcher.runBlockingTest { val assertionThrown = false try { val viewModel = MyViewModel() viewModel.throwException() } catch(e: Exception) { assertionThrown = true } assertTrue(assertionThrown) }

budius commented 3 years ago

@audkar , we debated a lot about whether to use a supervisor or not, at then end, decided to go w/ it because VM cannot destroy itself so in the case where VM is still alive but the scope is cancelled, it would be a very confusing inconsistent situation. also, developer is likely to launch unrelated stuff in that scope, just because one is cancelled usually does not mean desire to cancel the other one.

@ZakTaccardi VM uses the Main dispatcher so i think it would work fine as long as you swap the main dispatcher w/ the test dispatcher. I've not tried though.

Hi @yigit I see this thread is old, but still open, so maybe I can suggest a solution around that probably going to suit well as the Android team seems to be really invested in coroutine/Flows

I did a very quick test here adding this to a helloWorld-viewModel:

internal var otherScope: CoroutineScope? = null
fun ViewModel.findScope() :CoroutineScope = otherScope ?: viewModelScope

And used this findScope() in the model, and injecting the TestCoroutineScope in the test code and it behaves as expected.

With that in mind, I wonder if it would be a valid approach for the AndroidX / ViewModel team to release a ViewModelTestCoroutineDispatcherRule : TestWatcher that could be used similar to the InstantTaskExecutorRule.

ghost commented 3 years ago

Sorry, people! I have not found CoroutineScope.globalUncaughtException. I have rule and want to add Zac's suggestion. If I set thread unhandled exception handler it will fail the test but exception will be wrapped which is not ideal also.

Antimonit commented 3 years ago

This was causing us issues for a long time and today I've decided to fix it.

Observation: Whenever an exception is thrown within viewModelScope.launch during a test, the exception is intercepted, logged as an error, but does not fail the test. The test either fails at some point later due to an inconsistent state or in worse cases it even passes the test. In our case, this mainly pertains to exceptions caused by incorrect test logic, such as io.mockk.MockKException: no answer found for: Xxx, etc.

From my brief testing, this is not a fault of TestCoroutineDispatcher.runBlockingTest as the same behavior can be observed with runBlockingTest (the one without receiver), runBlocking (that does not automatically progress time), or without any blocking whatsoever (with Main or Main.immediate dispatcher).

Also, the implementation of viewModelScope is not at fault. This can be replicated even without the use of ViewModel at all. Running with GlobalScope or custom CoroutineScope(context) will exhibit the same behavior.

Also, it does not matter whether Job() or SupervisorJob() is used as the context of the coroutine scope.

Also, it does not matter what dispatcher is used. For Main and Main.immediate dispatchers, I did use CoroutinesTestRule(). For non-Main dispatchers, I used some form of runBlocking and delay(1000) to make sure the coroutine finishes before the test finishes.

Now, the very minimal reproducible example would look like

class CoroutineExceptionTest {

    @Test
    fun checkScope() {
        val scope = CoroutineScope(Dispatchers.Unconfined)
        scope.launch {
            throw Exception("Oh noes")
        }
    }
}

Executing the test, you can see the stacktrace in the log like

Exception in thread "main @coroutine#1" java.lang.Exception: Oh noes
    at me.khol.playground.CoroutineExceptionTest$checkScope$1.invokeSuspend(CoroutineExceptionTest.kt:19)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
    at kotlinx.coroutines.internal.DispatchedContinuationKt.resumeCancellableWith(DispatchedContinuation.kt:377)
    at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable(Cancellable.kt:30)
    at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable$default(Cancellable.kt:25)
    at kotlinx.coroutines.CoroutineStart.invoke(CoroutineStart.kt:110)
    at kotlinx.coroutines.AbstractCoroutine.start(AbstractCoroutine.kt:126)
    at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch(Builders.common.kt:56)
    at kotlinx.coroutines.BuildersKt.launch(Unknown Source)
    at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch$default(Builders.common.kt:47)
    at kotlinx.coroutines.BuildersKt.launch$default(Unknown Source)
    at me.khol.playground.CoroutineExceptionTest.checkScope(CoroutineExceptionTest.kt:18)

but the test passes nonetheless.

As mentioned by the great Roman Elizarov here, the same behavior can be observer when using plain threads:

    @Test
    fun checkThread() {
        val job = thread {
            throw IllegalStateException("why does this exception not fail the test?")
        }
        job.join()
    }

prints

Exception in thread "Thread-0" java.lang.IllegalStateException: why does this exception not fail the test?
    at me.khol.playground.CoroutineExceptionTest$checkThread$job$1.invoke(CoroutineExceptionTest.kt:27)
    at me.khol.playground.CoroutineExceptionTest$checkThread$job$1.invoke(CoroutineExceptionTest.kt:11)
    at kotlin.concurrent.ThreadsKt$thread$thread$1.run(Thread.kt:30)

but also passes the test.

Attempting to job.join() the job does not fail the test either.

I was not lucky using a CoroutineExceptionHandler either.

Solution 1

As mentioned in the same answer

If you want exception to propagate to the test, you shall replace launch with async and replace join with await in your code.

Indeed, running

@Test
fun checkAsync() {
    val scope = CoroutineScope(Dispatchers.Unconfined)
    val deferred = scope.async {
        throw Exception("Oh noes")
    }
    runBlocking {
        deferred.await()
    }
}

will fail the test with the expected exception, but now we have another problem - how to propagate the Deffered back to test so that we can await() it. This can be quite intrusive in an existing project and I don't even know how to solve this issue if we launch a coroutine in a constructor of a class that does not have a return value.

Solution 2

We can install a DefaultUncaughtExceptionHandler.

@Test
fun checkDefaultUncaughtExceptionHandler() {
    // TODO: Create a Rule and reset the DefaultUncaughtExceptionHandler
    //  to the original value so that it does not leak to other tests.
    Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
        throw throwable
    }
    val scope = CoroutineScope(Dispatchers.Unconfined)
    scope.launch {
        throw Exception("Oh noes")
    }
}

This will crash fail the test. Can be extracted into a Rule without the need to modify existing code. Exception stacktrace reports a wrapped exception though and can change the expected result of other, well-behaved tests. This is a similar approach as employed in okhttp3.testing.InstallUncaughtExceptionHandlerListener.

Solution 3

Although we cannot fail the test directly from a CoroutineExceptionHandler, we can keep the exception around and assert that no exception was thrown in our coroutine scope.

@Test
fun checkCoroutineExceptionHandler() {
    var failed: Throwable? = null
    val handler = CoroutineExceptionHandler { _, throwable ->
        failed = throwable
        throw throwable
    }

    val scope = CoroutineScope(Dispatchers.Unconfined)
    scope.launch(handler) {
        throw Exception("Oh noes")
    }
    failed?.let { throw it }
}

Requires to pass a handler as a context to the top-level launch. Reports the expected exception only, no wrapper exception. A somewhat similar approach is used in autodispose2.test.RxErrorsRule.

Solution 4

We can achieve the same thing as in solution 3 in a much cleaner way using experimental API

@Test
fun checkTestCoroutineExceptionHandler() {
    val testHandler = TestCoroutineExceptionHandler()
    val scope = CoroutineScope(Dispatchers.Unconfined)
    scope.launch(testHandler) {
        throw Exception("Oh noes")
    }
    testHandler.cleanupTestCoroutines()
}

Solution 5

Similarly to solution 4, we can use an experimental TestCoroutineScope. In addition to TestCoroutineExceptionHandler, the TestCoroutineScope also uses TestCoroutineDispatcher under the hood - the same one provided by CoroutinesTestRule.

@Test
fun checkTestCoroutineScope() {
    val scope = TestCoroutineScope()
    scope.launch {
        throw Exception("Oh noes")
    }
    scope.cleanupTestCoroutines()
}

lukasz-kalnik-gcx commented 3 years ago

If you want to just use a JUnit 4 test rule or JUnit 5 test extension in your tests, you can find it in this gist

Secculent commented 2 years ago

@Antimonit thanks for this response, it's really helpful

However, I'm experiencing an issue with Solution 2, I'm not quite sure if it's something related to coroutine's mechanics or a straight up bug.

Running exact same snippet as provided by You, leaves me with

Job "coroutine#1":StandaloneCoroutine{Cancelling}@3c06bf1d is already complete or completing, but is being completed with CompletedExceptionally[kotlinx.coroutines.CoroutinesInternalError: Fatal exception in coroutines machinery for DispatchedContinuation[Dispatchers.Unconfined, Continuation at RandomTest$checkDefaultUncaughtExceptionHandler2$2.invokeSuspend(RandomTest.kt)@7e1b045]. Please read KDoc to 'handleFatalException' method and report this incident to maintainers]

I can work around it by changing the code to something like that:

@Test(expected = IOException::class)
    fun checkDefaultUncaughtExceptionHandler() {
        var exception: Throwable? = null
        Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
            exception = throwable
        }
        val scope = CoroutineScope(Dispatchers.Unconfined)
        scope.launch {
            throw IOException("Oh noes")
        }
        exception?.let { throw it }
    }

Is this an expected behaviour?

My Kotlin/coroutines versions:

kotlin("jvm") version "1.5.10"
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.5.2")

drinkthestars commented 2 years ago

Seeing this happen with 1.6.0 and runTest as well:

class FooViewModelTest {

   @get:Rule
   var coroutinesTestRule = CoroutinesTestRule() // StandardTestDispatcher

   @Test
   fun shouldFailButDoesnt() = runTest {
        val viewModel = FooViewModel()
        viewModel.throwException()
        runCurrent()
   }
}

// ViewModel
class FooViewModel(): ViewModel() {
   fun throwException() {
        viewModelScope.launch {
            throw Exception("Oops!")
        }
    }
}

Should we be creating ExceptionHandlers somewhere to mitigate this?

Edit: let me know if this should be created as part of a separate issue since it is runTest.

ephemient commented 2 years ago

viewModelScope uses Dispatchers.Main.immediate which is not TestDispatcher. #3298

dkhalanskyjb commented 2 years ago

@ephemient, not quite. Dispatchers.Main.immediate is a TestDispatcher, it just doesn't have the correct properties. Fixing #3298 would fix this particular example, but not every case. So, the issue is still present, that's why we didn't close this one.

In general, the reproducer looks like this:

    @Test
    fun unreportedException() = runTest {
        val scope = CoroutineScope(EmptyCoroutineContext)
        scope.launch {
            throw RuntimeException()
        }
    }

It has several aspects:

We don't know when a coroutine is going to finish. In the provided example the exception may even be thrown after the test has finished. To solve this, one should either join the newly-created coroutines, or inject the test dispatcher to ensure their completion.
Even if we ensure that the dispatcher is correctly injected (like in the example by @drinkthestars), the dispatcher itself only controls the dispatching behavior, not how the exceptions are propagated.

The problem is that the exceptions are reported via the structured concurrency mechanism: coroutines that have a chain of inheritance with the TestScope do have their exceptions reported, and those that don't, don't. So, GlobalScope.launch { throw RuntimeException("...") }, for example, will not be caught by us.

Passing the CoroutineExceptionHandler to the coroutine looks ugly, but it helps:

    @Test
    fun reportedException() = runTest {
        val scope = CoroutineScope(coroutineContext[CoroutineExceptionHandler]!!)
        val job = scope.launch {
            throw RuntimeException()
        }
        job.join()
    }

Some of the solutions listed by @Antimonit still hold up:

Solution 1:

    @Test
    fun reportedException() = runTest {
        val scope = CoroutineScope(EmptyCoroutineContext)
        val deferred = scope.async {
            throw RuntimeException()
        }
        deferred.await()
    }

Solution 2 (JVM only):

    @Test
    fun reportedException() = runTest {
        Thread.setDefaultUncaughtExceptionHandler { thread, throwable ->
            throw throwable
        } // can be extracted into a rule
        val scope = CoroutineScope(StandardTestDispatcher(testScheduler))
        scope.launch {
            throw RuntimeException()
        }
    }

Solution 3 is also fine, but in the presence of the guaranteed CoroutineExceptionHandler in runTest, it should be easier to just access that.

The recommended way when possible is to use structured concurrency, that is, to ensure that the scopes you create are children of TestScope. It doesn't look like it's possible with viewModelScope, however.

lukaszkalnik commented 2 years ago

It doesn't look like it's possible with viewModelScope, however.

Wouldn't the solution be to add CoroutineContext to the viewModelScope?

class MyViewModel(
    coroutineContext: CoroutineContext = Dispatchers.Default
) : ViewModel() {
    val scope = viewModelScope + coroutineContext
}

And then in test:

runTest {
    val myViewModel = MyViewModel(coroutineContext)
}

ephemient commented 2 years ago

If you're going down that path, I'd rather something like

import viewModelScope as viewModelScope_
class MyViewModel(
    viewModelScope: CoroutineScope? = null,
) {
    val viewModelScope = viewModelScope ?: viewModelScope_

so that you can't accidentally use the wrong viewModelScope within the VM, and also make it easier to use a subscope.

dkhalanskyjb commented 2 years ago

In any case, this wouldn't help the (for example, third-party) code that hardcodes using viewModelScope, even with the name-shadowing trick, because this would not go through a dynamic dispatch, and instead will statically resolve to the extension value.

Also, even if you control all the code, whether or not the solution is well-structured depends on which coroutineContext you pass: to do this properly, you would need to establish a Job hierarchy between the passed context and TestScope. Also, you would need to be careful to pass a SupervisorJob, as otherwise, you would change the behavior of viewModelScope.

A much easier approach would not use structured concurrency and only pass the coroutineContext[CoroutineExceptionHandler]!! as the coroutineContext in your examples. This would lead to the exceptions being reported.

drinkthestars commented 2 years ago

Thanks @dkhalanskyjb for the explanations

I was confused about this:

coroutines that have a chain of inheritance with the TestScope do have their exceptions reported, and those that don't, don't

These exceptions (in viewModelScope/GlobalScope/someOtherScope) would otherwise crash during regular execution - what is it about not inheriting from TestScope that's preventing them from "crashing" the unit tests?

Is this a larger problem where if one is using some 3rd party library with thirdPartyScope, then unit tests can't ordinarily catch cases where exceptions get thrown in thirdPartyScope? If so, and if one is doing something with CoroutineExceptionHandler to get exceptions to fail tests (or some other strategy), then it would have to be the norm for all unit tests using a thirdPartyScope - doesn't seem trivial.

ryanholden8 commented 2 years ago

@drinkthestars That's a good way to put it:

These exceptions (in viewModelScope/GlobalScope/someOtherScope) would otherwise crash during regular execution - what is it about not inheriting from TestScope that's preventing them from "crashing" the unit tests?

Can we ask for some clarification on @drinkthestars's point? A basic view model test has become rather complex and our team is wondering how we can have reliable tests without writing all our view models to inject a scope.

The approaches here seem out of date with Google's documentation because, if I'm understanding correctly, the approach here is that shouldn't use viewModelScope directly to have reliable tests. However, Google's docs state we should use Dispatchers.setMain to override viewModelScope - https://developer.android.com/kotlin/coroutines/test#setting-main-dispatcher

To put it simply by using runTest - exceptions are swallowed where they otherwise would not be. Can runTest not introduce the swallowing of exceptions?

ryanholden8 commented 2 years ago

In case it's helpful, here's the helper method that we wrote to replace runTest with runReliableTest:

import androidx.lifecycle.ViewModel
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext

/**
 * The default [runTest] will swallow thrown [Throwable]s that occur within
 * [ViewModel].viewModelScope.launch calls. Thus, you may have [Throwable]s being thrown
 * and and yet passing unit tests. This helper method works around the issue by listening to the
 * Thread's UncaughtExceptionHandler and propagating the exception to fail the unit test.
 *
 * [This has been a known issue for the last 3 years.](https://github.com/Kotlin/kotlinx.coroutines/issues/1205)
 */
fun runReliableTest(
    context: CoroutineContext = EmptyCoroutineContext,
    dispatchTimeoutMs: Long = 30_000L,
    testBody: suspend TestScope.() -> Unit
) = runTest(context, dispatchTimeoutMs) {
    // Capture exceptions that may occur in `testBody()` since `runTest` will swallow
    // them if the exception occurs within another scope even though all dispatchers
    // should run without any delays - https://github.com/Kotlin/kotlinx.coroutines/issues/1205
    var error: Throwable? = null
    Thread.setDefaultUncaughtExceptionHandler { _, throwable -> error = throwable }

    // Run our actual test code
    testBody()

    // Throw exception here, otherwise it will be wrapped and hard to read if thrown from
    // within `Thread.setDefaultUncaughtExceptionHandler`
    error?.let { throw it }
}

qwwdfsad commented 2 years ago

(@dkhalanskyjb is on the vacation right now, so expect delayed responses here)

drinkthestars commented 2 years ago

Going back in the thread, came across this example where the exception would be printed but passes the test:

    @Test
    fun checkThread() {
        val job = thread {
            throw IllegalStateException("why does this exception not fail the test?")
        }
        job.join()
    }

Looks like runReliableTest() from your snippet above @ryanholden8 would solve this case as well?

ryanholden8 commented 2 years ago

@drinkthestars Yes, I tested locally and runReliableTest does fail the unit test.

import org.junit.Test
import kotlin.concurrent.thread

class ThreadTest {

    @Test
    fun checkThread() = runReliableTest {
        val job = thread {
            throw IllegalStateException("why does this exception not fail the test?")
        }
        job.join()
    }
}

mhernand40 commented 2 years ago

FWIW, there are still plenty of other areas outside of Coroutines where unhandled exceptions can be thrown and the unit test still "passes" even though errors may be logged to the console. I worked around this by applying the suggestion from this article and it's working great. It surfaced a decent number of unit tests that were "passing" but throwing unhandled exceptions.

dkhalanskyjb commented 2 years ago

I see some discontent in this discussion and so feel the need to clarify: in general, this problem is not with our library. There may be some specific cases where we could improve the situation (and we want to do so), which is why the issue is still not closed, but in general, this is certainly not our responsibility.

For example, https://github.com/Kotlin/kotlinx.coroutines/issues/1205#issuecomment-1209788685 here, our library is not used at all, but the issue is still present. Here's a discussion of the same issue for JUnit in general: https://stackoverflow.com/questions/36648317/how-to-capture-all-uncaucht-exceptions-on-junit-tests

These exceptions (in viewModelScope/GlobalScope/someOtherScope) would otherwise crash during regular execution - what is it about not inheriting from TestScope that's preventing them from "crashing" the unit tests?

When JUnit runs methods marked with @Test, it guarantees that the test will crash if the code inside the test body crashes. However, when you ask GlobalScope to execute some code, that code does not execute in the test body. Instead, the code is just being run on some other thread and doesn't know how to notify anyone about the exception. So, this thing is called: https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.UncaughtExceptionHandler.html, but by default (Java's, not ours), it just logs the exception instead of crashing the program.

To put it simply by using runTest - exceptions are swallowed where they otherwise would not be. Can runTest not introduce the swallowing of exceptions?

Not true. In the listed cases, runTest does not lose exceptions, it just does not go out of its way to grab every exception flying through the system. So, Java's default behavior of logging the exception is what happens instead.

runReliableTest

The solution that @mhernand40 very helpfully linked us to in https://github.com/Kotlin/kotlinx.coroutines/issues/1205#issuecomment-1232015300 is more robust: even if exceptions happen after the test already finished, they will still be reported, which is not the case with runReliableTest. However, if you either

can't make such intrusive changes to your build and have to work from inside the code, or
have some other UncaughtExceptionHandler that other tests use,

this still can be improved a bit:

/**
 * Runs a function, making sure that the exceptions that happen throughout its execution get reported.
 */
fun<T> runReliably(
    testBody: () -> T,
): T {
    val oldHandler = Thread.getDefaultUncaughtExceptionHandler()
    val errors = Vector<Throwable>()
    try {
        Thread.setDefaultUncaughtExceptionHandler { _, throwable -> errors.add(throwable) }
        val result = try {
            testBody()
        } catch (testError: Throwable) {
            for (e in errors) {
                testError.addSuppressed(e)
            }
            throw testError
        }
        errors.firstOrNull()?.apply {
            errors.drop(1).forEach { addSuppressed(it) }
            throw this
        }
        return result
    } finally {
        Thread.setDefaultUncaughtExceptionHandler(oldHandler)
    }
}

/**
 * Run a coroutine test, making sure the uncaught exceptions that happen during the execution of the test do get reported.
 */
fun runReliableTest(
    context: CoroutineContext = EmptyCoroutineContext,
    dispatchTimeoutMs: Long = 30_000L,
    testBody: suspend TestScope.() -> Unit
) = runReliably {
    runTest(context, dispatchTimeoutMs, testBody)
}

This has the advantage that, if several exceptions happen, all of them get reported, and also, the exception handler gets reset after the test; without that, the uncaught exceptions from subsequent tests that don't use this function will not even be logged.

I should note that providing runReliableTest instead of runTest by default is certainly not something we should do. For example, look here: https://grep.app/search?q=setDefaultUncaughtExceptionHandler A lot of people set the default uncaught exception handler to something else that makes sense for their use case. Silently overriding something that they rely on, in a coroutine test framework, just because JUnit does not do it, is out of the question.

I see a way right now to improve this experience, and that is to detect from the core coroutine library that the test module is used, and in this case, collect all the uncaught exceptions in addition to reporting them to the default uncaught exception handler, and then, when running tests, check if some exceptions were collected. This would automagically make the majority of tests shown here catch problems as intended, I think, but this needs research and careful testing (testing a test framework is also good entertainment, highly recommend).

Kotlin / kotlinx.coroutines