-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Decide on the behavior of the test module's global CoroutineExceptionHandler when it's not the only handler #3738
Comments
Thanks for opening this issue, @dkhalanskyjb. In my case, when I've updated from It seems previously swallowed errors: When they are run individually they work fine. But they fail when run together. |
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 |
Totally agree, Dmitry. For example, if I have a project with more than 100 modules, be able to "open" 1 module at a time. |
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? |
Hi, are there any updates here? We're basically blocked because the unit tests constantly fail on the CI build. 😢 |
See #3736 (comment) 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. |
@dkhalanskyjb thanks for your suggestion 🙏
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 :) |
@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:
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. |
Almost a year later, given that no one contacted us with a use case, it seems like the temporary workaround was enough. |
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 ownCoroutineExceptionHandler
instances or override the defaultThread.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 oneCoroutineExceptionHandler
can be installed in theCoroutineContext
, and it's considered to process the exception, whereas when usingCoroutineExceptionHandler
as aServiceLoader
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.The text was updated successfully, but these errors were encountered: