From a6d5bb47502e90e983c1e44a82190ea8f6dfd9c2 Mon Sep 17 00:00:00 2001 From: Kevin Han Date: Tue, 17 Mar 2020 13:55:49 -0700 Subject: [PATCH] Deflake NotificationEntryManagerInflationTest Deflake NotificationEntryManagerInflationTest by moving back to the countdown latch approach and providing our own executor to AsyncInflationTask. It was infeasible to depend on the exact number of main thread messages as other main thread messages from other tests could interfere. Thus. we move back to the countdown latch for when NotificationEntryManager finishes inflation. This makes us dependent on the listener API for determining when inflation is finished, but it's unlikely we can do better unless we can inject an executor into RowInflaterTask. In addition, using AsyncTask's SERIAL_EXECUTOR on hwasan builds seemed to be much slower than normal. This lead to other tests' AysncTask work delaying the work in this test and leading to the timeout happening before inflation finished. By providing a test executor instead and synchronously controlling its execution, we avoid this issue. Bug: 150618180 Test: atest --iterations 100 NotificationEntryManagerInflationTest Test: atest SystemUITests Test: A forrest run coming soon! Change-Id: I87c6e2d216c8f26aaf340d311f618c9dccaba8af --- .../row/NotificationContentInflater.java | 51 +++++++++---- .../row/NotificationContentInflaterTest.java | 5 +- ...NotificationEntryManagerInflationTest.java | 73 +++++++++++-------- .../row/NotificationTestHelper.java | 4 +- 4 files changed, 84 insertions(+), 49 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationContentInflater.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationContentInflater.java index f2ce1affba46a..719f74fdcde26 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationContentInflater.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/row/NotificationContentInflater.java @@ -36,6 +36,7 @@ import android.widget.RemoteViews; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.widget.ImageMessageConsumer; +import com.android.systemui.dagger.qualifiers.Background; import com.android.systemui.statusbar.InflationTask; import com.android.systemui.statusbar.NotificationRemoteInputManager; import com.android.systemui.statusbar.SmartReplyController; @@ -52,6 +53,7 @@ import com.android.systemui.statusbar.policy.SmartReplyConstants; import com.android.systemui.util.Assert; import java.util.HashMap; +import java.util.concurrent.Executor; import javax.inject.Inject; import javax.inject.Singleton; @@ -74,6 +76,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder private final Lazy mSmartReplyConstants; private final Lazy mSmartReplyController; private final ConversationNotificationProcessor mConversationProcessor; + private final Executor mBgExecutor; @Inject NotificationContentInflater( @@ -81,12 +84,14 @@ public class NotificationContentInflater implements NotificationRowContentBinder NotificationRemoteInputManager remoteInputManager, Lazy smartReplyConstants, Lazy smartReplyController, - ConversationNotificationProcessor conversationProcessor) { + ConversationNotificationProcessor conversationProcessor, + @Background Executor bgExecutor) { mRemoteViewCache = remoteViewCache; mRemoteInputManager = remoteInputManager; mSmartReplyConstants = smartReplyConstants; mSmartReplyController = smartReplyController; mConversationProcessor = conversationProcessor; + mBgExecutor = bgExecutor; } @Override @@ -114,6 +119,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder } AsyncInflationTask task = new AsyncInflationTask( + mBgExecutor, mInflateSynchronously, contentToBind, mRemoteViewCache, @@ -131,7 +137,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder if (mInflateSynchronously) { task.onPostExecute(task.doInBackground()); } else { - task.execute(); + task.executeOnExecutor(mBgExecutor); } } @@ -157,6 +163,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder row.getExistingSmartRepliesAndActions()); apply( + mBgExecutor, inflateSynchronously, result, reInflateFlags, @@ -280,6 +287,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder } private static CancellationSignal apply( + Executor bgExecutor, boolean inflateSynchronously, InflationProgress result, @InflationFlag int reInflateFlags, @@ -308,9 +316,10 @@ public class NotificationContentInflater implements NotificationRowContentBinder return result.newContentView; } }; - applyRemoteView(inflateSynchronously, result, reInflateFlags, flag, remoteViewCache, - entry, row, isNewView, remoteViewClickHandler, callback, privateLayout, - privateLayout.getContractedChild(), privateLayout.getVisibleWrapper( + applyRemoteView(bgExecutor, inflateSynchronously, result, reInflateFlags, flag, + remoteViewCache, entry, row, isNewView, remoteViewClickHandler, callback, + privateLayout, privateLayout.getContractedChild(), + privateLayout.getVisibleWrapper( NotificationContentView.VISIBLE_TYPE_CONTRACTED), runningInflations, applyCallback); } @@ -332,8 +341,8 @@ public class NotificationContentInflater implements NotificationRowContentBinder return result.newExpandedView; } }; - applyRemoteView(inflateSynchronously, result, reInflateFlags, flag, remoteViewCache, - entry, row, isNewView, remoteViewClickHandler, + applyRemoteView(bgExecutor, inflateSynchronously, result, reInflateFlags, flag, + remoteViewCache, entry, row, isNewView, remoteViewClickHandler, callback, privateLayout, privateLayout.getExpandedChild(), privateLayout.getVisibleWrapper( NotificationContentView.VISIBLE_TYPE_EXPANDED), runningInflations, @@ -358,8 +367,8 @@ public class NotificationContentInflater implements NotificationRowContentBinder return result.newHeadsUpView; } }; - applyRemoteView(inflateSynchronously, result, reInflateFlags, flag, remoteViewCache, - entry, row, isNewView, remoteViewClickHandler, + applyRemoteView(bgExecutor, inflateSynchronously, result, reInflateFlags, flag, + remoteViewCache, entry, row, isNewView, remoteViewClickHandler, callback, privateLayout, privateLayout.getHeadsUpChild(), privateLayout.getVisibleWrapper( VISIBLE_TYPE_HEADSUP), runningInflations, @@ -383,8 +392,8 @@ public class NotificationContentInflater implements NotificationRowContentBinder return result.newPublicView; } }; - applyRemoteView(inflateSynchronously, result, reInflateFlags, flag, remoteViewCache, - entry, row, isNewView, remoteViewClickHandler, callback, + applyRemoteView(bgExecutor, inflateSynchronously, result, reInflateFlags, flag, + remoteViewCache, entry, row, isNewView, remoteViewClickHandler, callback, publicLayout, publicLayout.getContractedChild(), publicLayout.getVisibleWrapper(NotificationContentView.VISIBLE_TYPE_CONTRACTED), runningInflations, applyCallback); @@ -401,6 +410,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder @VisibleForTesting static void applyRemoteView( + Executor bgExecutor, boolean inflateSynchronously, final InflationProgress result, final @InflationFlag int reInflateFlags, @@ -495,14 +505,14 @@ public class NotificationContentInflater implements NotificationRowContentBinder cancellationSignal = newContentView.applyAsync( result.packageContext, parentLayout, - null, + bgExecutor, listener, remoteViewClickHandler); } else { cancellationSignal = newContentView.reapplyAsync( result.packageContext, existingView, - null, + bgExecutor, listener, remoteViewClickHandler); } @@ -672,6 +682,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder private final NotifRemoteViewCache mRemoteViewCache; private final SmartReplyConstants mSmartReplyConstants; private final SmartReplyController mSmartReplyController; + private final Executor mBgExecutor; private ExpandableNotificationRow mRow; private Exception mError; private RemoteViews.OnClickHandler mRemoteViewClickHandler; @@ -679,6 +690,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder private final ConversationNotificationProcessor mConversationProcessor; private AsyncInflationTask( + Executor bgExecutor, boolean inflateSynchronously, @InflationFlag int reInflateFlags, NotifRemoteViewCache cache, @@ -697,6 +709,7 @@ public class NotificationContentInflater implements NotificationRowContentBinder mRow = row; mSmartReplyConstants = smartReplyConstants; mSmartReplyController = smartReplyController; + mBgExecutor = bgExecutor; mInflateSynchronously = inflateSynchronously; mReInflateFlags = reInflateFlags; mRemoteViewCache = cache; @@ -755,8 +768,16 @@ public class NotificationContentInflater implements NotificationRowContentBinder @Override protected void onPostExecute(InflationProgress result) { if (mError == null) { - mCancellationSignal = apply(mInflateSynchronously, result, mReInflateFlags, - mRemoteViewCache, mEntry, mRow, mRemoteViewClickHandler, this); + mCancellationSignal = apply( + mBgExecutor, + mInflateSynchronously, + result, + mReInflateFlags, + mRemoteViewCache, + mEntry, + mRow, + mRemoteViewClickHandler, + this); } else { handleError(mError); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationContentInflaterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationContentInflaterTest.java index 524afacd1ace8..6a65269c34b04 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationContentInflaterTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationContentInflaterTest.java @@ -35,6 +35,7 @@ import static org.mockito.Mockito.when; import android.app.Notification; import android.content.Context; +import android.os.AsyncTask; import android.os.CancellationSignal; import android.os.Handler; import android.os.Looper; @@ -104,7 +105,8 @@ public class NotificationContentInflaterTest extends SysuiTestCase { mock(NotificationRemoteInputManager.class), () -> smartReplyConstants, () -> smartReplyController, - mConversationNotificationProcessor); + mConversationNotificationProcessor, + mock(Executor.class)); } @Test @@ -191,6 +193,7 @@ public class NotificationContentInflaterTest extends SysuiTestCase { result.packageContext = mContext; CountDownLatch countDownLatch = new CountDownLatch(1); NotificationContentInflater.applyRemoteView( + AsyncTask.SERIAL_EXECUTOR, false /* inflateSynchronously */, result, FLAG_CONTENT_VIEW_EXPANDED, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java index 1a0f29147931a..bdd7a2e064287 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationEntryManagerInflationTest.java @@ -36,6 +36,7 @@ import android.service.notification.StatusBarNotification; import android.testing.AndroidTestingRunner; import android.testing.TestableLooper; +import androidx.asynclayoutinflater.view.AsyncLayoutInflater; import androidx.test.filters.SmallTest; import com.android.internal.util.NotificationMessagingUtil; @@ -68,7 +69,6 @@ import com.android.systemui.statusbar.notification.icon.IconManager; import com.android.systemui.statusbar.notification.interruption.NotificationInterruptStateProvider; import com.android.systemui.statusbar.notification.logging.NotificationLogger; import com.android.systemui.statusbar.notification.people.PeopleNotificationIdentifier; -import com.android.systemui.statusbar.notification.row.NotificationRowContentBinder.InflationFlag; import com.android.systemui.statusbar.notification.row.dagger.ExpandableNotificationRowComponent; import com.android.systemui.statusbar.notification.row.dagger.NotificationRowComponent; import com.android.systemui.statusbar.notification.stack.NotificationListContainer; @@ -76,12 +76,12 @@ import com.android.systemui.statusbar.phone.KeyguardBypassController; import com.android.systemui.statusbar.phone.NotificationGroupManager; import com.android.systemui.statusbar.policy.HeadsUpManager; import com.android.systemui.statusbar.policy.SmartReplyConstants; +import com.android.systemui.util.concurrency.FakeExecutor; import com.android.systemui.util.leak.LeakDetector; import com.android.systemui.util.time.FakeSystemClock; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; @@ -90,11 +90,12 @@ import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.stubbing.Answer; +import java.util.concurrent.CountDownLatch; + /** * Functional tests for notification inflation from {@link NotificationEntryManager}. */ @SmallTest -@Ignore("Flaking") @RunWith(AndroidTestingRunner.class) @TestableLooper.RunWithLooper(setAsMainLooper = true) public class NotificationEntryManagerInflationTest extends SysuiTestCase { @@ -136,6 +137,7 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { private NotificationEntryManager mEntryManager; private NotificationRowBinderImpl mRowBinder; private Handler mHandler; + private FakeExecutor mBgExecutor; @Before public void setUp() { @@ -180,12 +182,14 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { NotifBindPipeline pipeline = new NotifBindPipeline( mEntryManager, mock(NotifBindPipelineLogger.class)); + mBgExecutor = new FakeExecutor(new FakeSystemClock()); NotificationContentInflater binder = new NotificationContentInflater( cache, mRemoteInputManager, () -> mock(SmartReplyConstants.class), () -> mock(SmartReplyController.class), - mock(ConversationNotificationProcessor.class)); + mock(ConversationNotificationProcessor.class), + mBgExecutor); RowContentBindStage stage = new RowContentBindStage( binder, mock(NotifInflationErrorManager.class), @@ -310,9 +314,7 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { verify(mEntryListener).onPendingEntryAdded(entryCaptor.capture()); NotificationEntry entry = entryCaptor.getValue(); - // Wait for inflation - // row inflation, system notification, remote views, contracted view - waitForMessages(4); + waitForInflation(); // THEN the notification has its row inflated assertNotNull(entry.getRow()); @@ -339,7 +341,7 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { NotificationEntry.class); verify(mEntryListener).onPendingEntryAdded(entryCaptor.capture()); NotificationEntry entry = entryCaptor.getValue(); - waitForMessages(4); + waitForInflation(); Mockito.reset(mEntryListener); Mockito.reset(mPresenter); @@ -347,9 +349,7 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { // WHEN the notification is updated mEntryManager.updateNotification(mSbn, mRankingMap); - // Wait for inflation - // remote views, contracted view - waitForMessages(2); + waitForInflation(); // THEN the notification has its row and inflated assertNotNull(entry.getRow()); @@ -363,31 +363,40 @@ public class NotificationEntryManagerInflationTest extends SysuiTestCase { } /** - * Wait for a certain number of messages to finish before continuing, timing out if they never - * occur. + * Wait for inflation to finish. * - * As part of the inflation pipeline, the main thread is forced to deal with several callbacks - * due to the nature of the API used (generally because they're {@link android.os.AsyncTask} - * callbacks). In order, these are - * - * 1) Callback after row inflation. See {@link RowInflaterTask}. - * 2) Callback checking if row is system notification. See - * {@link ExpandableNotificationRow#setEntry} - * 3) Callback after remote views are created. See - * {@link NotificationContentInflater.AsyncInflationTask}. - * 4-6) Callback after each content view is inflated/rebound from remote view. See - * {@link NotificationContentInflater#applyRemoteView} and {@link InflationFlag}. - * - * Depending on the test, only some of these will be necessary. For example, generally, not - * every content view is inflated or the row may not be inflated if one already exists. - * - * Currently, the burden is on the developer to figure these out until we have a much more - * test-friendly way of executing inflation logic (i.e. pass in an executor). + * A few things to note + * 1) Row inflation is done via {@link AsyncLayoutInflater} on its own background thread that + * calls back to main thread which is why we wait on main thread. + * 2) Row *content* inflation is done on the {@link FakeExecutor} we pass in in this test class + * so we control when that work is done. The callback is still always on the main thread. */ - private void waitForMessages(int numMessages) { + private void waitForInflation() { mHandler.postDelayed(TIMEOUT_RUNNABLE, TIMEOUT_TIME); - TestableLooper.get(this).processMessages(numMessages); + final CountDownLatch latch = new CountDownLatch(1); + NotificationEntryListener inflationListener = new NotificationEntryListener() { + @Override + public void onEntryInflated(NotificationEntry entry) { + latch.countDown(); + } + + @Override + public void onEntryReinflated(NotificationEntry entry) { + latch.countDown(); + } + + @Override + public void onInflationError(StatusBarNotification notification, Exception exception) { + latch.countDown(); + } + }; + mEntryManager.addNotificationEntryListener(inflationListener); + while (latch.getCount() != 0) { + mBgExecutor.runAllReady(); + TestableLooper.get(this).processMessages(1); + } mHandler.removeCallbacks(TIMEOUT_RUNNABLE); + mEntryManager.removeNotificationEntryListener(inflationListener); } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java index 23a90f2fd84d7..74f80df6442b5 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/row/NotificationTestHelper.java @@ -71,6 +71,7 @@ import com.android.systemui.tests.R; import org.mockito.ArgumentCaptor; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; /** @@ -121,7 +122,8 @@ public class NotificationTestHelper { mock(NotificationRemoteInputManager.class), () -> mock(SmartReplyConstants.class), () -> mock(SmartReplyController.class), - mock(ConversationNotificationProcessor.class)); + mock(ConversationNotificationProcessor.class), + mock(Executor.class)); contentBinder.setInflateSynchronously(true); mBindStage = new RowContentBindStage(contentBinder, mock(NotifInflationErrorManager.class),