From b80e28c207c51e352f4b51be891005af3ed96e75 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Sun, 2 Jul 2023 16:41:33 -0700 Subject: [PATCH] Use onIdle to avoid a race in FlowTests Glide's executor thread notifies the flow in a loop while holding a lock on the resource. When it finishes, it releases the lock. If the flow wins the race and runs before Glide's executor releases the lock, the resource will not be recycled in the remainder of the test method. If the executor wins the race and releases the lock first, then the resource will be recycled in the rest of the test method. To fix this race, I've used Espresso's onIdle and idling resources, similar to the compose rule. --- integration/ktx/build.gradle | 2 ++ .../glide/integration/ktx/FlowsTest.kt | 21 ++++++++--- .../executor/GlideIdlingResourceInit.kt | 36 +++++++++++++++++++ settings.gradle | 4 +-- 4 files changed, 56 insertions(+), 7 deletions(-) create mode 100644 integration/ktx/src/test/java/com/bumptech/glide/load/engine/executor/GlideIdlingResourceInit.kt diff --git a/integration/ktx/build.gradle b/integration/ktx/build.gradle index b90be75c4c..b45e28d72e 100644 --- a/integration/ktx/build.gradle +++ b/integration/ktx/build.gradle @@ -42,6 +42,8 @@ dependencies { api project(":library") implementation libs.androidx.core.ktx implementation libs.coroutines.core + testImplementation libs.androidx.espresso + testImplementation libs.androidx.espresso.idling testImplementation libs.androidx.test.ktx testImplementation libs.kotlin.junit testImplementation libs.androidx.test.ktx.junit diff --git a/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt b/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt index bc784cd8e7..e13dc1886a 100644 --- a/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt +++ b/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt @@ -10,6 +10,7 @@ import android.graphics.drawable.ColorDrawable import android.graphics.drawable.Drawable import android.net.Uri import androidx.test.core.app.ApplicationProvider +import androidx.test.espresso.Espresso.onIdle import androidx.test.ext.junit.runners.AndroidJUnit4 import com.bumptech.glide.Glide import com.bumptech.glide.GlideBuilder @@ -20,6 +21,7 @@ import com.bumptech.glide.load.Options import com.bumptech.glide.load.engine.GlideException import com.bumptech.glide.load.engine.cache.MemoryCache import com.bumptech.glide.load.engine.executor.GlideExecutor +import com.bumptech.glide.load.engine.executor.GlideIdlingResources import com.bumptech.glide.load.model.ModelLoader import com.bumptech.glide.load.model.ModelLoaderFactory import com.bumptech.glide.load.model.MultiModelLoaderFactory @@ -47,6 +49,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.newSingleThreadContext import kotlinx.coroutines.test.runTest import org.junit.After +import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder @@ -54,12 +57,17 @@ import org.junit.runner.RunWith // newFile throws IOException, which triggers this warning even though there's no reasonable // alternative :/. -@Suppress("BlockingMethodInNonBlockingContext") +@Suppress("BlockingMethodInNonBlockingContext", "RedundantSuppression") @RunWith(AndroidJUnit4::class) class FlowsTest { private val context = ApplicationProvider.getApplicationContext() @get:Rule val temporaryFolder = TemporaryFolder() + @Before + fun setUp() { + GlideIdlingResources.initGlide() + } + @After fun tearDown() { Glide.tearDown() @@ -303,8 +311,7 @@ class FlowsTest { @Test fun flow_onClose_clearsTarget() = runTest { val inCache = AtomicReference?>() - Glide.init( - context, + GlideIdlingResources.initGlide( GlideBuilder() .setMemoryCache( object : MemoryCache { @@ -327,11 +334,15 @@ class FlowsTest { inCache.set(resource) return null } - } - ) + } + ) ) val data = Glide.with(context).load(newImageFile()).flow(100, 100).firstLoad().toList() assertThat(data).isNotEmpty() + // Glide's executor (in EngineJob's notify loop) and the coroutine race to close the resource. + // If Glide's executor wins, then the coroutine will be able to put the resource in the cache, + // but if not, the item won't be cached until after the coroutine starts back up. + onIdle() assertThat(inCache.get()).isNotNull() } diff --git a/integration/ktx/src/test/java/com/bumptech/glide/load/engine/executor/GlideIdlingResourceInit.kt b/integration/ktx/src/test/java/com/bumptech/glide/load/engine/executor/GlideIdlingResourceInit.kt new file mode 100644 index 0000000000..84f6508d32 --- /dev/null +++ b/integration/ktx/src/test/java/com/bumptech/glide/load/engine/executor/GlideIdlingResourceInit.kt @@ -0,0 +1,36 @@ +package com.bumptech.glide.load.engine.executor + +import androidx.test.core.app.ApplicationProvider +import androidx.test.espresso.IdlingRegistry +import androidx.test.espresso.idling.concurrent.IdlingThreadPoolExecutor +import com.bumptech.glide.Glide +import com.bumptech.glide.GlideBuilder +import java.util.concurrent.LinkedBlockingQueue +import java.util.concurrent.TimeUnit + +object GlideIdlingResources { + + fun initGlide(builder: GlideBuilder? = null) { + val registry = IdlingRegistry.getInstance() + val executor = + IdlingThreadPoolExecutor( + "glide_test_thread", + /* corePoolSize = */ 1, + /* maximumPoolSize = */ 1, + /* keepAliveTime = */ 1, + TimeUnit.SECONDS, + LinkedBlockingQueue() + ) { + Thread(it) + } + val glideExecutor = GlideExecutor(executor) + Glide.init( + ApplicationProvider.getApplicationContext(), + (builder ?: GlideBuilder()) + .setSourceExecutor(glideExecutor) + .setAnimationExecutor(glideExecutor) + .setDiskCacheExecutor(glideExecutor) + ) + registry.register(executor) + } +} diff --git a/settings.gradle b/settings.gradle index 1db1c3a8d6..f34066368e 100644 --- a/settings.gradle +++ b/settings.gradle @@ -54,14 +54,14 @@ dependencyResolutionManagement { // Versions for dependencies version('compose', '1.3.2') - version('coroutines', '1.6.4') + version('coroutines', '1.7.2') version('dagger', '2.46.1') version('errorprone', '2.18.0') version('kotlin', '1.7.0') version('mockito', '5.3.1') version('retrofit', '2.3.0') version('androidx-benchmark', '1.1.1') - version('androidx-espresso', '3.4.0') + version('androidx-espresso', '3.5.1') // At least versions 1.5 and later require java 8 desugaring, which Glide can't // currently use, so we're stuck on an older version. version('androidx-fragment', '1.3.6')