From 6c52412d49a53da742908dadd1ce28f38ec95831 Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy Date: Mon, 11 Oct 2021 21:31:05 +0300 Subject: [PATCH 1/5] Improve `TestCoroutineScope` * Add more detailed documentation; * Move most verification logic from `runBlockingTest` to `cleanupTestCoroutines` Fixes #1749 * Complete the scope's job if a new job was created for it Fixes #1772 --- .../common/src/TestBuilders.kt | 45 +--------- .../common/src/TestCoroutineScope.kt | 84 ++++++++++++++++--- .../common/test/TestBuildersTest.kt | 2 +- .../common/test/TestCoroutineSchedulerTest.kt | 1 + .../common/test/TestCoroutineScopeTest.kt | 70 ++++++++++++++++ 5 files changed, 149 insertions(+), 53 deletions(-) diff --git a/kotlinx-coroutines-test/common/src/TestBuilders.kt b/kotlinx-coroutines-test/common/src/TestBuilders.kt index f66f962be7..bf864333b4 100644 --- a/kotlinx-coroutines-test/common/src/TestBuilders.kt +++ b/kotlinx-coroutines-test/common/src/TestBuilders.kt @@ -43,25 +43,16 @@ import kotlin.coroutines.* */ @ExperimentalCoroutinesApi public fun runBlockingTest(context: CoroutineContext = EmptyCoroutineContext, testBody: suspend TestCoroutineScope.() -> Unit) { - val (safeContext, dispatcher) = context.checkTestScopeArguments() - val startingJobs = safeContext.activeJobs() - val scope = TestCoroutineScope(safeContext) + val scope = TestCoroutineScope(context) + val scheduler = scope.coroutineContext[TestCoroutineScheduler]!! val deferred = scope.async { scope.testBody() } - dispatcher.scheduler.advanceUntilIdle() + scheduler.advanceUntilIdle() deferred.getCompletionExceptionOrNull()?.let { throw it } scope.cleanupTestCoroutines() - val endingJobs = safeContext.activeJobs() - if ((endingJobs - startingJobs).isNotEmpty()) { - throw UncompletedCoroutinesError("Test finished with active jobs: $endingJobs") - } -} - -private fun CoroutineContext.activeJobs(): Set { - return checkNotNull(this[Job]).children.filter { it.isActive }.toSet() } /** @@ -78,33 +69,3 @@ public fun TestCoroutineScope.runBlockingTest(block: suspend TestCoroutineScope. @ExperimentalCoroutinesApi public fun TestCoroutineDispatcher.runBlockingTest(block: suspend TestCoroutineScope.() -> Unit): Unit = runBlockingTest(this, block) - -internal fun CoroutineContext.checkTestScopeArguments(): Pair { - val scheduler: TestCoroutineScheduler - val dispatcher = when (val dispatcher = get(ContinuationInterceptor)) { - is TestDispatcher -> { - val ctxScheduler = get(TestCoroutineScheduler) - if (ctxScheduler == null) { - scheduler = dispatcher.scheduler - } else { - require(dispatcher.scheduler === ctxScheduler) { - "Both a TestCoroutineScheduler $ctxScheduler and TestDispatcher $dispatcher linked to " + - "another scheduler were passed." - } - scheduler = ctxScheduler - } - dispatcher - } - null -> { - scheduler = get(TestCoroutineScheduler) ?: TestCoroutineScheduler() - TestCoroutineDispatcher(scheduler) - } - else -> throw IllegalArgumentException("Dispatcher must implement TestDispatcher: $dispatcher") - } - val exceptionHandler = get(CoroutineExceptionHandler).run { - this?.let { require(this is UncaughtExceptionCaptor) { "coroutineExceptionHandler must implement UncaughtExceptionCaptor: $this" } } - this ?: TestCoroutineExceptionHandler() - } - val job = get(Job) ?: SupervisorJob() - return Pair(this + scheduler + dispatcher + exceptionHandler + job, dispatcher) -} diff --git a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt index e4e92bd486..65a3d4002b 100644 --- a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt +++ b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt @@ -13,12 +13,13 @@ import kotlin.coroutines.* @ExperimentalCoroutinesApi public sealed interface TestCoroutineScope: CoroutineScope, UncaughtExceptionCaptor { /** - * Call after the test completes. + * Called after the test completes. + * * Calls [UncaughtExceptionCaptor.cleanupTestCoroutinesCaptor] and [DelayController.cleanupTestCoroutines]. + * If a new job was created for this scope, the job is completed. * * @throws Throwable the first uncaught exception, if there are any uncaught exceptions. - * @throws AssertionError if any pending tasks are active, however it will not throw for suspended - * coroutines. + * @throws AssertionError if any pending tasks are active. */ @ExperimentalCoroutinesApi public fun cleanupTestCoroutines() @@ -32,29 +33,92 @@ public sealed interface TestCoroutineScope: CoroutineScope, UncaughtExceptionCap private class TestCoroutineScopeImpl ( override val coroutineContext: CoroutineContext, - override val testScheduler: TestCoroutineScheduler + val ownJob: CompletableJob? ): TestCoroutineScope, UncaughtExceptionCaptor by coroutineContext.uncaughtExceptionCaptor { + override val testScheduler: TestCoroutineScheduler + get() = coroutineContext[TestCoroutineScheduler]!! + + /** These jobs existed before the coroutine scope was used, so it's alright if they don't get cancelled. */ + val initialJobs = coroutineContext.activeJobs() + override fun cleanupTestCoroutines() { coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutinesCaptor() coroutineContext.delayController?.cleanupTestCoroutines() + val jobs = coroutineContext.activeJobs() + if ((jobs - initialJobs).isNotEmpty()) { + throw UncompletedCoroutinesError("Test finished with active jobs: $jobs") + } + ownJob?.complete() } } +private fun CoroutineContext.activeJobs(): Set { + return checkNotNull(this[Job]).children.filter { it.isActive }.toSet() +} + /** - * A scope which provides detailed control over the execution of coroutines for tests. + * A coroutine scope for launching test coroutines. * - * If the provided context does not provide a [ContinuationInterceptor] (Dispatcher) or [CoroutineExceptionHandler], the - * scope adds [TestCoroutineDispatcher] and [TestCoroutineExceptionHandler] automatically. + * It ensures that all the test module machinery is properly initialized. + * * If [context] doesn't define a [TestCoroutineScheduler] for orchestrating the virtual time used for delay-skipping, + * a new one is created, unless a [TestDispatcher] is provided, in which case [TestDispatcher.scheduler] is used. + * * If [context] doesn't have a [ContinuationInterceptor], a [TestCoroutineDispatcher] is created. + * * If [context] does not provide a [CoroutineExceptionHandler], [TestCoroutineExceptionHandler] is created + * automatically. + * * If [context] provides a [Job], that job is used for the new scope, but is not completed once the scope completes. + * On the other hand, if there is no [Job] in the context, a [CompletableJob] is created and completed on + * [TestCoroutineScope.cleanupTestCoroutines]. * - * @param context an optional context that MAY provide [UncaughtExceptionCaptor] and/or [DelayController] + * @throws IllegalArgumentException if [context] has both [TestCoroutineScheduler] and a [TestDispatcher] linked to a + * different scheduler. + * @throws IllegalArgumentException if [context] has a [ContinuationInterceptor] that is not a [TestDispatcher]. + * @throws IllegalArgumentException if [context] has an [CoroutineExceptionHandler] that is not an + * [UncaughtExceptionCaptor]. */ @Suppress("FunctionName") @ExperimentalCoroutinesApi -public fun TestCoroutineScope(context: CoroutineContext = EmptyCoroutineContext): TestCoroutineScope = - context.checkTestScopeArguments().let { TestCoroutineScopeImpl(it.first, it.second.scheduler) } +public fun TestCoroutineScope(context: CoroutineContext = EmptyCoroutineContext): TestCoroutineScope { + val scheduler: TestCoroutineScheduler + val dispatcher = when (val dispatcher = context[ContinuationInterceptor]) { + is TestDispatcher -> { + scheduler = dispatcher.scheduler + val ctxScheduler = context[TestCoroutineScheduler] + if (ctxScheduler != null) { + require(dispatcher.scheduler === ctxScheduler) { + "Both a TestCoroutineScheduler $ctxScheduler and TestDispatcher $dispatcher linked to " + + "another scheduler were passed." + } + } + dispatcher + } + null -> { + scheduler = context[TestCoroutineScheduler] ?: TestCoroutineScheduler() + TestCoroutineDispatcher(scheduler) + } + else -> throw IllegalArgumentException("Dispatcher must implement TestDispatcher: $dispatcher") + } + val exceptionHandler = context[CoroutineExceptionHandler].run { + this?.let { + require(this is UncaughtExceptionCaptor) { + "coroutineExceptionHandler must implement UncaughtExceptionCaptor: $context" + } + } + this ?: TestCoroutineExceptionHandler() + } + val job: Job + val ownJob: CompletableJob? + if (context[Job] == null) { + ownJob = SupervisorJob() + job = ownJob + } else { + ownJob = null + job = context[Job]!! + } + return TestCoroutineScopeImpl(context + scheduler + dispatcher + exceptionHandler + job, ownJob) +} private inline val CoroutineContext.uncaughtExceptionCaptor: UncaughtExceptionCaptor get() { diff --git a/kotlinx-coroutines-test/common/test/TestBuildersTest.kt b/kotlinx-coroutines-test/common/test/TestBuildersTest.kt index a3167e5876..7fefaf78b5 100644 --- a/kotlinx-coroutines-test/common/test/TestBuildersTest.kt +++ b/kotlinx-coroutines-test/common/test/TestBuildersTest.kt @@ -104,7 +104,7 @@ class TestBuildersTest { } @Test - fun whenInrunBlocking_runBlockingTest_nestsProperly() { + fun whenInRunBlocking_runBlockingTest_nestsProperly() { // this is not a supported use case, but it is possible so ensure it works val scope = TestCoroutineScope() diff --git a/kotlinx-coroutines-test/common/test/TestCoroutineSchedulerTest.kt b/kotlinx-coroutines-test/common/test/TestCoroutineSchedulerTest.kt index ab6ced4741..f7d2ed13fd 100644 --- a/kotlinx-coroutines-test/common/test/TestCoroutineSchedulerTest.kt +++ b/kotlinx-coroutines-test/common/test/TestCoroutineSchedulerTest.kt @@ -5,6 +5,7 @@ package kotlinx.coroutines.test import kotlinx.coroutines.* +import kotlin.coroutines.* import kotlin.test.* class TestCoroutineSchedulerTest { diff --git a/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt b/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt index a7d4480d63..968b093c79 100644 --- a/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt +++ b/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt @@ -50,6 +50,76 @@ class TestCoroutineScopeTest { } } + /** Tests that the cleanup procedure throws if there were uncompleted delays by the end. */ + @Test + fun testPresentDelaysThrowing() { + val scope = TestCoroutineScope() + var result = false + scope.launch { + delay(5) + result = true + } + assertFalse(result) + assertFailsWith { scope.cleanupTestCoroutines() } + assertFalse(result) + } + + /** Tests that the cleanup procedure throws if there were active jobs by the end. */ + @Test + fun testActiveJobsThrowing() { + val scope = TestCoroutineScope() + var result = false + val deferred = CompletableDeferred() + scope.launch { + deferred.await() + result = true + } + assertFalse(result) + assertFailsWith { scope.cleanupTestCoroutines() } + assertFalse(result) + } + + /** Tests that the cleanup procedure doesn't throw if it detects that the job is already cancelled. */ + @Test + fun testCancelledDelaysNotThrowing() { + val scope = TestCoroutineScope() + var result = false + val deferred = CompletableDeferred() + val job = scope.launch { + deferred.await() + result = true + } + job.cancel() + assertFalse(result) + scope.cleanupTestCoroutines() + assertFalse(result) + } + + /** Tests that the coroutine scope completes its job if the job was not passed to it as an argument. */ + @Test + fun testCompletesOwnJob() { + val scope = TestCoroutineScope() + var handlerCalled = false + scope.coroutineContext.job.invokeOnCompletion { + handlerCalled = true + } + scope.cleanupTestCoroutines() + assertTrue(handlerCalled) + } + + /** Tests that the coroutine scope completes its job if the job was not passed to it as an argument. */ + @Test + fun testDoesNotCompleteGivenJob() { + var handlerCalled = false + val job = Job() + job.invokeOnCompletion { + handlerCalled = true + } + val scope = TestCoroutineScope(job) + scope.cleanupTestCoroutines() + assertFalse(handlerCalled) + } + private val invalidContexts = listOf( Dispatchers.Default, // not a [TestDispatcher] TestCoroutineDispatcher() + TestCoroutineScheduler(), // the dispatcher is not linked to the scheduler From b43b87ddd0c864baa77b0456cbd59d5a79cd1e03 Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy Date: Tue, 26 Oct 2021 10:27:41 +0300 Subject: [PATCH 2/5] Fixes --- .../common/src/TestCoroutineScope.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt index 65a3d4002b..ec7c332410 100644 --- a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt +++ b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt @@ -31,9 +31,9 @@ public sealed interface TestCoroutineScope: CoroutineScope, UncaughtExceptionCap public val testScheduler: TestCoroutineScheduler } -private class TestCoroutineScopeImpl ( +private class TestCoroutineScopeImpl( override val coroutineContext: CoroutineContext, - val ownJob: CompletableJob? + private val ownJob: CompletableJob? ): TestCoroutineScope, UncaughtExceptionCaptor by coroutineContext.uncaughtExceptionCaptor @@ -42,14 +42,16 @@ private class TestCoroutineScopeImpl ( get() = coroutineContext[TestCoroutineScheduler]!! /** These jobs existed before the coroutine scope was used, so it's alright if they don't get cancelled. */ - val initialJobs = coroutineContext.activeJobs() + private val initialJobs = coroutineContext.activeJobs() override fun cleanupTestCoroutines() { coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutinesCaptor() coroutineContext.delayController?.cleanupTestCoroutines() val jobs = coroutineContext.activeJobs() if ((jobs - initialJobs).isNotEmpty()) { - throw UncompletedCoroutinesError("Test finished with active jobs: $jobs") + val exception = UncompletedCoroutinesError("Test finished with active jobs: $jobs") + ownJob?.completeExceptionally(exception) + throw exception } ownJob?.complete() } From 2cddb284b5c015a6fc2ac1c77d6f5ca476b78049 Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy Date: Wed, 27 Oct 2021 10:43:26 +0300 Subject: [PATCH 3/5] Fix ownJob not always completing --- .../common/src/TestCoroutineScope.kt | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt index ec7c332410..69af8cd764 100644 --- a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt +++ b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt @@ -45,11 +45,13 @@ private class TestCoroutineScopeImpl( private val initialJobs = coroutineContext.activeJobs() override fun cleanupTestCoroutines() { - coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutinesCaptor() - coroutineContext.delayController?.cleanupTestCoroutines() - val jobs = coroutineContext.activeJobs() - if ((jobs - initialJobs).isNotEmpty()) { - val exception = UncompletedCoroutinesError("Test finished with active jobs: $jobs") + try { + coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutinesCaptor() + coroutineContext.delayController?.cleanupTestCoroutines() + val jobs = coroutineContext.activeJobs() + if ((jobs - initialJobs).isNotEmpty()) + throw UncompletedCoroutinesError("Test finished with active jobs: $jobs") + } catch (exception: Throwable) { ownJob?.completeExceptionally(exception) throw exception } From 1e9be832a68d402b527f21b219b90b2e916881f6 Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy Date: Thu, 28 Oct 2021 14:26:03 +0300 Subject: [PATCH 4/5] Don't complete the created job on TestCoroutineScope.cleanup --- .../common/src/TestCoroutineScope.kt | 35 +++++-------------- .../common/test/TestCoroutineScopeTest.kt | 25 ------------- 2 files changed, 9 insertions(+), 51 deletions(-) diff --git a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt index 69af8cd764..c2c67b1c56 100644 --- a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt +++ b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt @@ -32,8 +32,7 @@ public sealed interface TestCoroutineScope: CoroutineScope, UncaughtExceptionCap } private class TestCoroutineScopeImpl( - override val coroutineContext: CoroutineContext, - private val ownJob: CompletableJob? + override val coroutineContext: CoroutineContext ): TestCoroutineScope, UncaughtExceptionCaptor by coroutineContext.uncaughtExceptionCaptor @@ -45,17 +44,11 @@ private class TestCoroutineScopeImpl( private val initialJobs = coroutineContext.activeJobs() override fun cleanupTestCoroutines() { - try { - coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutinesCaptor() - coroutineContext.delayController?.cleanupTestCoroutines() - val jobs = coroutineContext.activeJobs() - if ((jobs - initialJobs).isNotEmpty()) - throw UncompletedCoroutinesError("Test finished with active jobs: $jobs") - } catch (exception: Throwable) { - ownJob?.completeExceptionally(exception) - throw exception - } - ownJob?.complete() + coroutineContext.uncaughtExceptionCaptor.cleanupTestCoroutinesCaptor() + coroutineContext.delayController?.cleanupTestCoroutines() + val jobs = coroutineContext.activeJobs() + if ((jobs - initialJobs).isNotEmpty()) + throw UncompletedCoroutinesError("Test finished with active jobs: $jobs") } } @@ -72,9 +65,7 @@ private fun CoroutineContext.activeJobs(): Set { * * If [context] doesn't have a [ContinuationInterceptor], a [TestCoroutineDispatcher] is created. * * If [context] does not provide a [CoroutineExceptionHandler], [TestCoroutineExceptionHandler] is created * automatically. - * * If [context] provides a [Job], that job is used for the new scope, but is not completed once the scope completes. - * On the other hand, if there is no [Job] in the context, a [CompletableJob] is created and completed on - * [TestCoroutineScope.cleanupTestCoroutines]. + * * If [context] provides a [Job], that job is used for the new scope; otherwise, a [CompletableJob] is created. * * @throws IllegalArgumentException if [context] has both [TestCoroutineScheduler] and a [TestDispatcher] linked to a * different scheduler. @@ -112,16 +103,8 @@ public fun TestCoroutineScope(context: CoroutineContext = EmptyCoroutineContext) } this ?: TestCoroutineExceptionHandler() } - val job: Job - val ownJob: CompletableJob? - if (context[Job] == null) { - ownJob = SupervisorJob() - job = ownJob - } else { - ownJob = null - job = context[Job]!! - } - return TestCoroutineScopeImpl(context + scheduler + dispatcher + exceptionHandler + job, ownJob) + val job: Job = context[Job] ?: SupervisorJob() + return TestCoroutineScopeImpl(context + scheduler + dispatcher + exceptionHandler + job) } private inline val CoroutineContext.uncaughtExceptionCaptor: UncaughtExceptionCaptor diff --git a/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt b/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt index 968b093c79..85e44351ee 100644 --- a/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt +++ b/kotlinx-coroutines-test/common/test/TestCoroutineScopeTest.kt @@ -95,31 +95,6 @@ class TestCoroutineScopeTest { assertFalse(result) } - /** Tests that the coroutine scope completes its job if the job was not passed to it as an argument. */ - @Test - fun testCompletesOwnJob() { - val scope = TestCoroutineScope() - var handlerCalled = false - scope.coroutineContext.job.invokeOnCompletion { - handlerCalled = true - } - scope.cleanupTestCoroutines() - assertTrue(handlerCalled) - } - - /** Tests that the coroutine scope completes its job if the job was not passed to it as an argument. */ - @Test - fun testDoesNotCompleteGivenJob() { - var handlerCalled = false - val job = Job() - job.invokeOnCompletion { - handlerCalled = true - } - val scope = TestCoroutineScope(job) - scope.cleanupTestCoroutines() - assertFalse(handlerCalled) - } - private val invalidContexts = listOf( Dispatchers.Default, // not a [TestDispatcher] TestCoroutineDispatcher() + TestCoroutineScheduler(), // the dispatcher is not linked to the scheduler From 74849aa59c83f65e8d36d3aedcded06e9d6e42c4 Mon Sep 17 00:00:00 2001 From: Dmitry Khalanskiy Date: Thu, 28 Oct 2021 15:06:37 +0300 Subject: [PATCH 5/5] Fix outdated doc --- kotlinx-coroutines-test/common/src/TestCoroutineScope.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt index c2c67b1c56..c37f834356 100644 --- a/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt +++ b/kotlinx-coroutines-test/common/src/TestCoroutineScope.kt @@ -16,7 +16,6 @@ public sealed interface TestCoroutineScope: CoroutineScope, UncaughtExceptionCap * Called after the test completes. * * Calls [UncaughtExceptionCaptor.cleanupTestCoroutinesCaptor] and [DelayController.cleanupTestCoroutines]. - * If a new job was created for this scope, the job is completed. * * @throws Throwable the first uncaught exception, if there are any uncaught exceptions. * @throws AssertionError if any pending tasks are active.