In the test module, collect and report the actual uncaught exceptions #3449

dkhalanskyjb · 2022-09-12T14:28:59Z

An attempt to mitigate #1205.

dkhalanskyjb · 2022-09-12T14:31:40Z

A draft because it's untested and I'm not sure about the whole ANDROID_DETECTED thing (in a separate commit). @qwwdfsad, what do you think of the general approach?

qwwdfsad

I'm not really happy with the approach, but it seems like we have no other (reasonable) options.

I'll revisit it this week along with an evaluation of user requests

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt

qwwdfsad

Otherwise LGTM

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt

dkhalanskyjb · 2022-12-27T14:23:41Z

@qwwdfsad, WDYT about this big rework? This is much less intrusive than the previous version, but it is still a bit unpleasant due to CoroutineExceptionHandler not being flexible enough. I described my thoughts about the underlying issue with CoroutineExceptionHandler here: #3554

qwwdfsad

I like the idea in general.

I'll take a final look after the rename and I believe it is good to go

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt

…utines`: common files are named as `Foo.common.kt`, while platform-specific files are named `Foo.kt`. See: Kotlin/kotlinx.coroutines#3449 (comment) Signed-off-by: Artyom Shendrik <[email protected]>

qwwdfsad · 2023-02-06T10:15:32Z

Let's finalize and merge it?

qwwdfsad · 2023-02-17T17:48:08Z

(preliminary looks good, will take a one final look when #3603 is merged)

qwwdfsad · 2023-02-21T18:01:49Z

Could you please rebase this one on the recent develop?

dkhalanskyjb · 2023-02-22T11:21:53Z

Sure, done.

qwwdfsad

LGTM module issues found.

While working on that, a few iterations were done, and it's easy to forget the reasning for the final solution over time. Issue fixed is also quite heavy on the discussion and potential solutions. While squashing, could you please provide an extensive commit message with the explanation of refactoring's rationale and problem solved?

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.common.kt

qwwdfsad · 2023-02-22T16:44:05Z

...coroutines-test/jvm/resources/META-INF/services/kotlinx.coroutines.CoroutineExceptionHandler

@@ -0,0 +1 @@
+kotlinx.coroutines.test.internal.ExceptionCollectorAsService


This one should have the corresponding provides in module-info.java

qwwdfsad · 2023-02-22T16:44:20Z

kotlinx-coroutines-test/common/src/internal/ExceptionCollector.kt

+ * The callbacks in this object are the last resort before relying on platform-dependent
+ * ways to report uncaught exceptions from coroutines.
+ */
+internal object ExceptionCollector: AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler {


Suggested change

internal object ExceptionCollector: AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler {

internal object ExceptionCollector : AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler {

qwwdfsad · 2023-02-22T16:50:31Z

kotlinx-coroutines-test/common/src/TestScope.kt

@@ -251,6 +259,8 @@ internal class TestScopeImpl(context: CoroutineContext) :
    fun legacyLeave(): List<Throwable> {
        val exceptions = synchronized(lock) {
            check(entered && !finished)
+            /** After [finished] becomes `true`, it is no longer valid to have [reportException] as the callback. */
+            ExceptionCollector.removeOnExceptionCallback(lock)


Seems like callbacks are never removed for non-legacy version, consuming the memory lineary to number of tests run, could you please address that?

Oh. You're right, of course. I simply rebased the code on develop but totally forgot that the leave function has split in two and needs changes in both places.

When when a `Job` doesn't have a parent, failures in it can not be reported via structured concurrency. Instead, the mechanism of unhandled exceptions is used. If there is a `CoroutineExceptionHandler` in the coroutine context or registered as a `ServiceLoader` service, this gets notified, and otherwise, something platform-specific happens: on Native, the program crashes, and on the JVM, by default, the exception is just logged, though this is configurable via `Thread.setUncaughtExceptionHandler`. With tests on the JVM, this is an issue: we want exceptions not simply *logged*, we want them to fail the test, and this extends beyond just coroutines. However, JUnit does not override the uncaught exception handler, and uncaught exceptions do just get logged: <https://stackoverflow.com/questions/36648317/how-to-capture-all-uncaucht-exceptions-on-junit-tests> This is a problem with a widely-used `viewModelScope` on Android. This is a scope without a parent, and so the exceptions in it are uncaught. On Android, such uncaught exceptions crash the app by default, but in tests, they just get logged. Clearly, without overriding the behavior of uncaught exceptions, the tests are lacking. This can be solved on the test writers' side via `setUncaughtExceptionHandler`, but one has to remember to do that. In this commit, we attempt to solve this issue for the overwhelming majority of users. To that end, when the test framework is used, we collect the uncaught exceptions and report them at the end of a test. This approach is marginally less robust than `setUncaughtExceptionHandler`: if an exception happened after the last test using `kotlinx-coroutines-test`, it won't get reported, for example. `CoroutineExceptionHandler` is designed in such a way that, when it is used in a coroutine context, its presence means that the exceptions are safe in its care and will not be propagated further, but when used as a service, it has no such property. We, however, know that our `CoroutineExceptionHandler` reports the exceptions properly and they don't need to be further logged, and so we had to extend the behavior of mechanism for uncaught exception handling so that it the handler throws a new kind of exception if the exception was processed successfully. Also, because there's no `ServiceLoader` mechanism on JS or Native, we had to refactor the whole uncaught exception handling mechanism a bit in the same vein as we had to adapt the `Main` dispatcher to `Dispatchers.setMain`: by introducing internal setter APIs that services have to call manually to register in. Fixes #1205 as thoroughly as we can given the circumstances.

dkhalanskyjb · 2023-02-23T13:07:23Z

I squashed the commits into one and wrote a detailed commit message. Did you have something like that in mind?

qwwdfsad · 2023-02-23T16:20:13Z

Exactly, nicely done!

With #3449, the unrelated uncaught exceptions can also be caught in this manner.

…3733) With #3449, the unrelated uncaught exceptions can also be caught in this manner.

dkhalanskyjb requested a review from qwwdfsad September 12, 2022 14:29

dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch from cebf6b4 to b9c3e77 Compare September 12, 2022 14:57

qwwdfsad reviewed Sep 12, 2022

View reviewed changes

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt Outdated Show resolved Hide resolved

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt Outdated Show resolved Hide resolved

dkhalanskyjb marked this pull request as ready for review September 19, 2022 11:54

dkhalanskyjb requested a review from qwwdfsad September 19, 2022 11:55

dkhalanskyjb mentioned this pull request Nov 10, 2022

kotlinx.coroutines.test: Option to disable virtual time and delay skipping #3179

Open

qwwdfsad reviewed Nov 21, 2022

View reviewed changes

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt Outdated Show resolved Hide resolved

dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch from ead5c0b to c4345c5 Compare December 16, 2022 12:31

dkhalanskyjb requested a review from qwwdfsad December 16, 2022 14:32

dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch from e5051ba to 7695863 Compare December 16, 2022 15:11

dkhalanskyjb requested review from qwwdfsad and removed request for qwwdfsad December 27, 2022 14:24

qwwdfsad reviewed Jan 5, 2023

View reviewed changes

kotlinx-coroutines-core/common/src/internal/CoroutineExceptionHandlerImpl.kt Outdated Show resolved Hide resolved

amal mentioned this pull request Jan 6, 2023

Support file naming scheme for expect/actuals as in kotlinx.coroutines for MatchingDeclarationName rule detekt/detekt#5678

Closed

dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch from 7695863 to b12a404 Compare February 9, 2023 14:49

dkhalanskyjb requested a review from qwwdfsad February 10, 2023 12:40

dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch 2 times, most recently from 9d51a36 to 57497ec Compare February 22, 2023 11:21

qwwdfsad approved these changes Feb 22, 2023

View reviewed changes

dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch from 2d5008a to 4fda39a Compare February 23, 2023 13:06

dkhalanskyjb merged commit 1245d7e into develop Feb 24, 2023

dkhalanskyjb deleted the TestScope-uncaught-exceptions-registry branch February 24, 2023 17:45

dkhalanskyjb added a commit that referenced this pull request May 2, 2023

Fix the error message if there were uncaught exceptions before test

7bb4caa

With #3449, the unrelated uncaught exceptions can also be caught in this manner.

dkhalanskyjb mentioned this pull request May 2, 2023

Fix the error message if there were uncaught exceptions before test #3733

Merged

dkhalanskyjb added a commit that referenced this pull request May 3, 2023

Fix the error message if there were uncaught exceptions before test (#…

298419f

…3733) With #3449, the unrelated uncaught exceptions can also be caught in this manner.

tyvsmith mentioned this pull request Aug 18, 2023

Enforce runTest in junit test when coroutine is used detekt/detekt#6410

Closed

lucas-livefront mentioned this pull request Sep 18, 2023

No longer able to catch exceptions from other coroutines while within runTest after 1.7.0 #3889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In the test module, collect and report the actual uncaught exceptions #3449

In the test module, collect and report the actual uncaught exceptions #3449

dkhalanskyjb commented Sep 12, 2022

dkhalanskyjb commented Sep 12, 2022 •

edited

Loading

qwwdfsad left a comment

qwwdfsad left a comment

dkhalanskyjb commented Dec 27, 2022

qwwdfsad left a comment

qwwdfsad commented Feb 6, 2023

qwwdfsad commented Feb 17, 2023

qwwdfsad commented Feb 21, 2023

dkhalanskyjb commented Feb 22, 2023

qwwdfsad left a comment

qwwdfsad Feb 22, 2023

qwwdfsad Feb 22, 2023

qwwdfsad Feb 22, 2023 •

edited

Loading

dkhalanskyjb Feb 22, 2023

dkhalanskyjb commented Feb 23, 2023

qwwdfsad commented Feb 23, 2023

		@@ -0,0 +1 @@
		kotlinx.coroutines.test.internal.ExceptionCollectorAsService

	internal object ExceptionCollector: AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler {
	internal object ExceptionCollector : AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler {

In the test module, collect and report the actual uncaught exceptions #3449

In the test module, collect and report the actual uncaught exceptions #3449

Conversation

dkhalanskyjb commented Sep 12, 2022

dkhalanskyjb commented Sep 12, 2022 • edited Loading

qwwdfsad left a comment

Choose a reason for hiding this comment

qwwdfsad left a comment

Choose a reason for hiding this comment

dkhalanskyjb commented Dec 27, 2022

qwwdfsad left a comment

Choose a reason for hiding this comment

qwwdfsad commented Feb 6, 2023

qwwdfsad commented Feb 17, 2023

qwwdfsad commented Feb 21, 2023

dkhalanskyjb commented Feb 22, 2023

qwwdfsad left a comment

Choose a reason for hiding this comment

qwwdfsad Feb 22, 2023

Choose a reason for hiding this comment

qwwdfsad Feb 22, 2023

Choose a reason for hiding this comment

qwwdfsad Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

dkhalanskyjb Feb 22, 2023

Choose a reason for hiding this comment

dkhalanskyjb commented Feb 23, 2023

qwwdfsad commented Feb 23, 2023

dkhalanskyjb commented Sep 12, 2022 •

edited

Loading

qwwdfsad Feb 22, 2023 •

edited

Loading