From 8f2d39c8a53adea32d016ef5fe1510a790c02421 Mon Sep 17 00:00:00 2001 From: Chris Banes Date: Thu, 15 Oct 2020 13:36:17 +0100 Subject: [PATCH 1/3] De-flake PicassoTest.loading_slot --- .../accompanist/picasso/PicassoTest.kt | 54 ++++++++-------- .../picasso/SingleThreadPauseableExecutor.kt | 64 +++++++++++++++++++ 2 files changed, 90 insertions(+), 28 deletions(-) create mode 100644 picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/SingleThreadPauseableExecutor.kt diff --git a/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/PicassoTest.kt b/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/PicassoTest.kt index 5935523be..ba01466ca 100644 --- a/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/PicassoTest.kt +++ b/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/PicassoTest.kt @@ -41,14 +41,13 @@ import androidx.ui.test.onNodeWithText import com.google.common.truth.Truth.assertThat import com.squareup.picasso.MemoryPolicy import com.squareup.picasso.NetworkPolicy +import com.squareup.picasso.Picasso import dev.chrisbanes.accompanist.imageloading.ImageLoadState import dev.chrisbanes.accompanist.picasso.test.R import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.test.TestCoroutineDispatcher -import kotlinx.coroutines.test.runBlockingTest import kotlinx.coroutines.withTimeout import kotlinx.coroutines.withTimeoutOrNull import okhttp3.mockwebserver.Dispatcher @@ -58,7 +57,6 @@ import okhttp3.mockwebserver.RecordedRequest import okio.Buffer import org.junit.After import org.junit.Before -import org.junit.Ignore import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -359,38 +357,38 @@ class PicassoTest { .assertPixels { Color.Cyan } } - @OptIn(ExperimentalCoroutinesApi::class) @Test - @Ignore("Flakey: https://github.com/chrisbanes/accompanist/issues/105") fun loading_slot() { - val dispatcher = TestCoroutineDispatcher() val loadLatch = CountDownLatch(1) - dispatcher.runBlockingTest { - pauseDispatcher() - - composeTestRule.setContent { - PicassoImage( - data = server.url("/image"), - modifier = Modifier.preferredSize(128.dp, 128.dp), - // Disable any caches. If the item is in the cache, the fetch is - // synchronous which means the Loading state is skipped - requestBuilder = { - networkPolicy(NetworkPolicy.NO_CACHE) - memoryPolicy(MemoryPolicy.NO_CACHE) - }, - loading = { Text(text = "Loading") }, - onRequestCompleted = { loadLatch.countDown() } - ) - } - - // Assert that the loading component is displayed - composeTestRule.onNodeWithText("Loading").assertIsDisplayed() + // Create an executor which is paused, and build a Picasso instance which uses it + val executor = SingleThreadPausableExecutor(paused = true) + val picasso = Picasso.Builder(InstrumentationRegistry.getInstrumentation().targetContext) + .executor(executor) + .build() - // Now resume the dispatcher to start the Coil request - dispatcher.resumeDispatcher() + composeTestRule.setContent { + PicassoImage( + data = server.url("/image"), + picasso = picasso, + modifier = Modifier.preferredSize(128.dp, 128.dp), + // Disable any caches. If the item is in the cache, the fetch is + // synchronous which means the Loading state is skipped + requestBuilder = { + networkPolicy(NetworkPolicy.NO_CACHE) + memoryPolicy(MemoryPolicy.NO_CACHE) + }, + loading = { Text(text = "Loading") }, + onRequestCompleted = { loadLatch.countDown() } + ) } + // Assert that the loading component is displayed + composeTestRule.onNodeWithText("Loading").assertIsDisplayed() + + // Now resume the executor and let the Picasso request run + executor.resume() + // We now wait for the request to complete loadLatch.await(5, TimeUnit.SECONDS) diff --git a/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/SingleThreadPauseableExecutor.kt b/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/SingleThreadPauseableExecutor.kt new file mode 100644 index 000000000..7e9366ce7 --- /dev/null +++ b/picasso/src/androidTest/java/dev/chrisbanes/accompanist/picasso/SingleThreadPauseableExecutor.kt @@ -0,0 +1,64 @@ +/* + * Copyright 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package dev.chrisbanes.accompanist.picasso + +import java.util.concurrent.LinkedBlockingQueue +import java.util.concurrent.ThreadPoolExecutor +import java.util.concurrent.TimeUnit +import java.util.concurrent.locks.ReentrantLock + +/** + * This is copied from the javadoc on [ThreadPoolExecutor]. + */ +internal class SingleThreadPausableExecutor(paused: Boolean = false) : ThreadPoolExecutor( + 1, 1, 0L, TimeUnit.MILLISECONDS, LinkedBlockingQueue() +) { + private var isPaused = paused + private val pauseLock = ReentrantLock() + private val unpaused = pauseLock.newCondition() + + override fun beforeExecute(t: Thread, r: Runnable) { + super.beforeExecute(t, r) + pauseLock.lock() + try { + while (isPaused) unpaused.await() + } catch (ie: InterruptedException) { + t.interrupt() + } finally { + pauseLock.unlock() + } + } + + fun pause() { + pauseLock.lock() + try { + isPaused = true + } finally { + pauseLock.unlock() + } + } + + fun resume() { + pauseLock.lock() + try { + isPaused = false + unpaused.signalAll() + } finally { + pauseLock.unlock() + } + } +} From a6ad973954cefa226438917bb6965e1409216f3c Mon Sep 17 00:00:00 2001 From: Chris Banes Date: Thu, 15 Oct 2020 13:46:14 +0100 Subject: [PATCH 2/3] Remove coroutines-test from Picasso --- picasso/build.gradle | 2 -- 1 file changed, 2 deletions(-) diff --git a/picasso/build.gradle b/picasso/build.gradle index 9088bb39a..8d0e3c73a 100644 --- a/picasso/build.gradle +++ b/picasso/build.gradle @@ -95,8 +95,6 @@ dependencies { androidTestImplementation Libs.OkHttp.mockWebServer - androidTestImplementation Libs.Coroutines.test - androidTestImplementation Libs.AndroidX.Compose.test androidTestImplementation Libs.AndroidX.Compose.ui androidTestImplementation Libs.AndroidX.Test.rules From 4ef0fc7c310b1dbea2d2e139cc661d96f53105f4 Mon Sep 17 00:00:00 2001 From: Chris Banes Date: Thu, 15 Oct 2020 13:50:30 +0100 Subject: [PATCH 3/3] De-flake CoilTest.loading_slot --- .../chrisbanes/accompanist/coil/CoilTest.kt | 52 ++++++++++--------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt b/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt index f7c1d433c..37e8196f8 100644 --- a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt +++ b/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt @@ -57,7 +57,6 @@ import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.TestCoroutineDispatcher -import kotlinx.coroutines.test.runBlockingTest import kotlinx.coroutines.withTimeout import kotlinx.coroutines.withTimeoutOrNull import okhttp3.mockwebserver.Dispatcher @@ -67,7 +66,6 @@ import okhttp3.mockwebserver.RecordedRequest import okio.Buffer import org.junit.After import org.junit.Before -import org.junit.Ignore import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -443,41 +441,45 @@ class CoilTest { @OptIn(ExperimentalCoroutinesApi::class) @Test - @Ignore("Flakey: https://github.com/chrisbanes/accompanist/issues/105") fun loading_slot() { - val dispatcher = TestCoroutineDispatcher() val loadLatch = CountDownLatch(1) - dispatcher.runBlockingTest { - pauseDispatcher() - - composeTestRule.setContent { - CoilImage( - request = ImageRequest.Builder(ContextAmbient.current) - .data(server.url("/image")) - .dispatcher(dispatcher) - .build(), - modifier = Modifier.preferredSize(128.dp, 128.dp), - // Disable memory cache. If the item is in the cache, the fetch is - // synchronous and the dispatcher pause has no effect - imageLoader = noCacheImageLoader(), - loading = { Text(text = "Loading") }, - onRequestCompleted = { loadLatch.countDown() } - ) - } + // Create a test dispatcher and immediately pause it + val dispatcher = TestCoroutineDispatcher() + dispatcher.pauseDispatcher() - // Assert that the loading component is displayed - composeTestRule.onNodeWithText("Loading").assertIsDisplayed() + val context = InstrumentationRegistry.getInstrumentation().targetContext + val imageLoader = ImageLoader.Builder(context) + // Load on our test dispatcher + .dispatcher(dispatcher) + // Disable memory cache. If the item is in the cache, the fetch is + // synchronous and the dispatcher pause has no effect + .memoryCachePolicy(CachePolicy.DISABLED) + .build() - // Now resume the dispatcher to start the Coil request - dispatcher.resumeDispatcher() + composeTestRule.setContent { + CoilImage( + data = server.url("/image"), + imageLoader = imageLoader, + modifier = Modifier.preferredSize(128.dp, 128.dp), + loading = { Text(text = "Loading") }, + onRequestCompleted = { loadLatch.countDown() } + ) } + // Assert that the loading component is displayed + composeTestRule.onNodeWithText("Loading").assertIsDisplayed() + + // Now resume the dispatcher to start the Coil request + dispatcher.resumeDispatcher() + // We now wait for the request to complete loadLatch.await(5, TimeUnit.SECONDS) // And assert that the loading component no longer exists composeTestRule.onNodeWithText("Loading").assertDoesNotExist() + + dispatcher.cleanupTestCoroutines() } @Test