Skip to content
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

Closed
dkhalanskyjb opened this issue May 3, 2023 · 9 comments
Labels

Comments

@dkhalanskyjb
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator Author

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
Copy link

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
Copy link

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
Copy link

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

@dkhalanskyjb
Copy link
Collaborator Author

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.

@Alexs784
Copy link

@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
Copy link
Collaborator Author

@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 Support disabling reporting of uncaught exceptions in tests #3736 (comment) 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
Copy link
Collaborator Author

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

@dkhalanskyjb dkhalanskyjb closed this as not planned Won't fix, can't repro, duplicate, stale Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants