From 660d0a7cb85cf7bba335433dee58549f4ad25147 Mon Sep 17 00:00:00 2001 From: Beverly Date: Wed, 26 Feb 2020 12:32:42 -0500 Subject: [PATCH 1/2] Inject DeviceConfig into SystemUI classes So the tests don't actually register listeners to the DeviceConfig, but can still test with a fake device config. Previously, tests were registering and never unregistering a listener to the DeviceConfig. Test: atest SystemUITests Fixes: 150226783 Change-Id: I59aa77f337a95aa1890570f5a7e30fe9cd58036a --- .../keyguard/KeyguardViewMediator.java | 34 ++++++---- .../keyguard/dagger/KeyguardModule.java | 7 +- .../systemui/pip/phone/PipManager.java | 6 +- .../pip/phone/PipResizeGestureHandler.java | 6 +- .../systemui/pip/phone/PipTouchHandler.java | 7 +- .../statusbar/NotificationMediaManager.java | 6 +- .../dagger/StatusBarDependenciesModule.java | 7 +- .../statusbar/policy/SmartReplyConstants.java | 49 ++++++++------ .../keyguard/KeyguardViewMediatorTest.java | 5 +- .../policy/SmartReplyConstantsTest.java | 65 ++++--------------- 10 files changed, 94 insertions(+), 98 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java index d688f0aa12e16..c129035e66a76 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java +++ b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java @@ -96,6 +96,7 @@ import com.android.systemui.statusbar.phone.NotificationPanelViewController; import com.android.systemui.statusbar.phone.NotificationShadeWindowController; import com.android.systemui.statusbar.phone.StatusBar; import com.android.systemui.statusbar.phone.StatusBarKeyguardViewManager; +import com.android.systemui.util.DeviceConfigProxy; import com.android.systemui.util.InjectionInflationController; import java.io.FileDescriptor; @@ -378,6 +379,17 @@ public class KeyguardViewMediator extends SystemUI implements Dumpable { private IKeyguardDrawnCallback mDrawnCallback; private CharSequence mCustomMessage; + private final DeviceConfig.OnPropertiesChangedListener mOnPropertiesChangedListener = + new DeviceConfig.OnPropertiesChangedListener() { + @Override + public void onPropertiesChanged(DeviceConfig.Properties properties) { + if (properties.getKeyset().contains(NAV_BAR_HANDLE_SHOW_OVER_LOCKSCREEN)) { + mShowHomeOverLockscreen = properties.getBoolean( + NAV_BAR_HANDLE_SHOW_OVER_LOCKSCREEN, true /* defaultValue */); + } + } + }; + KeyguardUpdateMonitorCallback mUpdateCallback = new KeyguardUpdateMonitorCallback() { @Override @@ -692,6 +704,8 @@ public class KeyguardViewMediator extends SystemUI implements Dumpable { } }; + private DeviceConfigProxy mDeviceConfig; + /** * Injected constructor. See {@link KeyguardModule}. */ @@ -705,7 +719,8 @@ public class KeyguardViewMediator extends SystemUI implements Dumpable { DismissCallbackRegistry dismissCallbackRegistry, KeyguardUpdateMonitor keyguardUpdateMonitor, DumpManager dumpManager, @UiBackground Executor uiBgExecutor, PowerManager powerManager, - TrustManager trustManager) { + TrustManager trustManager, + DeviceConfigProxy deviceConfig) { super(context); mFalsingManager = falsingManager; mLockPatternUtils = lockPatternUtils; @@ -718,20 +733,15 @@ public class KeyguardViewMediator extends SystemUI implements Dumpable { mPM = powerManager; mTrustManager = trustManager; dumpManager.registerDumpable(getClass().getName(), this); - mShowHomeOverLockscreen = DeviceConfig.getBoolean( + mDeviceConfig = deviceConfig; + mShowHomeOverLockscreen = mDeviceConfig.getBoolean( DeviceConfig.NAMESPACE_SYSTEMUI, NAV_BAR_HANDLE_SHOW_OVER_LOCKSCREEN, /* defaultValue = */ true); - DeviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_SYSTEMUI, mHandler::post, - new DeviceConfig.OnPropertiesChangedListener() { - @Override - public void onPropertiesChanged(DeviceConfig.Properties properties) { - if (properties.getKeyset().contains(NAV_BAR_HANDLE_SHOW_OVER_LOCKSCREEN)) { - mShowHomeOverLockscreen = properties.getBoolean( - NAV_BAR_HANDLE_SHOW_OVER_LOCKSCREEN, true /* defaultValue */); - } - } - }); + mDeviceConfig.addOnPropertiesChangedListener( + DeviceConfig.NAMESPACE_SYSTEMUI, + mHandler::post, + mOnPropertiesChangedListener); } public void userActivity() { diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/dagger/KeyguardModule.java b/packages/SystemUI/src/com/android/systemui/keyguard/dagger/KeyguardModule.java index d7af36010dfa0..367f46406ee88 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/dagger/KeyguardModule.java +++ b/packages/SystemUI/src/com/android/systemui/keyguard/dagger/KeyguardModule.java @@ -31,6 +31,7 @@ import com.android.systemui.plugins.FalsingManager; import com.android.systemui.statusbar.phone.NotificationShadeWindowController; import com.android.systemui.statusbar.phone.StatusBar; import com.android.systemui.statusbar.phone.StatusBarKeyguardViewManager; +import com.android.systemui.util.DeviceConfigProxy; import java.util.concurrent.Executor; @@ -62,7 +63,8 @@ public class KeyguardModule { DumpManager dumpManager, PowerManager powerManager, TrustManager trustManager, - @UiBackground Executor uiBgExecutor) { + @UiBackground Executor uiBgExecutor, + DeviceConfigProxy deviceConfig) { return new KeyguardViewMediator( context, falsingManager, @@ -75,6 +77,7 @@ public class KeyguardModule { dumpManager, uiBgExecutor, powerManager, - trustManager); + trustManager, + deviceConfig); } } diff --git a/packages/SystemUI/src/com/android/systemui/pip/phone/PipManager.java b/packages/SystemUI/src/com/android/systemui/pip/phone/PipManager.java index e98dec0835bfd..d448c371540c5 100644 --- a/packages/SystemUI/src/com/android/systemui/pip/phone/PipManager.java +++ b/packages/SystemUI/src/com/android/systemui/pip/phone/PipManager.java @@ -48,6 +48,7 @@ import com.android.systemui.shared.system.InputConsumerController; import com.android.systemui.shared.system.PinnedStackListenerForwarder.PinnedStackListener; import com.android.systemui.shared.system.TaskStackChangeListener; import com.android.systemui.shared.system.WindowManagerWrapper; +import com.android.systemui.util.DeviceConfigProxy; import com.android.systemui.util.FloatingContentCoordinator; import com.android.systemui.wm.DisplayChangeController; import com.android.systemui.wm.DisplayController; @@ -204,7 +205,8 @@ public class PipManager implements BasePipManager, PipTaskOrganizer.PipTransitio @Inject public PipManager(Context context, BroadcastDispatcher broadcastDispatcher, DisplayController displayController, - FloatingContentCoordinator floatingContentCoordinator) { + FloatingContentCoordinator floatingContentCoordinator, + DeviceConfigProxy deviceConfig) { mContext = context; mActivityManager = ActivityManager.getService(); @@ -225,7 +227,7 @@ public class PipManager implements BasePipManager, PipTaskOrganizer.PipTransitio mInputConsumerController); mTouchHandler = new PipTouchHandler(context, mActivityManager, activityTaskManager, mMenuController, mInputConsumerController, mPipBoundsHandler, mPipTaskOrganizer, - floatingContentCoordinator); + floatingContentCoordinator, deviceConfig); mAppOpsListener = new PipAppOpsListener(context, mActivityManager, mTouchHandler.getMotionHelper()); displayController.addDisplayChangingController(mRotationController); diff --git a/packages/SystemUI/src/com/android/systemui/pip/phone/PipResizeGestureHandler.java b/packages/SystemUI/src/com/android/systemui/pip/phone/PipResizeGestureHandler.java index 9fb623471bf7a..389793e6fc94c 100644 --- a/packages/SystemUI/src/com/android/systemui/pip/phone/PipResizeGestureHandler.java +++ b/packages/SystemUI/src/com/android/systemui/pip/phone/PipResizeGestureHandler.java @@ -41,6 +41,7 @@ import android.view.MotionEvent; import com.android.internal.policy.TaskResizingAlgorithm; import com.android.systemui.R; import com.android.systemui.pip.PipBoundsHandler; +import com.android.systemui.util.DeviceConfigProxy; import java.util.concurrent.Executor; @@ -77,7 +78,8 @@ public class PipResizeGestureHandler { private int mCtrlType; public PipResizeGestureHandler(Context context, PipBoundsHandler pipBoundsHandler, - PipTouchHandler pipTouchHandler, PipMotionHelper motionHelper) { + PipTouchHandler pipTouchHandler, PipMotionHelper motionHelper, + DeviceConfigProxy deviceConfig) { final Resources res = context.getResources(); context.getDisplay().getMetrics(mDisplayMetrics); mDisplayId = context.getDisplayId(); @@ -93,7 +95,7 @@ public class PipResizeGestureHandler { DeviceConfig.NAMESPACE_SYSTEMUI, PIP_USER_RESIZE, /* defaultValue = */ false); - DeviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_SYSTEMUI, mMainExecutor, + deviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_SYSTEMUI, mMainExecutor, new DeviceConfig.OnPropertiesChangedListener() { @Override public void onPropertiesChanged(DeviceConfig.Properties properties) { diff --git a/packages/SystemUI/src/com/android/systemui/pip/phone/PipTouchHandler.java b/packages/SystemUI/src/com/android/systemui/pip/phone/PipTouchHandler.java index 79a25b2269f6b..4ac839a32e827 100644 --- a/packages/SystemUI/src/com/android/systemui/pip/phone/PipTouchHandler.java +++ b/packages/SystemUI/src/com/android/systemui/pip/phone/PipTouchHandler.java @@ -48,6 +48,7 @@ import com.android.systemui.pip.PipSnapAlgorithm; import com.android.systemui.pip.PipTaskOrganizer; import com.android.systemui.shared.system.InputConsumerController; import com.android.systemui.statusbar.FlingAnimationUtils; +import com.android.systemui.util.DeviceConfigProxy; import com.android.systemui.util.FloatingContentCoordinator; import java.io.PrintWriter; @@ -162,7 +163,8 @@ public class PipTouchHandler { InputConsumerController inputConsumerController, PipBoundsHandler pipBoundsHandler, PipTaskOrganizer pipTaskOrganizer, - FloatingContentCoordinator floatingContentCoordinator) { + FloatingContentCoordinator floatingContentCoordinator, + DeviceConfigProxy deviceConfig) { // Initialize the Pip input consumer mContext = context; mActivityManager = activityManager; @@ -177,7 +179,8 @@ public class PipTouchHandler { mMotionHelper = new PipMotionHelper(mContext, activityTaskManager, pipTaskOrganizer, mMenuController, mSnapAlgorithm, mFlingAnimationUtils, floatingContentCoordinator); mPipResizeGestureHandler = - new PipResizeGestureHandler(context, pipBoundsHandler, this, mMotionHelper); + new PipResizeGestureHandler(context, pipBoundsHandler, this, mMotionHelper, + deviceConfig); mTouchState = new PipTouchState(ViewConfiguration.get(context), mHandler, () -> mMenuController.showMenu(MENU_STATE_FULL, mMotionHelper.getBounds(), mMovementBounds, true /* allowMenuTimeout */, willResizeMenu())); diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java index f8db922e8758c..cdb2c53136d39 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationMediaManager.java @@ -65,6 +65,7 @@ import com.android.systemui.statusbar.phone.ScrimController; import com.android.systemui.statusbar.phone.ScrimState; import com.android.systemui.statusbar.phone.StatusBar; import com.android.systemui.statusbar.policy.KeyguardStateController; +import com.android.systemui.util.DeviceConfigProxy; import java.io.FileDescriptor; import java.io.PrintWriter; @@ -183,7 +184,8 @@ public class NotificationMediaManager implements Dumpable { NotificationEntryManager notificationEntryManager, MediaArtworkProcessor mediaArtworkProcessor, KeyguardBypassController keyguardBypassController, - @Main Executor mainExecutor) { + @Main Executor mainExecutor, + DeviceConfigProxy deviceConfig) { mContext = context; mMediaArtworkProcessor = mediaArtworkProcessor; mKeyguardBypassController = keyguardBypassController; @@ -221,7 +223,7 @@ public class NotificationMediaManager implements Dumpable { DeviceConfig.getProperty(DeviceConfig.NAMESPACE_SYSTEMUI, SystemUiDeviceConfigFlags.COMPACT_MEDIA_SEEKBAR_ENABLED)); - DeviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_SYSTEMUI, + deviceConfig.addOnPropertiesChangedListener(DeviceConfig.NAMESPACE_SYSTEMUI, mContext.getMainExecutor(), mPropertiesChangedListener); } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java index cd5bb775de8c6..10bb5a4bb57ee 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/dagger/StatusBarDependenciesModule.java @@ -42,6 +42,7 @@ import com.android.systemui.statusbar.phone.NotificationShadeWindowController; import com.android.systemui.statusbar.phone.StatusBar; import com.android.systemui.statusbar.policy.RemoteInputUriController; import com.android.systemui.tracing.ProtoTracer; +import com.android.systemui.util.DeviceConfigProxy; import java.util.concurrent.Executor; @@ -91,7 +92,8 @@ public interface StatusBarDependenciesModule { NotificationEntryManager notificationEntryManager, MediaArtworkProcessor mediaArtworkProcessor, KeyguardBypassController keyguardBypassController, - @Main Executor mainExecutor) { + @Main Executor mainExecutor, + DeviceConfigProxy deviceConfigProxy) { return new NotificationMediaManager( context, statusBarLazy, @@ -99,7 +101,8 @@ public interface StatusBarDependenciesModule { notificationEntryManager, mediaArtworkProcessor, keyguardBypassController, - mainExecutor); + mainExecutor, + deviceConfigProxy); } /** */ diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/SmartReplyConstants.java b/packages/SystemUI/src/com/android/systemui/statusbar/policy/SmartReplyConstants.java index 86fe300820a39..311e873812ac4 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/SmartReplyConstants.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/SmartReplyConstants.java @@ -25,10 +25,10 @@ import android.text.TextUtils; import android.util.KeyValueListParser; import android.util.Log; -import com.android.internal.annotations.VisibleForTesting; import com.android.internal.config.sysui.SystemUiDeviceConfigFlags; import com.android.systemui.R; import com.android.systemui.dagger.qualifiers.Main; +import com.android.systemui.util.DeviceConfigProxy; import javax.inject.Inject; import javax.inject.Singleton; @@ -62,10 +62,15 @@ public final class SmartReplyConstants { private final Handler mHandler; private final Context mContext; + private final DeviceConfigProxy mDeviceConfig; private final KeyValueListParser mParser = new KeyValueListParser(','); @Inject - public SmartReplyConstants(@Main Handler handler, Context context) { + public SmartReplyConstants( + @Main Handler handler, + Context context, + DeviceConfigProxy deviceConfig + ) { mHandler = handler; mContext = context; final Resources resources = mContext.getResources(); @@ -86,31 +91,35 @@ public final class SmartReplyConstants { mDefaultOnClickInitDelay = resources.getInteger( R.integer.config_smart_replies_in_notifications_onclick_init_delay); + mDeviceConfig = deviceConfig; registerDeviceConfigListener(); updateConstants(); } private void registerDeviceConfigListener() { - DeviceConfig.addOnPropertiesChangedListener( + mDeviceConfig.addOnPropertiesChangedListener( DeviceConfig.NAMESPACE_SYSTEMUI, this::postToHandler, - (properties) -> onDeviceConfigPropertiesChanged(properties.getNamespace())); + mOnPropertiesChangedListener); } private void postToHandler(Runnable r) { this.mHandler.post(r); } - @VisibleForTesting - void onDeviceConfigPropertiesChanged(String namespace) { - if (!DeviceConfig.NAMESPACE_SYSTEMUI.equals(namespace)) { - Log.e(TAG, "Received update from DeviceConfig for unrelated namespace: " - + namespace); - return; - } - - updateConstants(); - } + private final DeviceConfig.OnPropertiesChangedListener mOnPropertiesChangedListener = + new DeviceConfig.OnPropertiesChangedListener() { + @Override + public void onPropertiesChanged(DeviceConfig.Properties properties) { + if (!DeviceConfig.NAMESPACE_SYSTEMUI.equals(properties.getNamespace())) { + Log.e(TAG, + "Received update from DeviceConfig for unrelated namespace: " + + properties.getNamespace()); + return; + } + updateConstants(); + } + }; private void updateConstants() { synchronized (SmartReplyConstants.this) { @@ -120,7 +129,7 @@ public final class SmartReplyConstants { mRequiresTargetingP = readDeviceConfigBooleanOrDefaultIfEmpty( SystemUiDeviceConfigFlags.SSIN_REQUIRES_TARGETING_P, mDefaultRequiresP); - mMaxSqueezeRemeasureAttempts = DeviceConfig.getInt( + mMaxSqueezeRemeasureAttempts = mDeviceConfig.getInt( DeviceConfig.NAMESPACE_SYSTEMUI, SystemUiDeviceConfigFlags.SSIN_MAX_SQUEEZE_REMEASURE_ATTEMPTS, mDefaultMaxSqueezeRemeasureAttempts); @@ -130,24 +139,24 @@ public final class SmartReplyConstants { mShowInHeadsUp = readDeviceConfigBooleanOrDefaultIfEmpty( SystemUiDeviceConfigFlags.SSIN_SHOW_IN_HEADS_UP, mDefaultShowInHeadsUp); - mMinNumSystemGeneratedReplies = DeviceConfig.getInt( + mMinNumSystemGeneratedReplies = mDeviceConfig.getInt( DeviceConfig.NAMESPACE_SYSTEMUI, SystemUiDeviceConfigFlags.SSIN_MIN_NUM_SYSTEM_GENERATED_REPLIES, mDefaultMinNumSystemGeneratedReplies); - mMaxNumActions = DeviceConfig.getInt( + mMaxNumActions = mDeviceConfig.getInt( DeviceConfig.NAMESPACE_SYSTEMUI, SystemUiDeviceConfigFlags.SSIN_MAX_NUM_ACTIONS, mDefaultMaxNumActions); - mOnClickInitDelay = DeviceConfig.getInt( + mOnClickInitDelay = mDeviceConfig.getInt( DeviceConfig.NAMESPACE_SYSTEMUI, SystemUiDeviceConfigFlags.SSIN_ONCLICK_INIT_DELAY, mDefaultOnClickInitDelay); } } - private static boolean readDeviceConfigBooleanOrDefaultIfEmpty(String propertyName, + private boolean readDeviceConfigBooleanOrDefaultIfEmpty(String propertyName, boolean defaultValue) { - String value = DeviceConfig.getProperty(DeviceConfig.NAMESPACE_SYSTEMUI, propertyName); + String value = mDeviceConfig.getProperty(DeviceConfig.NAMESPACE_SYSTEMUI, propertyName); if (TextUtils.isEmpty(value)) { return defaultValue; } diff --git a/packages/SystemUI/tests/src/com/android/systemui/keyguard/KeyguardViewMediatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/keyguard/KeyguardViewMediatorTest.java index 9ef5520ee7115..8320b058208c3 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/keyguard/KeyguardViewMediatorTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/keyguard/KeyguardViewMediatorTest.java @@ -44,6 +44,8 @@ import com.android.systemui.classifier.FalsingManagerFake; import com.android.systemui.dump.DumpManager; import com.android.systemui.statusbar.phone.NotificationShadeWindowController; import com.android.systemui.statusbar.phone.StatusBarKeyguardViewManager; +import com.android.systemui.util.DeviceConfigProxy; +import com.android.systemui.util.DeviceConfigProxyFake; import com.android.systemui.util.concurrency.FakeExecutor; import com.android.systemui.util.time.FakeSystemClock; @@ -69,6 +71,7 @@ public class KeyguardViewMediatorTest extends SysuiTestCase { private @Mock DumpManager mDumpManager; private @Mock PowerManager mPowerManager; private @Mock TrustManager mTrustManager; + private DeviceConfigProxy mDeviceConfig = new DeviceConfigProxyFake(); private FakeExecutor mUiBgExecutor = new FakeExecutor(new FakeSystemClock()); private FalsingManagerFake mFalsingManager; @@ -85,7 +88,7 @@ public class KeyguardViewMediatorTest extends SysuiTestCase { mContext, mFalsingManager, mLockPatternUtils, mBroadcastDispatcher, mNotificationShadeWindowController, () -> mStatusBarKeyguardViewManager, mDismissCallbackRegistry, mUpdateMonitor, mDumpManager, mUiBgExecutor, - mPowerManager, mTrustManager); + mPowerManager, mTrustManager, mDeviceConfig); mViewMediator.start(); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SmartReplyConstantsTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SmartReplyConstantsTest.java index c761a44170c3e..e4e0dc9ec39ce 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SmartReplyConstantsTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SmartReplyConstantsTest.java @@ -22,7 +22,6 @@ import static junit.framework.Assert.assertTrue; import android.app.RemoteInput; import android.os.Handler; -import android.os.Looper; import android.provider.DeviceConfig; import android.test.suitebuilder.annotation.SmallTest; import android.testing.AndroidTestingRunner; @@ -32,8 +31,8 @@ import android.testing.TestableResources; import com.android.internal.config.sysui.SystemUiDeviceConfigFlags; import com.android.systemui.R; import com.android.systemui.SysuiTestCase; +import com.android.systemui.util.DeviceConfigProxyFake; -import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -42,14 +41,13 @@ import org.junit.runner.RunWith; @TestableLooper.RunWithLooper @SmallTest public class SmartReplyConstantsTest extends SysuiTestCase { - - private static final int CONTENT_OBSERVER_TIMEOUT_SECONDS = 10; - private SmartReplyConstants mConstants; + private DeviceConfigProxyFake mDeviceConfig; + private TestableLooper mTestableLooper; @Before public void setUp() { - resetAllDeviceConfigFlags(); + mDeviceConfig = new DeviceConfigProxyFake(); TestableResources resources = mContext.getOrCreateTestableResources(); resources.addOverride(R.bool.config_smart_replies_in_notifications_enabled, true); resources.addOverride( @@ -62,12 +60,12 @@ public class SmartReplyConstantsTest extends SysuiTestCase { 2); resources.addOverride( R.integer.config_smart_replies_in_notifications_max_num_actions, -1); - mConstants = new SmartReplyConstants(Handler.createAsync(Looper.myLooper()), mContext); - } - - @After - public void tearDown() { - resetAllDeviceConfigFlags(); + mTestableLooper = TestableLooper.get(this); + mConstants = new SmartReplyConstants( + new Handler(mTestableLooper.getLooper()), + mContext, + mDeviceConfig + ); } @Test @@ -78,25 +76,21 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testIsEnabledWithInvalidConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_ENABLED, "invalid config"); - triggerConstantsOnChange(); assertTrue(mConstants.isEnabled()); } @Test public void testIsEnabledWithValidConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_ENABLED, "false"); - triggerConstantsOnChange(); assertFalse(mConstants.isEnabled()); } @Test public void testRequiresTargetingPConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_REQUIRES_TARGETING_P, "false"); - triggerConstantsOnChange(); assertEquals(false, mConstants.requiresTargetingP()); overrideSetting(SystemUiDeviceConfigFlags.SSIN_REQUIRES_TARGETING_P, null); - triggerConstantsOnChange(); assertEquals(true, mConstants.requiresTargetingP()); } @@ -110,20 +104,17 @@ public class SmartReplyConstantsTest extends SysuiTestCase { public void testGetMaxSqueezeRemeasureAttemptsWithInvalidConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_MAX_SQUEEZE_REMEASURE_ATTEMPTS, "invalid config"); - triggerConstantsOnChange(); assertEquals(7, mConstants.getMaxSqueezeRemeasureAttempts()); } @Test public void testGetMaxSqueezeRemeasureAttemptsWithValidConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_MAX_SQUEEZE_REMEASURE_ATTEMPTS, "5"); - triggerConstantsOnChange(); assertEquals(5, mConstants.getMaxSqueezeRemeasureAttempts()); } @Test public void testGetEffectiveEditChoicesBeforeSendingWithNoConfig() { - triggerConstantsOnChange(); assertFalse( mConstants.getEffectiveEditChoicesBeforeSending( RemoteInput.EDIT_CHOICES_BEFORE_SENDING_AUTO)); @@ -138,7 +129,6 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testGetEffectiveEditChoicesBeforeSendingWithEnabledConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_EDIT_CHOICES_BEFORE_SENDING, "true"); - triggerConstantsOnChange(); assertTrue( mConstants.getEffectiveEditChoicesBeforeSending( RemoteInput.EDIT_CHOICES_BEFORE_SENDING_AUTO)); @@ -153,7 +143,6 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testGetEffectiveEditChoicesBeforeSendingWithDisabledConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_EDIT_CHOICES_BEFORE_SENDING, "false"); - triggerConstantsOnChange(); assertFalse( mConstants.getEffectiveEditChoicesBeforeSending( RemoteInput.EDIT_CHOICES_BEFORE_SENDING_AUTO)); @@ -174,14 +163,12 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testShowInHeadsUpEnabled() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_SHOW_IN_HEADS_UP, "true"); - triggerConstantsOnChange(); assertTrue(mConstants.getShowInHeadsUp()); } @Test public void testShowInHeadsUpDisabled() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_SHOW_IN_HEADS_UP, "false"); - triggerConstantsOnChange(); assertFalse(mConstants.getShowInHeadsUp()); } @@ -194,7 +181,6 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testGetMinNumSystemGeneratedRepliesWithValidConfig() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_MIN_NUM_SYSTEM_GENERATED_REPLIES, "5"); - triggerConstantsOnChange(); assertEquals(5, mConstants.getMinNumSystemGeneratedReplies()); } @@ -207,7 +193,6 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testMaxNumActionsSet() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_MAX_NUM_ACTIONS, "10"); - triggerConstantsOnChange(); assertEquals(10, mConstants.getMaxNumActions()); } @@ -219,38 +204,12 @@ public class SmartReplyConstantsTest extends SysuiTestCase { @Test public void testOnClickInitDelaySet() { overrideSetting(SystemUiDeviceConfigFlags.SSIN_ONCLICK_INIT_DELAY, "50"); - triggerConstantsOnChange(); assertEquals(50, mConstants.getOnClickInitDelay()); } private void overrideSetting(String propertyName, String value) { - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, + mDeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, propertyName, value, false /* makeDefault */); - } - - private void triggerConstantsOnChange() { - mConstants.onDeviceConfigPropertiesChanged(DeviceConfig.NAMESPACE_SYSTEMUI); - } - - private void resetAllDeviceConfigFlags() { - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_ENABLED, null, false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_REQUIRES_TARGETING_P, null, false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_MAX_SQUEEZE_REMEASURE_ATTEMPTS, null, - false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_EDIT_CHOICES_BEFORE_SENDING, null, - false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_SHOW_IN_HEADS_UP, null, false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_MIN_NUM_SYSTEM_GENERATED_REPLIES, null, - false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_MAX_NUM_ACTIONS, null, false /* makeDefault */); - DeviceConfig.setProperty(DeviceConfig.NAMESPACE_SYSTEMUI, - SystemUiDeviceConfigFlags.SSIN_ONCLICK_INIT_DELAY, null, false /* makeDefault */); + mTestableLooper.processAllMessages(); } } From 11425c919048ae35dfec26c522513316f7cc38ee Mon Sep 17 00:00:00 2001 From: Beverly Date: Wed, 26 Feb 2020 14:23:16 -0500 Subject: [PATCH 2/2] Use a BgExecutor in SecurityControllerImpl For more robust tests! Now, we can make sure the executor runs all its runnables instead of waiting (test was previously very flaky). Test: atest SystemUITests Fixes: 149204632 Change-Id: I8ad047538b31709837c68399dcc1ee5b950283f9 --- .../policy/SecurityControllerImpl.java | 67 ++++++-------- .../policy/SecurityControllerTest.java | 89 +++++++++---------- 2 files changed, 70 insertions(+), 86 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/SecurityControllerImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/policy/SecurityControllerImpl.java index 312c4ac75bfaa..d29f4fcd0c262 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/SecurityControllerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/SecurityControllerImpl.java @@ -31,14 +31,12 @@ import android.net.IConnectivityManager; import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkRequest; -import android.os.AsyncTask; import android.os.Handler; import android.os.RemoteException; import android.os.ServiceManager; import android.os.UserHandle; import android.os.UserManager; import android.security.KeyChain; -import android.security.KeyChain.KeyChainConnection; import android.util.ArrayMap; import android.util.Log; import android.util.Pair; @@ -55,6 +53,7 @@ import com.android.systemui.settings.CurrentUserTracker; import java.io.FileDescriptor; import java.io.PrintWriter; import java.util.ArrayList; +import java.util.concurrent.Executor; import javax.inject.Inject; import javax.inject.Singleton; @@ -85,7 +84,7 @@ public class SecurityControllerImpl extends CurrentUserTracker implements Securi private final DevicePolicyManager mDevicePolicyManager; private final PackageManager mPackageManager; private final UserManager mUserManager; - private final Handler mBgHandler; + private final Executor mBgExecutor; @GuardedBy("mCallbacks") private final ArrayList mCallbacks = new ArrayList<>(); @@ -101,16 +100,14 @@ public class SecurityControllerImpl extends CurrentUserTracker implements Securi /** */ @Inject - public SecurityControllerImpl(Context context, @Background Handler bgHandler, - BroadcastDispatcher broadcastDispatcher) { - this(context, bgHandler, broadcastDispatcher, null); - } - - public SecurityControllerImpl(Context context, Handler bgHandler, - BroadcastDispatcher broadcastDispatcher, SecurityControllerCallback callback) { + public SecurityControllerImpl( + Context context, + @Background Handler bgHandler, + BroadcastDispatcher broadcastDispatcher, + @Background Executor bgExecutor + ) { super(broadcastDispatcher); mContext = context; - mBgHandler = bgHandler; mDevicePolicyManager = (DevicePolicyManager) context.getSystemService(Context.DEVICE_POLICY_SERVICE); mConnectivityManager = (ConnectivityManager) @@ -118,10 +115,8 @@ public class SecurityControllerImpl extends CurrentUserTracker implements Securi mConnectivityManagerService = IConnectivityManager.Stub.asInterface( ServiceManager.getService(Context.CONNECTIVITY_SERVICE)); mPackageManager = context.getPackageManager(); - mUserManager = (UserManager) - context.getSystemService(Context.USER_SERVICE); - - addCallback(callback); + mUserManager = (UserManager) context.getSystemService(Context.USER_SERVICE); + mBgExecutor = bgExecutor; IntentFilter filter = new IntentFilter(); filter.addAction(KeyChain.ACTION_TRUST_STORE_CHANGED); @@ -305,7 +300,23 @@ public class SecurityControllerImpl extends CurrentUserTracker implements Securi } private void refreshCACerts(int userId) { - new CACertLoader().execute(userId); + mBgExecutor.execute(() -> { + Pair idWithCert = null; + try (KeyChain.KeyChainConnection conn = KeyChain.bindAsUser(mContext, + UserHandle.of(userId))) { + boolean hasCACerts = !(conn.getService().getUserCaAliases().getList().isEmpty()); + idWithCert = new Pair(userId, hasCACerts); + } catch (RemoteException | InterruptedException | AssertionError e) { + Log.i(TAG, "failed to get CA certs", e); + idWithCert = new Pair(userId, null); + } finally { + if (DEBUG) Log.d(TAG, "Refreshing CA Certs " + idWithCert); + if (idWithCert != null && idWithCert.second != null) { + mHasCACerts.put(idWithCert.first, idWithCert.second); + fireCallbacks(); + } + } + }); } private String getNameForVpnConfig(VpnConfig cfg, UserHandle user) { @@ -408,28 +419,4 @@ public class SecurityControllerImpl extends CurrentUserTracker implements Securi } } }; - - protected class CACertLoader extends AsyncTask > { - - @Override - protected Pair doInBackground(Integer... userId) { - try (KeyChainConnection conn = KeyChain.bindAsUser(mContext, - UserHandle.of(userId[0]))) { - boolean hasCACerts = !(conn.getService().getUserCaAliases().getList().isEmpty()); - return new Pair(userId[0], hasCACerts); - } catch (RemoteException | InterruptedException | AssertionError e) { - Log.i(TAG, "failed to get CA certs", e); - return new Pair(userId[0], null); - } - } - - @Override - protected void onPostExecute(Pair result) { - if (DEBUG) Log.d(TAG, "onPostExecute " + result); - if (result.second != null) { - mHasCACerts.put(result.first, result.second); - fireCallbacks(); - } - } - } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SecurityControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SecurityControllerTest.java index e6b04405a4cec..44184ee8eeedb 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SecurityControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/SecurityControllerTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.argThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -28,6 +29,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.app.admin.DevicePolicyManager; +import android.content.BroadcastReceiver; import android.content.ComponentName; import android.content.Context; import android.content.Intent; @@ -37,7 +39,6 @@ import android.net.ConnectivityManager; import android.net.ConnectivityManager.NetworkCallback; import android.net.NetworkRequest; import android.os.Handler; -import android.os.Looper; import android.os.UserManager; import android.security.IKeyChainService; import android.test.suitebuilder.annotation.SmallTest; @@ -46,34 +47,30 @@ import androidx.test.runner.AndroidJUnit4; import com.android.systemui.SysuiTestCase; import com.android.systemui.broadcast.BroadcastDispatcher; -import com.android.systemui.statusbar.policy.SecurityController.SecurityControllerCallback; +import com.android.systemui.util.concurrency.FakeExecutor; +import com.android.systemui.util.time.FakeSystemClock; -import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; @SmallTest @RunWith(AndroidJUnit4.class) -public class SecurityControllerTest extends SysuiTestCase implements SecurityControllerCallback { +public class SecurityControllerTest extends SysuiTestCase { private final DevicePolicyManager mDevicePolicyManager = mock(DevicePolicyManager.class); private final IKeyChainService.Stub mKeyChainService = mock(IKeyChainService.Stub.class); private final UserManager mUserManager = mock(UserManager.class); + private final BroadcastDispatcher mBroadcastDispatcher = mock(BroadcastDispatcher.class); + private final Handler mHandler = mock(Handler.class); private SecurityControllerImpl mSecurityController; - private CountDownLatch mStateChangedLatch; private ConnectivityManager mConnectivityManager = mock(ConnectivityManager.class); - - // implementing SecurityControllerCallback - @Override - public void onStateChanged() { - mStateChangedLatch.countDown(); - } + private FakeExecutor mBgExecutor; + private BroadcastReceiver mBroadcastReceiver; @Before public void setUp() throws Exception { @@ -95,18 +92,23 @@ public class SecurityControllerTest extends SysuiTestCase implements SecurityCon when(mKeyChainService.queryLocalInterface("android.security.IKeyChainService")) .thenReturn(mKeyChainService); - // Wait for callbacks from the onUserSwitched() function in the - // constructor of mSecurityController - mStateChangedLatch = new CountDownLatch(1); - // TODO: Migrate this test to TestableLooper and use a handler attached - // to that. - mSecurityController = new SecurityControllerImpl(mContext, - new Handler(Looper.getMainLooper()), mock(BroadcastDispatcher.class), this); - } + ArgumentCaptor brCaptor = + ArgumentCaptor.forClass(BroadcastReceiver.class); - @After - public void tearDown() { - mSecurityController.removeCallback(this); + mBgExecutor = new FakeExecutor(new FakeSystemClock()); + mSecurityController = new SecurityControllerImpl( + mContext, + mHandler, + mBroadcastDispatcher, + mBgExecutor); + + verify(mBroadcastDispatcher).registerReceiverWithHandler( + brCaptor.capture(), + anyObject(), + anyObject(), + anyObject()); + + mBroadcastReceiver = brCaptor.getValue(); } @Test @@ -126,8 +128,6 @@ public class SecurityControllerTest extends SysuiTestCase implements SecurityCon @Test public void testWorkAccount() throws Exception { - // Wait for the callbacks from setUp() - assertTrue(mStateChangedLatch.await(1, TimeUnit.SECONDS)); assertFalse(mSecurityController.hasCACertInCurrentUser()); final int PRIMARY_USER_ID = 0; @@ -140,53 +140,41 @@ public class SecurityControllerTest extends SysuiTestCase implements SecurityCon assertTrue(mSecurityController.hasWorkProfile()); assertFalse(mSecurityController.hasCACertInWorkProfile()); - mStateChangedLatch = new CountDownLatch(1); - when(mKeyChainService.getUserCaAliases()) .thenReturn(new StringParceledListSlice(Arrays.asList("One CA Alias"))); - mSecurityController.new CACertLoader() - .execute(MANAGED_USER_ID); + refreshCACerts(MANAGED_USER_ID); + mBgExecutor.runAllReady(); - assertTrue(mStateChangedLatch.await(3, TimeUnit.SECONDS)); assertTrue(mSecurityController.hasCACertInWorkProfile()); } @Test public void testCaCertLoader() throws Exception { - // Wait for the callbacks from setUp() - assertTrue(mStateChangedLatch.await(1, TimeUnit.SECONDS)); assertFalse(mSecurityController.hasCACertInCurrentUser()); // With a CA cert - mStateChangedLatch = new CountDownLatch(1); - when(mKeyChainService.getUserCaAliases()) .thenReturn(new StringParceledListSlice(Arrays.asList("One CA Alias"))); - mSecurityController.new CACertLoader() - .execute(0); + refreshCACerts(0); + mBgExecutor.runAllReady(); - assertTrue(mStateChangedLatch.await(3, TimeUnit.SECONDS)); assertTrue(mSecurityController.hasCACertInCurrentUser()); // Exception - mStateChangedLatch = new CountDownLatch(1); - when(mKeyChainService.getUserCaAliases()) .thenThrow(new AssertionError("Test AssertionError")) .thenReturn(new StringParceledListSlice(new ArrayList())); - mSecurityController.new CACertLoader() - .execute(0); + refreshCACerts(0); + mBgExecutor.runAllReady(); - assertFalse(mStateChangedLatch.await(1, TimeUnit.SECONDS)); assertTrue(mSecurityController.hasCACertInCurrentUser()); - mSecurityController.new CACertLoader() - .execute(0); + refreshCACerts(0); + mBgExecutor.runAllReady(); - assertTrue(mStateChangedLatch.await(1, TimeUnit.SECONDS)); assertFalse(mSecurityController.hasCACertInCurrentUser()); } @@ -197,4 +185,13 @@ public class SecurityControllerTest extends SysuiTestCase implements SecurityCon && request.networkCapabilities.getCapabilities().length == 0 ), any(NetworkCallback.class)); } + + /** + * refresh CA certs by sending a user unlocked broadcast for the desired user + */ + private void refreshCACerts(int userId) { + Intent intent = new Intent(Intent.ACTION_USER_UNLOCKED); + intent.putExtra(Intent.EXTRA_USER_HANDLE, userId); + mBroadcastReceiver.onReceive(mContext, intent); + } }