Skip to content

Commit

Permalink
Fix #90: Introduce new coroutine live data mechanism (#928)
Browse files Browse the repository at this point in the history 
* Initial introduction of test coroutine dispatchers to replace Kotlin's
test coroutine dispatcher.

This includes introducing a test-only module to contain testing
dependencies.

* Introduce a new LiveData mechanism to bridge coroutines from
DataProviders in a way that doesn't have the same limitations as the
previous MutableLiveData-based bridge.

* Early work at introducing FakeSystemClock tests (not yet complete).

* Remove infeasible testing structures, add documentation, and clean up
implementation to prepare for code review.

* Add notice that the dispatchers utility is temporary.

* Cleanup new LiveData bridge, add tests for it, and migrate other
DataProviders tests to using TestCoroutineDispatchers utility.

* Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments.

* Use ktlint to reformat TestCoroutineDispatchers per reviewer comment
thread.

* Reformat files failing linter check.

* Address reviewer comment.
  • Loading branch information
@BenHenning
BenHenning authored May 28, 2020
1 parent 6f31261 commit 9cb5817
Show file tree
Hide file tree
Showing 13 changed files with 1,031 additions and 508 deletions.
1 change: 1 addition & 0 deletions data/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ dependencies {
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
'org.mockito:mockito-core:2.19.0',
'org.robolectric:robolectric:4.3',
project(":testing"),
)
// TODO (#59): Remove this once Bazel is introduced
// TODO (#97): Isolate retrofit-mock dependency from production
Expand Down
21 changes: 9 additions & 12 deletions data/src/main/java/org/oppia/data/persistence/PersistentCacheStore.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ class PersistentCacheStore<T : MessageLite> private constructor(
@GuardedBy("failureLock") private var deferredLoadCacheFailure: Throwable? = null
private val cache = cacheFactory.create(CachePayload(state = CacheState.UNLOADED, value = initialValue))

init {
cache.observeChanges {
asyncDataSubscriptionManager.notifyChange(providerId)
}
}

override fun getId(): Any {
return providerId
}
Expand All @@ -54,7 +60,7 @@ class PersistentCacheStore<T : MessageLite> private constructor(
cache.readIfPresentAsync().await().let { cachePayload ->
// First, determine whether the current cache has been attempted to be retrieved from disk.
if (cachePayload.state == CacheState.UNLOADED) {
deferLoadFileAndNotify()
deferLoadFile()
return AsyncResult.pending()
}

Expand Down Expand Up @@ -109,7 +115,6 @@ class PersistentCacheStore<T : MessageLite> private constructor(
val deferred = cache.updateWithCustomChannelIfPresentAsync { cachePayload ->
if (cachePayload.state == CacheState.UNLOADED) {
val filePayload = loadFileCache(cachePayload)
asyncDataSubscriptionManager.notifyChange(providerId)
Pair(filePayload, filePayload.value)
} else {
Pair(cachePayload, cachePayload.value)
Expand All @@ -135,9 +140,6 @@ class PersistentCacheStore<T : MessageLite> private constructor(
*/
fun storeDataAsync(updateInMemoryCache: Boolean = true, update: (T) -> T): Deferred<Any> {
return cache.updateIfPresentAsync { cachedPayload ->
// Although it's odd to notify before the change is made, the single threaded nature of the blocking cache ensures
// nothing can read from it until this update completes.
asyncDataSubscriptionManager.notifyChange(providerId)
val updatedPayload = storeFileCache(cachedPayload, update)
if (updateInMemoryCache) updatedPayload else cachedPayload
}
Expand All @@ -146,16 +148,13 @@ class PersistentCacheStore<T : MessageLite> private constructor(
/** See [storeDataAsync]. Stores data and allows for a custom deferred result. */
fun <V> storeDataWithCustomChannelAsync(updateInMemoryCache: Boolean = true, update: (T) -> Pair<T, V>): Deferred<V> {
return cache.updateWithCustomChannelIfPresentAsync { cachedPayload ->
// Although it's odd to notify before the change is made, the single threaded nature of the blocking cache ensures
// nothing can read from it until this update completes.
asyncDataSubscriptionManager.notifyChange(providerId)
val (updatedPayload, customResult) = storeFileCacheWithCustomChannel(cachedPayload, update)
if (updateInMemoryCache) Pair(updatedPayload, customResult) else Pair(cachedPayload, customResult)
}
}

/**
* Returns a [Deferred] indicating when the cache was cleared and its on-disk file, removed. This does not notify
* Returns a [Deferred] indicating when the cache was cleared and its on-disk file, removed. This does notify
* subscribers.
*/
fun clearCacheAsync(): Deferred<Any> {
Expand All @@ -172,10 +171,8 @@ class PersistentCacheStore<T : MessageLite> private constructor(
}
}

private fun deferLoadFileAndNotify() {
// Schedule another update to the cache that actually loads the file from memory. Record any potential failures.
private fun deferLoadFile() {
cache.updateIfPresentAsync { cachePayload ->
asyncDataSubscriptionManager.notifyChange(providerId)
loadFileCache(cachePayload)
}.invokeOnCompletion {
failureLock.withLock {
Expand Down
202 changes: 106 additions & 96 deletions data/src/test/java/org/oppia/data/persistence/PersistentCacheStoreTest.kt
View file

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions domain/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,13 @@ dependencies {
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
'org.mockito:mockito-core:2.19.0',
'org.robolectric:robolectric:4.3',
project(":testing"),
)
kapt(
'com.google.dagger:dagger-compiler:2.24'
'com.google.dagger:dagger-compiler:2.24',
)
kaptTest(
'com.google.dagger:dagger-compiler:2.24'
'com.google.dagger:dagger-compiler:2.24',
)
// TODO(#59): Avoid needing to expose data implementations to other modules in order to make Oppia symbols
// sufficiently visible for generated Dagger code. This can be done more cleanly via Bazel since dependencies can be
Expand Down
96 changes: 26 additions & 70 deletions domain/src/test/java/org/oppia/domain/audio/CellularAudioDialogControllerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@ import dagger.BindsInstance
import dagger.Component
import dagger.Module
import dagger.Provides
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.InternalCoroutinesApi
import kotlinx.coroutines.ObsoleteCoroutinesApi
import kotlinx.coroutines.test.TestCoroutineDispatcher
import kotlinx.coroutines.test.runBlockingTest
import org.junit.Before
import org.junit.Rule
import org.junit.Test
Expand All @@ -27,21 +25,21 @@ import org.mockito.Mockito.verify
import org.mockito.junit.MockitoJUnit
import org.mockito.junit.MockitoRule
import org.oppia.app.model.CellularDataPreference
import org.oppia.testing.TestCoroutineDispatchers
import org.oppia.testing.TestDispatcherModule
import org.oppia.util.data.AsyncResult
import org.oppia.util.logging.EnableConsoleLog
import org.oppia.util.logging.EnableFileLog
import org.oppia.util.logging.GlobalLogLevel
import org.oppia.util.logging.LogLevel
import org.oppia.util.threading.BackgroundDispatcher
import org.oppia.util.threading.BlockingDispatcher
import org.robolectric.annotation.Config
import org.robolectric.annotation.LooperMode
import javax.inject.Inject
import javax.inject.Qualifier
import javax.inject.Singleton
import kotlin.coroutines.EmptyCoroutineContext

@RunWith(AndroidJUnit4::class)
@Config(manifest = Config.NONE)
@LooperMode(LooperMode.Mode.PAUSED)
class CellularAudioDialogControllerTest {
@Rule
@JvmField
Expand All @@ -50,16 +48,8 @@ class CellularAudioDialogControllerTest {
@Inject lateinit var cellularAudioDialogController: CellularAudioDialogController

@Inject
@field:TestDispatcher
lateinit var testDispatcher: CoroutineDispatcher

@Inject
@field:TestBlockingDispatcher
lateinit var testBlockingDispatcher: TestCoroutineDispatcher

private val coroutineContext by lazy {
EmptyCoroutineContext + testDispatcher
}
@InternalCoroutinesApi
lateinit var testCoroutineDispatchers: TestCoroutineDispatchers

@Mock
lateinit var mockCellularDataObserver: Observer<AsyncResult<CellularDataPreference>>
Expand All @@ -72,8 +62,6 @@ class CellularAudioDialogControllerTest {
@ObsoleteCoroutinesApi
fun setUp() {
setUpTestApplicationComponent()
// Separate dispatcher is needed for PersistentCacheStore to behave similar to production.
testBlockingDispatcher.pauseDispatcher()
}

private fun setUpTestApplicationComponent() {
Expand All @@ -85,11 +73,11 @@ class CellularAudioDialogControllerTest {

@Test
@ExperimentalCoroutinesApi
fun testController_providesInitialLiveData_indicatesToNotHideDialogAndNotUseCellularData()
= runBlockingTest(coroutineContext) {
@InternalCoroutinesApi
fun testController_providesInitialLiveData_indicatesToNotHideDialogAndNotUseCellularData() {
val cellularDataPreference = cellularAudioDialogController.getCellularDataPreference()
cellularDataPreference.observeForever(mockCellularDataObserver)
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

verify(mockCellularDataObserver, atLeastOnce()).onChanged(cellularDataResultCaptor.capture())
assertThat(cellularDataResultCaptor.value.isSuccess()).isTrue()
Expand All @@ -99,13 +87,13 @@ class CellularAudioDialogControllerTest {

@Test
@ExperimentalCoroutinesApi
fun testController_setNeverUseCellularDataPref_providesLiveData_indicatesToHideDialogAndNotUseCellularData()
= runBlockingTest(coroutineContext) {
@InternalCoroutinesApi
fun testController_setNeverUseCellularDataPref_providesLiveData_indicatesToHideDialogAndNotUseCellularData() {
val appHistory = cellularAudioDialogController.getCellularDataPreference()

appHistory.observeForever(mockCellularDataObserver)
cellularAudioDialogController.setNeverUseCellularDataPreference()
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

verify(mockCellularDataObserver, atLeastOnce()).onChanged(cellularDataResultCaptor.capture())
assertThat(cellularDataResultCaptor.value.isSuccess()).isTrue()
Expand All @@ -115,13 +103,13 @@ class CellularAudioDialogControllerTest {

@Test
@ExperimentalCoroutinesApi
fun testController_setAlwaysUseCellularDataPref_providesLiveData_indicatesToHideDialogAndUseCellularData()
= runBlockingTest(coroutineContext) {
@InternalCoroutinesApi
fun testController_setAlwaysUseCellularDataPref_providesLiveData_indicatesToHideDialogAndUseCellularData() {
val appHistory = cellularAudioDialogController.getCellularDataPreference()

appHistory.observeForever(mockCellularDataObserver)
cellularAudioDialogController.setAlwaysUseCellularDataPreference()
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

verify(mockCellularDataObserver, atLeastOnce()).onChanged(cellularDataResultCaptor.capture())
assertThat(cellularDataResultCaptor.value.getOrThrow().hideDialog).isTrue()
Expand All @@ -130,15 +118,16 @@ class CellularAudioDialogControllerTest {

@Test
@ExperimentalCoroutinesApi
fun testController_setNeverUseCellularDataPref_observedNewController_indicatesToHideDialogAndNotUseCellularData()
= runBlockingTest(coroutineContext) {
@InternalCoroutinesApi
fun testController_setNeverUseCellularDataPref_observedNewController_indicatesToHideDialogAndNotUseCellularData() {
// Pause immediate dispatching to avoid an infinite loop within the provider pipeline.
cellularAudioDialogController.setNeverUseCellularDataPreference()
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

setUpTestApplicationComponent()
val appHistory = cellularAudioDialogController.getCellularDataPreference()
appHistory.observeForever(mockCellularDataObserver)
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

verify(mockCellularDataObserver, atLeastOnce()).onChanged(cellularDataResultCaptor.capture())
assertThat(cellularDataResultCaptor.value.isSuccess()).isTrue()
Expand All @@ -148,25 +137,22 @@ class CellularAudioDialogControllerTest {

@Test
@ExperimentalCoroutinesApi
fun testController_setAlwaysUseCellularDataPref_observedNewController_indicatesToHideDialogAndUseCellularData()
= runBlockingTest(coroutineContext) {
@InternalCoroutinesApi
fun testController_setAlwaysUseCellularDataPref_observedNewController_indicatesToHideDialogAndUseCellularData() {
cellularAudioDialogController.setAlwaysUseCellularDataPreference()
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

setUpTestApplicationComponent()
val appHistory = cellularAudioDialogController.getCellularDataPreference()
appHistory.observeForever(mockCellularDataObserver)
testBlockingDispatcher.advanceUntilIdle()
testCoroutineDispatchers.advanceUntilIdle()

verify(mockCellularDataObserver, atLeastOnce()).onChanged(cellularDataResultCaptor.capture())
assertThat(cellularDataResultCaptor.value.isSuccess()).isTrue()
assertThat(cellularDataResultCaptor.value.getOrThrow().hideDialog).isTrue()
assertThat(cellularDataResultCaptor.value.getOrThrow().useCellularData).isTrue()
}

@Qualifier annotation class TestDispatcher
@Qualifier annotation class TestBlockingDispatcher

// TODO(#89): Move this to a common test application component.
@Module
class TestModule {
Expand All @@ -176,36 +162,6 @@ class CellularAudioDialogControllerTest {
return application
}

@ExperimentalCoroutinesApi
@Singleton
@Provides
@TestDispatcher
fun provideTestDispatcher(): CoroutineDispatcher {
return TestCoroutineDispatcher()
}

@Singleton
@Provides
@BackgroundDispatcher
fun provideBackgroundDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher {
return testDispatcher
}

@Singleton
@Provides
@BlockingDispatcher
fun provideBlockingDispatcher(@TestBlockingDispatcher testDispatcher: TestCoroutineDispatcher): CoroutineDispatcher {
return testDispatcher
}

@ExperimentalCoroutinesApi
@Singleton
@Provides
@TestBlockingDispatcher
fun provideTestBlockingDispatcher(): TestCoroutineDispatcher {
return TestCoroutineDispatcher()
}

// 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
Expand All @@ -223,7 +179,7 @@ class CellularAudioDialogControllerTest {

// TODO(#89): Move this to a common test application component.
@Singleton
@Component(modules = [TestModule::class])
@Component(modules = [TestDispatcherModule::class, TestModule::class])
interface TestApplicationComponent {
@Component.Builder
interface Builder {
Expand Down
10 changes: 7 additions & 3 deletions testing/src/main/java/org/oppia/testing/FakeSystemClock.kt
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
package org.oppia.testing

import android.os.SystemClock
import org.robolectric.Robolectric
import java.util.concurrent.atomic.AtomicLong
import javax.inject.Inject

// TODO(#89): Actually finish this implementation so that it properly works across Robolectric and
// Espresso, and add tests for it.
// Espresso, and add tests for it.
/**
* A Robolectric-specific fake for the system clock that can be used to manipulate time in a
* consistent way.
*/
class FakeSystemClock @Inject constructor() {
private val currentTimeMillis = AtomicLong(0L)
private val currentTimeMillis: AtomicLong

init {
SystemClock.setCurrentTimeMillis(0)
val initialMillis = Robolectric.getForegroundThreadScheduler().currentTime
SystemClock.setCurrentTimeMillis(initialMillis)
currentTimeMillis = AtomicLong(initialMillis)
}

fun getTimeMillis(): Long = currentTimeMillis.get()

fun advanceTime(millis: Long): Long {
val newTime = currentTimeMillis.addAndGet(millis)
Robolectric.getForegroundThreadScheduler().advanceTo(newTime)
SystemClock.setCurrentTimeMillis(newTime)
return newTime
}
Expand Down
1 change: 1 addition & 0 deletions utility/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ dependencies {
'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2',
'org.mockito:mockito-core:2.19.0',
'org.robolectric:robolectric:4.3',
project(":testing"),
)
kapt(
'com.github.bumptech.glide:compiler:4.9.0',
Expand Down
19 changes: 10 additions & 9 deletions utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import javax.inject.Inject
import javax.inject.Singleton
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import org.oppia.util.threading.BackgroundDispatcher
import org.oppia.util.threading.ConcurrentQueueMap
Expand Down Expand Up @@ -52,19 +51,21 @@ class AsyncDataSubscriptionManager @Inject constructor(
associatedIds.enqueue(parentId, childId)
}

/**
* Removes the specified association between parent and child IDs to prevent notifications to the parent ID from also
* notifying observers of the child ID.
*/
internal fun dissociateIds(childId: Any, parentId: Any) {
associatedIds.dequeue(parentId, childId)
}

/**
* Notifies all subscribers of the specified [DataProvider] id that the provider has been changed and should be
* re-queried for its latest state.
*/
suspend fun notifyChange(id: Any) {
// Ensure observed changes are called specifically on the main thread since that's what NotifiableAsyncLiveData
// expects.
// TODO(#90): Update NotifiableAsyncLiveData so that observeChange() can occur on background threads to avoid any
// load on the UI thread until the final data value is ready for delivery.
val scope = CoroutineScope(Dispatchers.Main)
scope.launch {
subscriptionMap.getQueue(id).forEach { observeChange -> observeChange() }
}
// Notify subscribers.
subscriptionMap.getQueue(id).forEach { observeChange -> observeChange() }

// Also notify all children observing this parent.
associatedIds.getQueue(id).forEach { childId -> notifyChange(childId) }
Expand Down
Loading

0 comments on commit 9cb5817

Please sign in to comment.