feat: allow disabling tests using checkin

Author: DecFox

internal/checkincache/checkincache.go

@@ -42,19 +42,24 @@ func Store(kvStore model.KeyValueStore, resp *model.OOAPICheckInResult) error {
 
 // GetFeatureFlag returns the value of a check-in feature flag. In case of any
 // error this function will always return a false value.
-func GetFeatureFlag(kvStore model.KeyValueStore, name string) bool {
+func GetFeatureFlag(kvStore model.KeyValueStore, name string, defaultFlag bool) bool {
 	data, err := kvStore.Get(CheckInFlagsState)
 	if err != nil {
-		return false // as documented
+		return defaultFlag // as documented
 	}
 	var wrapper checkInFlagsWrapper
 	if err := json.Unmarshal(data, &wrapper); err != nil {
-		return false // as documented
+		return defaultFlag // as documented
 	}
+	fmt.Println(wrapper)
 	if time.Now().After(wrapper.Expire) {
-		return false // as documented
+		return defaultFlag // as documented
 	}
-	return wrapper.Flags[name] // works even if map is nil
+	flag, ok := wrapper.Flags[name]
+	if ok {
+		return flag
+	}
+	return defaultFlag // default to true if the flag is not present
 }
 
 // ExperimentEnabledKey returns the [model.KeyValueStore] key to use to
@@ -66,6 +71,6 @@ func ExperimentEnabledKey(name string) string {
 // ExperimentEnabled returns whether a given experiment has been enabled by a previous
 // execution of check-in. Some experiments are disabled by default for different reasons
 // and we use the check-in API to control whether and when they should be enabled.
-func ExperimentEnabled(kvStore model.KeyValueStore, name string) bool {
-	return GetFeatureFlag(kvStore, ExperimentEnabledKey(name))
+func ExperimentEnabled(kvStore model.KeyValueStore, name string, defaultFlag bool) bool {
+	return GetFeatureFlag(kvStore, ExperimentEnabledKey(name), defaultFlag)
 }

internal/checkincache/checkincache_test.go

@@ -65,7 +65,7 @@ func TestGetFeatureFlag(t *testing.T) {
 				return nil, expectedErr
 			},
 		}
-		if GetFeatureFlag(memstore, "antani") {
+		if GetFeatureFlag(memstore, "antani", false) {
 			t.Fatal("expected to see false here")
 		}
 	})
@@ -76,7 +76,7 @@ func TestGetFeatureFlag(t *testing.T) {
 				return []byte(`{`), nil
 			},
 		}
-		if GetFeatureFlag(memstore, "antani") {
+		if GetFeatureFlag(memstore, "antani", false) {
 			t.Fatal("expected to see false here")
 		}
 	})
@@ -88,7 +88,7 @@ func TestGetFeatureFlag(t *testing.T) {
 				return []byte(response), nil
 			},
 		}
-		if GetFeatureFlag(memstore, "antani") {
+		if GetFeatureFlag(memstore, "antani", false) {
 			t.Fatal("expected to see false here")
 		}
 	})
@@ -109,7 +109,7 @@ func TestGetFeatureFlag(t *testing.T) {
 				return data, nil
 			},
 		}
-		if !GetFeatureFlag(memstore, "antani") {
+		if !GetFeatureFlag(memstore, "antani", false) {
 			t.Fatal("expected to see true here")
 		}
 	})
@@ -128,7 +128,7 @@ func TestGetFeatureFlag(t *testing.T) {
 				return data, nil
 			},
 		}
-		if GetFeatureFlag(memstore, "antani") {
+		if GetFeatureFlag(memstore, "antani", false) {
 			t.Fatal("expected to see false here")
 		}
 	})

internal/probeservices/checkin_test.go

@@ -391,10 +391,10 @@ func TestCheckIn(t *testing.T) {
 		}
 
 		// make sure we have written non-default values into the key-value store
-		if !checkincache.GetFeatureFlag(client.KVStore, "torsf_enabled") {
+		if !checkincache.GetFeatureFlag(client.KVStore, "torsf_enabled", false) {
 			t.Fatal("expected to see true here")
 		}
-		if !checkincache.GetFeatureFlag(client.KVStore, "vanilla_tor_enabled") {
+		if !checkincache.GetFeatureFlag(client.KVStore, "vanilla_tor_enabled", false) {
 			t.Fatal("expected to see true here")
 		}
 		checkinrawdata, err := client.KVStore.Get(checkincache.CheckInFlagsState)
@@ -502,10 +502,10 @@ func TestCheckIn(t *testing.T) {
 		}
 
 		// make sure we are still getting the default values here
-		if checkincache.GetFeatureFlag(client.KVStore, "torsf_enabled") {
+		if checkincache.GetFeatureFlag(client.KVStore, "torsf_enabled", false) {
 			t.Fatal("expected to see false here")
 		}
-		if checkincache.GetFeatureFlag(client.KVStore, "vanilla_tor_enabled") {
+		if checkincache.GetFeatureFlag(client.KVStore, "vanilla_tor_enabled", false) {
 			t.Fatal("expected to see false here")
 		}
 		checkinrawdata, err := client.KVStore.Get(checkincache.CheckInFlagsState)

internal/registry/factory.go

@@ -345,7 +345,7 @@ func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (
 	// and improve the LTE implementation so that we can always use it. See the actual
 	// issue test for additional details on this planned A/B test.
 	switch {
-	case name == "web_connectivity" && checkincache.GetFeatureFlag(kvStore, "webconnectivity_0.5"):
+	case name == "web_connectivity" && checkincache.GetFeatureFlag(kvStore, "webconnectivity_0.5", false):
 		// use LTE rather than the normal webconnectivity when the
 		// feature flag has been set through the check-in API
 		logger.Infof("using webconnectivity LTE")
@@ -367,16 +367,19 @@ func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (
 	//
 	// Note: check-in flags expire after 24h.
 	//
-	// TODO(https://github.com/ooni/probe/issues/2554): we need to restructure
-	// how we run experiments to make sure check-in flags are always fresh.
+	//
+	//
 	if factory.enabledByDefault {
-		return factory, nil // enabled by default
+		if !checkincache.ExperimentEnabled(kvStore, name, true) {
+			return nil, fmt.Errorf("%s: %w", name, ErrRequiresForceEnable)
+		}
+		return factory, nil
 	}
 	if os.Getenv(OONI_FORCE_ENABLE_EXPERIMENT) == "1" {
 		return factory, nil // enabled by environment variable
 	}
-	if checkincache.ExperimentEnabled(kvStore, name) {
-		return factory, nil // enabled by check-in
+	if checkincache.ExperimentEnabled(kvStore, name, false) {
+		return factory, nil
 	}
 
 	logger.Warnf(experimentDisabledByCheckInWarning, name)

internal/registry/torsf.go

@@ -20,7 +20,7 @@ func init() {
 			},
 			canonicalName:    canonicalName,
 			config:           &torsf.Config{},
-			enabledByDefault: false,
+			enabledByDefault: true,
 			inputPolicy:      model.InputNone,
 		}
 	}