From 595956489d320d8df63ac1a5deaecbb26fd9e3f7 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 3 Dec 2021 02:55:01 +0000 Subject: [PATCH 1/2] FeatureFlags Listeners: * Convert FlagReader interface to FlagListenable. * Move Flag reading to FeatureFlags interface. * Remove isEnabled(id, boolean) from all interfaces; make private in implementations where used. Bug: 209081785 Test: atest FlagManagerTest Change-Id: I8c90a67def242b5d2fa55d5effab8270ecafabb4 --- .../{FlagReader.kt => FlagListenable.kt} | 32 ++- .../com/android/systemui/flags/FlagManager.kt | 88 ++++--- .../systemui/flags/FlagSettingsHelper.kt | 42 ++++ .../{FeatureFlags.java => FeatureFlags.kt} | 13 +- .../systemui/flags/FeatureFlagsDebug.java | 41 ++-- .../systemui/flags/FeatureFlagsRelease.java | 10 +- .../flags/FeatureFlagsReleaseTest.java | 6 +- .../android/systemui/flags/FlagManagerTest.kt | 214 ++++++++++++++++++ 8 files changed, 374 insertions(+), 72 deletions(-) rename packages/SystemUI/shared/src/com/android/systemui/flags/{FlagReader.kt => FlagListenable.kt} (61%) create mode 100644 packages/SystemUI/shared/src/com/android/systemui/flags/FlagSettingsHelper.kt rename packages/SystemUI/src/com/android/systemui/flags/{FeatureFlags.java => FeatureFlags.kt} (66%) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagListenable.kt similarity index 61% rename from packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt rename to packages/SystemUI/shared/src/com/android/systemui/flags/FlagListenable.kt index 26c6a4b096fc8..195ba46515cd2 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagReader.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagListenable.kt @@ -18,28 +18,24 @@ package com.android.systemui.flags /** * Plugin for loading flag values */ -interface FlagReader { - /** Returns a boolean value for the given flag. */ - fun isEnabled(flag: BooleanFlag): Boolean { - return flag.default - } - - fun isEnabled(flag: ResourceBooleanFlag): Boolean - - /** Returns a boolean value for the given flag. */ - fun isEnabled(id: Int, def: Boolean): Boolean { - return def - } - - /** Add a listener to be alerted when any flag changes. */ - fun addListener(listener: Listener) {} +interface FlagListenable { + /** Add a listener to be alerted when the given flag changes. */ + fun addListener(flag: Flag<*>, listener: Listener) /** Remove a listener to be alerted when any flag changes. */ - fun removeListener(listener: Listener) {} + fun removeListener(listener: Listener) /** A simple listener to be alerted when a flag changes. */ fun interface Listener { - /** */ - fun onFlagChanged(id: Int) + /** Called when the flag changes */ + fun onFlagChanged(event: FlagEvent) + } + + /** An event representing the change */ + interface FlagEvent { + /** the id of the flag which changed */ + val flagId: Int + /** if all listeners alerted invoke this method, the restart will be skipped */ + fun requestNoRestart() } } \ No newline at end of file diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt index b2ca2d7765d9f..42fec7775a421 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt @@ -24,16 +24,17 @@ import android.database.ContentObserver import android.net.Uri import android.os.Bundle import android.os.Handler -import android.provider.Settings import androidx.concurrent.futures.CallbackToFutureAdapter import com.google.common.util.concurrent.ListenableFuture import org.json.JSONException import org.json.JSONObject +import java.util.function.Consumer class FlagManager constructor( private val context: Context, + private val settings: FlagSettingsHelper, private val handler: Handler -) : FlagReader { +) : FlagListenable { companion object { const val RECEIVING_PACKAGE = "com.android.systemui" const val ACTION_SET_FLAG = "com.android.systemui.action.SET_FLAG" @@ -47,7 +48,19 @@ class FlagManager constructor( private const val SETTINGS_PREFIX = "systemui/flags" } - private val listeners: MutableSet = mutableSetOf() + constructor(context: Context, handler: Handler) : this( + context, + FlagSettingsHelper(context.contentResolver), + handler + ) + + /** + * An action called on restart which takes as an argument whether the listeners requested + * that the restart be suppressed + */ + var restartAction: Consumer? = null + var clearCacheAction: Consumer? = null + private val listeners: MutableSet = mutableSetOf() private val settingsObserver: ContentObserver = SettingsObserver() fun getFlagsFuture(): ListenableFuture>> { @@ -86,14 +99,9 @@ class FlagManager constructor( context.sendBroadcast(intent) } - override fun isEnabled(id: Int, def: Boolean): Boolean { - return isEnabled(id) ?: def - } - /** Returns the stored value or null if not set. */ fun isEnabled(id: Int): Boolean? { - val data: String? = Settings.Secure.getString( - context.contentResolver, keyToSettingsPrefix(id)) + val data = settings.getString(keyToSettingsPrefix(id)) if (data == null || data?.isEmpty()) { return null } @@ -108,27 +116,24 @@ class FlagManager constructor( } } - override fun isEnabled(flag: ResourceBooleanFlag): Boolean { - throw RuntimeException("Not implemented in FlagManager") - } - - override fun addListener(listener: FlagReader.Listener) { + override fun addListener(flag: Flag<*>, listener: FlagListenable.Listener) { synchronized(listeners) { val registerNeeded = listeners.isEmpty() - listeners.add(listener) + listeners.add(PerFlagListener(flag.id, listener)) if (registerNeeded) { - context.contentResolver.registerContentObserver( - Settings.Secure.getUriFor(SETTINGS_PREFIX), true, settingsObserver) + settings.registerContentObserver(SETTINGS_PREFIX, true, settingsObserver) } } } - override fun removeListener(listener: FlagReader.Listener) { + override fun removeListener(listener: FlagListenable.Listener) { synchronized(listeners) { - val isRegistered = !listeners.isEmpty() - listeners.remove(listener) - if (isRegistered && listeners.isEmpty()) { - context.contentResolver.unregisterContentObserver(settingsObserver) + if (listeners.isEmpty()) { + return + } + listeners.removeIf { it.listener == listener } + if (listeners.isEmpty()) { + settings.unregisterContentObserver(settingsObserver) } } } @@ -142,7 +147,7 @@ class FlagManager constructor( } fun keyToSettingsPrefix(key: Int): String { - return SETTINGS_PREFIX + "/" + key + return "$SETTINGS_PREFIX/$key" } private fun assertType(json: JSONObject, type: String): Boolean { @@ -160,14 +165,39 @@ class FlagManager constructor( } val parts = uri.pathSegments val idStr = parts[parts.size - 1] - try { - val id = idStr.toInt() - listeners.forEach { l -> l.onFlagChanged(id) } - } catch (e: NumberFormatException) { - // no-op - } + val id = try { idStr.toInt() } catch (e: NumberFormatException) { return } + clearCacheAction?.accept(id) + dispatchListenersAndMaybeRestart(id) } } + + fun dispatchListenersAndMaybeRestart(id: Int) { + val filteredListeners: List = synchronized(listeners) { + listeners.mapNotNull { if (it.id == id) it.listener else null } + } + // If there are no listeners, there's nothing to dispatch to, and nothing to suppress it. + if (filteredListeners.isEmpty()) { + restartAction?.accept(false) + return + } + // Dispatch to every listener and save whether each one called requestNoRestart. + val suppressRestartList: List = filteredListeners.map { listener -> + var didRequestNoRestart = false + val event = object : FlagListenable.FlagEvent { + override val flagId = id + override fun requestNoRestart() { + didRequestNoRestart = true + } + } + listener.onFlagChanged(event) + didRequestNoRestart + } + // Suppress restart only if ALL listeners request it. + val suppressRestart = suppressRestartList.all { it } + restartAction?.accept(suppressRestart) + } + + private data class PerFlagListener(val id: Int, val listener: FlagListenable.Listener) } class InvalidFlagStorageException : Exception("Data found but is invalid") diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagSettingsHelper.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagSettingsHelper.kt new file mode 100644 index 0000000000000..742bb0b6f954d --- /dev/null +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagSettingsHelper.kt @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2021 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 + * + * http://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 com.android.systemui.flags + +import android.content.ContentResolver +import android.database.ContentObserver +import android.provider.Settings + +class FlagSettingsHelper(private val contentResolver: ContentResolver) { + + fun getString(key: String): String? = Settings.Secure.getString(contentResolver, key) + + fun registerContentObserver( + name: String, + notifyForDescendants: Boolean, + observer: ContentObserver + ) { + contentResolver.registerContentObserver( + Settings.Secure.getUriFor(name), + notifyForDescendants, + observer + ) + } + + fun unregisterContentObserver(observer: ContentObserver) { + contentResolver.unregisterContentObserver(observer) + } +} \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt similarity index 66% rename from packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java rename to packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt index fbb78c86a52a2..d4b23c7aa7d3d 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt @@ -14,12 +14,17 @@ * limitations under the License. */ -package com.android.systemui.flags; +package com.android.systemui.flags /** * Class to manage simple DeviceConfig-based feature flags. * - * See {@link Flags} for instructions on defining new flags. + * See [Flags] for instructions on defining new flags. */ -public interface FeatureFlags extends FlagReader { -} +interface FeatureFlags : FlagListenable { + /** Returns a boolean value for the given flag. */ + fun isEnabled(flag: BooleanFlag): Boolean + + /** Returns a boolean value for the given flag. */ + fun isEnabled(flag: ResourceBooleanFlag): Boolean +} \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java index 3f00b87188d3f..b5916f19669ab 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java @@ -22,7 +22,6 @@ import static com.android.systemui.flags.FlagManager.FIELD_FLAGS; import static com.android.systemui.flags.FlagManager.FIELD_ID; import static com.android.systemui.flags.FlagManager.FIELD_VALUE; -import android.annotation.Nullable; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -32,6 +31,7 @@ import android.os.Bundle; import android.util.Log; import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import com.android.systemui.Dumpable; import com.android.systemui.dagger.SysUISingleton; @@ -81,6 +81,8 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { IntentFilter filter = new IntentFilter(); filter.addAction(ACTION_SET_FLAG); filter.addAction(ACTION_GET_FLAGS); + flagManager.setRestartAction(this::restartSystemUI); + flagManager.setClearCacheAction(this::removeFromCache); context.registerReceiver(mReceiver, filter, null, null); dumpManager.registerDumpable(TAG, this); } @@ -106,9 +108,8 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { return mBooleanFlagCache.get(id); } - /** Return a {@link BooleanFlag}'s value. */ - @Override - public boolean isEnabled(int id, boolean defaultValue) { + /** Return a flag's value. */ + private boolean isEnabled(int id, boolean defaultValue) { Boolean result = isEnabledInternal(id); return result == null ? defaultValue : result; } @@ -136,17 +137,19 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { json.put(FlagManager.FIELD_TYPE, FlagManager.TYPE_BOOLEAN); json.put(FIELD_VALUE, value); mSecureSettings.putString(mFlagManager.keyToSettingsPrefix(id), json.toString()); - Log.i(TAG, "Set id " + id + " to " + value); - restartSystemUI(); } catch (JSONException e) { - // no-op + return; // ignore } + Log.i(TAG, "Set id " + id + " to " + value); + removeFromCache(id); + mFlagManager.dispatchListenersAndMaybeRestart(id); } /** Erase a flag's overridden value if there is one. */ public void eraseFlag(int id) { eraseInternal(id); - restartSystemUI(); + removeFromCache(id); + mFlagManager.dispatchListenersAndMaybeRestart(id); } /** Works just like {@link #eraseFlag(int)} except that it doesn't restart SystemUI. */ @@ -157,16 +160,20 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { } @Override - public void addListener(Listener run) { - mFlagManager.addListener(run); + public void addListener(@NonNull Flag flag, @NonNull Listener listener) { + mFlagManager.addListener(flag, listener); } @Override - public void removeListener(Listener run) { - mFlagManager.removeListener(run); + public void removeListener(@NonNull Listener listener) { + mFlagManager.removeListener(listener); } - private void restartSystemUI() { + private void restartSystemUI(boolean requestSuppress) { + if (requestSuppress) { + Log.i(TAG, "SystemUI Restart Suppressed"); + return; + } Log.i(TAG, "Restarting SystemUI"); // SysUI starts back when up exited. Is there a better way to do this? System.exit(0); @@ -202,6 +209,10 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { } private void handleSetFlag(Bundle extras) { + if (extras == null) { + Log.w(TAG, "No extras"); + return; + } int id = extras.getInt(FIELD_ID); if (id <= 0) { Log.w(TAG, "ID not set or less than or equal to 0: " + id); @@ -245,6 +256,10 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { } }; + private void removeFromCache(int id) { + mBooleanFlagCache.remove(id); + } + @Override public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) { pw.println("can override: true"); diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java index 5b6404fa6a1f5..d82b89b6dfd4e 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java @@ -48,10 +48,10 @@ public class FeatureFlagsRelease implements FeatureFlags, Dumpable { } @Override - public void addListener(Listener run) {} + public void addListener(@NonNull Flag flag, @NonNull Listener listener) {} @Override - public void removeListener(Listener run) {} + public void removeListener(@NonNull Listener listener) {} @Override public boolean isEnabled(BooleanFlag flag) { @@ -68,8 +68,7 @@ public class FeatureFlagsRelease implements FeatureFlags, Dumpable { return mFlagCache.valueAt(cacheIndex); } - @Override - public boolean isEnabled(int key, boolean defaultValue) { + private boolean isEnabled(int key, boolean defaultValue) { mFlagCache.append(key, defaultValue); return defaultValue; } @@ -79,8 +78,7 @@ public class FeatureFlagsRelease implements FeatureFlags, Dumpable { pw.println("can override: false"); int size = mFlagCache.size(); for (int i = 0; i < size; i++) { - pw.println(" sysui_flag_" + mFlagCache.keyAt(i) - + ": " + mFlagCache.valueAt(i)); + pw.println(" sysui_flag_" + mFlagCache.keyAt(i) + ": " + mFlagCache.valueAt(i)); } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java index dea42f969866b..b0fdcf4336bdb 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java @@ -81,21 +81,23 @@ public class FeatureFlagsReleaseTest extends SysuiTestCase { public void testDump() { int flagIdA = 213; int flagIdB = 18; + int flagIdC = 1; int flagResourceId = 3; BooleanFlag flagA = new BooleanFlag(flagIdA, true); ResourceBooleanFlag flagB = new ResourceBooleanFlag(flagIdB, flagResourceId); + BooleanFlag flagC = new BooleanFlag(flagIdC, false); when(mResources.getBoolean(flagResourceId)).thenReturn(true); // WHEN the flags have been accessed - assertThat(mFeatureFlagsRelease.isEnabled(1, false)).isFalse(); assertThat(mFeatureFlagsRelease.isEnabled(flagA)).isTrue(); assertThat(mFeatureFlagsRelease.isEnabled(flagB)).isTrue(); + assertThat(mFeatureFlagsRelease.isEnabled(flagC)).isFalse(); // THEN the dump contains the flags and the default values String dump = dumpToString(); - assertThat(dump).contains(" sysui_flag_1: false\n"); assertThat(dump).contains(" sysui_flag_" + flagIdA + ": true\n"); assertThat(dump).contains(" sysui_flag_" + flagIdB + ": true\n"); + assertThat(dump).contains(" sysui_flag_" + flagIdC + ": false\n"); } private String dumpToString() { diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt new file mode 100644 index 0000000000000..aba656f7aae73 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt @@ -0,0 +1,214 @@ +/* + * Copyright (C) 2021 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 + * + * http://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 com.android.systemui.flags + +import android.content.Context +import android.database.ContentObserver +import android.net.Uri +import android.os.Handler +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.eq +import com.android.systemui.util.mockito.mock +import com.android.systemui.util.mockito.withArgCaptor +import com.google.common.truth.Truth.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.Mockito.times +import org.mockito.Mockito.verify +import org.mockito.Mockito.verifyNoMoreInteractions +import org.mockito.MockitoAnnotations +import java.util.function.Consumer + +/** + * NOTE: This test is for the version of FeatureFlagManager in src-release, which should not allow + * overriding, and should never return any value other than the one provided as the default. + */ +@SmallTest +class FlagManagerTest : SysuiTestCase() { + private lateinit var mFlagManager: FlagManager + + @Mock private lateinit var mMockContext: Context + @Mock private lateinit var mFlagSettingsHelper: FlagSettingsHelper + @Mock private lateinit var mHandler: Handler + + @Before + fun setup() { + MockitoAnnotations.initMocks(this) + mFlagManager = FlagManager(mMockContext, mFlagSettingsHelper, mHandler) + } + + @Test + fun testContentObserverAddedAndRemoved() { + val listener1 = mock() + val listener2 = mock() + + // no interactions before adding listener + verifyNoMoreInteractions(mFlagSettingsHelper) + + // adding the first listener registers the observer + mFlagManager.addListener(BooleanFlag(1, true), listener1) + val observer = withArgCaptor { + verify(mFlagSettingsHelper).registerContentObserver(any(), any(), capture()) + } + verifyNoMoreInteractions(mFlagSettingsHelper) + + // adding another listener does nothing + mFlagManager.addListener(BooleanFlag(2, true), listener2) + verifyNoMoreInteractions(mFlagSettingsHelper) + + // removing the original listener does nothing with second one still present + mFlagManager.removeListener(listener1) + verifyNoMoreInteractions(mFlagSettingsHelper) + + // removing the final listener unregisters the observer + mFlagManager.removeListener(listener2) + verify(mFlagSettingsHelper).unregisterContentObserver(eq(observer)) + verifyNoMoreInteractions(mFlagSettingsHelper) + } + + @Test + fun testObserverClearsCache() { + val listener = mock() + val clearCacheAction = mock>() + mFlagManager.clearCacheAction = clearCacheAction + mFlagManager.addListener(BooleanFlag(1, true), listener) + val observer = withArgCaptor { + verify(mFlagSettingsHelper).registerContentObserver(any(), any(), capture()) + } + observer.onChange(false, flagUri(1)) + verify(clearCacheAction).accept(eq(1)) + } + + @Test + fun testObserverInvokesListeners() { + val listener1 = mock() + val listener10 = mock() + mFlagManager.addListener(BooleanFlag(1, true), listener1) + mFlagManager.addListener(BooleanFlag(10, true), listener10) + val observer = withArgCaptor { + verify(mFlagSettingsHelper).registerContentObserver(any(), any(), capture()) + } + observer.onChange(false, flagUri(1)) + val flagEvent1 = withArgCaptor { + verify(listener1).onFlagChanged(capture()) + } + assertThat(flagEvent1.flagId).isEqualTo(1) + verifyNoMoreInteractions(listener1, listener10) + + observer.onChange(false, flagUri(10)) + val flagEvent10 = withArgCaptor { + verify(listener10).onFlagChanged(capture()) + } + assertThat(flagEvent10.flagId).isEqualTo(10) + verifyNoMoreInteractions(listener1, listener10) + } + + fun flagUri(id: Int): Uri = Uri.parse("content://settings/system/systemui/flags/$id") + + @Test + fun testOnlySpecificFlagListenerIsInvoked() { + val listener1 = mock() + val listener10 = mock() + mFlagManager.addListener(BooleanFlag(1, true), listener1) + mFlagManager.addListener(BooleanFlag(10, true), listener10) + + mFlagManager.dispatchListenersAndMaybeRestart(1) + val flagEvent1 = withArgCaptor { + verify(listener1).onFlagChanged(capture()) + } + assertThat(flagEvent1.flagId).isEqualTo(1) + verifyNoMoreInteractions(listener1, listener10) + + mFlagManager.dispatchListenersAndMaybeRestart(10) + val flagEvent10 = withArgCaptor { + verify(listener10).onFlagChanged(capture()) + } + assertThat(flagEvent10.flagId).isEqualTo(10) + verifyNoMoreInteractions(listener1, listener10) + } + + @Test + fun testSameListenerCanBeUsedForMultipleFlags() { + val listener = mock() + mFlagManager.addListener(BooleanFlag(1, true), listener) + mFlagManager.addListener(BooleanFlag(10, true), listener) + + mFlagManager.dispatchListenersAndMaybeRestart(1) + val flagEvent1 = withArgCaptor { + verify(listener).onFlagChanged(capture()) + } + assertThat(flagEvent1.flagId).isEqualTo(1) + verifyNoMoreInteractions(listener) + + mFlagManager.dispatchListenersAndMaybeRestart(10) + val flagEvent10 = withArgCaptor { + verify(listener, times(2)).onFlagChanged(capture()) + } + assertThat(flagEvent10.flagId).isEqualTo(10) + verifyNoMoreInteractions(listener) + } + + @Test + fun testRestartWithNoListeners() { + val restartAction = mock>() + mFlagManager.restartAction = restartAction + mFlagManager.dispatchListenersAndMaybeRestart(1) + verify(restartAction).accept(eq(false)) + verifyNoMoreInteractions(restartAction) + } + + @Test + fun testListenerCanSuppressRestart() { + val restartAction = mock>() + mFlagManager.restartAction = restartAction + mFlagManager.addListener(BooleanFlag(1, true)) { event -> + event.requestNoRestart() + } + mFlagManager.dispatchListenersAndMaybeRestart(1) + verify(restartAction).accept(eq(true)) + verifyNoMoreInteractions(restartAction) + } + + @Test + fun testListenerOnlySuppressesRestartForOwnFlag() { + val restartAction = mock>() + mFlagManager.restartAction = restartAction + mFlagManager.addListener(BooleanFlag(10, true)) { event -> + event.requestNoRestart() + } + mFlagManager.dispatchListenersAndMaybeRestart(1) + verify(restartAction).accept(eq(false)) + verifyNoMoreInteractions(restartAction) + } + + @Test + fun testRestartWhenNotAllListenersRequestSuppress() { + val restartAction = mock>() + mFlagManager.restartAction = restartAction + mFlagManager.addListener(BooleanFlag(10, true)) { event -> + event.requestNoRestart() + } + mFlagManager.addListener(BooleanFlag(10, true)) { + // do not request + } + mFlagManager.dispatchListenersAndMaybeRestart(1) + verify(restartAction).accept(eq(false)) + verifyNoMoreInteractions(restartAction) + } +} \ No newline at end of file From 3b6176a6bd90cd99c59c35bc1df7223b19fa737b Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 3 Dec 2021 20:12:54 +0000 Subject: [PATCH 2/2] Generalize some flag manager methods by flag type. Bug: 209081785 Test: atest FeatureFlagsDebugTest FlagManagerTest Change-Id: Ie92667e33e7c086258cdd03e7b9ebccdefef5938 --- .../com/android/systemui/flags/FlagManager.kt | 57 ++-- .../android/systemui/flags/FlagSerializer.kt | 80 +++++ .../com/android/systemui/flags/FlagsModule.kt | 5 + .../android/systemui/flags/FeatureFlags.kt | 6 + .../systemui/flags/FeatureFlagsDebug.java | 137 +++++--- .../systemui/flags/FeatureFlagsRelease.java | 50 ++- .../systemui/flags/FeatureFlagsDebugTest.kt | 320 ++++++++++++++++++ .../flags/FeatureFlagsReleaseTest.java | 111 ------ .../systemui/flags/FeatureFlagsReleaseTest.kt | 127 +++++++ .../android/systemui/flags/FlagManagerTest.kt | 88 +++++ .../util/mockito/KotlinMockitoHelpers.kt | 5 + 11 files changed, 784 insertions(+), 202 deletions(-) create mode 100644 packages/SystemUI/shared/src/com/android/systemui/flags/FlagSerializer.kt create mode 100644 packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt delete mode 100644 packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java create mode 100644 packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt index 42fec7775a421..ec619dd6eea3d 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagManager.kt @@ -26,8 +26,6 @@ import android.os.Bundle import android.os.Handler import androidx.concurrent.futures.CallbackToFutureAdapter import com.google.common.util.concurrent.ListenableFuture -import org.json.JSONException -import org.json.JSONObject import java.util.function.Consumer class FlagManager constructor( @@ -40,11 +38,9 @@ class FlagManager constructor( const val ACTION_SET_FLAG = "com.android.systemui.action.SET_FLAG" const val ACTION_GET_FLAGS = "com.android.systemui.action.GET_FLAGS" const val FLAGS_PERMISSION = "com.android.systemui.permission.FLAGS" - const val FIELD_ID = "id" - const val FIELD_VALUE = "value" - const val FIELD_TYPE = "type" - const val FIELD_FLAGS = "flags" - const val TYPE_BOOLEAN = "boolean" + const val EXTRA_ID = "id" + const val EXTRA_VALUE = "value" + const val EXTRA_FLAGS = "flags" private const val SETTINGS_PREFIX = "systemui/flags" } @@ -74,7 +70,7 @@ class FlagManager constructor( override fun onReceive(context: Context, intent: Intent) { val extras: Bundle? = getResultExtras(false) val listOfFlags: java.util.ArrayList>? = - extras?.getParcelableArrayList(FIELD_FLAGS) + extras?.getParcelableArrayList(EXTRA_FLAGS) if (listOfFlags != null) { completer.set(listOfFlags) } else { @@ -86,9 +82,19 @@ class FlagManager constructor( } as ListenableFuture>> } + /** + * Returns the stored value or null if not set. + * This API is used by TheFlippinApp. + */ + fun isEnabled(id: Int): Boolean? = readFlagValue(id, BooleanFlagSerializer) + + /** + * Sets the value of a boolean flag. + * This API is used by TheFlippinApp. + */ fun setFlagValue(id: Int, enabled: Boolean) { val intent = createIntent(id) - intent.putExtra(FIELD_VALUE, enabled) + intent.putExtra(EXTRA_VALUE, enabled) context.sendBroadcast(intent) } @@ -100,20 +106,9 @@ class FlagManager constructor( } /** Returns the stored value or null if not set. */ - fun isEnabled(id: Int): Boolean? { - val data = settings.getString(keyToSettingsPrefix(id)) - if (data == null || data?.isEmpty()) { - return null - } - val json: JSONObject - try { - json = JSONObject(data) - return if (!assertType(json, TYPE_BOOLEAN)) { - null - } else json.getBoolean(FIELD_VALUE) - } catch (e: JSONException) { - throw InvalidFlagStorageException() - } + fun readFlagValue(id: Int, serializer: FlagSerializer): T? { + val data = settings.getString(idToSettingsKey(id)) + return serializer.fromSettingsData(data) } override fun addListener(flag: Flag<*>, listener: FlagListenable.Listener) { @@ -141,21 +136,13 @@ class FlagManager constructor( private fun createIntent(id: Int): Intent { val intent = Intent(ACTION_SET_FLAG) intent.setPackage(RECEIVING_PACKAGE) - intent.putExtra(FIELD_ID, id) + intent.putExtra(EXTRA_ID, id) return intent } - fun keyToSettingsPrefix(key: Int): String { - return "$SETTINGS_PREFIX/$key" - } - - private fun assertType(json: JSONObject, type: String): Boolean { - return try { - json.getString(FIELD_TYPE) == TYPE_BOOLEAN - } catch (e: JSONException) { - false - } + fun idToSettingsKey(id: Int): String { + return "$SETTINGS_PREFIX/$id" } inner class SettingsObserver : ContentObserver(handler) { @@ -200,7 +187,5 @@ class FlagManager constructor( private data class PerFlagListener(val id: Int, val listener: FlagListenable.Listener) } -class InvalidFlagStorageException : Exception("Data found but is invalid") - class NoFlagResultsException : Exception( "SystemUI failed to communicate its flags back successfully") \ No newline at end of file diff --git a/packages/SystemUI/shared/src/com/android/systemui/flags/FlagSerializer.kt b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagSerializer.kt new file mode 100644 index 0000000000000..e9ea19dad4240 --- /dev/null +++ b/packages/SystemUI/shared/src/com/android/systemui/flags/FlagSerializer.kt @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2021 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 + * + * http://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 com.android.systemui.flags + +import android.util.Log +import org.json.JSONException +import org.json.JSONObject + +private const val FIELD_VALUE = "value" +private const val FIELD_TYPE = "type" +private const val TYPE_BOOLEAN = "boolean" +private const val TYPE_STRING = "string" + +private const val TAG = "FlagSerializer" + +abstract class FlagSerializer( + private val type: String, + private val setter: (JSONObject, String, T) -> Unit, + private val getter: (JSONObject, String) -> T +) { + fun toSettingsData(value: T): String? { + return try { + JSONObject() + .put(FIELD_TYPE, type) + .also { setter(it, FIELD_VALUE, value) } + .toString() + } catch (e: JSONException) { + Log.w(TAG, "write error", e) + null + } + } + + /** + * @throws InvalidFlagStorageException + */ + fun fromSettingsData(data: String?): T? { + if (data == null || data.isEmpty()) { + return null + } + try { + val json = JSONObject(data) + return if (json.getString(FIELD_TYPE) == type) { + getter(json, FIELD_VALUE) + } else { + null + } + } catch (e: JSONException) { + Log.w(TAG, "read error", e) + throw InvalidFlagStorageException() + } + } +} + +object BooleanFlagSerializer : FlagSerializer( + TYPE_BOOLEAN, + JSONObject::put, + JSONObject::getBoolean +) + +object StringFlagSerializer : FlagSerializer( + TYPE_STRING, + JSONObject::put, + JSONObject::getString +) + +class InvalidFlagStorageException : Exception("Data found but is invalid") diff --git a/packages/SystemUI/src-debug/com/android/systemui/flags/FlagsModule.kt b/packages/SystemUI/src-debug/com/android/systemui/flags/FlagsModule.kt index 10ceee90a173a..adfc87241460d 100644 --- a/packages/SystemUI/src-debug/com/android/systemui/flags/FlagsModule.kt +++ b/packages/SystemUI/src-debug/com/android/systemui/flags/FlagsModule.kt @@ -23,6 +23,7 @@ import com.android.systemui.util.settings.SettingsUtilModule import dagger.Binds import dagger.Module import dagger.Provides +import java.util.function.Supplier @Module(includes = [ SettingsUtilModule::class @@ -38,5 +39,9 @@ abstract class FlagsModule { fun provideFlagManager(context: Context, @Main handler: Handler): FlagManager { return FlagManager(context, handler) } + + @JvmStatic + @Provides + fun providesFlagCollector(): Supplier>>? = null } } \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt index d4b23c7aa7d3d..96a90dfc7fe95 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlags.kt @@ -27,4 +27,10 @@ interface FeatureFlags : FlagListenable { /** Returns a boolean value for the given flag. */ fun isEnabled(flag: ResourceBooleanFlag): Boolean + + /** Returns a string value for the given flag. */ + fun getString(flag: StringFlag): String + + /** Returns a string value for the given flag. */ + fun getString(flag: ResourceStringFlag): String } \ No newline at end of file diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java index b5916f19669ab..89623f4566bd8 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java @@ -18,9 +18,11 @@ package com.android.systemui.flags; import static com.android.systemui.flags.FlagManager.ACTION_GET_FLAGS; import static com.android.systemui.flags.FlagManager.ACTION_SET_FLAG; -import static com.android.systemui.flags.FlagManager.FIELD_FLAGS; -import static com.android.systemui.flags.FlagManager.FIELD_ID; -import static com.android.systemui.flags.FlagManager.FIELD_VALUE; +import static com.android.systemui.flags.FlagManager.EXTRA_FLAGS; +import static com.android.systemui.flags.FlagManager.EXTRA_ID; +import static com.android.systemui.flags.FlagManager.EXTRA_VALUE; + +import static java.util.Objects.requireNonNull; import android.content.BroadcastReceiver; import android.content.Context; @@ -39,14 +41,13 @@ import com.android.systemui.dagger.qualifiers.Main; import com.android.systemui.dump.DumpManager; import com.android.systemui.util.settings.SecureSettings; -import org.json.JSONException; -import org.json.JSONObject; - import java.io.FileDescriptor; import java.io.PrintWriter; import java.util.ArrayList; -import java.util.HashMap; import java.util.Map; +import java.util.Objects; +import java.util.TreeMap; +import java.util.function.Supplier; import javax.inject.Inject; @@ -66,7 +67,9 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { private final FlagManager mFlagManager; private final SecureSettings mSecureSettings; private final Resources mResources; - private final Map mBooleanFlagCache = new HashMap<>(); + private final Supplier>> mFlagsCollector; + private final Map mBooleanFlagCache = new TreeMap<>(); + private final Map mStringFlagCache = new TreeMap<>(); @Inject public FeatureFlagsDebug( @@ -74,10 +77,12 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { Context context, SecureSettings secureSettings, @Main Resources resources, - DumpManager dumpManager) { + DumpManager dumpManager, + @Nullable Supplier>> flagsCollector) { mFlagManager = flagManager; mSecureSettings = secureSettings; mResources = resources; + mFlagsCollector = flagsCollector != null ? flagsCollector : Flags::collectFlags; IntentFilter filter = new IntentFilter(); filter.addAction(ACTION_SET_FLAG); filter.addAction(ACTION_GET_FLAGS); @@ -88,58 +93,85 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { } @Override - public boolean isEnabled(BooleanFlag flag) { + public boolean isEnabled(@NonNull BooleanFlag flag) { int id = flag.getId(); if (!mBooleanFlagCache.containsKey(id)) { - mBooleanFlagCache.put(id, isEnabled(id, flag.getDefault())); + mBooleanFlagCache.put(id, + readFlagValue(id, flag.getDefault(), BooleanFlagSerializer.INSTANCE)); } return mBooleanFlagCache.get(id); } @Override - public boolean isEnabled(ResourceBooleanFlag flag) { + public boolean isEnabled(@NonNull ResourceBooleanFlag flag) { int id = flag.getId(); if (!mBooleanFlagCache.containsKey(id)) { - mBooleanFlagCache.put( - id, isEnabled(id, mResources.getBoolean(flag.getResourceId()))); + mBooleanFlagCache.put(id, + readFlagValue(id, mResources.getBoolean(flag.getResourceId()), + BooleanFlagSerializer.INSTANCE)); } return mBooleanFlagCache.get(id); } - /** Return a flag's value. */ - private boolean isEnabled(int id, boolean defaultValue) { - Boolean result = isEnabledInternal(id); + @NonNull + @Override + public String getString(@NonNull StringFlag flag) { + int id = flag.getId(); + if (!mStringFlagCache.containsKey(id)) { + mStringFlagCache.put(id, + readFlagValue(id, flag.getDefault(), StringFlagSerializer.INSTANCE)); + } + + return mStringFlagCache.get(id); + } + + @NonNull + @Override + public String getString(@NonNull ResourceStringFlag flag) { + int id = flag.getId(); + if (!mStringFlagCache.containsKey(id)) { + mStringFlagCache.put(id, + readFlagValue(id, mResources.getString(flag.getResourceId()), + StringFlagSerializer.INSTANCE)); + } + + return mStringFlagCache.get(id); + } + + @NonNull + private T readFlagValue(int id, @NonNull T defaultValue, FlagSerializer serializer) { + requireNonNull(defaultValue, "defaultValue"); + T result = readFlagValueInternal(id, serializer); return result == null ? defaultValue : result; } /** Returns the stored value or null if not set. */ - private Boolean isEnabledInternal(int id) { + @Nullable + private T readFlagValueInternal(int id, FlagSerializer serializer) { try { - return mFlagManager.isEnabled(id); + return mFlagManager.readFlagValue(id, serializer); } catch (Exception e) { eraseInternal(id); } return null; } - /** Set whether a given {@link BooleanFlag} is enabled or not. */ - public void setEnabled(int id, boolean value) { - Boolean currentValue = isEnabledInternal(id); - if (currentValue != null && currentValue == value) { + private void setFlagValue(int id, @NonNull T value, FlagSerializer serializer) { + requireNonNull(value, "Cannot set a null value"); + T currentValue = readFlagValueInternal(id, serializer); + if (Objects.equals(currentValue, value)) { + Log.i(TAG, "Flag id " + id + " is already " + value); return; } - - JSONObject json = new JSONObject(); - try { - json.put(FlagManager.FIELD_TYPE, FlagManager.TYPE_BOOLEAN); - json.put(FIELD_VALUE, value); - mSecureSettings.putString(mFlagManager.keyToSettingsPrefix(id), json.toString()); - } catch (JSONException e) { - return; // ignore + final String data = serializer.toSettingsData(value); + if (data == null) { + Log.w(TAG, "Failed to set id " + id + " to " + value); + return; } + mSecureSettings.putString(mFlagManager.idToSettingsKey(id), data); Log.i(TAG, "Set id " + id + " to " + value); removeFromCache(id); mFlagManager.dispatchListenersAndMaybeRestart(id); @@ -155,7 +187,7 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { /** Works just like {@link #eraseFlag(int)} except that it doesn't restart SystemUI. */ private void eraseInternal(int id) { // We can't actually "erase" things from sysprops, but we can set them to empty! - mSecureSettings.putString(mFlagManager.keyToSettingsPrefix(id), ""); + mSecureSettings.putString(mFlagManager.idToSettingsKey(id), ""); Log.i(TAG, "Erase id " + id); } @@ -182,14 +214,14 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { private final BroadcastReceiver mReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - String action = intent.getAction(); + String action = intent == null ? null : intent.getAction(); if (action == null) { return; } if (ACTION_SET_FLAG.equals(action)) { handleSetFlag(intent.getExtras()); } else if (ACTION_GET_FLAGS.equals(action)) { - Map> knownFlagMap = Flags.collectFlags(); + Map> knownFlagMap = mFlagsCollector.get(); ArrayList> flags = new ArrayList<>(knownFlagMap.values()); // Convert all flags to parcelable flags. @@ -203,7 +235,7 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { Bundle extras = getResultExtras(true); if (extras != null) { - extras.putParcelableArrayList(FIELD_FLAGS, pFlags); + extras.putParcelableArrayList(EXTRA_FLAGS, pFlags); } } } @@ -213,26 +245,37 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { Log.w(TAG, "No extras"); return; } - int id = extras.getInt(FIELD_ID); + int id = extras.getInt(EXTRA_ID); if (id <= 0) { Log.w(TAG, "ID not set or less than or equal to 0: " + id); return; } - Map> flagMap = Flags.collectFlags(); + Map> flagMap = mFlagsCollector.get(); if (!flagMap.containsKey(id)) { Log.w(TAG, "Tried to set unknown id: " + id); return; } Flag flag = flagMap.get(id); - if (!extras.containsKey(FIELD_VALUE)) { + if (!extras.containsKey(EXTRA_VALUE)) { eraseFlag(id); return; } - if (flag instanceof BooleanFlag) { - setEnabled(id, extras.getBoolean(FIELD_VALUE)); + Object value = extras.get(EXTRA_VALUE); + if (flag instanceof BooleanFlag && value instanceof Boolean) { + setFlagValue(id, (Boolean) value, BooleanFlagSerializer.INSTANCE); + } else if (flag instanceof ResourceBooleanFlag && value instanceof Boolean) { + setFlagValue(id, (Boolean) value, BooleanFlagSerializer.INSTANCE); + } else if (flag instanceof StringFlag && value instanceof String) { + setFlagValue(id, (String) value, StringFlagSerializer.INSTANCE); + } else if (flag instanceof ResourceStringFlag && value instanceof String) { + setFlagValue(id, (String) value, StringFlagSerializer.INSTANCE); + } else { + Log.w(TAG, + "Unable to set " + id + " of type " + flag.getClass() + " to value of type " + + (value == null ? null : value.getClass())); } } @@ -258,18 +301,16 @@ public class FeatureFlagsDebug implements FeatureFlags, Dumpable { private void removeFromCache(int id) { mBooleanFlagCache.remove(id); + mStringFlagCache.remove(id); } @Override public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) { pw.println("can override: true"); - ArrayList flagStrings = new ArrayList<>(mBooleanFlagCache.size()); - for (Map.Entry entry : mBooleanFlagCache.entrySet()) { - flagStrings.add(" sysui_flag_" + entry.getKey() + ": " + entry.getValue()); - } - flagStrings.sort(String.CASE_INSENSITIVE_ORDER); - for (String flagString : flagStrings) { - pw.println(flagString); - } + pw.println("booleans: " + mBooleanFlagCache.size()); + mBooleanFlagCache.forEach((key, value) -> pw.println(" sysui_flag_" + key + ": " + value)); + pw.println("Strings: " + mStringFlagCache.size()); + mStringFlagCache.forEach((key, value) -> pw.println(" sysui_flag_" + key + + ": [length=" + value.length() + "] \"" + value + "\"")); } } diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java index d82b89b6dfd4e..348a8e20122ea 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java @@ -16,7 +16,10 @@ package com.android.systemui.flags; +import static java.util.Objects.requireNonNull; + import android.content.res.Resources; +import android.util.SparseArray; import android.util.SparseBooleanArray; import androidx.annotation.NonNull; @@ -40,7 +43,8 @@ import javax.inject.Inject; @SysUISingleton public class FeatureFlagsRelease implements FeatureFlags, Dumpable { private final Resources mResources; - SparseBooleanArray mFlagCache = new SparseBooleanArray(); + SparseBooleanArray mBooleanCache = new SparseBooleanArray(); + SparseArray mStringCache = new SparseArray<>(); @Inject public FeatureFlagsRelease(@Main Resources resources, DumpManager dumpManager) { mResources = resources; @@ -60,25 +64,57 @@ public class FeatureFlagsRelease implements FeatureFlags, Dumpable { @Override public boolean isEnabled(ResourceBooleanFlag flag) { - int cacheIndex = mFlagCache.indexOfKey(flag.getId()); + int cacheIndex = mBooleanCache.indexOfKey(flag.getId()); if (cacheIndex < 0) { return isEnabled(flag.getId(), mResources.getBoolean(flag.getResourceId())); } - return mFlagCache.valueAt(cacheIndex); + return mBooleanCache.valueAt(cacheIndex); } private boolean isEnabled(int key, boolean defaultValue) { - mFlagCache.append(key, defaultValue); + mBooleanCache.append(key, defaultValue); + return defaultValue; + } + + @NonNull + @Override + public String getString(@NonNull StringFlag flag) { + return getString(flag.getId(), flag.getDefault()); + } + + @NonNull + @Override + public String getString(@NonNull ResourceStringFlag flag) { + int cacheIndex = mStringCache.indexOfKey(flag.getId()); + if (cacheIndex < 0) { + return getString(flag.getId(), + requireNonNull(mResources.getString(flag.getResourceId()))); + } + + return mStringCache.valueAt(cacheIndex); + } + + private String getString(int key, String defaultValue) { + mStringCache.append(key, defaultValue); return defaultValue; } @Override public void dump(@NonNull FileDescriptor fd, @NonNull PrintWriter pw, @NonNull String[] args) { pw.println("can override: false"); - int size = mFlagCache.size(); - for (int i = 0; i < size; i++) { - pw.println(" sysui_flag_" + mFlagCache.keyAt(i) + ": " + mFlagCache.valueAt(i)); + int numBooleans = mBooleanCache.size(); + pw.println("booleans: " + numBooleans); + for (int i = 0; i < numBooleans; i++) { + pw.println(" sysui_flag_" + mBooleanCache.keyAt(i) + ": " + mBooleanCache.valueAt(i)); + } + int numStrings = mStringCache.size(); + pw.println("Strings: " + numStrings); + for (int i = 0; i < numStrings; i++) { + final int id = mStringCache.keyAt(i); + final String value = mStringCache.valueAt(i); + final int length = value.length(); + pw.println(" sysui_flag_" + id + ": [length=" + length + "] \"" + value + "\""); } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt new file mode 100644 index 0000000000000..cb16becc48fdc --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt @@ -0,0 +1,320 @@ +/* + * Copyright (C) 2021 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 + * + * http://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 com.android.systemui.flags + +import android.content.BroadcastReceiver +import android.content.Context +import android.content.Intent +import android.content.pm.PackageManager +import android.content.pm.PackageManager.NameNotFoundException +import android.content.res.Resources +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.dump.DumpManager +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.eq +import com.android.systemui.util.mockito.mock +import com.android.systemui.util.mockito.nullable +import com.android.systemui.util.mockito.withArgCaptor +import com.android.systemui.util.settings.SecureSettings +import com.google.common.truth.Truth.assertThat +import org.junit.Assert +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.Mockito.inOrder +import org.mockito.Mockito.times +import org.mockito.Mockito.verify +import org.mockito.Mockito.verifyNoMoreInteractions +import org.mockito.MockitoAnnotations +import java.io.PrintWriter +import java.io.Serializable +import java.io.StringWriter +import java.util.function.Consumer +import org.mockito.Mockito.`when` as whenever + +/** + * NOTE: This test is for the version of FeatureFlagManager in src-release, which should not allow + * overriding, and should never return any value other than the one provided as the default. + */ +@SmallTest +class FeatureFlagsDebugTest : SysuiTestCase() { + private lateinit var mFeatureFlagsDebug: FeatureFlagsDebug + + @Mock private lateinit var mFlagManager: FlagManager + @Mock private lateinit var mMockContext: Context + @Mock private lateinit var mSecureSettings: SecureSettings + @Mock private lateinit var mResources: Resources + @Mock private lateinit var mDumpManager: DumpManager + private val mFlagMap = mutableMapOf>() + private lateinit var mBroadcastReceiver: BroadcastReceiver + private lateinit var mClearCacheAction: Consumer + + @Before + fun setup() { + MockitoAnnotations.initMocks(this) + mFeatureFlagsDebug = FeatureFlagsDebug( + mFlagManager, + mMockContext, + mSecureSettings, + mResources, + mDumpManager, + { mFlagMap } + ) + verify(mFlagManager).restartAction = any() + mBroadcastReceiver = withArgCaptor { + verify(mMockContext).registerReceiver(capture(), any(), nullable(), nullable()) + } + mClearCacheAction = withArgCaptor { + verify(mFlagManager).clearCacheAction = capture() + } + whenever(mFlagManager.idToSettingsKey(any())).thenAnswer { "key-${it.arguments[0]}" } + } + + @Test + fun testReadBooleanFlag() { + whenever(mFlagManager.readFlagValue(eq(3), any())).thenReturn(true) + whenever(mFlagManager.readFlagValue(eq(4), any())).thenReturn(false) + assertThat(mFeatureFlagsDebug.isEnabled(BooleanFlag(1, false))).isFalse() + assertThat(mFeatureFlagsDebug.isEnabled(BooleanFlag(2, true))).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(BooleanFlag(3, false))).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(BooleanFlag(4, true))).isFalse() + } + + @Test + fun testReadResourceBooleanFlag() { + whenever(mResources.getBoolean(1001)).thenReturn(false) + whenever(mResources.getBoolean(1002)).thenReturn(true) + whenever(mResources.getBoolean(1003)).thenReturn(false) + whenever(mResources.getBoolean(1004)).thenAnswer { throw NameNotFoundException() } + whenever(mResources.getBoolean(1005)).thenAnswer { throw NameNotFoundException() } + + whenever(mFlagManager.readFlagValue(eq(3), any())).thenReturn(true) + whenever(mFlagManager.readFlagValue(eq(5), any())).thenReturn(false) + + assertThat(mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(1, 1001))).isFalse() + assertThat(mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(2, 1002))).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(3, 1003))).isTrue() + + Assert.assertThrows(NameNotFoundException::class.java) { + mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(4, 1004)) + } + // Test that resource is loaded (and validated) even when the setting is set. + // This prevents developers from not noticing when they reference an invalid resource. + Assert.assertThrows(NameNotFoundException::class.java) { + mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(5, 1005)) + } + } + + @Test + fun testReadStringFlag() { + whenever(mFlagManager.readFlagValue(eq(3), any())).thenReturn("foo") + whenever(mFlagManager.readFlagValue(eq(4), any())).thenReturn("bar") + assertThat(mFeatureFlagsDebug.getString(StringFlag(1, "biz"))).isEqualTo("biz") + assertThat(mFeatureFlagsDebug.getString(StringFlag(2, "baz"))).isEqualTo("baz") + assertThat(mFeatureFlagsDebug.getString(StringFlag(3, "buz"))).isEqualTo("foo") + assertThat(mFeatureFlagsDebug.getString(StringFlag(4, "buz"))).isEqualTo("bar") + } + + @Test + fun testReadResourceStringFlag() { + whenever(mResources.getString(1001)).thenReturn("") + whenever(mResources.getString(1002)).thenReturn("resource2") + whenever(mResources.getString(1003)).thenReturn("resource3") + whenever(mResources.getString(1004)).thenReturn(null) + whenever(mResources.getString(1005)).thenAnswer { throw NameNotFoundException() } + whenever(mResources.getString(1006)).thenAnswer { throw NameNotFoundException() } + + whenever(mFlagManager.readFlagValue(eq(3), any())).thenReturn("override3") + whenever(mFlagManager.readFlagValue(eq(4), any())).thenReturn("override4") + whenever(mFlagManager.readFlagValue(eq(6), any())).thenReturn("override6") + + assertThat(mFeatureFlagsDebug.getString(ResourceStringFlag(1, 1001))).isEqualTo("") + assertThat(mFeatureFlagsDebug.getString(ResourceStringFlag(2, 1002))).isEqualTo("resource2") + assertThat(mFeatureFlagsDebug.getString(ResourceStringFlag(3, 1003))).isEqualTo("override3") + + Assert.assertThrows(NullPointerException::class.java) { + mFeatureFlagsDebug.getString(ResourceStringFlag(4, 1004)) + } + Assert.assertThrows(NameNotFoundException::class.java) { + mFeatureFlagsDebug.getString(ResourceStringFlag(5, 1005)) + } + // Test that resource is loaded (and validated) even when the setting is set. + // This prevents developers from not noticing when they reference an invalid resource. + Assert.assertThrows(NameNotFoundException::class.java) { + mFeatureFlagsDebug.getString(ResourceStringFlag(6, 1005)) + } + } + + @Test + fun testBroadcastReceiverIgnoresInvalidData() { + addFlag(BooleanFlag(1, false)) + addFlag(ResourceBooleanFlag(2, 1002)) + addFlag(StringFlag(3, "flag3")) + addFlag(ResourceStringFlag(4, 1004)) + + mBroadcastReceiver.onReceive(mMockContext, null) + mBroadcastReceiver.onReceive(mMockContext, Intent()) + mBroadcastReceiver.onReceive(mMockContext, Intent("invalid action")) + mBroadcastReceiver.onReceive(mMockContext, Intent(FlagManager.ACTION_SET_FLAG)) + setByBroadcast(0, false) // unknown id does nothing + setByBroadcast(1, "string") // wrong type does nothing + setByBroadcast(2, 123) // wrong type does nothing + setByBroadcast(3, false) // wrong type does nothing + setByBroadcast(4, 123) // wrong type does nothing + verifyNoMoreInteractions(mFlagManager, mSecureSettings) + } + + @Test + fun testIntentWithIdButNoValueKeyClears() { + addFlag(BooleanFlag(1, false)) + + // trying to erase an id not in the map does noting + mBroadcastReceiver.onReceive( + mMockContext, + Intent(FlagManager.ACTION_SET_FLAG).putExtra(FlagManager.EXTRA_ID, 0) + ) + verifyNoMoreInteractions(mFlagManager, mSecureSettings) + + // valid id with no value puts empty string in the setting + mBroadcastReceiver.onReceive( + mMockContext, + Intent(FlagManager.ACTION_SET_FLAG).putExtra(FlagManager.EXTRA_ID, 1) + ) + verifyPutData(1, "", numReads = 0) + } + + @Test + fun testSetBooleanFlag() { + addFlag(BooleanFlag(1, false)) + addFlag(BooleanFlag(2, false)) + addFlag(ResourceBooleanFlag(3, 1003)) + addFlag(ResourceBooleanFlag(4, 1004)) + + setByBroadcast(1, false) + verifyPutData(1, "{\"type\":\"boolean\",\"value\":false}") + + setByBroadcast(2, true) + verifyPutData(2, "{\"type\":\"boolean\",\"value\":true}") + + setByBroadcast(3, false) + verifyPutData(3, "{\"type\":\"boolean\",\"value\":false}") + + setByBroadcast(4, true) + verifyPutData(4, "{\"type\":\"boolean\",\"value\":true}") + } + + @Test + fun testSetStringFlag() { + addFlag(StringFlag(1, "flag1")) + addFlag(ResourceStringFlag(2, 1002)) + + setByBroadcast(1, "override1") + verifyPutData(1, "{\"type\":\"string\",\"value\":\"override1\"}") + + setByBroadcast(2, "override2") + verifyPutData(2, "{\"type\":\"string\",\"value\":\"override2\"}") + } + + @Test + fun testSetFlagClearsCache() { + val flag1 = addFlag(StringFlag(1, "flag1")) + whenever(mFlagManager.readFlagValue(eq(1), any())).thenReturn("original") + + // gets the flag & cache it + assertThat(mFeatureFlagsDebug.getString(flag1)).isEqualTo("original") + verify(mFlagManager).readFlagValue(eq(1), eq(StringFlagSerializer)) + + // hit the cache + assertThat(mFeatureFlagsDebug.getString(flag1)).isEqualTo("original") + verifyNoMoreInteractions(mFlagManager) + + // set the flag + setByBroadcast(1, "new") + verifyPutData(1, "{\"type\":\"string\",\"value\":\"new\"}", numReads = 2) + whenever(mFlagManager.readFlagValue(eq(1), any())).thenReturn("new") + + assertThat(mFeatureFlagsDebug.getString(flag1)).isEqualTo("new") + verify(mFlagManager, times(3)).readFlagValue(eq(1), eq(StringFlagSerializer)) + } + + private fun verifyPutData(id: Int, data: String, numReads: Int = 1) { + inOrder(mFlagManager, mSecureSettings).apply { + verify(mFlagManager, times(numReads)).readFlagValue(eq(id), any>()) + verify(mFlagManager).idToSettingsKey(eq(id)) + verify(mSecureSettings).putString(eq("key-$id"), eq(data)) + verify(mFlagManager).dispatchListenersAndMaybeRestart(eq(id)) + }.verifyNoMoreInteractions() + verifyNoMoreInteractions(mFlagManager, mSecureSettings) + } + + private fun setByBroadcast(id: Int, value: Serializable?) { + val intent = Intent(FlagManager.ACTION_SET_FLAG) + intent.putExtra(FlagManager.EXTRA_ID, id) + intent.putExtra(FlagManager.EXTRA_VALUE, value) + mBroadcastReceiver.onReceive(mMockContext, intent) + } + + private fun > addFlag(flag: F): F { + val old = mFlagMap.put(flag.id, flag) + check(old == null) { "Flag ${flag.id} already registered" } + return flag + } + + @Test + fun testDump() { + val flag1 = BooleanFlag(1, true) + val flag2 = ResourceBooleanFlag(2, 1002) + val flag3 = BooleanFlag(3, false) + val flag4 = StringFlag(4, "") + val flag5 = StringFlag(5, "flag5default") + val flag6 = ResourceStringFlag(6, 1006) + val flag7 = ResourceStringFlag(7, 1007) + + whenever(mResources.getBoolean(1002)).thenReturn(true) + whenever(mResources.getString(1006)).thenReturn("resource1006") + whenever(mResources.getString(1007)).thenReturn("resource1007") + whenever(mFlagManager.readFlagValue(eq(7), eq(StringFlagSerializer))) + .thenReturn("override7") + + // WHEN the flags have been accessed + assertThat(mFeatureFlagsDebug.isEnabled(flag1)).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(flag2)).isTrue() + assertThat(mFeatureFlagsDebug.isEnabled(flag3)).isFalse() + assertThat(mFeatureFlagsDebug.getString(flag4)).isEmpty() + assertThat(mFeatureFlagsDebug.getString(flag5)).isEqualTo("flag5default") + assertThat(mFeatureFlagsDebug.getString(flag6)).isEqualTo("resource1006") + assertThat(mFeatureFlagsDebug.getString(flag7)).isEqualTo("override7") + + // THEN the dump contains the flags and the default values + val dump = dumpToString() + assertThat(dump).contains(" sysui_flag_1: true\n") + assertThat(dump).contains(" sysui_flag_2: true\n") + assertThat(dump).contains(" sysui_flag_3: false\n") + assertThat(dump).contains(" sysui_flag_4: [length=0] \"\"\n") + assertThat(dump).contains(" sysui_flag_5: [length=12] \"flag5default\"\n") + assertThat(dump).contains(" sysui_flag_6: [length=12] \"resource1006\"\n") + assertThat(dump).contains(" sysui_flag_7: [length=9] \"override7\"\n") + } + + private fun dumpToString(): String { + val sw = StringWriter() + val pw = PrintWriter(sw) + mFeatureFlagsDebug.dump(mock(), pw, emptyArray()) + pw.flush() + return sw.toString() + } +} \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java deleted file mode 100644 index b0fdcf4336bdb..0000000000000 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.java +++ /dev/null @@ -1,111 +0,0 @@ -/* - * Copyright (C) 2021 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 - * - * http://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 com.android.systemui.flags; - -import static com.google.common.truth.Truth.assertThat; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; -import static org.mockito.Mockito.when; - -import android.content.res.Resources; - -import androidx.test.filters.SmallTest; - -import com.android.systemui.SysuiTestCase; -import com.android.systemui.dump.DumpManager; - -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.mockito.Mock; -import org.mockito.MockitoAnnotations; - -import java.io.FileDescriptor; -import java.io.PrintWriter; -import java.io.StringWriter; - -/** - * NOTE: This test is for the version of FeatureFlagManager in src-release, which should not allow - * overriding, and should never return any value other than the one provided as the default. - */ -@SmallTest -public class FeatureFlagsReleaseTest extends SysuiTestCase { - FeatureFlagsRelease mFeatureFlagsRelease; - - @Mock private Resources mResources; - @Mock private DumpManager mDumpManager; - - @Before - public void setup() { - MockitoAnnotations.initMocks(this); - - mFeatureFlagsRelease = new FeatureFlagsRelease(mResources, mDumpManager); - } - - @After - public void onFinished() { - // The dump manager should be registered with even for the release version, but that's it. - verify(mDumpManager).registerDumpable(anyString(), any()); - verifyNoMoreInteractions(mDumpManager); - } - - @Test - public void testBooleanResourceFlag() { - int flagId = 213; - int flagResourceId = 3; - ResourceBooleanFlag flag = new ResourceBooleanFlag(flagId, flagResourceId); - when(mResources.getBoolean(flagResourceId)).thenReturn(true); - - assertThat(mFeatureFlagsRelease.isEnabled(flag)).isTrue(); - } - - @Test - public void testDump() { - int flagIdA = 213; - int flagIdB = 18; - int flagIdC = 1; - int flagResourceId = 3; - BooleanFlag flagA = new BooleanFlag(flagIdA, true); - ResourceBooleanFlag flagB = new ResourceBooleanFlag(flagIdB, flagResourceId); - BooleanFlag flagC = new BooleanFlag(flagIdC, false); - when(mResources.getBoolean(flagResourceId)).thenReturn(true); - - // WHEN the flags have been accessed - assertThat(mFeatureFlagsRelease.isEnabled(flagA)).isTrue(); - assertThat(mFeatureFlagsRelease.isEnabled(flagB)).isTrue(); - assertThat(mFeatureFlagsRelease.isEnabled(flagC)).isFalse(); - - // THEN the dump contains the flags and the default values - String dump = dumpToString(); - assertThat(dump).contains(" sysui_flag_" + flagIdA + ": true\n"); - assertThat(dump).contains(" sysui_flag_" + flagIdB + ": true\n"); - assertThat(dump).contains(" sysui_flag_" + flagIdC + ": false\n"); - } - - private String dumpToString() { - StringWriter sw = new StringWriter(); - PrintWriter pw = new PrintWriter(sw); - mFeatureFlagsRelease.dump(mock(FileDescriptor.class), pw, new String[0]); - pw.flush(); - String dump = sw.toString(); - return dump; - } -} diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt new file mode 100644 index 0000000000000..b5e6602594eb4 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt @@ -0,0 +1,127 @@ +/* + * Copyright (C) 2021 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 + * + * http://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 com.android.systemui.flags + +import android.content.pm.PackageManager.NameNotFoundException +import android.content.res.Resources +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.dump.DumpManager +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.mock +import com.google.common.truth.Truth.assertThat +import org.junit.After +import org.junit.Assert.assertThrows +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.Mockito.verify +import org.mockito.Mockito.verifyNoMoreInteractions +import org.mockito.MockitoAnnotations +import java.io.PrintWriter +import java.io.StringWriter +import org.mockito.Mockito.`when` as whenever + +/** + * NOTE: This test is for the version of FeatureFlagManager in src-release, which should not allow + * overriding, and should never return any value other than the one provided as the default. + */ +@SmallTest +class FeatureFlagsReleaseTest : SysuiTestCase() { + private lateinit var mFeatureFlagsRelease: FeatureFlagsRelease + + @Mock private lateinit var mResources: Resources + @Mock private lateinit var mDumpManager: DumpManager + + @Before + fun setup() { + MockitoAnnotations.initMocks(this) + mFeatureFlagsRelease = FeatureFlagsRelease(mResources, mDumpManager) + } + + @After + fun onFinished() { + // The dump manager should be registered with even for the release version, but that's it. + verify(mDumpManager).registerDumpable(any(), any()) + verifyNoMoreInteractions(mDumpManager) + } + + @Test + fun testBooleanResourceFlag() { + val flagId = 213 + val flagResourceId = 3 + val flag = ResourceBooleanFlag(flagId, flagResourceId) + whenever(mResources.getBoolean(flagResourceId)).thenReturn(true) + assertThat(mFeatureFlagsRelease.isEnabled(flag)).isTrue() + } + + @Test + fun testReadResourceStringFlag() { + whenever(mResources.getString(1001)).thenReturn("") + whenever(mResources.getString(1002)).thenReturn("res2") + whenever(mResources.getString(1003)).thenReturn(null) + whenever(mResources.getString(1004)).thenAnswer { throw NameNotFoundException() } + + assertThat(mFeatureFlagsRelease.getString(ResourceStringFlag(1, 1001))).isEqualTo("") + assertThat(mFeatureFlagsRelease.getString(ResourceStringFlag(2, 1002))).isEqualTo("res2") + + assertThrows(NullPointerException::class.java) { + mFeatureFlagsRelease.getString(ResourceStringFlag(3, 1003)) + } + assertThrows(NameNotFoundException::class.java) { + mFeatureFlagsRelease.getString(ResourceStringFlag(4, 1004)) + } + } + + @Test + fun testDump() { + val flag1 = BooleanFlag(1, true) + val flag2 = ResourceBooleanFlag(2, 1002) + val flag3 = BooleanFlag(3, false) + val flag4 = StringFlag(4, "") + val flag5 = StringFlag(5, "flag5default") + val flag6 = ResourceStringFlag(6, 1006) + + whenever(mResources.getBoolean(1002)).thenReturn(true) + whenever(mResources.getString(1006)).thenReturn("resource1006") + whenever(mResources.getString(1007)).thenReturn("resource1007") + + // WHEN the flags have been accessed + assertThat(mFeatureFlagsRelease.isEnabled(flag1)).isTrue() + assertThat(mFeatureFlagsRelease.isEnabled(flag2)).isTrue() + assertThat(mFeatureFlagsRelease.isEnabled(flag3)).isFalse() + assertThat(mFeatureFlagsRelease.getString(flag4)).isEmpty() + assertThat(mFeatureFlagsRelease.getString(flag5)).isEqualTo("flag5default") + assertThat(mFeatureFlagsRelease.getString(flag6)).isEqualTo("resource1006") + + // THEN the dump contains the flags and the default values + val dump = dumpToString() + assertThat(dump).contains(" sysui_flag_1: true\n") + assertThat(dump).contains(" sysui_flag_2: true\n") + assertThat(dump).contains(" sysui_flag_3: false\n") + assertThat(dump).contains(" sysui_flag_4: [length=0] \"\"\n") + assertThat(dump).contains(" sysui_flag_5: [length=12] \"flag5default\"\n") + assertThat(dump).contains(" sysui_flag_6: [length=12] \"resource1006\"\n") + } + + private fun dumpToString(): String { + val sw = StringWriter() + val pw = PrintWriter(sw) + mFeatureFlagsRelease.dump(mock(), pw, emptyArray()) + pw.flush() + return sw.toString() + } +} \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt index aba656f7aae73..644bd21476116 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FlagManagerTest.kt @@ -26,6 +26,7 @@ import com.android.systemui.util.mockito.eq import com.android.systemui.util.mockito.mock import com.android.systemui.util.mockito.withArgCaptor import com.google.common.truth.Truth.assertThat +import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test import org.mockito.Mock @@ -34,6 +35,7 @@ import org.mockito.Mockito.verify import org.mockito.Mockito.verifyNoMoreInteractions import org.mockito.MockitoAnnotations import java.util.function.Consumer +import org.mockito.Mockito.`when` as whenever /** * NOTE: This test is for the version of FeatureFlagManager in src-release, which should not allow @@ -211,4 +213,90 @@ class FlagManagerTest : SysuiTestCase() { verify(restartAction).accept(eq(false)) verifyNoMoreInteractions(restartAction) } + + @Test + fun testReadBooleanFlag() { + // test that null string returns null + whenever(mFlagSettingsHelper.getString(any())).thenReturn(null) + assertThat(mFlagManager.readFlagValue(1, BooleanFlagSerializer)).isNull() + + // test that empty string returns null + whenever(mFlagSettingsHelper.getString(any())).thenReturn("") + assertThat(mFlagManager.readFlagValue(1, BooleanFlagSerializer)).isNull() + + // test false + whenever(mFlagSettingsHelper.getString(any())) + .thenReturn("{\"type\":\"boolean\",\"value\":false}") + assertThat(mFlagManager.readFlagValue(1, BooleanFlagSerializer)).isFalse() + + // test true + whenever(mFlagSettingsHelper.getString(any())) + .thenReturn("{\"type\":\"boolean\",\"value\":true}") + assertThat(mFlagManager.readFlagValue(1, BooleanFlagSerializer)).isTrue() + + // Reading a value of a different type should just return null + whenever(mFlagSettingsHelper.getString(any())) + .thenReturn("{\"type\":\"string\",\"value\":\"foo\"}") + assertThat(mFlagManager.readFlagValue(1, BooleanFlagSerializer)).isNull() + + // Reading a value that isn't json should throw an exception + assertThrows(InvalidFlagStorageException::class.java) { + whenever(mFlagSettingsHelper.getString(any())).thenReturn("1") + mFlagManager.readFlagValue(1, BooleanFlagSerializer) + } + } + + @Test + fun testSerializeBooleanFlag() { + // test false + assertThat(BooleanFlagSerializer.toSettingsData(false)) + .isEqualTo("{\"type\":\"boolean\",\"value\":false}") + + // test true + assertThat(BooleanFlagSerializer.toSettingsData(true)) + .isEqualTo("{\"type\":\"boolean\",\"value\":true}") + } + + @Test + fun testReadStringFlag() { + // test that null string returns null + whenever(mFlagSettingsHelper.getString(any())).thenReturn(null) + assertThat(mFlagManager.readFlagValue(1, StringFlagSerializer)).isNull() + + // test that empty string returns null + whenever(mFlagSettingsHelper.getString(any())).thenReturn("") + assertThat(mFlagManager.readFlagValue(1, StringFlagSerializer)).isNull() + + // test json with the empty string value returns empty string + whenever(mFlagSettingsHelper.getString(any())) + .thenReturn("{\"type\":\"string\",\"value\":\"\"}") + assertThat(mFlagManager.readFlagValue(1, StringFlagSerializer)).isEqualTo("") + + // test string with value is returned + whenever(mFlagSettingsHelper.getString(any())) + .thenReturn("{\"type\":\"string\",\"value\":\"foo\"}") + assertThat(mFlagManager.readFlagValue(1, StringFlagSerializer)).isEqualTo("foo") + + // Reading a value of a different type should just return null + whenever(mFlagSettingsHelper.getString(any())) + .thenReturn("{\"type\":\"boolean\",\"value\":false}") + assertThat(mFlagManager.readFlagValue(1, StringFlagSerializer)).isNull() + + // Reading a value that isn't json should throw an exception + assertThrows(InvalidFlagStorageException::class.java) { + whenever(mFlagSettingsHelper.getString(any())).thenReturn("1") + mFlagManager.readFlagValue(1, StringFlagSerializer) + } + } + + @Test + fun testSerializeStringFlag() { + // test empty string + assertThat(StringFlagSerializer.toSettingsData("")) + .isEqualTo("{\"type\":\"string\",\"value\":\"\"}") + + // test string "foo" + assertThat(StringFlagSerializer.toSettingsData("foo")) + .isEqualTo("{\"type\":\"string\",\"value\":\"foo\"}") + } } \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/util/mockito/KotlinMockitoHelpers.kt b/packages/SystemUI/tests/src/com/android/systemui/util/mockito/KotlinMockitoHelpers.kt index eb54fe0ca1919..0f1b65cb7f04a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/util/mockito/KotlinMockitoHelpers.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/util/mockito/KotlinMockitoHelpers.kt @@ -43,6 +43,11 @@ fun eq(obj: T): T = Mockito.eq(obj) fun any(type: Class): T = Mockito.any(type) inline fun any(): T = any(T::class.java) +/** + * Kotlin type-inferred version of Mockito.nullable() + */ +inline fun nullable(): T? = Mockito.nullable(T::class.java) + /** * Returns ArgumentCaptor.capture() as nullable type to avoid java.lang.IllegalStateException * when null is returned.