From dfd5b2c2254662d5c2150463df06b3dd4a3f7b43 Mon Sep 17 00:00:00 2001 From: Alan Evans Date: Thu, 7 Jan 2021 12:01:03 -0400 Subject: [PATCH] Ensure consistency and completeness of feature flag remote capable designation. Make CustomVideoMuxer flag remote capable. --- .../securesms/util/FeatureFlags.java | 28 ++++--- .../securesms/util/FeatureFlagsTest.java | 5 -- .../util/FeatureFlags_ConsistencyTest.java | 79 +++++++++++++++++++ 3 files changed, 98 insertions(+), 14 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlags_ConsistencyTest.java diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index b71e6aa75c..d5ce2bd4fd 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -65,13 +65,14 @@ public final class FeatureFlags { private static final String GV1_FORCED_MIGRATE = "android.groupsV1Migration.forced"; private static final String GV1_MIGRATION_JOB = "android.groupsV1Migration.job"; private static final String SEND_VIEWED_RECEIPTS = "android.sendViewedReceipts"; - private static final String DISABLE_CUSTOM_VIDEO_MUXER = "android.disableCustomVideoMuxer"; + private static final String CUSTOM_VIDEO_MUXER = "android.customVideoMuxer"; /** * We will only store remote values for flags in this set. If you want a flag to be controllable * remotely, place it in here. */ - private static final Set REMOTE_CAPABLE = SetUtil.newHashSet( + @VisibleForTesting + static final Set REMOTE_CAPABLE = SetUtil.newHashSet( GROUPS_V2_RECOMMENDED_LIMIT, GROUPS_V2_HARD_LIMIT, INTERNAL_USER, @@ -85,7 +86,13 @@ public final class FeatureFlags { GV1_MANUAL_MIGRATE, GV1_FORCED_MIGRATE, GROUP_CALLING, - SEND_VIEWED_RECEIPTS + SEND_VIEWED_RECEIPTS, + CUSTOM_VIDEO_MUXER + ); + + @VisibleForTesting + static final Set NOT_REMOTE_CAPABLE = SetUtil.newHashSet( + PHONE_NUMBER_PRIVACY_VERSION ); /** @@ -95,7 +102,8 @@ public final class FeatureFlags { * an addition to this map. */ @SuppressWarnings("MismatchedQueryAndUpdateOfCollection") - private static final Map FORCED_VALUES = new HashMap() {{ + @VisibleForTesting + static final Map FORCED_VALUES = new HashMap() {{ }}; /** @@ -105,20 +113,22 @@ public final class FeatureFlags { * will be updated arbitrarily at runtime. This will make values more responsive, but also places * more burden on the reader to ensure that the app experience remains consistent. */ - private static final Set HOT_SWAPPABLE = SetUtil.newHashSet( + @VisibleForTesting + static final Set HOT_SWAPPABLE = SetUtil.newHashSet( VERIFY_V2, CLIENT_EXPIRATION, GROUP_CALLING, GV1_MIGRATION_JOB, - DISABLE_CUSTOM_VIDEO_MUXER + CUSTOM_VIDEO_MUXER ); /** * Flags in this set will stay true forever once they receive a true value from a remote config. */ - private static final Set STICKY = SetUtil.newHashSet( + @VisibleForTesting + static final Set STICKY = SetUtil.newHashSet( VERIFY_V2 - ); + ); /** * Listeners that are called when the value in {@link #REMOTE_VALUES} changes. That means that @@ -257,7 +267,7 @@ public final class FeatureFlags { /** Whether to use the custom streaming muxer or built in android muxer. */ public static boolean useStreamingVideoMuxer() { - return !getBoolean(DISABLE_CUSTOM_VIDEO_MUXER, false); + return getBoolean(CUSTOM_VIDEO_MUXER, false); } /** Only for rendering debug info. */ diff --git a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java index 78c3d92650..9d4114beb9 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java @@ -21,11 +21,6 @@ public class FeatureFlagsTest extends BaseUnitTest { private static final String A = "A"; private static final String B = "B"; - @Test - public void disallowForcedFlags() { - assertTrue(FeatureFlags.getForcedValues().isEmpty()); - } - @Test public void updateInternal_newValue_ignoreNotInRemoteCapable() { UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true, diff --git a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlags_ConsistencyTest.java b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlags_ConsistencyTest.java new file mode 100644 index 0000000000..e91c865962 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlags_ConsistencyTest.java @@ -0,0 +1,79 @@ +package org.thoughtcrime.securesms.util; + +import com.annimon.stream.Collectors; +import com.annimon.stream.Stream; + +import org.junit.Test; + +import java.util.Set; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public final class FeatureFlags_ConsistencyTest { + + /** + * Ensures developer makes decision on whether a flag should or should not be remote capable. + */ + @Test + public void no_flags_are_in_both_lists() { + Set intersection = SetUtil.intersection(FeatureFlags.REMOTE_CAPABLE, + FeatureFlags.NOT_REMOTE_CAPABLE); + + assertTrue(intersection.isEmpty()); + } + + /** + * Ensures developer makes decision on whether a flag should or should not be remote capable. + */ + @Test + public void all_flags_are_in_one_list_or_another() { + Set flagsByReflection = Stream.of(FeatureFlags.class.getDeclaredFields()) + .filter(f -> f.getType() == String.class) + .filter(f -> !f.getName().equals("TAG")) + .map(f -> { + try { + f.setAccessible(true); + return (String) f.get(null); + } catch (IllegalAccessException e) { + throw new AssertionError(e); + } + }) + .collect(Collectors.toSet()); + + Set flagsInBothSets = SetUtil.union(FeatureFlags.REMOTE_CAPABLE, + FeatureFlags.NOT_REMOTE_CAPABLE); + + assertEquals(flagsInBothSets, flagsByReflection); + } + + /** + * Ensures we don't leave old feature flag values in the hot swap list. + */ + @Test + public void all_hot_swap_values_are_defined_capable_or_not() { + Set flagsInBothSets = SetUtil.union(FeatureFlags.REMOTE_CAPABLE, + FeatureFlags.NOT_REMOTE_CAPABLE); + + assertTrue(flagsInBothSets.containsAll(FeatureFlags.HOT_SWAPPABLE)); + } + + /** + * Ensures we don't leave old feature flag values in the sticky list. + */ + @Test + public void all_sticky_values_are_defined_capable_or_not() { + Set flagsInBothSets = SetUtil.union(FeatureFlags.REMOTE_CAPABLE, + FeatureFlags.NOT_REMOTE_CAPABLE); + + assertTrue(flagsInBothSets.containsAll(FeatureFlags.STICKY)); + } + + /** + * Ensures we don't release with forced values which is intended for local development only. + */ + @Test + public void no_values_are_forced() { + assertTrue(FeatureFlags.FORCED_VALUES.isEmpty()); + } +}