Properly handle background thread processing in AndroidX tests #89
Assignees
Labels
enhancement
End user-perceivable enhancements.
Impact: Low
Low perceived user impact (e.g. edge cases).
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Work: Medium
The means to find the solution is clear, but it isn't at good-first-issue level yet.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Comments
BenHenning
added
Type: Improvement
Priority: Important
BenHenning
changed the title
|
This is actually blocked on #59 since this partially requires support for testing libraries in order to have shared testing infrastructure. |
|
NB: Kotlin/kotlinx.coroutines#242 might be an interesting solution to this if it becomes available. |
Closed
8 tasks
|
NB: this probably blocks exploration fragment tests since the exploration takes a while to load, and we need Espresso to properly idle until all the background processing is complete. |
8 tasks
This was referenced Dec 2, 2019
BenHenning
added a commit
that referenced
this issue
Jun 24, 2020
* Update hints & solutions handling to more closely follow the intended behavior graph (and added an actual visual representation of this graph in code to simplify maintaining the solution in the future). Tests will be added as part of solving #1273. * Add support for sharing the test application component between UI dependencies and tests. Also, fix the test coroutine dispatcher to properly manage time in tests. This enables the test coroutine dispatcher for app module tests. * Clean up the documentation for test utilities. * Address reviewer comment.
This was referenced Aug 12, 2020
BenHenning
added a commit
that referenced
this issue
Aug 12, 2020
* 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. * Add thorough documentation for new dispatchers. * Clean up comments & add additional documentation. * Fix lint errors. * Fix broken test after changes to FakeSystemClock.
BenHenning
added a commit
that referenced
this issue
Aug 12, 2020
* 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. * Add thorough documentation for new dispatchers. * Broadly clean up test dispatchers. TestCoroutineDispatchers have been used in a few heavy Robolectric tests for a couple months now, without issue. This change broadly migrates all test dispatchers over to TestCoroutineDispatchers and TestDispatcherModule, resulting in significant and broad cleanups. This change also includes broadly removing now-unnecessary uses of various Kotlin coroutine annotations, and runBlockingTest() since TestCoroutineDispatchers replaces this functionality & does it more correctly. Some things to note: 1. This change also includes moving ProfileTestHelper to the testing module since it's been changed to rely on TestCoroutineDispatcher (which improves downstream test stability in the app module). 2. MainThreadExecutor was removed as the test disaptcher in StateFragmentTest and HomeActivityTest which may cause failures in these tests. Since neither test is part of the non-flaky test run, this is considered fine here. 3. Notice that runCurrent() has been moved around or added in a bunch of places--this is to be expected. TestCoroutineDispatchers always operates in a paused state, whereas before all test dispatchers were generally operating in a running state. These additional calls are needed to synchronize access. In some special cases, an additional coroutine dispatcher is being used to demonstrate blocking functionality without interfering with TestCoroutineDispatcher functionality. Also, note that missing runCurrent() in app module tests that depend on waitFor() can result in long failures (30 seconds for the timeout), so long-term tests should opt to avoid using waitFor() since TestCoroutineDispatchers also has an idling resource for Espresso use cases. * Update all tests to use paused looper with Robolectric. This is the recommended mode to run all Robolectric tests, and it interoperates better with TestCoroutineDispatchers. Note that this commit also includes some functional changes to AudioPlayerControllerTest. Before, the audio track being played was 1 second long, and 1 second needs to be waited before audio is detected as playing. These race against each otehr in paused looper mode, so extending the track duration to 2 seconds fixes the situation. I also made some minor cleanups elsewhere in the test. * Add testing package to CI & ktlint. * Clean up comments & add additional documentation. * Fix lint errors. * Fix broken test after changes to FakeSystemClock. * Fix linter failures. * Fix broken BindableAdapterTest post update with develop. The adapter test requires synchronization when running with a paused looper mode to make sure state is propagated when desired. * Clean up new test post-merge.
8 tasks
Merged
8 tasks
Broppia
added
Impact: Low
BenHenning
moved this to Needs Triage
in [Team] Developer Workflow & Infrastructure - Android
BenHenning
added
Z-ibt
seanlip
added
enhancement
MohitGupta121
added
the
Work: Medium
MohitGupta121
moved this from Todo
to In Progress
in [Team] Developer Workflow & Infrastructure - Android
6 tasks
seanlip
removed
the
Priority: Important
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
tl;dr: The project needs a way to consistently test operations which require background processing in a way that's consistent with production environments,
UI-centric AndroidX tests can run on both Robolectric and Espresso. Both Robolectric and Espresso have their own ways to block on background tasks completing.
Robolectric running in legacy mode can deterministically advance the system clock and looper to execute pending tasks. This, combined with Kotlin's experimental runBlockingTest provides a nice way to at least test background tasks that are blocking on coroutines. However, this isn't fool-proof, and has its own issues (e.g. best practice is to use InstantTaskExecutorRule which actually forces synchronous execution).
Espresso, on the other hand, has the choice of idling resources. These allow the app to communicate to Espresso when a non-UI bound task has completed which pairs well with Espresso's internal idling mechanism. This helps improve test determinism for Espresso.
Having two solutions isn't viable for shared AndroidX tests, and Robolectric's approach results in synchronizing background processing which reduces the chance of discovering race conditions. Instead, Oppia Android should have a way to run background tasks similar to production (i.e. by not synchronizing them or forcing them to run on the main thread), and ensure both testing frameworks can consistently wait for completion (probably by using idling resource since Robolectric should support them, or using either idling resource or a Robolectric-specific solution). This should also be done in conjunction with running Robolectric in a paused looper mode to have better parity with Android and try to detect real timing regressions that could occur in production.
The text was updated successfully, but these errors were encountered: