Fix part of #973: Stabilize state fragment tests [Blocked: #1764] (#1630

)

* Introduce test coroutine dispatchers support in Espresso.

This piggybacks off of the solution introduced in #1276 for Robolectric.
That PR allows Robolectric tests in app module to share dependencies
with production components by creating a test application & telling
Robolectric to use it instead of OppiaApplication via a @config
annotation.

This PR achieves the same thing by using a custom test runner that reads
the same annotation and forces Espresso & instrumentation to use the
test application instead of the default OppiaApplication. This setup
will be easier once #59 is finished since we can specify the application
in per-test manifests that both Robolectric & Espresso will respect.

Note that while this lets the same test coroutine dispatchers work in
both production & test code for Espresso, some additional work was
needed to ensure the coroutines behave correctly. In particular, the
coroutines use a fake system clock in Robolectric that requires explicit
synchronization points in the tests to allow the clock to move forward
& properly coordinate execution between background & main thread
operations. However, in Espresso, since everything is using a real clock
an idling resource is the preferred way to synchronize execution: it
allows the background coroutines to operate in real-time much like they
would in production, and then notify Espresso when completed. The test
dispatchers API is now setup to support both synchronization mechanisms
for both Robolectric & Espresso (the idling resource does nothing on
Robolectric and the force synchronization effectively does nothing on
Espresso).

The first test being demonstrated as now stable is SplashActivityTest
(as part of downstream work in #1397.

* Revert "Fixes #941: Add radar effect in Hints and solution (#1475)"

This reverts commit 41eb10b.

* Stabilize StateFragmentTest such that it passes on both Robolectric and
Espresso.

Note that some issues were found during this: #1612 (#1611 was found a
few weeks ago, but it also affects these tests). To ensure the tests can
still be run, a @runon annotation was added to allow tests to target
specific test platforms. The tests that currently fail on Robolectric
due to #1611 and #1612 are disabled for that platform. The test suite as
a whole has been verified to pass in its current state on both
Robolectric and Espresso (on a Pixel XL).

The aim of this PR is to actually enable critical state fragment tests
in CI, so both StateFragmentTest and StateFragmentLocalTest are being
enabled in GitHub actions.

* Enable StateFragmentTest (Robolectric) & StateFragmentLocalTest for CI.

* Add thorough documentation for new dispatchers.

* Clean up comments & add additional documentation.

* Fix lint errors.

* Fix broken test after changes to FakeSystemClock.

* Fix linter errors.

* Use a custom executor service for Glide requests that coordinates with
Oppia's test dispatchers. Note that this does not actually introduce the
service--that will happen in a new branch.

* Introduce new executor service which allows interop with Kotlin
coroutines, plus a test to verify that it fundamentally follows one
interpretation of ExecutorService's API.

* Fix flaky timeout tests by improving cancellation cooperation for
invokeAny() and provide longer timeouts for tests that are
CPU-sensitive.

* Add documentation & clean up unused code.

* Lint fixes.

* Significantly reorganize invokeAll() to try and make it more cooperative
for cancellation, and increase timeout times in tests to reduce
flakiness for time-sensitive tests. Some tests are remaining flaky, so
ignoring those.

Re-add maybeWithTimeoutOrNull since it actually was needed.

* Lint fixes.

* Post-merge module fixes.

* Post-merge fixes with ratio input & add a TODO to improve speed of the
new coroutine executor service.

* Revert "Fixes part of #40 & #42: Generalisation Highfi Mobile Portrait + Landscape - Buttons (#1653)"

This reverts commit 1bb1ffa.

* Ensure terminated tasks do not interfere with one another (timeouts
should happen individually for each task during termination). This fixes
a failure observed in StateFragmentLocalTest in #1630.

* Ignore failing tests until #1769 is resolved.

* Fix awaitTermination & improve test. Improve stack trace for test
dispatcher timeouts.

* Fix slow & broken tests in Robolectric for StateFragmentLocalTest.

* Add missing deps for StateFragmentLocalTest.

* Address reviewer comments.

.github/workflows/main.yml

@@ -120,10 +120,10 @@ jobs:
  with:
  java-version: 1.9
 
- - name: Robolectric tests - FAQ, Help, Mydownloads, Parser, ProfileProgress, RecyclerView, Story, Utility tests
+ - name: Robolectric tests - FAQ, Help, Mydownloads, Parser, ProfileProgress, RecyclerView, State, Story, Utility tests
  # We require 'sudo' to avoid an error of the existing android sdk. See https://github.com/actions/starter-workflows/issues/58
  run: |
- sudo ./gradlew :app:testDebugUnitTest --tests org.oppia.app.faq* --tests org.oppia.app.help* --tests org.oppia.app.mydownloads* --tests org.oppia.app.parser* --tests org.oppia.app.profileprogress* --tests org.oppia.app.recyclerview* --tests org.oppia.app.splash* --tests org.oppia.app.story* --tests org.oppia.app.utility* --tests org.oppia.app.topic.questionplayer*
+ sudo ./gradlew :app:testDebugUnitTest --tests org.oppia.app.faq* --tests org.oppia.app.help* --tests org.oppia.app.mydownloads* --tests org.oppia.app.parser* --tests org.oppia.app.player.state* --tests org.oppia.app.profileprogress* --tests org.oppia.app.recyclerview* --tests org.oppia.app.splash* --tests org.oppia.app.story* --tests org.oppia.app.utility* --tests org.oppia.app.topic.questionplayer*
  - name: Upload App Test Reports
  uses: actions/upload-artifact@v2
  if: ${{ always() }} # IMPORTANT: Upload reports regardless of status

WORKSPACE

@@ -142,6 +142,7 @@ maven_install(
  "com.chaos.view:pinview:1.4.3",
  "com.crashlytics.sdk.android:crashlytics:2.9.8",
  "com.github.bumptech.glide:glide:4.11.0",
+ "com.github.bumptech.glide:mocks:4.11.0",
  "com.google.android.material:material:1.2.0-alpha02",
  "com.google.firebase:firebase-analytics:17.4.4",
  "com.google.firebase:firebase-crashlytics:17.1.1",

app/BUILD.bazel

@@ -637,6 +637,7 @@ TEST_DEPS = [
  artifact("androidx.test.espresso:espresso-intents:3.1.0"),
  artifact("androidx.test.ext:junit"),
  artifact("androidx.test:runner:1.2.0"),
+ artifact("com.github.bumptech.glide:mocks:4.11.0"),
  artifact("com.google.truth:truth"),
  artifact("org.jetbrains.kotlin:kotlin-test-junit"),
  artifact("org.jetbrains.kotlin:kotlin-reflect"),

app/build.gradle

@@ -85,7 +85,7 @@ dependencies {
  'androidx.multidex:multidex:2.0.1',
  'androidx.recyclerview:recyclerview:1.0.0',
  'com.chaos.view:pinview:1.4.3',
- 'com.github.bumptech.glide:glide:4.9.0',
+ 'com.github.bumptech.glide:glide:4.11.0',
  'com.google.android.material:material:1.2.0-alpha02',
  'com.google.dagger:dagger:2.24',
  'com.google.firebase:firebase-analytics-ktx:17.4.2',
@@ -106,6 +106,7 @@ dependencies {
  'androidx.test.espresso:espresso-intents:3.1.0',
  'androidx.test.ext:junit:1.1.1',
  'com.google.truth:truth:0.43',
+ 'com.github.bumptech.glide:mocks:4.11.0',
  'org.robolectric:annotations:4.3',
  'org.robolectric:robolectric:4.3',
  'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',

app/src/test/java/org/oppia/app/player/state/StateFragmentLocalTest.kt

@@ -27,13 +27,18 @@ import androidx.test.espresso.matcher.ViewMatchers.withId
 import androidx.test.espresso.matcher.ViewMatchers.withSubstring
 import androidx.test.espresso.matcher.ViewMatchers.withText
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.bumptech.glide.Glide
+import com.bumptech.glide.GlideBuilder
+import com.bumptech.glide.load.engine.executor.MockGlideExecutor
 import com.google.common.truth.Truth.assertThat
 import dagger.Component
+import kotlinx.coroutines.CoroutineDispatcher
 import org.hamcrest.BaseMatcher
 import org.hamcrest.CoreMatchers.allOf
 import org.hamcrest.CoreMatchers.not
 import org.hamcrest.Description
 import org.hamcrest.Matcher
+import org.junit.After
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -76,6 +81,7 @@ import org.oppia.domain.topic.FRACTIONS_EXPLORATION_ID_1
 import org.oppia.domain.topic.PrimeTopicAssetsControllerModule
 import org.oppia.domain.topic.TEST_STORY_ID_0
 import org.oppia.domain.topic.TEST_TOPIC_ID_0
+import org.oppia.testing.CoroutineExecutorService
 import org.oppia.testing.TestAccessibilityModule
 import org.oppia.testing.TestCoroutineDispatchers
 import org.oppia.testing.TestDispatcherModule
@@ -87,6 +93,7 @@ import org.oppia.util.logging.LoggerModule
 import org.oppia.util.parser.GlideImageLoaderModule
 import org.oppia.util.parser.HtmlParserEntityTypeModule
 import org.oppia.util.parser.ImageParsingModule
+import org.oppia.util.threading.BackgroundDispatcher
 import org.robolectric.annotation.Config
 import org.robolectric.annotation.LooperMode
 import org.robolectric.shadows.ShadowMediaPlayer
@@ -108,26 +115,44 @@ class StateFragmentLocalTest {
  createAudioUrl(explorationId = "MjZzEVOG47_1", audioFileName = "content-en-ouqm7j21vt8.mp3")
  private val audioDataSource1 = DataSource.toDataSource(AUDIO_URL_1, /* headers= */ null)
 
+ @Inject lateinit var profileTestHelper: ProfileTestHelper
+ @Inject lateinit var testCoroutineDispatchers: TestCoroutineDispatchers
+ @Inject @field:ApplicationContext lateinit var context: Context
  @Inject
- lateinit var profileTestHelper: ProfileTestHelper
-
- @Inject
- lateinit var testCoroutineDispatchers: TestCoroutineDispatchers
-
- @Inject
- @field:ApplicationContext
- lateinit var context: Context
+ @field:BackgroundDispatcher
+ lateinit var backgroundCoroutineDispatcher: CoroutineDispatcher
 
  private val internalProfileId: Int = 1
  private val solutionIndex: Int = 4
 
  @Before
  fun setUp() {
  setUpTestApplicationComponent()
+
+ // Initialize Glide such that all of its executors use the same shared dispatcher pool as the
+ // rest of Oppia so that thread execution can be synchronized via Oppia's test coroutine
+ // dispatchers.
+ val executorService = MockGlideExecutor.newTestExecutor(
+ CoroutineExecutorService(backgroundCoroutineDispatcher)
+ )
+ Glide.init(
+ context,
+ GlideBuilder().setDiskCacheExecutor(executorService)
+ .setAnimationExecutor(executorService)
+ .setSourceExecutor(executorService)
+ )
+
  profileTestHelper.initializeProfiles()
  ShadowMediaPlayer.addException(audioDataSource1, IOException("Test does not have networking"))
  }
 
+ @After
+ fun tearDown() {
+ // Ensure lingering tasks are completed (otherwise Glide can enter a permanently broken state
+ // during initialization for the next test).
+ testCoroutineDispatchers.advanceUntilIdle()
+ }
+
  @Test
  fun testStateFragment_loadExploration_explorationLoads() {
  launchForExploration(FRACTIONS_EXPLORATION_ID_1).use {
@@ -145,11 +170,11 @@ class StateFragmentLocalTest {
 
  onView(withId(R.id.state_recycler_view)).perform(scrollToViewType(SELECTION_INTERACTION))
  onView(withSubstring("the pieces must be the same size.")).perform(click())
- testCoroutineDispatchers.advanceUntilIdle()
+ testCoroutineDispatchers.runCurrent()
  onView(withId(R.id.state_recycler_view)).perform(scrollToViewType(CONTINUE_NAVIGATION_BUTTON))
- testCoroutineDispatchers.advanceUntilIdle()
+ testCoroutineDispatchers.runCurrent()
  onView(withId(R.id.continue_navigation_button)).perform(click())
- testCoroutineDispatchers.advanceUntilIdle()
+ testCoroutineDispatchers.runCurrent()
 
  onView(withSubstring("of the above circle is red?")).check(matches(isDisplayed()))
  }

testing/src/main/java/org/oppia/testing/OppiaTestAnnotations.kt

@@ -0,0 +1,28 @@
+package org.oppia.testing
+
+import org.oppia.testing.TestPlatform.ESPRESSO
+import org.oppia.testing.TestPlatform.ROBOLECTRIC
+
+/** Specifies a test platform to target in conjunction with [RunOn]. */
+enum class TestPlatform {
+ /** Corresponds to local tests run in the Java VM via Robolectric. */
+ ROBOLECTRIC,
+
+ /** Corresponds to instrumented tests that can run on a real device or emulator via Espresso. */
+ ESPRESSO
+}
+
+/**
+ * Test class or method annotation for specifying all of platforms which either the tests of the
+ * class or the specific method may run on. By default, tests are assumed to be able to run on both
+ * Espresso & Robolectric.
+ *
+ * The target platforms are specified as varargs of [TestPlatform]s.
+ *
+ * Note that this annotation only works if the test also has an [OppiaTestRule] hooked up.
+ *
+ * Note that when defined on both a class and a method, the list of platforms defined on the method
+ * is used and any defined at the class level are ignored.
+ */
+@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION)
+annotation class RunOn(vararg val testPlatforms: TestPlatform = [ROBOLECTRIC, ESPRESSO])

testing/src/main/java/org/oppia/testing/OppiaTestRule.kt

@@ -0,0 +1,56 @@
+package org.oppia.testing
+
+import android.os.Build
+import org.junit.AssumptionViolatedException
+import org.junit.rules.TestRule
+import org.junit.runner.Description
+import org.junit.runners.model.Statement
+
+/** JUnit rule to enable [RunOn] test targeting. */
+class OppiaTestRule : TestRule {
+ override fun apply(base: Statement?, description: Description?): Statement {
+ return object : Statement() {
+ override fun evaluate() {
+ val targetPlatforms = description.getTargetPlatforms()
+ val currentPlatform = getCurrentPlatform()
+ if (currentPlatform in targetPlatforms) {
+ // Only run this test if it's targeting the current platform.
+ base?.evaluate()
+ } else {
+ // See https://github.com/junit-team/junit4/issues/116 for context.
+ throw AssumptionViolatedException(
+ "Test targeting ${targetPlatforms.toPluralDescription()} ignored on $currentPlatform"
+ )
+ }
+ }
+ }
+ }
+
+ private fun getCurrentPlatform(): TestPlatform {
+ return if (Build.FINGERPRINT.contains("robolectric", ignoreCase = true)) {
+ TestPlatform.ROBOLECTRIC
+ } else {
+ TestPlatform.ESPRESSO
+ }
+ }
+
+ private companion object {
+ private fun Array<out TestPlatform>.toPluralDescription(): String {
+ return if (size > 1) "platforms ${this.joinToString()}" else "platform ${this.first()}"
+ }
+
+ private fun Description?.getTargetPlatforms(): Array<out TestPlatform> {
+ val methodTargetPlatforms = this?.getTargetTestPlatforms()
+ val classTargetPlatforms = this?.testClass?.getTargetTestPlatforms()
+ return methodTargetPlatforms ?: classTargetPlatforms ?: TestPlatform.values()
+ }
+
+ private fun Description.getTargetTestPlatforms(): Array<out TestPlatform>? {
+ return getAnnotation(RunOn::class.java)?.testPlatforms
+ }
+
+ private fun <T> Class<T>.getTargetTestPlatforms(): Array<out TestPlatform>? {
+ return getAnnotation(RunOn::class.java)?.testPlatforms
+ }
+ }
+}

utility/build.gradle

@@ -48,7 +48,7 @@ dependencies {
  'androidx.appcompat:appcompat:1.0.2',
  'androidx.lifecycle:lifecycle-livedata-ktx:2.2.0-alpha03',
  'com.caverock:androidsvg-aar:1.4',
- 'com.github.bumptech.glide:glide:4.9.0',
+ 'com.github.bumptech.glide:glide:4.11.0',
  'com.google.dagger:dagger:2.24',
  'com.google.firebase:firebase-analytics-ktx:17.4.2',
  'com.google.firebase:firebase-core:17.4.2',
@@ -68,7 +68,7 @@ dependencies {
  project(":testing"),
  )
  kapt(
- 'com.github.bumptech.glide:compiler:4.9.0',
+ 'com.github.bumptech.glide:compiler:4.11.0',
  'com.google.dagger:dagger-compiler:2.24'
  )
  kaptTest(