Refactor FeatureFlags.

app/src/test/java/org/thoughtcrime/securesms/crash/CrashConfigTest.kt

@@ -45,31 +45,31 @@ class CrashConfigTest {
 
   @Test
   fun `simple name pattern`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "name": "test", "percent": 100 } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "name": "test", "percent": 100 } ]"""
     CrashConfig.computePatterns() assertIs listOf(CrashConfig.CrashPattern(namePattern = "test"))
   }
 
   @Test
   fun `simple message pattern`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "message": "test", "percent": 100 } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "message": "test", "percent": 100 } ]"""
     CrashConfig.computePatterns() assertIs listOf(CrashConfig.CrashPattern(messagePattern = "test"))
   }
 
   @Test
   fun `simple stackTrace pattern`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "stackTrace": "test", "percent": 100 } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "stackTrace": "test", "percent": 100 } ]"""
     CrashConfig.computePatterns() assertIs listOf(CrashConfig.CrashPattern(stackTracePattern = "test"))
   }
 
   @Test
   fun `all fields set`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "name": "test1", "message": "test2", "stackTrace": "test3", "percent": 100 } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "name": "test1", "message": "test2", "stackTrace": "test3", "percent": 100 } ]"""
     CrashConfig.computePatterns() assertIs listOf(CrashConfig.CrashPattern(namePattern = "test1", messagePattern = "test2", stackTracePattern = "test3"))
   }
 
   @Test
   fun `multiple configs`() {
-    every { FeatureFlags.crashPromptConfig() } returns
+    every { FeatureFlags.crashPromptConfig } returns
       """
       [ 
         { "name": "test1", "percent": 100 },
@@ -87,7 +87,7 @@ class CrashConfigTest {
 
   @Test
   fun `empty fields are considered null`() {
-    every { FeatureFlags.crashPromptConfig() } returns
+    every { FeatureFlags.crashPromptConfig } returns
       """
       [ 
         { "name": "", "percent": 100 },
@@ -104,31 +104,31 @@ class CrashConfigTest {
 
   @Test
   fun `ignore zero percent`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "name": "test", "percent": 0 } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "name": "test", "percent": 0 } ]"""
     CrashConfig.computePatterns() assertIs emptyList()
   }
 
   @Test
   fun `not setting percent is the same as zero percent`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "name": "test" } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "name": "test" } ]"""
     CrashConfig.computePatterns() assertIs emptyList()
   }
 
   @Test
   fun `ignore configs without a pattern`() {
-    every { FeatureFlags.crashPromptConfig() } returns """[ { "percent": 100 } ]"""
+    every { FeatureFlags.crashPromptConfig } returns """[ { "percent": 100 } ]"""
     CrashConfig.computePatterns() assertIs emptyList()
   }
 
   @Test
   fun `ignore invalid json`() {
-    every { FeatureFlags.crashPromptConfig() } returns "asdf"
+    every { FeatureFlags.crashPromptConfig } returns "asdf"
     CrashConfig.computePatterns() assertIs emptyList()
   }
 
   @Test
   fun `ignore empty json`() {
-    every { FeatureFlags.crashPromptConfig() } returns ""
+    every { FeatureFlags.crashPromptConfig } returns ""
     CrashConfig.computePatterns() assertIs emptyList()
   }
 }

app/src/test/java/org/thoughtcrime/securesms/keyvalue/PaymentsValuesTest.kt

@@ -48,8 +48,8 @@ class PaymentsValuesTest {
       }
     )
 
-    every { FeatureFlags.payments() } returns false
-    every { FeatureFlags.paymentsCountryBlocklist() } returns ""
+    every { FeatureFlags.payments } returns false
+    every { FeatureFlags.paymentsCountryBlocklist } returns ""
 
     assertEquals(PaymentsAvailability.DISABLED_REMOTELY, SignalStore.paymentsValues().paymentsAvailability)
   }
@@ -64,8 +64,8 @@ class PaymentsValuesTest {
       }
     )
 
-    every { FeatureFlags.payments() } returns false
-    every { FeatureFlags.paymentsCountryBlocklist() } returns ""
+    every { FeatureFlags.payments } returns false
+    every { FeatureFlags.paymentsCountryBlocklist } returns ""
 
     assertEquals(PaymentsAvailability.WITHDRAW_ONLY, SignalStore.paymentsValues().paymentsAvailability)
   }
@@ -80,8 +80,8 @@ class PaymentsValuesTest {
       }
     )
 
-    every { FeatureFlags.payments() } returns true
-    every { FeatureFlags.paymentsCountryBlocklist() } returns ""
+    every { FeatureFlags.payments } returns true
+    every { FeatureFlags.paymentsCountryBlocklist } returns ""
 
     assertEquals(PaymentsAvailability.REGISTRATION_AVAILABLE, SignalStore.paymentsValues().paymentsAvailability)
   }
@@ -96,8 +96,8 @@ class PaymentsValuesTest {
       }
     )
 
-    every { FeatureFlags.payments() } returns true
-    every { FeatureFlags.paymentsCountryBlocklist() } returns ""
+    every { FeatureFlags.payments } returns true
+    every { FeatureFlags.paymentsCountryBlocklist } returns ""
 
     assertEquals(PaymentsAvailability.WITHDRAW_AND_SEND, SignalStore.paymentsValues().paymentsAvailability)
   }
@@ -112,8 +112,8 @@ class PaymentsValuesTest {
       }
     )
 
-    every { FeatureFlags.payments() } returns true
-    every { FeatureFlags.paymentsCountryBlocklist() } returns "1"
+    every { FeatureFlags.payments } returns true
+    every { FeatureFlags.paymentsCountryBlocklist } returns "1"
 
     assertEquals(PaymentsAvailability.NOT_IN_REGION, SignalStore.paymentsValues().paymentsAvailability)
   }
@@ -128,8 +128,8 @@ class PaymentsValuesTest {
       }
     )
 
-    every { FeatureFlags.payments() } returns true
-    every { FeatureFlags.paymentsCountryBlocklist() } returns "1"
+    every { FeatureFlags.payments } returns true
+    every { FeatureFlags.paymentsCountryBlocklist } returns "1"
 
     assertEquals(PaymentsAvailability.WITHDRAW_ONLY, SignalStore.paymentsValues().paymentsAvailability)
   }

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

@@ -1,80 +0,0 @@
-package org.thoughtcrime.securesms.util;
-
-import com.annimon.stream.Collectors;
-import com.annimon.stream.Stream;
-
-import org.junit.Test;
-import org.signal.core.util.SetUtil;
-
-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<String> 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<String> 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<String> 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<String> 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<String> 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());
-  }
-}

app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlags_StaticValuesTest.kt

@@ -0,0 +1,79 @@
+package org.thoughtcrime.securesms.util
+
+import org.junit.Test
+import kotlin.reflect.KProperty1
+import kotlin.reflect.KVisibility
+import kotlin.reflect.full.memberProperties
+
+/**
+ * Ensures we don't release with forced values which is intended for local development only.
+ */
+class FeatureFlags_StaticValuesTest {
+
+  /**
+   * This test cycles the REMOTE_VALUES through a bunch of different inputs, then looks at all of the public getters and checks to see if they return different
+   * values when the inputs change. If they don't, then it's likely that the getter is returning a static value, which was likely introduced during testing
+   * and not something we actually want to commit.
+   */
+  @Test
+  fun `Ensure there's no static values`() {
+    // A list of inputs we'll cycle the remote config values to in order to see if it changes the outputs of the getters
+    val remoteTestInputs = listOf(
+      true,
+      false,
+      "true",
+      "false",
+      "cat",
+      "dog",
+      "1",
+      "100",
+      "12345678910111213141516",
+      "*"
+    )
+
+    val configKeys = FeatureFlags.configsByKey.keys
+
+    val ignoreList = setOf(
+      "REMOTE_VALUES",
+      "configsByKey",
+      "debugMemoryValues",
+      "debugDiskValues",
+      "debugPendingDiskValues",
+      "CRASH_PROMPT_CONFIG",
+      "PROMPT_BATTERY_SAVER",
+      "PROMPT_FOR_NOTIFICATION_LOGS"
+    )
+
+    val publicVals: List<KProperty1<*, *>> = FeatureFlags::class.memberProperties
+      .filter { it.visibility == KVisibility.PUBLIC }
+      .filterNot { ignoreList.contains(it.name) }
+
+    val publicValOutputs: MutableMap<String, MutableSet<Any?>> = mutableMapOf()
+
+    for (input in remoteTestInputs) {
+      for (key in configKeys) {
+        FeatureFlags.REMOTE_VALUES[key] = input
+      }
+
+      for (publicVal in publicVals) {
+        val output: Any? = publicVal.getter.call(FeatureFlags)
+        val existingOutputs: MutableSet<Any?> = publicValOutputs.getOrDefault(publicVal.name, mutableSetOf())
+        existingOutputs.add(output)
+        publicValOutputs[publicVal.name] = existingOutputs
+      }
+    }
+
+    for (entry in publicValOutputs) {
+      val getter = entry.key
+      val outputs = entry.value
+
+      if (outputs.size == 0) {
+        throw AssertionError("Getter $getter has no outputs! Something is wrong.")
+      }
+
+      if (outputs.size == 1) {
+        throw AssertionError("Getter '$getter' had the same output every time (value = ${outputs.first()})! Did you accidentally set it to a constant? Or, if you think this is a mistake, add a value to the inputs of this test that would case the value to change.")
+      }
+    }
+  }
+}