Add tests for TestCoroutineDispatcher.

This required a bunch of refactoring, adding the first examples of dedicated Bazel test build files, and introducing a new test pattern for sharing code between different implementations. Further, it refined the API for TesCoroutineDispatcher & fixed some issues in the implementations (especially Robolectric). Tests verified as non-flaky in Robolectric across 1000 runs, and 0.2% flaky across 1000 runs for Espresso (though that may be fixed; waiting on a follow-up run to confirm stability).
oppia · BenHenning · Apr 23, 2021 · Mar 23, 2021 · Mar 23, 2021 · Mar 23, 2021
commit 1e138b95fa27d626d4105d859226f6971652eda8
@@ -76,8 +76,9 @@ TEST_DEPS = [
 # Qualified file paths for test classes that have been migrated over to their own packages &
 # shouldn't be defined as module-level tests.
 # keep sorted
-MIGRATED_TESTS = [
-]
+MIGRATED_TESTS = glob([
+    "src/test/java/org/oppia/android/testing/threading/*.kt",
+])
 
 [testing_test(
     name = test_file_path,

diff --git a/testing/src/main/java/org/oppia/android/testing/threading/BUILD.bazel b/testing/src/main/java/org/oppia/android/testing/threading/BUILD.bazel
@@ -20,12 +20,18 @@ kt_android_library(
     ],
 )
 
+# Note that this library has restricted visibility since tests should only use the dependency
+# directly in special circumstances. In general, you don't need this. Use TestCoroutineDispatchers
+# instead.
 kt_android_library(
     name = "test_coroutine_dispatcher",
     testonly = True,
     srcs = [
         "TestCoroutineDispatcher.kt",
     ],
+    visibility = [
+        "//testing/src/test/java/org/oppia/android/testing/threading:__pkg__",
+    ],
     deps = [
         "//third_party:org_jetbrains_kotlinx_kotlinx-coroutines-core",
     ],

diff --git a/testing/src/main/java/org/oppia/android/testing/threading/TestCoroutineDispatcher.kt b/testing/src/main/java/org/oppia/android/testing/threading/TestCoroutineDispatcher.kt
@@ -33,16 +33,19 @@ abstract class TestCoroutineDispatcher : CoroutineDispatcher() {
    * Returns whether there are any tasks known to the dispatcher that have not yet been started.
    *
    * Note that some of these tasks may be scheduled for the future. This is meant to be used in
-   * conjunction with [org.oppia.android.testing.FakeSystemClock.advanceTime] since that along with
-   * [runCurrent] will execute all tasks up to the new time. If the time returned by
+   * conjunction with [org.oppia.android.testing.time.FakeSystemClock.advanceTime] since that along
+   * with [runCurrent] will execute all tasks up to the new time. If the time returned by
    * [getNextFutureTaskCompletionTimeMillis] plus the current time is passed to
-   * [org.oppia.android.testing.FakeSystemClock.advanceTime], this dispatcher guarantees that
+   * [org.oppia.android.testing.time.FakeSystemClock.advanceTime], this dispatcher guarantees that
    * [hasPendingTasks] will return false after a call to [runCurrent] returns.
    *
    * This function makes no guarantees about idleness with respect to other dispatchers (e.g. even
    * if all tasks are executed, another dispatcher could schedule another task on this dispatcher in
    * response to a task from this dispatcher being executed). Cross-thread communication should be
    * managed using [TestCoroutineDispatchers], instead.
+   *
+   * Note that it's up to the implementation to define what constitutes as 'pending' as this may
+   * differ for different testing environments.
    */
   abstract fun hasPendingTasks(): Boolean
 
@@ -58,6 +61,9 @@ abstract class TestCoroutineDispatcher : CoroutineDispatcher() {
    * If [runCurrent] is used, this function is guaranteed to return false after that function
    * returns. Note that the same threading caveats mentioned for [hasPendingTasks] also pertains to
    * this function.
+   *
+   * Note that it's up to the implementation to define what constitutes as 'completable' as this may
+   * differ for different testing environments.
    */
   abstract fun hasPendingCompletableTasks(): Boolean
 

diff --git a/.../src/main/java/org/oppia/android/testing/threading/TestCoroutineDispatcherEspressoImpl.kt b/.../src/main/java/org/oppia/android/testing/threading/TestCoroutineDispatcherEspressoImpl.kt
@@ -12,7 +12,6 @@ import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicInteger
 import javax.inject.Inject
 import kotlin.coroutines.CoroutineContext
-import kotlin.math.min
 import kotlinx.coroutines.delay as delayInScope // Needed to avoid conflict with Delay.delay().
 
 /**
@@ -44,26 +43,23 @@ class TestCoroutineDispatcherEspressoImpl private constructor(
     // Tasks immediately will start running, so track the task immediately.
     executingTaskCount.incrementAndGet()
     notifyIfRunning()
-    realCoroutineDispatcher.dispatch(
-      context,
-      kotlinx.coroutines.Runnable {
-        try {
-          block.run()
-        } finally {
-          executingTaskCount.decrementAndGet()
-          taskCompletionTimes.remove(taskId)
-        }
-        notifyIfIdle()
+    realCoroutineDispatcher.dispatch(context) {
+      try {
+        block.run()
+      } finally {
+        executingTaskCount.decrementAndGet()
+        taskCompletionTimes.remove(taskId)
       }
-    )
+      notifyIfIdle()
+    }
   }
 
   override fun scheduleResumeAfterDelay(
     timeMillis: Long,
     continuation: CancellableContinuation<Unit>
   ) {
     val taskId = totalTaskCount.incrementAndGet()
-    taskCompletionTimes[taskId] = timeMillis
+    taskCompletionTimes[taskId] = System.currentTimeMillis() + timeMillis
     val block: CancellableContinuation<Unit>.() -> Unit = {
       realCoroutineDispatcher.resumeUndispatched(Unit)
     }
@@ -93,14 +89,8 @@ class TestCoroutineDispatcherEspressoImpl private constructor(
   override fun hasPendingTasks(): Boolean = executingTaskCount.get() != 0
 
   override fun getNextFutureTaskCompletionTimeMillis(timeMillis: Long): Long? {
-    var nextFutureTaskTime: Long? = null
     // Find the next most recent task completion time that's after the specified time.
-    for (entry in taskCompletionTimes.entries) {
-      if (entry.value > timeMillis) {
-        nextFutureTaskTime = nextFutureTaskTime?.let { min(it, entry.value) } ?: entry.value
-      }
-    }
-    return nextFutureTaskTime
+    return taskCompletionTimes.values.filter { it > timeMillis }.minOrNull()
   }
 
   override fun hasPendingCompletableTasks(): Boolean {

diff --git a/testing/src/test/BUILD.bazel b/testing/src/test/BUILD.bazel
@@ -0,0 +1,10 @@
+# TODO(#1532): Rename file to 'BUILD' post-Gradle.
+"""
+Top-level testing module files & targets.
+"""
+
+filegroup(
+    name = "test_manifest",
+    srcs = ["AndroidManifest.xml"],
+    visibility = ["//:oppia_testing_visibility"],
+)
diff --git a/testing/src/test/java/org/oppia/android/testing/threading/BUILD.bazel b/testing/src/test/java/org/oppia/android/testing/threading/BUILD.bazel
@@ -0,0 +1,69 @@
+# TODO(#1532): Rename file to 'BUILD' post-Gradle.
+"""
+Tests for threading-specific test utilities.
+"""
+
+load("@dagger//:workspace_defs.bzl", "dagger_rules")
+load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")
+load("//:oppia_android_test.bzl", "oppia_android_test")
+
+kt_android_library(
+    name = "test_coroutine_dispatcher_test_base",
+    testonly = True,
+    srcs = ["TestCoroutineDispatcherTestBase.kt"],
+    deps = [
+        "//testing/src/main/java/org/oppia/android/testing/threading:annotations",
+        "//testing/src/main/java/org/oppia/android/testing/threading:test_coroutine_dispatcher",
+        "//third_party:com_google_truth_truth",
+        "//third_party:org_mockito_mockito-core",
+    ],
+)
+
+oppia_android_test(
+    name = "TestCoroutineDispatcherEspressoImplTest",
+    srcs = ["TestCoroutineDispatcherEspressoImplTest.kt"],
+    custom_package = "org.oppia.android.data.testing",
+    test_class = "org.oppia.android.testing.threading.TestCoroutineDispatcherEspressoImplTest",
+    test_manifest = "//testing/src/test:test_manifest",
+    deps = [
+        ":dagger",
+        ":test_coroutine_dispatcher_test_base",
+        "//testing",
+        "//testing/src/main/java/org/oppia/android/testing/threading:test_module",
+        "//third_party:robolectric_android-all",
+        "//utility",
+    ],
+)
+
+oppia_android_test(
+    name = "TestCoroutineDispatcherRobolectricImplTest",
+    srcs = ["TestCoroutineDispatcherRobolectricImplTest.kt"],
+    custom_package = "org.oppia.android.data.testing",
+    test_class = "org.oppia.android.testing.threading.TestCoroutineDispatcherRobolectricImplTest",
+    test_manifest = "//testing/src/test:test_manifest",
+    deps = [
+        ":dagger",
+        ":test_coroutine_dispatcher_test_base",
+        "//testing",
+        "//testing/src/main/java/org/oppia/android/testing/threading:test_module",
+        "//third_party:robolectric_android-all",
+        "//utility",
+    ],
+)
+
+oppia_android_test(
+    name = "TestCoroutineDispatcherTest",
+    srcs = ["TestCoroutineDispatcherTest.kt"],
+    custom_package = "org.oppia.android.data.testing",
+    test_class = "org.oppia.android.testing.threading.TestCoroutineDispatcherTest",
+    test_manifest = "//testing/src/test:test_manifest",
+    deps = [
+        ":dagger",
+        "//testing",
+        "//testing/src/main/java/org/oppia/android/testing/threading:test_module",
+        "//third_party:robolectric_android-all",
+        "//utility",
+    ],
+)
+
+dagger_rules()
diff --git a/.../test/java/org/oppia/android/testing/threading/TestCoroutineDispatcherEspressoImplTest.kt b/.../test/java/org/oppia/android/testing/threading/TestCoroutineDispatcherEspressoImplTest.kt
@@ -0,0 +1,184 @@
+package org.oppia.android.testing.threading
+
+import android.app.Application
+import android.content.Context
+import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.google.common.truth.Truth
+import com.google.common.truth.Truth.assertThat
+import dagger.BindsInstance
+import dagger.Component
+import dagger.Module
+import dagger.Provides
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.InternalCoroutinesApi
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mockito.Mockito.mock
+import org.mockito.Mockito.timeout
+import org.mockito.Mockito.verify
+import org.oppia.android.testing.TestLogReportingModule
+import org.oppia.android.testing.robolectric.IsOnRobolectric
+import org.oppia.android.util.data.DataProvidersInjector
+import org.oppia.android.util.data.DataProvidersInjectorProvider
+import org.oppia.android.util.logging.EnableConsoleLog
+import org.oppia.android.util.logging.EnableFileLog
+import org.oppia.android.util.logging.GlobalLogLevel
+import org.oppia.android.util.logging.LogLevel
+import org.robolectric.annotation.Config
+import org.robolectric.annotation.LooperMode
+import javax.inject.Singleton
+
+/**
+ * Tests for [TestCoroutineDispatcherEspressoImpl].
+ *
+ * Note that this is testing the Espresso implementation of the test coroutine dispatcher using
+ * Robolectric which means certain time constructs need to be used to try and control threading
+ * since the tests can't rely on synchronization barriers (like the dispatcher itself).
+ */
+@RunWith(AndroidJUnit4::class)
+@LooperMode(LooperMode.Mode.PAUSED)
+@Config(
+  application = TestCoroutineDispatcherEspressoImplTest.TestApplication::class,
+  qualifiers = "port-xxhdpi"
+)
+class TestCoroutineDispatcherEspressoImplTest: TestCoroutineDispatcherTestBase(
+  shortTaskDelayMillis = 5L,
+  longTaskDelayMillis = 15000L,
+  longTaskDelayDeltaCheckMillis = 1000L
+) {
+  @Before
+  @InternalCoroutinesApi
+  @ExperimentalCoroutinesApi
+  override fun setUp() {
+    setUpTestApplicationComponent()
+    verifyDispatcherImplementation<TestCoroutineDispatcherEspressoImpl>()
+  }
+
+  override fun getCurrentTimeMillis(): Long = System.currentTimeMillis()
+
+  override fun advanceTimeBy(timeMillis: Long) {
+    // While an antipattern, the tests are configured per the task delay passed in the constructor
+    // to use small delays so that different test scenarios can be tested in a real thread
+    // environment.
+    Thread.sleep(timeMillis)
+  }
+
+  @Suppress("ControlFlowWithEmptyBody")
+  override fun stabilizeAfterDispatcherFlush() {
+    // Spin the test thread until the dispatcher has finished.
+    while (backgroundTestDispatcher.hasPendingTasks());
+  }
+
+  override fun ensureFutureTasksAreScheduled() {
+    val mockRunnable = mock(Runnable::class.java)
+    scheduleFutureTask(delayMs = 1L, mockRunnable)
+
+    // Wait up to 500ms for the delayed runnable to be called. This should be sufficiently less than
+    // the long time delay for tests to complete.
+    verify(mockRunnable, timeout(/* millis= */ 500L).atLeastOnce()).run()
+
+    // Wait a bit longer in case the quick execution introduces an ordering issue.
+    advanceTimeBy(10L)
+  }
+
+  // Note that this test intentionally differs from Robolectric: the Espresso implementation chooses
+  // to consider a future task as in-progress even during the delay since it needs to communicate
+  // back to Espresso (via an IdlingResource) that a task needs to be run. This intentionally
+  // differs from the Robolectric version which aims to allow idleness between calls to
+  // advanceTime().
+  @Test
+  fun testDispatcher_hasPendingCompletableTasks_advanceBeforeTask_returnsTrue() {
+    // Use a longer task time to avoid inadvertently running the task.
+    scheduleFutureTask(longTaskDelayMillis, mockRunnable1)
+    ensureFutureTasksAreScheduled()
+
+    advanceTimeBy(shortTaskDelayMillis - 1)
+    val hasCompletableTasks = backgroundTestDispatcher.hasPendingCompletableTasks()
+
+    // Any scheduled task is considered executing.
+    assertThat(hasCompletableTasks).isTrue()
+  }
+
+  // For similar reasons, testing the 'true' case requires some deviation here. Tasks are considered
+  // pending & completable immediately after being scheduled in a real threaded environment, unlike
+  // the Robolectric variant of TestCoroutineDispatcher.
+  @Test
+  fun testDispatcher_hasPendingCompletableTasks_futureTask_returnsTrue() {
+    // Use a longer delay to provide time for the pending tasks check.
+    scheduleFutureTask(longTaskDelayMillis, mockRunnable1)
+    ensureFutureTasksAreScheduled()
+
+    val hasCompletableTasks = backgroundTestDispatcher.hasPendingCompletableTasks()
+
+    // The immediate task is completable now.
+    assertThat(hasCompletableTasks).isTrue()
+  }
+
+  private fun setUpTestApplicationComponent() {
+    ApplicationProvider.getApplicationContext<TestApplication>().inject(this)
+  }
+
+  // TODO(#89): Move this to a common test application component.
+  @Module
+  class TestModule {
+    @Provides
+    @Singleton
+    fun provideContext(application: Application): Context {
+      return application
+    }
+
+    // TODO(#59): Either isolate these to their own shared test module, or use the real logging
+    // module in tests to avoid needing to specify these settings for tests.
+    @EnableConsoleLog
+    @Provides
+    fun provideEnableConsoleLog(): Boolean = true
+
+    @EnableFileLog
+    @Provides
+    fun provideEnableFileLog(): Boolean = false
+
+    @GlobalLogLevel
+    @Provides
+    fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE
+
+    @Provides
+    @IsOnRobolectric
+    fun provideIsOnRobolectric(): Boolean = false
+  }
+
+  // TODO(#59): Figure out a way to reuse modules instead of needing to re-declare them.
+  @Singleton
+  @Component(
+    modules = [
+      TestDispatcherModule::class,
+      TestModule::class,
+      TestLogReportingModule::class
+    ]
+  )
+  interface TestApplicationComponent : DataProvidersInjector {
+    @Component.Builder
+    interface Builder {
+      @BindsInstance
+      fun setApplication(application: Application): Builder
+      fun build(): TestApplicationComponent
+    }
+
+    fun inject(test: TestCoroutineDispatcherEspressoImplTest)
+  }
+
+  class TestApplication : Application(), DataProvidersInjectorProvider {
+    private val component: TestApplicationComponent by lazy {
+      DaggerTestCoroutineDispatcherEspressoImplTest_TestApplicationComponent.builder()
+        .setApplication(this)
+        .build()
+    }
+
+    fun inject(test: TestCoroutineDispatcherEspressoImplTest) {
+      component.inject(test)
+    }
+
+    override fun getDataProvidersInjector(): DataProvidersInjector = component
+  }
+}