From 541393dede67ecebd0b3b8980f24dc76ffa9d05a Mon Sep 17 00:00:00 2001 From: Colin White Date: Sun, 10 May 2020 21:47:40 -0700 Subject: [PATCH] Make RequestDisposableTest less flaky. (#401) * Make RequestDisposableTest less flaky. * Test again. * Test. * Prevent race conditions. * Minor cleanup. * Use different isDisposed implementation. * Test. * Debug. * Fix. * Restart CI. * Print stack trace. * Debug. * Test. * Attempt. * Remove debug code. * Remove repeat. --- .../coil/request/RequestDisposableTest.kt | 93 ++++++++++--------- .../java/coil/util/SystemCallbacksTest.kt | 1 + .../coil/memory/ViewTargetRequestManager.kt | 16 ++-- 3 files changed, 60 insertions(+), 50 deletions(-) diff --git a/coil-base/src/androidTest/java/coil/request/RequestDisposableTest.kt b/coil-base/src/androidTest/java/coil/request/RequestDisposableTest.kt index 2dd00641f2..dcf630dec5 100644 --- a/coil-base/src/androidTest/java/coil/request/RequestDisposableTest.kt +++ b/coil-base/src/androidTest/java/coil/request/RequestDisposableTest.kt @@ -1,24 +1,24 @@ package coil.request -import android.content.ContentResolver.SCHEME_CONTENT +import android.content.ContentResolver.SCHEME_FILE import android.content.Context +import android.graphics.Bitmap import android.graphics.drawable.Drawable import android.widget.ImageView import androidx.test.core.app.ApplicationProvider import coil.ImageLoader -import coil.RealImageLoader import coil.annotation.ExperimentalCoilApi -import coil.transition.Transition -import coil.transition.TransitionTarget +import coil.bitmappool.BitmapPool +import coil.fetch.AssetUriFetcher.Companion.ASSET_FILE_PATH_ROOT +import coil.size.Size +import coil.transform.Transformation import coil.util.CoilUtils import coil.util.requestManager import coil.util.runBlockingTest import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.FlowPreview -import kotlinx.coroutines.channels.ConflatedBroadcastChannel -import kotlinx.coroutines.flow.asFlow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.first -import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Before import org.junit.Test @@ -31,12 +31,14 @@ import kotlin.test.assertTrue class RequestDisposableTest { private lateinit var context: Context - private lateinit var imageLoader: RealImageLoader + private lateinit var imageLoader: ImageLoader @Before fun before() { context = ApplicationProvider.getApplicationContext() - imageLoader = ImageLoader(context) as RealImageLoader + imageLoader = ImageLoader.Builder(context) + .memoryCachePolicy(CachePolicy.DISABLED) + .build() } @After @@ -45,9 +47,11 @@ class RequestDisposableTest { } @Test - fun baseTargetRequestDisposable_dispose() { + fun baseTargetRequestDisposable_dispose() = runBlockingTest { val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") + .size(100, 100) + .transformations(GateTransformation()) .target { /** Do nothing. */ } .build() val disposable = imageLoader.execute(request) @@ -59,29 +63,32 @@ class RequestDisposableTest { } @Test - fun baseTargetRequestDisposable_await() { + fun baseTargetRequestDisposable_await() = runBlockingTest { + val transformation = GateTransformation() var result: Drawable? = null val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") + .size(100, 100) + .transformations(transformation) .target { result = it } .build() val disposable = imageLoader.execute(request) assertTrue(disposable is BaseTargetRequestDisposable) assertNull(result) - runBlocking { disposable.await() } + transformation.open() + disposable.await() assertNotNull(result) } @Test - fun viewTargetRequestDisposable_dispose() { - val transition = GateTransition() + fun viewTargetRequestDisposable_dispose() = runBlockingTest { val imageView = ImageView(context) val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured. .size(100, 100) - .transition(transition) + .transformations(GateTransformation()) .target(imageView) .build() val disposable = imageLoader.execute(request) @@ -93,34 +100,34 @@ class RequestDisposableTest { } @Test - fun viewTargetRequestDisposable_await() { - val transition = GateTransition() + fun viewTargetRequestDisposable_await() = runBlockingTest { + val transformation = GateTransformation() val imageView = ImageView(context) val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured. .size(100, 100) - .transition(transition) + .transformations(transformation) .target(imageView) .build() val disposable = imageLoader.execute(request) assertTrue(disposable is ViewTargetRequestDisposable) assertNull(imageView.drawable) - transition.open() - runBlocking { disposable.await() } + transformation.open() + disposable.await() assertNotNull(imageView.drawable) } @Test fun viewTargetRequestDisposable_restart() = runBlockingTest { - val transition = GateTransition() + val transformation = GateTransformation() val imageView = ImageView(context) val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured. .size(100, 100) - .transition(transition) + .transformations(transformation) .target(imageView) .build() val disposable = imageLoader.execute(request) @@ -128,7 +135,7 @@ class RequestDisposableTest { assertTrue(disposable is ViewTargetRequestDisposable) assertFalse(disposable.isDisposed) - transition.open() + transformation.open() disposable.await() assertFalse(disposable.isDisposed) @@ -144,15 +151,14 @@ class RequestDisposableTest { @Test fun viewTargetRequestDisposable_replace() = runBlockingTest { - val transition = GateTransition() val imageView = ImageView(context) fun launchNewRequest(): RequestDisposable { val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured. .size(100, 100) - .transition(transition) + .transformations(GateTransformation()) .target(imageView) .build() return imageLoader.execute(request) @@ -172,13 +178,12 @@ class RequestDisposableTest { @Test fun viewTargetRequestDisposable_clear() = runBlockingTest { - val transition = GateTransition() val imageView = ImageView(context) val request = LoadRequest.Builder(context) - .data("$SCHEME_CONTENT://coil/normal.jpg") + .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg") // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured. .size(100, 100) - .transition(transition) + .transformations(GateTransformation()) .target(imageView) .build() val disposable = imageLoader.execute(request) @@ -192,21 +197,21 @@ class RequestDisposableTest { * Prevent completing the [LoadRequest] until [open] is called. * This is to avoid our test assertions racing the image request. */ - private class GateTransition : Transition { + private class GateTransformation : Transformation { + + private val isOpen = MutableStateFlow(false) - private val isOpen = ConflatedBroadcastChannel(false) + override fun key() = GateTransformation::class.java.name - override suspend fun transition( - target: TransitionTarget<*>, - result: RequestResult - ) { + override suspend fun transform(pool: BitmapPool, input: Bitmap, size: Size): Bitmap { // Suspend until the gate is open. - isOpen.asFlow().first { it } + isOpen.first { it } - // Delegate to the empty transition. - Transition.NONE.transition(target, result) + return input } - fun open() = isOpen.offer(true) + fun open() { + isOpen.value = true + } } } diff --git a/coil-base/src/androidTest/java/coil/util/SystemCallbacksTest.kt b/coil-base/src/androidTest/java/coil/util/SystemCallbacksTest.kt index 3e272a78e8..0815b32dca 100644 --- a/coil-base/src/androidTest/java/coil/util/SystemCallbacksTest.kt +++ b/coil-base/src/androidTest/java/coil/util/SystemCallbacksTest.kt @@ -43,6 +43,7 @@ class SystemCallbacksTest { // Keep allocating bitmaps until either the image loader is freed or we run out of memory. bitmaps += createBitmap(500, 500, Bitmap.Config.ARGB_8888) } + bitmaps.clear() // Ensure that the next system callback is called. systemCallbacks.onTrimMemory(TRIM_MEMORY_BACKGROUND) diff --git a/coil-base/src/main/java/coil/memory/ViewTargetRequestManager.kt b/coil-base/src/main/java/coil/memory/ViewTargetRequestManager.kt index b55dbedf29..9c1faf5c89 100644 --- a/coil-base/src/main/java/coil/memory/ViewTargetRequestManager.kt +++ b/coil-base/src/main/java/coil/memory/ViewTargetRequestManager.kt @@ -31,6 +31,7 @@ internal class ViewTargetRequestManager : View.OnAttachStateChangeListener { // A pending operation that is posting to the main thread to clear the current request. @Volatile private var pendingClear: Job? = null + // Only accessed from the main thread. private var isRestart = false private var skipAttach = true @@ -73,11 +74,14 @@ internal class ViewTargetRequestManager : View.OnAttachStateChangeListener { override fun onViewAttachedToWindow(v: View) { if (skipAttach) { skipAttach = false - } else { - currentRequest?.let { request -> - isRestart = true - request.restart() - } + return + } + + currentRequest?.let { request -> + // As this is called from the main thread, isRestart will + // be cleared synchronously as part of request.restart(). + isRestart = true + request.restart() } } @@ -93,7 +97,7 @@ internal class ViewTargetRequestManager : View.OnAttachStateChangeListener { // Return the current request ID if this is a restarted request. // Restarted requests are always launched from the main thread. val requestId = currentRequestId - if (requestId != null && isMainThread() && isRestart) { + if (requestId != null && isRestart && isMainThread()) { return requestId }