From c2fcb4099bd4fad19d9bcff4ca197f0ab0c904a2 Mon Sep 17 00:00:00 2001 From: Joshua Trask Date: Tue, 26 Sep 2023 13:53:36 +0000 Subject: ResolverListAdapter: Switch `Handler`->`Executor` With the new `mDestroyed` bookkeeping in ag/24854314, we no longer need the richer (but less-testable/etc) API of a `Handler`. In the first snapshot, this retains the ad-hoc flow control mechanisms that direct the `testPostListReadyAtEndOfRebuild_` tests to show the direct before-and-after equivalence against the `Handler` model. All the existing tests still pass on the new code. The second snapshot removes those ad-hoc mechanisms because the tests now have complete control of the execution via `TestExecutor`. Bug: (general code cleanup) Test: `IntentResolverUnitTests` / `CtsSharesheetDeviceTest` Change-Id: I3063d45aedec47a81bf9bc7f76c26873de1383af --- .../intentresolver/ResolverListAdapterTest.kt | 153 ++++++++------------- 1 file changed, 54 insertions(+), 99 deletions(-) (limited to 'java/tests') diff --git a/java/tests/src/com/android/intentresolver/ResolverListAdapterTest.kt b/java/tests/src/com/android/intentresolver/ResolverListAdapterTest.kt index 0f381aac..53c90199 100644 --- a/java/tests/src/com/android/intentresolver/ResolverListAdapterTest.kt +++ b/java/tests/src/com/android/intentresolver/ResolverListAdapterTest.kt @@ -22,16 +22,12 @@ import android.content.Intent import android.content.pm.ActivityInfo import android.content.pm.ApplicationInfo import android.content.pm.ResolveInfo -import android.os.Handler -import android.os.Looper import android.os.UserHandle import android.view.LayoutInflater import com.android.intentresolver.ResolverListAdapter.ResolverListCommunicator import com.android.intentresolver.icons.TargetDataLoader import com.android.intentresolver.util.TestExecutor -import com.android.intentresolver.util.TestImmediateHandler import com.google.common.truth.Truth.assertThat -import java.util.concurrent.CountDownLatch import java.util.concurrent.atomic.AtomicInteger import org.junit.Test import org.mockito.Mockito.anyBoolean @@ -44,12 +40,10 @@ private const val PKG_NAME_TWO = "org.pkgtwo.app" private const val CLASS_NAME = "org.pkg.app.TheClass" class ResolverListAdapterTest { - private val testHandler = TestImmediateHandler() private val layoutInflater = mock() private val context = mock { whenever(getSystemService(Context.LAYOUT_INFLATER_SERVICE)).thenReturn(layoutInflater) - whenever(mainThreadHandler).thenReturn(testHandler) } private val targetIntent = Intent(Intent.ACTION_SEND) private val payloadIntents = listOf(targetIntent) @@ -61,7 +55,8 @@ class ResolverListAdapterTest { private val resolverListCommunicator = FakeResolverListCommunicator() private val userHandle = UserHandle.of(0) private val targetDataLoader = mock() - private val executor = TestExecutor() + private val backgroundExecutor = TestExecutor() + private val immediateExecutor = TestExecutor(immediate = true) @Test fun test_oneTargetNoLastChosen_oneTargetInAdapter() { @@ -89,8 +84,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true @@ -104,7 +99,7 @@ class ResolverListAdapterTest { assertThat(testSubject.filteredPosition).isLessThan(0) assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isTrue() - assertThat(executor.pendingCommandCount).isEqualTo(0) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(0) assertThat(resolverListCommunicator.updateProfileViewButtonCount).isEqualTo(0) assertThat(resolverListCommunicator.sendVoiceCommandCount).isEqualTo(1) } @@ -137,8 +132,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true @@ -152,7 +147,7 @@ class ResolverListAdapterTest { assertThat(testSubject.filteredPosition).isEqualTo(0) assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isTrue() - assertThat(executor.pendingCommandCount).isEqualTo(0) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(0) } @Test @@ -183,8 +178,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true @@ -198,7 +193,7 @@ class ResolverListAdapterTest { assertThat(testSubject.filteredPosition).isLessThan(0) assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isTrue() - assertThat(executor.pendingCommandCount).isEqualTo(0) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(0) } @Test @@ -229,8 +224,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true @@ -301,8 +296,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true @@ -317,11 +312,11 @@ class ResolverListAdapterTest { assertThat(testSubject.filteredPosition).isLessThan(0) assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isFalse() - assertThat(executor.pendingCommandCount).isEqualTo(1) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(1) assertThat(resolverListCommunicator.updateProfileViewButtonCount).isEqualTo(0) assertThat(resolverListCommunicator.sendVoiceCommandCount).isEqualTo(0) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // we don't reset placeholder count (legacy logic, likely an oversight?) assertThat(testSubject.placeholderCount).isEqualTo(placeholderCount) @@ -339,7 +334,7 @@ class ResolverListAdapterTest { assertThat(testSubject.isTabLoaded).isTrue() assertThat(resolverListCommunicator.updateProfileViewButtonCount).isEqualTo(1) assertThat(resolverListCommunicator.sendVoiceCommandCount).isEqualTo(1) - assertThat(executor.pendingCommandCount).isEqualTo(0) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(0) } @Test @@ -374,8 +369,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = false @@ -390,10 +385,10 @@ class ResolverListAdapterTest { assertThat(testSubject.filteredPosition).isLessThan(0) assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isFalse() - assertThat(executor.pendingCommandCount).isEqualTo(1) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(1) assertThat(resolverListCommunicator.updateProfileViewButtonCount).isEqualTo(0) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // we don't reset placeholder count (legacy logic, likely an oversight?) assertThat(testSubject.placeholderCount).isEqualTo(placeholderCount) @@ -404,7 +399,7 @@ class ResolverListAdapterTest { assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isTrue() assertThat(resolverListCommunicator.updateProfileViewButtonCount).isEqualTo(0) - assertThat(executor.pendingCommandCount).isEqualTo(0) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(0) } @Test @@ -441,8 +436,8 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true @@ -457,7 +452,7 @@ class ResolverListAdapterTest { assertThat(testSubject.filteredPosition).isLessThan(0) assertThat(testSubject.unfilteredResolveList).containsExactlyElementsIn(resolvedTargets) assertThat(testSubject.isTabLoaded).isTrue() - assertThat(executor.pendingCommandCount).isEqualTo(0) + assertThat(backgroundExecutor.pendingCommandCount).isEqualTo(0) } @Suppress("UNCHECKED_CAST") @@ -496,14 +491,14 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true testSubject.rebuildList(doPostProcessing) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // we don't reset placeholder count (legacy logic, likely an oversight?) assertThat(testSubject.count).isEqualTo(resolvedTargets.size) @@ -551,14 +546,14 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true testSubject.rebuildList(doPostProcessing) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // we don't reset placeholder count (legacy logic, likely an oversight?) assertThat(testSubject.count).isEqualTo(1) @@ -602,14 +597,14 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true testSubject.rebuildList(doPostProcessing) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // we don't reset placeholder count (legacy logic, likely an oversight?) assertThat(testSubject.count).isEqualTo(2) @@ -654,14 +649,14 @@ class ResolverListAdapterTest { resolverListCommunicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = true testSubject.rebuildList(doPostProcessing) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // we don't reset placeholder count (legacy logic, likely an oversight?) assertThat(testSubject.count).isEqualTo(1) @@ -686,14 +681,13 @@ class ResolverListAdapterTest { communicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = false - executor.runUntilIdle() - testSubject.rebuildList(doPostProcessing) + verify(communicator).onPostListReady(testSubject, doPostProcessing, true) } @@ -739,14 +733,14 @@ class ResolverListAdapterTest { communicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - testHandler, + backgroundExecutor, + immediateExecutor, ) val doPostProcessing = false testSubject.rebuildList(doPostProcessing) - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() val inOrder = inOrder(communicator) inOrder.verify(communicator).onPostListReady(testSubject, doPostProcessing, false) @@ -755,16 +749,7 @@ class ResolverListAdapterTest { @Test fun testPostListReadyAtEndOfRebuild_queued() { - // Set up a runnable that blocks the callback handler until we're ready to start - // processing queued messages. This is to test against a legacy bug where subsequent - // list-ready notifications could be dropped if the older one wasn't yet dequeued, even if - // the newer ones had different parameters. - // TODO: after removing the logic responsible for this "dropping" we could migrate off the - // `Handler` API (to `Executor` or similar) and use a simpler mechanism to test. - val callbackHandler = Handler(Looper.getMainLooper()) - val unblockCallbacksSignal = CountDownLatch(1) - val countdownBlockingRunnable = Runnable { unblockCallbacksSignal.await() } - callbackHandler.post(countdownBlockingRunnable) + val queuedCallbacksExecutor = TestExecutor() // We need at least two targets to trigger asynchronous sorting/"staged" progress callbacks. val resolvedTargets = @@ -806,28 +791,16 @@ class ResolverListAdapterTest { communicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - callbackHandler + backgroundExecutor, + queuedCallbacksExecutor ) val doPostProcessing = false testSubject.rebuildList(doPostProcessing) // Finish all the background work (enqueueing both the "partial" and "complete" progress // callbacks) before dequeueing either callback. - executor.runUntilIdle() - - // Allow the handler to flush out and process both callbacks. - unblockCallbacksSignal.countDown() - - // Finally, force a synchronization in the other direction to ensure that we've finished - // processing any callbacks before we start making assertions about them. - // TODO: there are less "ad-hoc" ways to write this (e.g. with a special `Handler` subclass - // for tests), but we should switch off using `Handler` altogether (in favor of `Executor` - // or otherwise), and then it'll be much simpler to clean up this boilerplate. - val unblockAssertionsSignal = CountDownLatch(1) - val countdownAssertionsRunnable = Runnable { unblockAssertionsSignal.countDown() } - callbackHandler.post(countdownAssertionsRunnable) - unblockAssertionsSignal.await() + backgroundExecutor.runUntilIdle() + queuedCallbacksExecutor.runUntilIdle() // TODO: we may not necessarily care to assert that there's a "partial progress" callback in // this case, since there won't be a chance to reflect the "partial" state in the UI before @@ -841,14 +814,7 @@ class ResolverListAdapterTest { @Test fun testPostListReadyAtEndOfRebuild_skippedIfStillQueuedOnDestroy() { - // Set up a runnable that blocks the callback handler until we're ready to start - // processing queued messages. - // TODO: after removing the logic responsible for this "dropping" we could migrate off the - // `Handler` API (to `Executor` or similar) and use a simpler mechanism to test. - val callbackHandler = Handler(Looper.getMainLooper()) - val unblockCallbacksSignal = CountDownLatch(1) - val countdownBlockingRunnable = Runnable { unblockCallbacksSignal.await() } - callbackHandler.post(countdownBlockingRunnable) + val queuedCallbacksExecutor = TestExecutor() // We need at least two targets to trigger asynchronous sorting/"staged" progress callbacks. val resolvedTargets = @@ -890,31 +856,20 @@ class ResolverListAdapterTest { communicator, /*initialIntentsUserSpace=*/ userHandle, targetDataLoader, - executor, - callbackHandler + backgroundExecutor, + queuedCallbacksExecutor ) val doPostProcessing = false testSubject.rebuildList(doPostProcessing) // Finish all the background work (enqueueing both the "partial" and "complete" progress // callbacks) before dequeueing either callback. - executor.runUntilIdle() + backgroundExecutor.runUntilIdle() // Notify that our activity is being destroyed while the callbacks are still queued. testSubject.onDestroy() - // Allow the handler to flush out, but now the callbacks are gone. - unblockCallbacksSignal.countDown() - - // Finally, force a synchronization in the other direction to ensure that we've finished - // processing any callbacks before we start making assertions about them. - // TODO: there are less "ad-hoc" ways to write this (e.g. with a special `Handler` subclass - // for tests), but we should switch off using `Handler` altogether (in favor of `Executor` - // or otherwise), and then it'll be much simpler to clean up this boilerplate. - val unblockAssertionsSignal = CountDownLatch(1) - val countdownAssertionsRunnable = Runnable { unblockAssertionsSignal.countDown() } - callbackHandler.post(countdownAssertionsRunnable) - unblockAssertionsSignal.await() + queuedCallbacksExecutor.runUntilIdle() verify(communicator, never()).onPostListReady(eq(testSubject), eq(doPostProcessing), any()) } -- cgit v1.2.3-59-g8ed1b