Skip to content

Commit dfd5b2c

Browse files
alan-signalalan-signal
committed
Ensure consistency and completeness of feature flag remote capable designation.
Make CustomVideoMuxer flag remote capable.
1 parent e850d8e commit dfd5b2c

3 files changed

Lines changed: 98 additions & 14 deletions

File tree

‎app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,14 @@ public final class FeatureFlags {
6565
private static final String GV1_FORCED_MIGRATE = "android.groupsV1Migration.forced";
6666
private static final String GV1_MIGRATION_JOB = "android.groupsV1Migration.job";
6767
private static final String SEND_VIEWED_RECEIPTS = "android.sendViewedReceipts";
68-
private static final String DISABLE_CUSTOM_VIDEO_MUXER = "android.disableCustomVideoMuxer";
68+
private static final String CUSTOM_VIDEO_MUXER = "android.customVideoMuxer";
6969

7070
/**
7171
* We will only store remote values for flags in this set. If you want a flag to be controllable
7272
* remotely, place it in here.
7373
*/
74-
private static final Set<String> REMOTE_CAPABLE = SetUtil.newHashSet(
74+
@VisibleForTesting
75+
static final Set<String> REMOTE_CAPABLE = SetUtil.newHashSet(
7576
GROUPS_V2_RECOMMENDED_LIMIT,
7677
GROUPS_V2_HARD_LIMIT,
7778
INTERNAL_USER,
@@ -85,7 +86,13 @@ public final class FeatureFlags {
8586
GV1_MANUAL_MIGRATE,
8687
GV1_FORCED_MIGRATE,
8788
GROUP_CALLING,
88-
SEND_VIEWED_RECEIPTS
89+
SEND_VIEWED_RECEIPTS,
90+
CUSTOM_VIDEO_MUXER
91+
);
92+
93+
@VisibleForTesting
94+
static final Set<String> NOT_REMOTE_CAPABLE = SetUtil.newHashSet(
95+
PHONE_NUMBER_PRIVACY_VERSION
8996
);
9097

9198
/**
@@ -95,7 +102,8 @@ public final class FeatureFlags {
95102
* an addition to this map.
96103
*/
97104
@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
98-
private static final Map<String, Object> FORCED_VALUES = new HashMap<String, Object>() {{
105+
@VisibleForTesting
106+
static final Map<String, Object> FORCED_VALUES = new HashMap<String, Object>() {{
99107
}};
100108

101109
/**
@@ -105,20 +113,22 @@ public final class FeatureFlags {
105113
* will be updated arbitrarily at runtime. This will make values more responsive, but also places
106114
* more burden on the reader to ensure that the app experience remains consistent.
107115
*/
108-
private static final Set<String> HOT_SWAPPABLE = SetUtil.newHashSet(
116+
@VisibleForTesting
117+
static final Set<String> HOT_SWAPPABLE = SetUtil.newHashSet(
109118
VERIFY_V2,
110119
CLIENT_EXPIRATION,
111120
GROUP_CALLING,
112121
GV1_MIGRATION_JOB,
113-
DISABLE_CUSTOM_VIDEO_MUXER
122+
CUSTOM_VIDEO_MUXER
114123
);
115124

116125
/**
117126
* Flags in this set will stay true forever once they receive a true value from a remote config.
118127
*/
119-
private static final Set<String> STICKY = SetUtil.newHashSet(
128+
@VisibleForTesting
129+
static final Set<String> STICKY = SetUtil.newHashSet(
120130
VERIFY_V2
121-
);
131+
);
122132

123133
/**
124134
* Listeners that are called when the value in {@link #REMOTE_VALUES} changes. That means that
@@ -257,7 +267,7 @@ public static boolean sendViewedReceipts() {
257267

258268
/** Whether to use the custom streaming muxer or built in android muxer. */
259269
public static boolean useStreamingVideoMuxer() {
260-
return !getBoolean(DISABLE_CUSTOM_VIDEO_MUXER, false);
270+
return getBoolean(CUSTOM_VIDEO_MUXER, false);
261271
}
262272

263273
/** Only for rendering debug info. */

‎app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,6 @@ public class FeatureFlagsTest extends BaseUnitTest {
2121
private static final String A = "A";
2222
private static final String B = "B";
2323

24-
@Test
25-
public void disallowForcedFlags() {
26-
assertTrue(FeatureFlags.getForcedValues().isEmpty());
27-
}
28-
2924
@Test
3025
public void updateInternal_newValue_ignoreNotInRemoteCapable() {
3126
UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true,

‎app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlags_ConsistencyTest.java‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
package org.thoughtcrime.securesms.util;
2+
3+
import com.annimon.stream.Collectors;
4+
import com.annimon.stream.Stream;
5+
6+
import org.junit.Test;
7+
8+
import java.util.Set;
9+
10+
import static org.junit.Assert.assertEquals;
11+
import static org.junit.Assert.assertTrue;
12+
13+
public final class FeatureFlags_ConsistencyTest {
14+
15+
/**
16+
* Ensures developer makes decision on whether a flag should or should not be remote capable.
17+
*/
18+
@Test
19+
public void no_flags_are_in_both_lists() {
20+
Set<String> intersection = SetUtil.intersection(FeatureFlags.REMOTE_CAPABLE,
21+
FeatureFlags.NOT_REMOTE_CAPABLE);
22+
23+
assertTrue(intersection.isEmpty());
24+
}
25+
26+
/**
27+
* Ensures developer makes decision on whether a flag should or should not be remote capable.
28+
*/
29+
@Test
30+
public void all_flags_are_in_one_list_or_another() {
31+
Set<String> flagsByReflection = Stream.of(FeatureFlags.class.getDeclaredFields())
32+
.filter(f -> f.getType() == String.class)
33+
.filter(f -> !f.getName().equals("TAG"))
34+
.map(f -> {
35+
try {
36+
f.setAccessible(true);
37+
return (String) f.get(null);
38+
} catch (IllegalAccessException e) {
39+
throw new AssertionError(e);
40+
}
41+
})
42+
.collect(Collectors.toSet());
43+
44+
Set<String> flagsInBothSets = SetUtil.union(FeatureFlags.REMOTE_CAPABLE,
45+
FeatureFlags.NOT_REMOTE_CAPABLE);
46+
47+
assertEquals(flagsInBothSets, flagsByReflection);
48+
}
49+
50+
/**
51+
* Ensures we don't leave old feature flag values in the hot swap list.
52+
*/
53+
@Test
54+
public void all_hot_swap_values_are_defined_capable_or_not() {
55+
Set<String> flagsInBothSets = SetUtil.union(FeatureFlags.REMOTE_CAPABLE,
56+
FeatureFlags.NOT_REMOTE_CAPABLE);
57+
58+
assertTrue(flagsInBothSets.containsAll(FeatureFlags.HOT_SWAPPABLE));
59+
}
60+
61+
/**
62+
* Ensures we don't leave old feature flag values in the sticky list.
63+
*/
64+
@Test
65+
public void all_sticky_values_are_defined_capable_or_not() {
66+
Set<String> flagsInBothSets = SetUtil.union(FeatureFlags.REMOTE_CAPABLE,
67+
FeatureFlags.NOT_REMOTE_CAPABLE);
68+
69+
assertTrue(flagsInBothSets.containsAll(FeatureFlags.STICKY));
70+
}
71+
72+
/**
73+
* Ensures we don't release with forced values which is intended for local development only.
74+
*/
75+
@Test
76+
public void no_values_are_forced() {
77+
assertTrue(FeatureFlags.FORCED_VALUES.isEmpty());
78+
}
79+
}

0 commit comments

Comments
 (0)