diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java index 5c310c317b99f..2c11d78f534b9 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsDebug.java @@ -86,9 +86,34 @@ public class FeatureFlagsDebug implements FeatureFlags { private final ServerFlagReader.ChangeListener mOnPropertiesChanged = new ServerFlagReader.ChangeListener() { @Override - public void onChange(Flag flag) { - mRestarter.restartSystemUI( - "Server flag change: " + flag.getNamespace() + "." + flag.getName()); + public void onChange(Flag flag, String value) { + boolean shouldRestart = false; + if (mBooleanFlagCache.containsKey(flag.getName())) { + boolean newValue = value == null ? false : Boolean.parseBoolean(value); + if (mBooleanFlagCache.get(flag.getName()) != newValue) { + shouldRestart = true; + } + } else if (mStringFlagCache.containsKey(flag.getName())) { + String newValue = value == null ? "" : value; + if (mStringFlagCache.get(flag.getName()) != value) { + shouldRestart = true; + } + } else if (mIntFlagCache.containsKey(flag.getName())) { + int newValue = 0; + try { + newValue = value == null ? 0 : Integer.parseInt(value); + } catch (NumberFormatException e) { + } + if (mIntFlagCache.get(flag.getName()) != newValue) { + shouldRestart = true; + } + } + if (shouldRestart) { + mRestarter.restartSystemUI( + "Server flag change: " + flag.getNamespace() + "." + + flag.getName()); + + } } }; diff --git a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java index 9859ff6b4917b..9d19a7dc46042 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java +++ b/packages/SystemUI/src/com/android/systemui/flags/FeatureFlagsRelease.java @@ -53,13 +53,38 @@ public class FeatureFlagsRelease implements FeatureFlags { private final Map> mAllFlags; private final Map mBooleanCache = new HashMap<>(); private final Map mStringCache = new HashMap<>(); + private final Map mIntCache = new HashMap<>(); private final ServerFlagReader.ChangeListener mOnPropertiesChanged = new ServerFlagReader.ChangeListener() { @Override - public void onChange(Flag flag) { - mRestarter.restartSystemUI( - "Server flag change: " + flag.getNamespace() + "." + flag.getName()); + public void onChange(Flag flag, String value) { + boolean shouldRestart = false; + if (mBooleanCache.containsKey(flag.getName())) { + boolean newValue = value == null ? false : Boolean.parseBoolean(value); + if (mBooleanCache.get(flag.getName()) != newValue) { + shouldRestart = true; + } + } else if (mStringCache.containsKey(flag.getName())) { + String newValue = value == null ? "" : value; + if (mStringCache.get(flag.getName()) != newValue) { + shouldRestart = true; + } + } else if (mIntCache.containsKey(flag.getName())) { + int newValue = 0; + try { + newValue = value == null ? 0 : Integer.parseInt(value); + } catch (NumberFormatException e) { + } + if (mIntCache.get(flag.getName()) != newValue) { + shouldRestart = true; + } + } + if (shouldRestart) { + mRestarter.restartSystemUI( + "Server flag change: " + flag.getNamespace() + "." + + flag.getName()); + } } }; @@ -97,68 +122,97 @@ public class FeatureFlagsRelease implements FeatureFlags { @Override public boolean isEnabled(@NotNull ReleasedFlag flag) { - return mServerFlagReader.readServerOverride(flag.getNamespace(), flag.getName(), true); + // Fill the cache. + return isEnabledInternal(flag.getName(), + mServerFlagReader.readServerOverride(flag.getNamespace(), flag.getName(), true)); } @Override public boolean isEnabled(ResourceBooleanFlag flag) { - if (!mBooleanCache.containsKey(flag.getName())) { - return isEnabled(flag.getName(), mResources.getBoolean(flag.getResourceId())); - } - - return mBooleanCache.get(flag.getName()); + // Fill the cache. + return isEnabledInternal(flag.getName(), mResources.getBoolean(flag.getResourceId())); } @Override public boolean isEnabled(SysPropBooleanFlag flag) { - if (!mBooleanCache.containsKey(flag.getName())) { - return isEnabled( - flag.getName(), - mSystemProperties.getBoolean(flag.getName(), flag.getDefault())); - } - - return mBooleanCache.get(flag.getName()); + // Fill the cache. + return isEnabledInternal( + flag.getName(), + mSystemProperties.getBoolean(flag.getName(), flag.getDefault())); } - private boolean isEnabled(String name, boolean defaultValue) { - mBooleanCache.put(name, defaultValue); - return defaultValue; + /** + * Checks and fills the boolean cache. This is important, Always call through to this method! + * + * We use the cache as a way to decide if we need to restart the process when server-side + * changes occur. + */ + private boolean isEnabledInternal(String name, boolean defaultValue) { + // Fill the cache. + if (!mBooleanCache.containsKey(name)) { + mBooleanCache.put(name, defaultValue); + } + + return mBooleanCache.get(name); } @NonNull @Override public String getString(@NonNull StringFlag flag) { - return getString(flag.getName(), flag.getDefault()); + // Fill the cache. + return getStringInternal(flag.getName(), flag.getDefault()); } @NonNull @Override public String getString(@NonNull ResourceStringFlag flag) { - if (!mStringCache.containsKey(flag.getName())) { - return getString(flag.getName(), - requireNonNull(mResources.getString(flag.getResourceId()))); - } - - return mStringCache.get(flag.getName()); + // Fill the cache. + return getStringInternal(flag.getName(), + requireNonNull(mResources.getString(flag.getResourceId()))); } - private String getString(String name, String defaultValue) { - mStringCache.put(name, defaultValue); - return defaultValue; + /** + * Checks and fills the String cache. This is important, Always call through to this method! + * + * We use the cache as a way to decide if we need to restart the process when server-side + * changes occur. + */ + private String getStringInternal(String name, String defaultValue) { + if (!mStringCache.containsKey(name)) { + mStringCache.put(name, defaultValue); + } + + return mStringCache.get(name); } @NonNull @Override public int getInt(@NonNull IntFlag flag) { - return flag.getDefault(); + // Fill the cache. + return getIntInternal(flag.getName(), flag.getDefault()); } @NonNull @Override public int getInt(@NonNull ResourceIntFlag flag) { + // Fill the cache. return mResources.getInteger(flag.getResourceId()); } + /** + * Checks and fills the integer cache. This is important, Always call through to this method! + * + * We use the cache as a way to decide if we need to restart the process when server-side + * changes occur. + */ + private int getIntInternal(String name, int defaultValue) { + if (!mIntCache.containsKey(name)) { + mIntCache.put(name, defaultValue); + } + + return mIntCache.get(name); + } + @Override public void dump(@NonNull PrintWriter pw, @NonNull String[] args) { pw.println("can override: false"); diff --git a/packages/SystemUI/src/com/android/systemui/flags/ServerFlagReader.kt b/packages/SystemUI/src/com/android/systemui/flags/ServerFlagReader.kt index 9b748d0a0eb23..eaf5eac155eff 100644 --- a/packages/SystemUI/src/com/android/systemui/flags/ServerFlagReader.kt +++ b/packages/SystemUI/src/com/android/systemui/flags/ServerFlagReader.kt @@ -37,7 +37,7 @@ interface ServerFlagReader { fun listenForChanges(values: Collection>, listener: ChangeListener) interface ChangeListener { - fun onChange(flag: Flag<*>) + fun onChange(flag: Flag<*>, value: String?) } } @@ -67,7 +67,7 @@ class ServerFlagReaderImpl @Inject constructor( propLoop@ for (propName in properties.keyset) { for (flag in flags) { if (propName == flag.name) { - listener.onChange(flag) + listener.onChange(flag, properties.getString(propName, null)) break@propLoop } } @@ -144,7 +144,7 @@ class ServerFlagReaderFake : ServerFlagReader { for ((listener, flags) in listeners) { flagLoop@ for (flag in flags) { if (name == flag.name) { - listener.onChange(flag) + listener.onChange(flag, if (value) "true" else "false") break@flagLoop } } @@ -159,5 +159,6 @@ class ServerFlagReaderFake : ServerFlagReader { flags: Collection>, listener: ServerFlagReader.ChangeListener ) { + listeners.add(Pair(listener, flags)) } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt index 2bcd75b7c707c..18f7db18b1f4d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsDebugTest.kt @@ -37,6 +37,7 @@ import org.mockito.Mock import org.mockito.Mockito.anyBoolean import org.mockito.Mockito.anyString import org.mockito.Mockito.inOrder +import org.mockito.Mockito.never import org.mockito.Mockito.times import org.mockito.Mockito.verify import org.mockito.Mockito.verifyNoMoreInteractions @@ -53,7 +54,7 @@ import org.mockito.Mockito.`when` as whenever */ @SmallTest class FeatureFlagsDebugTest : SysuiTestCase() { - private lateinit var mFeatureFlagsDebug: FeatureFlagsDebug + private lateinit var featureFlagsDebug: FeatureFlagsDebug @Mock private lateinit var flagManager: FlagManager @@ -85,7 +86,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { flagMap.put(Flags.TEAMFOOD.name, Flags.TEAMFOOD) flagMap.put(teamfoodableFlagA.name, teamfoodableFlagA) flagMap.put(teamfoodableFlagB.name, teamfoodableFlagB) - mFeatureFlagsDebug = FeatureFlagsDebug( + featureFlagsDebug = FeatureFlagsDebug( flagManager, mockContext, globalSettings, @@ -95,7 +96,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { flagMap, restarter ) - mFeatureFlagsDebug.init() + featureFlagsDebug.init() verify(flagManager).onSettingsChangedAction = any() broadcastReceiver = withArgCaptor { verify(mockContext).registerReceiver( @@ -116,7 +117,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { whenever(flagManager.readFlagValue(eq("4"), any())).thenReturn(false) assertThat( - mFeatureFlagsDebug.isEnabled( + featureFlagsDebug.isEnabled( ReleasedFlag( 2, name = "2", @@ -125,7 +126,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ).isTrue() assertThat( - mFeatureFlagsDebug.isEnabled( + featureFlagsDebug.isEnabled( UnreleasedFlag( 3, name = "3", @@ -134,7 +135,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ).isTrue() assertThat( - mFeatureFlagsDebug.isEnabled( + featureFlagsDebug.isEnabled( ReleasedFlag( 4, name = "4", @@ -143,7 +144,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ).isFalse() assertThat( - mFeatureFlagsDebug.isEnabled( + featureFlagsDebug.isEnabled( UnreleasedFlag( 5, name = "5", @@ -157,8 +158,8 @@ class FeatureFlagsDebugTest : SysuiTestCase() { fun teamFoodFlag_False() { whenever(flagManager.readFlagValue( eq(Flags.TEAMFOOD.name), any())).thenReturn(false) - assertThat(mFeatureFlagsDebug.isEnabled(teamfoodableFlagA)).isFalse() - assertThat(mFeatureFlagsDebug.isEnabled(teamfoodableFlagB)).isTrue() + assertThat(featureFlagsDebug.isEnabled(teamfoodableFlagA)).isFalse() + assertThat(featureFlagsDebug.isEnabled(teamfoodableFlagB)).isTrue() // Regular boolean flags should still test the same. // Only our teamfoodableFlag should change. @@ -169,8 +170,8 @@ class FeatureFlagsDebugTest : SysuiTestCase() { fun teamFoodFlag_True() { whenever(flagManager.readFlagValue( eq(Flags.TEAMFOOD.name), any())).thenReturn(true) - assertThat(mFeatureFlagsDebug.isEnabled(teamfoodableFlagA)).isTrue() - assertThat(mFeatureFlagsDebug.isEnabled(teamfoodableFlagB)).isTrue() + assertThat(featureFlagsDebug.isEnabled(teamfoodableFlagA)).isTrue() + assertThat(featureFlagsDebug.isEnabled(teamfoodableFlagB)).isTrue() // Regular boolean flags should still test the same. // Only our teamfoodableFlag should change. @@ -185,8 +186,8 @@ class FeatureFlagsDebugTest : SysuiTestCase() { .thenReturn(false) whenever(flagManager.readFlagValue( eq(Flags.TEAMFOOD.name), any())).thenReturn(true) - assertThat(mFeatureFlagsDebug.isEnabled(teamfoodableFlagA)).isTrue() - assertThat(mFeatureFlagsDebug.isEnabled(teamfoodableFlagB)).isFalse() + assertThat(featureFlagsDebug.isEnabled(teamfoodableFlagA)).isTrue() + assertThat(featureFlagsDebug.isEnabled(teamfoodableFlagB)).isFalse() // Regular boolean flags should still test the same. // Only our teamfoodableFlag should change. @@ -205,7 +206,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { whenever(flagManager.readFlagValue(eq("5"), any())).thenReturn(false) assertThat( - mFeatureFlagsDebug.isEnabled( + featureFlagsDebug.isEnabled( ResourceBooleanFlag( 1, "1", @@ -214,16 +215,16 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ) ).isFalse() - assertThat(mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(2, "2", "test", 1002))).isTrue() - assertThat(mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(3, "3", "test", 1003))).isTrue() + assertThat(featureFlagsDebug.isEnabled(ResourceBooleanFlag(2, "2", "test", 1002))).isTrue() + assertThat(featureFlagsDebug.isEnabled(ResourceBooleanFlag(3, "3", "test", 1003))).isTrue() Assert.assertThrows(NameNotFoundException::class.java) { - mFeatureFlagsDebug.isEnabled(ResourceBooleanFlag(4, "4", "test", 1004)) + featureFlagsDebug.isEnabled(ResourceBooleanFlag(4, "4", "test", 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, "5", "test", 1005)) + featureFlagsDebug.isEnabled(ResourceBooleanFlag(5, "5", "test", 1005)) } } @@ -236,11 +237,11 @@ class FeatureFlagsDebugTest : SysuiTestCase() { return@thenAnswer it.getArgument(1) } - assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(1, "a", "test"))).isFalse() - assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(2, "b", "test"))).isTrue() - assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(3, "c", "test", true))).isTrue() + assertThat(featureFlagsDebug.isEnabled(SysPropBooleanFlag(1, "a", "test"))).isFalse() + assertThat(featureFlagsDebug.isEnabled(SysPropBooleanFlag(2, "b", "test"))).isTrue() + assertThat(featureFlagsDebug.isEnabled(SysPropBooleanFlag(3, "c", "test", true))).isTrue() assertThat( - mFeatureFlagsDebug.isEnabled( + featureFlagsDebug.isEnabled( SysPropBooleanFlag( 4, "d", @@ -249,17 +250,17 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ) ).isFalse() - assertThat(mFeatureFlagsDebug.isEnabled(SysPropBooleanFlag(5, "e", "test"))).isFalse() + assertThat(featureFlagsDebug.isEnabled(SysPropBooleanFlag(5, "e", "test"))).isFalse() } @Test fun readStringFlag() { whenever(flagManager.readFlagValue(eq("3"), any())).thenReturn("foo") whenever(flagManager.readFlagValue(eq("4"), any())).thenReturn("bar") - assertThat(mFeatureFlagsDebug.getString(StringFlag(1, "1", "test", "biz"))).isEqualTo("biz") - assertThat(mFeatureFlagsDebug.getString(StringFlag(2, "2", "test", "baz"))).isEqualTo("baz") - assertThat(mFeatureFlagsDebug.getString(StringFlag(3, "3", "test", "buz"))).isEqualTo("foo") - assertThat(mFeatureFlagsDebug.getString(StringFlag(4, "4", "test", "buz"))).isEqualTo("bar") + assertThat(featureFlagsDebug.getString(StringFlag(1, "1", "test", "biz"))).isEqualTo("biz") + assertThat(featureFlagsDebug.getString(StringFlag(2, "2", "test", "baz"))).isEqualTo("baz") + assertThat(featureFlagsDebug.getString(StringFlag(3, "3", "test", "buz"))).isEqualTo("foo") + assertThat(featureFlagsDebug.getString(StringFlag(4, "4", "test", "buz"))).isEqualTo("bar") } @Test @@ -276,7 +277,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { whenever(flagManager.readFlagValue(eq("6"), any())).thenReturn("override6") assertThat( - mFeatureFlagsDebug.getString( + featureFlagsDebug.getString( ResourceStringFlag( 1, "1", @@ -286,7 +287,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ).isEqualTo("") assertThat( - mFeatureFlagsDebug.getString( + featureFlagsDebug.getString( ResourceStringFlag( 2, "2", @@ -296,7 +297,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ) ).isEqualTo("resource2") assertThat( - mFeatureFlagsDebug.getString( + featureFlagsDebug.getString( ResourceStringFlag( 3, "3", @@ -307,15 +308,15 @@ class FeatureFlagsDebugTest : SysuiTestCase() { ).isEqualTo("override3") Assert.assertThrows(NullPointerException::class.java) { - mFeatureFlagsDebug.getString(ResourceStringFlag(4, "4", "test", 1004)) + featureFlagsDebug.getString(ResourceStringFlag(4, "4", "test", 1004)) } Assert.assertThrows(NameNotFoundException::class.java) { - mFeatureFlagsDebug.getString(ResourceStringFlag(5, "5", "test", 1005)) + featureFlagsDebug.getString(ResourceStringFlag(5, "5", "test", 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, "6", "test", 1005)) + featureFlagsDebug.getString(ResourceStringFlag(6, "6", "test", 1005)) } } @@ -323,10 +324,10 @@ class FeatureFlagsDebugTest : SysuiTestCase() { fun readIntFlag() { whenever(flagManager.readFlagValue(eq("3"), any())).thenReturn(22) whenever(flagManager.readFlagValue(eq("4"), any())).thenReturn(48) - assertThat(mFeatureFlagsDebug.getInt(IntFlag(1, "1", "test", 12))).isEqualTo(12) - assertThat(mFeatureFlagsDebug.getInt(IntFlag(2, "2", "test", 93))).isEqualTo(93) - assertThat(mFeatureFlagsDebug.getInt(IntFlag(3, "3", "test", 8))).isEqualTo(22) - assertThat(mFeatureFlagsDebug.getInt(IntFlag(4, "4", "test", 234))).isEqualTo(48) + assertThat(featureFlagsDebug.getInt(IntFlag(1, "1", "test", 12))).isEqualTo(12) + assertThat(featureFlagsDebug.getInt(IntFlag(2, "2", "test", 93))).isEqualTo(93) + assertThat(featureFlagsDebug.getInt(IntFlag(3, "3", "test", 8))).isEqualTo(22) + assertThat(featureFlagsDebug.getInt(IntFlag(4, "4", "test", 234))).isEqualTo(48) } @Test @@ -342,17 +343,17 @@ class FeatureFlagsDebugTest : SysuiTestCase() { whenever(flagManager.readFlagValue(eq(4), any())).thenReturn(500) whenever(flagManager.readFlagValue(eq(5), any())).thenReturn(9519) - assertThat(mFeatureFlagsDebug.getInt(ResourceIntFlag(1, "1", "test", 1001))).isEqualTo(88) - assertThat(mFeatureFlagsDebug.getInt(ResourceIntFlag(2, "2", "test", 1002))).isEqualTo(61) - assertThat(mFeatureFlagsDebug.getInt(ResourceIntFlag(3, "3", "test", 1003))).isEqualTo(20) + assertThat(featureFlagsDebug.getInt(ResourceIntFlag(1, "1", "test", 1001))).isEqualTo(88) + assertThat(featureFlagsDebug.getInt(ResourceIntFlag(2, "2", "test", 1002))).isEqualTo(61) + assertThat(featureFlagsDebug.getInt(ResourceIntFlag(3, "3", "test", 1003))).isEqualTo(20) Assert.assertThrows(NotFoundException::class.java) { - mFeatureFlagsDebug.getInt(ResourceIntFlag(4, "4", "test", 1004)) + featureFlagsDebug.getInt(ResourceIntFlag(4, "4", "test", 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(NotFoundException::class.java) { - mFeatureFlagsDebug.getInt(ResourceIntFlag(5, "5", "test", 1005)) + featureFlagsDebug.getInt(ResourceIntFlag(5, "5", "test", 1005)) } } @@ -432,11 +433,11 @@ class FeatureFlagsDebugTest : SysuiTestCase() { whenever(flagManager.readFlagValue(eq("1"), any())).thenReturn("original") // gets the flag & cache it - assertThat(mFeatureFlagsDebug.getString(flag1)).isEqualTo("original") + assertThat(featureFlagsDebug.getString(flag1)).isEqualTo("original") verify(flagManager, times(1)).readFlagValue(eq("1"), eq(StringFlagSerializer)) // hit the cache - assertThat(mFeatureFlagsDebug.getString(flag1)).isEqualTo("original") + assertThat(featureFlagsDebug.getString(flag1)).isEqualTo("original") verifyNoMoreInteractions(flagManager) // set the flag @@ -444,7 +445,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { verifyPutData("1", "{\"type\":\"string\",\"value\":\"new\"}", numReads = 2) whenever(flagManager.readFlagValue(eq("1"), any())).thenReturn("new") - assertThat(mFeatureFlagsDebug.getString(flag1)).isEqualTo("new") + assertThat(featureFlagsDebug.getString(flag1)).isEqualTo("new") verify(flagManager, times(3)).readFlagValue(eq("1"), eq(StringFlagSerializer)) } @@ -454,7 +455,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { serverFlagReader.setFlagValue(flag.namespace, flag.name, false) - assertThat(mFeatureFlagsDebug.isEnabled(flag)).isFalse() + assertThat(featureFlagsDebug.isEnabled(flag)).isFalse() } @Test @@ -462,7 +463,33 @@ class FeatureFlagsDebugTest : SysuiTestCase() { val flag = UnreleasedFlag(100, name = "100", namespace = "test") serverFlagReader.setFlagValue(flag.namespace, flag.name, true) - assertThat(mFeatureFlagsDebug.isEnabled(flag)).isTrue() + assertThat(featureFlagsDebug.isEnabled(flag)).isTrue() + } + + @Test + fun serverSide_OverrideUncached_NoRestart() { + // No one has read the flag, so it's not in the cache. + serverFlagReader.setFlagValue( + teamfoodableFlagA.namespace, teamfoodableFlagA.name, !teamfoodableFlagA.default) + verify(restarter, never()).restartSystemUI(anyString()) + } + + @Test + fun serverSide_Override_Restarts() { + // Read it to put it in the cache. + featureFlagsDebug.isEnabled(teamfoodableFlagA) + serverFlagReader.setFlagValue( + teamfoodableFlagA.namespace, teamfoodableFlagA.name, !teamfoodableFlagA.default) + verify(restarter).restartSystemUI(anyString()) + } + + @Test + fun serverSide_RedundantOverride_NoRestart() { + // Read it to put it in the cache. + featureFlagsDebug.isEnabled(teamfoodableFlagA) + serverFlagReader.setFlagValue( + teamfoodableFlagA.namespace, teamfoodableFlagA.name, teamfoodableFlagA.default) + verify(restarter, never()).restartSystemUI(anyString()) } @Test @@ -482,13 +509,13 @@ class FeatureFlagsDebugTest : SysuiTestCase() { .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") + assertThat(featureFlagsDebug.isEnabled(flag1)).isTrue() + assertThat(featureFlagsDebug.isEnabled(flag2)).isTrue() + assertThat(featureFlagsDebug.isEnabled(flag3)).isFalse() + assertThat(featureFlagsDebug.getString(flag4)).isEmpty() + assertThat(featureFlagsDebug.getString(flag5)).isEqualTo("flag5default") + assertThat(featureFlagsDebug.getString(flag6)).isEqualTo("resource1006") + assertThat(featureFlagsDebug.getString(flag7)).isEqualTo("override7") // THEN the dump contains the flags and the default values val dump = dumpToString() @@ -527,7 +554,7 @@ class FeatureFlagsDebugTest : SysuiTestCase() { private fun dumpToString(): String { val sw = StringWriter() val pw = PrintWriter(sw) - mFeatureFlagsDebug.dump(pw, emptyArray()) + featureFlagsDebug.dump(pw, emptyArray()) pw.flush() return sw.toString() } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt index 4c6028c4c9b7c..917147b17517b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/FeatureFlagsReleaseTest.kt @@ -24,6 +24,8 @@ import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test import org.mockito.Mock +import org.mockito.Mockito +import org.mockito.Mockito.never import org.mockito.MockitoAnnotations import org.mockito.Mockito.`when` as whenever @@ -33,7 +35,7 @@ import org.mockito.Mockito.`when` as whenever */ @SmallTest class FeatureFlagsReleaseTest : SysuiTestCase() { - private lateinit var mFeatureFlagsRelease: FeatureFlagsRelease + private lateinit var featureFlagsRelease: FeatureFlagsRelease @Mock private lateinit var mResources: Resources @Mock private lateinit var mSystemProperties: SystemPropertiesHelper @@ -41,15 +43,21 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { private val flagMap = mutableMapOf>() private val serverFlagReader = ServerFlagReaderFake() + + private val flagA = ReleasedFlag(501, name = "a", namespace = "test") + @Before fun setup() { MockitoAnnotations.initMocks(this) - mFeatureFlagsRelease = FeatureFlagsRelease( + flagMap.put(flagA.name, flagA) + featureFlagsRelease = FeatureFlagsRelease( mResources, mSystemProperties, serverFlagReader, flagMap, restarter) + + featureFlagsRelease.init() } @Test @@ -60,7 +68,7 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { val flagNamespace = "test" val flag = ResourceBooleanFlag(flagId, flagName, flagNamespace, flagResourceId) whenever(mResources.getBoolean(flagResourceId)).thenReturn(true) - assertThat(mFeatureFlagsRelease.isEnabled(flag)).isTrue() + assertThat(featureFlagsRelease.isEnabled(flag)).isTrue() } @Test @@ -70,16 +78,16 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { whenever(mResources.getString(1003)).thenReturn(null) whenever(mResources.getString(1004)).thenAnswer { throw NameNotFoundException() } - assertThat(mFeatureFlagsRelease.getString( + assertThat(featureFlagsRelease.getString( ResourceStringFlag(1, "1", "test", 1001))).isEqualTo("") - assertThat(mFeatureFlagsRelease.getString( + assertThat(featureFlagsRelease.getString( ResourceStringFlag(2, "2", "test", 1002))).isEqualTo("res2") assertThrows(NullPointerException::class.java) { - mFeatureFlagsRelease.getString(ResourceStringFlag(3, "3", "test", 1003)) + featureFlagsRelease.getString(ResourceStringFlag(3, "3", "test", 1003)) } assertThrows(NameNotFoundException::class.java) { - mFeatureFlagsRelease.getString(ResourceStringFlag(4, "4", "test", 1004)) + featureFlagsRelease.getString(ResourceStringFlag(4, "4", "test", 1004)) } } @@ -92,7 +100,7 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { val flag = SysPropBooleanFlag(flagId, flagName, flagNamespace, flagDefault) whenever(mSystemProperties.getBoolean(flagName, flagDefault)).thenReturn(flagDefault) - assertThat(mFeatureFlagsRelease.isEnabled(flag)).isEqualTo(flagDefault) + assertThat(featureFlagsRelease.isEnabled(flag)).isEqualTo(flagDefault) } @Test @@ -101,7 +109,7 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { serverFlagReader.setFlagValue(flag.namespace, flag.name, false) - assertThat(mFeatureFlagsRelease.isEnabled(flag)).isFalse() + assertThat(featureFlagsRelease.isEnabled(flag)).isFalse() } @Test @@ -110,6 +118,32 @@ class FeatureFlagsReleaseTest : SysuiTestCase() { serverFlagReader.setFlagValue(flag.namespace, flag.name, true) - assertThat(mFeatureFlagsRelease.isEnabled(flag)).isFalse() + assertThat(featureFlagsRelease.isEnabled(flag)).isFalse() + } + + @Test + fun serverSide_OverrideUncached_NoRestart() { + // No one has read the flag, so it's not in the cache. + serverFlagReader.setFlagValue( + flagA.namespace, flagA.name, !flagA.default) + Mockito.verify(restarter, never()).restartSystemUI(Mockito.anyString()) + } + + @Test + fun serverSide_Override_Restarts() { + // Read it to put it in the cache. + featureFlagsRelease.isEnabled(flagA) + serverFlagReader.setFlagValue( + flagA.namespace, flagA.name, !flagA.default) + Mockito.verify(restarter).restartSystemUI(Mockito.anyString()) + } + + @Test + fun serverSide_RedundantOverride_NoRestart() { + // Read it to put it in the cache. + featureFlagsRelease.isEnabled(flagA) + serverFlagReader.setFlagValue( + flagA.namespace, flagA.name, flagA.default) + Mockito.verify(restarter, never()).restartSystemUI(Mockito.anyString()) } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/flags/ServerFlagReaderImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/flags/ServerFlagReaderImplTest.kt index 2e9800606edff..953b7fb32d56d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/flags/ServerFlagReaderImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/flags/ServerFlagReaderImplTest.kt @@ -21,11 +21,13 @@ import android.testing.AndroidTestingRunner import com.android.systemui.SysuiTestCase import com.android.systemui.util.DeviceConfigProxyFake import com.android.systemui.util.concurrency.FakeExecutor +import com.android.systemui.util.mockito.any import com.android.systemui.util.time.FakeSystemClock import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock +import org.mockito.Mockito.anyString import org.mockito.Mockito.never import org.mockito.Mockito.verify import org.mockito.MockitoAnnotations @@ -57,18 +59,18 @@ class ServerFlagReaderImplTest : SysuiTestCase() { deviceConfig.setProperty(NAMESPACE, "flag_1", "1", false) executor.runAllReady() - verify(changeListener).onChange(flag) + verify(changeListener).onChange(flag, "1") } @Test fun testChange_ignoresListenersDuringTest() { val serverFlagReader = ServerFlagReaderImpl(NAMESPACE, deviceConfig, executor, true) - val flag = ReleasedFlag(1, "1", "test") + val flag = ReleasedFlag(1, "1", " test") serverFlagReader.listenForChanges(listOf(flag), changeListener) deviceConfig.setProperty(NAMESPACE, "flag_override_1", "1", false) executor.runAllReady() - verify(changeListener, never()).onChange(flag) + verify(changeListener, never()).onChange(any(), anyString()) } }