Decide on the behavior of the test module's global CoroutineExceptionHandler when it's not the only handler

[dkhalanskyjb]

To solve #1205, we changed runTest so that it installs a handler to catch all the exceptions flying through the coroutine machinery. However, it's not universally useful: in some elaborate scenarios, people have their own CoroutineExceptionHandler instances or override the default Thread.UncaughtExceptionHandler on the JVM. In these scenarios, we suddenly started intercepting exceptions that weren't in fact lost but were going to be processed further.

CoroutineExceptionHandler doesn't have the notion of priority, nor, currently, does it need one: only one CoroutineExceptionHandler can be installed in the CoroutineContext, and it's considered to process the exception, whereas when using CoroutineExceptionHandler as a ServiceLoader service, there's also no need for the notion of priority because, currently, all the handlers get notified, in addition to the platform-specific behavior always getting invoked.

Ideally, if a CoroutineExceptionHandler is installed already, the exception handler from the test module shouldn't get invoked, but it's unclear how to ensure that without special-casing the exact scenario.

[soygabimoreno]

Thanks for opening this issue, @dkhalanskyjb.

In my case, when I've updated from 1.6.4 to 1.7.0 there are many tests that are failing.

It seems previously swallowed errors:MockKException, UncompletedCoroutinesError, etc., are being triggered now.

When they are run individually they work fine. But they fail when run together.

[dkhalanskyjb]

The best course of action is to fix the exceptions that surfaced this way, not to prevent them from being reported. After all, you don't wrap the whole test suite in a try { ... } catch (e: Exception) { } (I hope), even though it would guarantee you 100% green tests all the time. Seeing the test failures is good, it shows you the errors in your code.

[soygabimoreno]

The best course of action is to fix the exceptions that surfaced this way, not to prevent them from being reported. After all, you don't wrap the whole test suite in a try { ... } catch (e: Exception) { } (I hope), even though it would guarantee you 100% green tests all the time. Seeing the test failures is good, it shows you the errors in your code.

Totally agree, Dmitry. The drawback in my case is having to update many many tests in the same PR. I would like to do it gradually.

For example, if I have a project with more than 100 modules, be able to "open" 1 module at a time.

[Alexs784]

I second what @soygabimoreno is mentioning. Plus I don't know about others, but since the errors happen only when running all the test together, is really hard for us to even understand what is the actual issue. We have even different tests failing if I re-run the same task (=run all unit tests) without any changes.

Any hints on how to approach this?

[kevinguitar]

Hi, are there any updates here? We're basically blocked because the unit tests constantly fail on the CI build. 😢

[dkhalanskyjb]

See https://github.com/Kotlin/kotlinx.coroutines/pull/3736#issuecomment-1542274701 for a workaround. This code will disable the new test framework behavior of catching arbitrary exceptions. Currently, it looks like this workaround is just that, a workaround that should be removed once the problematic tests are fixed.

Maybe it turns out this is not just a workaround and someone will need to disable the new test framework behavior permanently. If so, we will need to provide a better API. This issue is for us to collect the use cases for that, and "our tests are broken and there are too many of them to fix" is not that use case. For that purpose, a temporary ugly workaround is fine.

[Alexs784]

@dkhalanskyjb thanks for your suggestion 🙏 Regarding your comment, I agree in general, let me just clarify something:

This issue is for us to collect the use cases for that, and "our tests are broken and there are too many of them to fix" is not that use case.

I don't think we are just saying "our tests are broken and there are too many of them to fix". There are numerous reports here of users having tests running fine until the last update. That means something has changed, right? Perhaps a behavioural change? (like you indeed state at the beginning of the issue).

If you consider a large codebase with thousands of tests, you can imagine "our tests are broken and there are too many of them to fix" could be also read as "we relied on this library for a long time, following certain conventions/assumptions that are now broken and is unrealistic to fix potentially thousands of failing tests in one go".

Thanks again for looking into this :)

[dkhalanskyjb]

@Alexs784, okay, let me rephrase this. We're not trying to blame anyone. The fact is, there are two options between which we're trying to distinguish:

• Option 1: There is a legitimate use case where disabling our exception catching via https://github.com/Kotlin/kotlinx.coroutines/pull/3736#issuecomment-1542274701 is the best way to do something. If this is true, we have to do better than that nasty workaround, defining some proper API that some people would proudly invoke, knowing they are doing the right thing.
• Option 2: Maybe the need for this workaround is always a problem. Whether your tests were subtly broken and this is now exposed, or you are personally lazy and don't want to fix your tests, or you didn't treat flyby exceptions as problems until the change forced you to (but now you agree that they are not desirable), to us, this is all the same.

If your code base fits option 1, we want to learn about it. What if our exception-catching is not a universally good decision? On the other hand, if you agree that disabling our exception catching is just a workaround you'll be happy to get rid of eventually, you're already covered; the workaround is enough.

[dkhalanskyjb]

Almost a year later, given that no one contacted us with a use case, it seems like the temporary workaround was enough.