From 4ea67b84d909271907d39c2842fe4f29943c405b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 11:13:39 +0200 Subject: [PATCH] feat: mvp of conditionally enabling experiments (#1355) ## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: https://github.com/ooni/probe/issues/2553 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: N/A - [x] if you changed code inside an experiment, make sure you bump its version number ## Description We have a class of experiments that I call experimental experiments. These are experiments that we just added, so we're not 100% sure about them, or experiments that are historically flaky, so we want to be defensive. Currently, riseupvpn and echechk fall into this class. The idea of this diff is to implement the functional specification described by issue https://github.com/ooni/probe/issues/2553. While there, make sure we have more robust testing for the `./internal/registry` package. While there, make sure package creation logic lives in `./internal/registry` rather than in `./internal/engine`. --- internal/checkincache/checkincache.go | 17 + .../engine/experiment_integration_test.go | 7 + internal/engine/experimentbuilder.go | 2 +- internal/engine/inputloader.go | 6 + internal/engine/session.go | 12 - internal/registry/allexperiments.go | 3 + internal/registry/dash.go | 7 +- internal/registry/dnscheck.go | 5 +- internal/registry/dnsping.go | 5 +- internal/registry/example.go | 5 +- internal/registry/factory.go | 62 ++- internal/registry/factory_test.go | 403 ++++++++++++++++++ internal/registry/fbmessenger.go | 5 +- internal/registry/hhfm.go | 5 +- internal/registry/hirl.go | 5 +- internal/registry/httphostheader.go | 5 +- internal/registry/ndt.go | 7 +- internal/registry/portfiltering.go | 7 +- internal/registry/psiphon.go | 5 +- internal/registry/quicping.go | 5 +- internal/registry/run.go | 5 +- internal/registry/signal.go | 5 +- internal/registry/simplequicping.go | 5 +- internal/registry/sniblocking.go | 5 +- internal/registry/stunreachability.go | 5 +- internal/registry/tcpping.go | 5 +- internal/registry/telegram.go | 7 +- internal/registry/tlsmiddlebox.go | 5 +- internal/registry/tlsping.go | 5 +- internal/registry/tlstool.go | 5 +- internal/registry/tor.go | 5 +- internal/registry/torsf.go | 5 +- internal/registry/urlgetter.go | 5 +- internal/registry/vanillator.go | 5 +- internal/registry/webconnectivity.go | 7 +- internal/registry/webconnectivityv05.go | 7 +- internal/registry/whatsapp.go | 5 +- 37 files changed, 590 insertions(+), 79 deletions(-) diff --git a/internal/checkincache/checkincache.go b/internal/checkincache/checkincache.go index 6d16e18a6a..4b6e0442ab 100644 --- a/internal/checkincache/checkincache.go +++ b/internal/checkincache/checkincache.go @@ -3,6 +3,7 @@ package checkincache import ( "encoding/json" + "fmt" "time" "github.com/ooni/probe-cli/v3/internal/model" @@ -25,6 +26,9 @@ type checkInFlagsWrapper struct { } // Store stores the result of the latest check-in in the given key-value store. +// +// We store check-in feature flags in a file called checkinflags.state. These flags +// are valid for 24 hours, after which we consider them stale. func Store(kvStore model.KeyValueStore, resp *model.OOAPICheckInResult) error { // store the check-in flags in the key-value store wrapper := &checkInFlagsWrapper{ @@ -52,3 +56,16 @@ func GetFeatureFlag(kvStore model.KeyValueStore, name string) bool { } return wrapper.Flags[name] // works even if map is nil } + +// ExperimentEnabledKey returns the [model.KeyValueStore] key to use to +// know whether a disabled experiment has been enabled via check-in. +func ExperimentEnabledKey(name string) string { + return fmt.Sprintf("%s_enabled", name) +} + +// 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)) +} diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index 865a45203b..81aeb28e09 100644 --- a/internal/engine/experiment_integration_test.go +++ b/internal/engine/experiment_integration_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/registry" ) func TestCreateAll(t *testing.T) { @@ -21,6 +22,12 @@ func TestCreateAll(t *testing.T) { } sess := newSessionForTesting(t) defer sess.Close() + + // Since https://github.com/ooni/probe-cli/pull/1355, some experiments are disabled + // by default and we need an environment variable to instantiate them + os.Setenv(registry.OONI_FORCE_ENABLE_EXPERIMENT, "1") + defer os.Unsetenv(registry.OONI_FORCE_ENABLE_EXPERIMENT) + for _, name := range AllExperiments() { builder, err := sess.NewExperimentBuilder(name) if err != nil { diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 3cbd46f218..a60d0a50d5 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -62,7 +62,7 @@ func (b *experimentBuilder) NewExperiment() model.Experiment { // newExperimentBuilder creates a new experimentBuilder instance. func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) { - factory, err := registry.NewFactory(name) + factory, err := registry.NewFactory(name, session.kvStore, session.logger) if err != nil { return nil, err } diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 5a50505fb9..9bc8b0a86e 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -219,6 +219,12 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // Implementation note: we may be called from pkg/oonimkall // with a non-canonical experiment name, so we need to convert // the experiment name to be canonical before proceeding. + // + // TODO(https://github.com/ooni/probe/issues/1390): serve DNSCheck + // inputs using richer input (aka check-in v2). + // + // TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability + // inputs using richer input (aka check-in v2). switch registry.CanonicalizeExperimentName(name) { case "dnscheck": return dnsCheckDefaultInput, nil diff --git a/internal/engine/session.go b/internal/engine/session.go index 6d6bae043f..27e897d48c 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -11,7 +11,6 @@ import ( "sync/atomic" "github.com/ooni/probe-cli/v3/internal/bytecounter" - "github.com/ooni/probe-cli/v3/internal/checkincache" "github.com/ooni/probe-cli/v3/internal/enginelocate" "github.com/ooni/probe-cli/v3/internal/enginenetx" "github.com/ooni/probe-cli/v3/internal/engineresolver" @@ -19,7 +18,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/platform" "github.com/ooni/probe-cli/v3/internal/probeservices" - "github.com/ooni/probe-cli/v3/internal/registry" "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/ooni/probe-cli/v3/internal/tunnel" "github.com/ooni/probe-cli/v3/internal/version" @@ -406,16 +404,6 @@ var ErrAlreadyUsingProxy = errors.New( // for the experiment with the given name, or an error if // there's no such experiment with the given name func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, error) { - name = registry.CanonicalizeExperimentName(name) - switch { - case name == "web_connectivity" && checkincache.GetFeatureFlag(s.kvStore, "webconnectivity_0.5"): - // use LTE rather than the normal webconnectivity when the - // feature flag has been set through the check-in API - s.Logger().Infof("using webconnectivity LTE") - name = "web_connectivity@v0.5" - default: - // nothing - } eb, err := newExperimentBuilder(s, name) if err != nil { return nil, err diff --git a/internal/registry/allexperiments.go b/internal/registry/allexperiments.go index d191b6c515..39ce61226b 100644 --- a/internal/registry/allexperiments.go +++ b/internal/registry/allexperiments.go @@ -1,5 +1,7 @@ package registry +import "sort" + // Where we register all the available experiments. var AllExperiments = map[string]*Factory{} @@ -8,5 +10,6 @@ func ExperimentNames() (names []string) { for key := range AllExperiments { names = append(names, key) } + sort.Strings(names) // sort by name to always provide predictable output return } diff --git a/internal/registry/dash.go b/internal/registry/dash.go index 8551f6a20a..59175fca0c 100644 --- a/internal/registry/dash.go +++ b/internal/registry/dash.go @@ -16,8 +16,9 @@ func init() { *config.(*dash.Config), ) }, - config: &dash.Config{}, - interruptible: true, - inputPolicy: model.InputNone, + config: &dash.Config{}, + enabledByDefault: true, + interruptible: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/dnscheck.go b/internal/registry/dnscheck.go index f949886938..3d474613a2 100644 --- a/internal/registry/dnscheck.go +++ b/internal/registry/dnscheck.go @@ -16,7 +16,8 @@ func init() { *config.(*dnscheck.Config), ) }, - config: &dnscheck.Config{}, - inputPolicy: model.InputOrStaticDefault, + config: &dnscheck.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, } } diff --git a/internal/registry/dnsping.go b/internal/registry/dnsping.go index 362c9b7f01..1ccb3122c0 100644 --- a/internal/registry/dnsping.go +++ b/internal/registry/dnsping.go @@ -16,7 +16,8 @@ func init() { *config.(*dnsping.Config), ) }, - config: &dnsping.Config{}, - inputPolicy: model.InputOrStaticDefault, + config: &dnsping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, } } diff --git a/internal/registry/example.go b/internal/registry/example.go index e8af265d8e..e487b22f04 100644 --- a/internal/registry/example.go +++ b/internal/registry/example.go @@ -22,7 +22,8 @@ func init() { Message: "Good day from the example experiment!", SleepTime: int64(time.Second), }, - interruptible: true, - inputPolicy: model.InputNone, + enabledByDefault: true, + interruptible: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 9789bb3688..21a45ee02b 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -7,9 +7,11 @@ package registry import ( "errors" "fmt" + "os" "reflect" "strconv" + "github.com/ooni/probe-cli/v3/internal/checkincache" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/strcasex" ) @@ -22,6 +24,9 @@ type Factory struct { // config contains the experiment's config. config any + // enabledByDefault indicates whether this experiment is enabled by default. + enabledByDefault bool + // inputPolicy contains the experiment's InputPolicy. inputPolicy model.InputPolicy @@ -218,12 +223,65 @@ func CanonicalizeExperimentName(name string) string { // ErrNoSuchExperiment indicates a given experiment does not exist. var ErrNoSuchExperiment = errors.New("no such experiment") +// ErrRequiresForceEnable is returned for experiments that are not enabled by default and are also +// not enabled by the most recent check-in API call. +var ErrRequiresForceEnable = errors.New("experiment not enabled by check-in API") + +const experimentDisabledByCheckInWarning = `experiment '%s' is not enabled by default and the +most recent check-in API call did not enable this experiment as well. You can bypass this restriction +by setting the OONI_FORCE_ENABLE_EXPERIMENT environment variable to the string "1". On Unix like +systems, you can use 'export OONI_FORCE_ENABLE_EXPERIMENT=1' to set this environment variable.` + +// OONI_FORCE_ENABLE_EXPERIMENT is the name of the environment variable you should set to "1" +// to bypass the algorithm preventing disabled by default experiments to be instantiated. +const OONI_FORCE_ENABLE_EXPERIMENT = "OONI_FORCE_ENABLE_EXPERIMENT" + // NewFactory creates a new Factory instance. -func NewFactory(name string) (*Factory, error) { +func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (*Factory, error) { + // Make sure we are deadling with the canonical experiment name. Historically MK used + // names such as WebConnectivity and we want to continue supporting this use case. name = CanonicalizeExperimentName(name) + + // Handle A/B testing where we dynamically choose LTE for some users. The current policy + // only relates to a few users to collect data. + // + // TODO(https://github.com/ooni/probe/issues/2555): perform the actual comparison + // 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"): + // use LTE rather than the normal webconnectivity when the + // feature flag has been set through the check-in API + logger.Infof("using webconnectivity LTE") + name = "web_connectivity@v0.5" + + default: + // nothing + } + + // Obtain the factory for the canonical name. factory := AllExperiments[name] if factory == nil { return nil, fmt.Errorf("%w: %s", ErrNoSuchExperiment, name) } - return factory, nil + + // Some experiments are not enabled by default. To enable them we use + // the cached check-in response or an environment variable. + // + // 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 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 + } + + logger.Warnf(experimentDisabledByCheckInWarning, name) + return nil, fmt.Errorf("%s: %w", name, ErrRequiresForceEnable) } diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index f881ffdd81..0d7b14997e 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -2,9 +2,15 @@ package registry import ( "errors" + "fmt" + "os" "testing" "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/checkincache" + "github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte" + "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/ooni/probe-cli/v3/internal/model" ) type fakeExperimentConfig struct { @@ -345,3 +351,400 @@ func TestExperimentBuilderSetOptionsAny(t *testing.T) { } }) } + +func TestNewFactory(t *testing.T) { + // experimentSpecificExpectations contains expectations for an experiment + type experimentSpecificExpectations struct { + // enabledByDefault contains the expected value for the enabledByDefault factory field. + enabledByDefault bool + + // inputPolicy contains the expected value for the input policy. + inputPolicy model.InputPolicy + + // interruptible contains the expected value for interrupted. + interruptible bool + } + + // expectationsMap contains expectations for each experiment that exists + expectationsMap := map[string]*experimentSpecificExpectations{ + "dash": { + enabledByDefault: true, + inputPolicy: model.InputNone, + interruptible: true, + }, + "dnscheck": { + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, + }, + "dnsping": { + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, + }, + "echcheck": { + // Note: echcheck is not enabled by default because we just introduced it + // into 3.19.0-alpha, which makes it a relatively new experiment. + //enabledByDefault: false, + inputPolicy: model.InputOptional, + }, + "example": { + enabledByDefault: true, + inputPolicy: model.InputNone, + interruptible: true, + }, + "facebook_messenger": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "http_header_field_manipulation": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "http_host_header": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "http_invalid_request_line": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "ndt": { + enabledByDefault: true, + inputPolicy: model.InputNone, + interruptible: true, + }, + "portfiltering": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "psiphon": { + enabledByDefault: true, + inputPolicy: model.InputOptional, + }, + "quicping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "riseupvpn": { + // Note: riseupvpn is not enabled by default because it has been flaky + // in the past and we want to be defensive here. + //enabledByDefault: false, + inputPolicy: model.InputNone, + }, + "run": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "signal": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "simple_sni": { + // Note: simple_sni is not enabled by default because it has only been + // introduced for writing tutorials and should not be used. + //enabledByDefault: false, + inputPolicy: model.InputOrQueryBackend, + }, + "simplequicping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "sni_blocking": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "stunreachability": { + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, + }, + "tcpping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "tlsmiddlebox": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "telegram": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "tlsping": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "tlstool": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "tor": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "torsf": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "urlgetter": { + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, + }, + "vanilla_tor": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + "web_connectivity": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "web_connectivity@v0.5": { + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, + }, + "whatsapp": { + enabledByDefault: true, + inputPolicy: model.InputNone, + }, + } + + // testCase is a test case checked by this func + type testCase struct { + // description describes the test case + description string + + // experimentName is the experiment experimentName + experimentName string + + // kvStore is the key-value store to use + kvStore model.KeyValueStore + + // setForceEnableExperiment sets the OONI_FORCE_ENABLE_EXPERIMENT=1 env variable + setForceEnableExperiment bool + + // expectErr is the error we expect when calling NewFactory + expectErr error + } + + // allCases contains all test cases + allCases := []*testCase{} + + // create test cases for canonical experiment names + for _, name := range ExperimentNames() { + allCases = append(allCases, &testCase{ + description: name, + experimentName: name, + kvStore: &kvstore.Memory{}, + expectErr: (func() error { + expectations := expectationsMap[name] + if expectations == nil { + t.Fatal("no expectations for", name) + } + if !expectations.enabledByDefault { + return ErrRequiresForceEnable + } + return nil + }()), + }) + } + + // add additional test for the ndt7 experiment name + allCases = append(allCases, &testCase{ + description: "the ndt7 name still works", + experimentName: "ndt7", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // add additional test for the dns_check experiment name + allCases = append(allCases, &testCase{ + description: "the dns_check name still works", + experimentName: "dns_check", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // add additional test for the stun_reachability experiment name + allCases = append(allCases, &testCase{ + description: "the stun_reachability name still works", + experimentName: "stun_reachability", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // add additional test for the web_connectivity@v_0_5 experiment name + allCases = append(allCases, &testCase{ + description: "the web_connectivity@v_0_5 name still works", + experimentName: "web_connectivity@v_0_5", + kvStore: &kvstore.Memory{}, + expectErr: nil, + }) + + // make sure we can create default-not-enabled experiments if we + // configure the proper environment variable + for name, expectations := range expectationsMap { + if expectations.enabledByDefault { + continue + } + + allCases = append(allCases, &testCase{ + description: fmt.Sprintf("we can create %s with OONI_FORCE_ENABLE_EXPERIMENT=1", name), + experimentName: name, + kvStore: &kvstore.Memory{}, + setForceEnableExperiment: true, + expectErr: nil, + }) + } + + // make sure we can create default-not-enabled experiments if we + // configure the proper check-in flags + for name, expectations := range expectationsMap { + if expectations.enabledByDefault { + continue + } + + // create a check-in configuration with the experiment being enabled + store := &kvstore.Memory{} + checkincache.Store(store, &model.OOAPICheckInResult{ + Conf: model.OOAPICheckInResultConfig{ + Features: map[string]bool{ + checkincache.ExperimentEnabledKey(name): true, + }, + }, + }) + + allCases = append(allCases, &testCase{ + description: fmt.Sprintf("we can create %s with the proper check-in config", name), + experimentName: name, + kvStore: store, + setForceEnableExperiment: false, + expectErr: nil, + }) + } + + // perform checks for each name + for _, tc := range allCases { + t.Run(tc.description, func(t *testing.T) { + // make sure the bypass environment variable is not set + if os.Getenv(OONI_FORCE_ENABLE_EXPERIMENT) != "" { + t.Fatal("the OONI_FORCE_ENABLE_EXPERIMENT env variable shouldn't be set") + } + + // if needed, set the environment variable for the scope of the func + if tc.setForceEnableExperiment { + os.Setenv(OONI_FORCE_ENABLE_EXPERIMENT, "1") + defer os.Unsetenv(OONI_FORCE_ENABLE_EXPERIMENT) + } + + t.Log("experimentName:", tc.experimentName) + + // get experiment expectations -- note that here we must canonicalize the + // experiment name otherwise we won't find it into the map when testing non-canonical names + expectations := expectationsMap[CanonicalizeExperimentName(tc.experimentName)] + if expectations == nil { + t.Fatal("no expectations for", tc.experimentName) + } + + t.Logf("expectations: %+v", expectations) + + // get the experiment factory + factory, err := NewFactory(tc.experimentName, tc.kvStore, model.DiscardLogger) + + t.Logf("NewFactory returned: %+v %+v", factory, err) + + // make sure the returned error makes sense + switch { + case tc.expectErr == nil && err != nil: + t.Fatal(tc.experimentName, ": expected", tc.expectErr, "got", err) + + case tc.expectErr != nil && err == nil: + t.Fatal(tc.experimentName, ": expected", tc.expectErr, "got", err) + + case tc.expectErr != nil && err != nil: + if !errors.Is(err, tc.expectErr) { + t.Fatal(tc.experimentName, ": expected", tc.expectErr, "got", err) + } + return + + case tc.expectErr == nil && err == nil: + // fallthrough + } + + // make sure the enabled by default field is consistent with expectations + if factory.enabledByDefault != expectations.enabledByDefault { + t.Fatal(tc.experimentName, ": expected", expectations.enabledByDefault, "got", factory.enabledByDefault) + } + + // make sure the input policy is the expected one + if v := factory.InputPolicy(); v != expectations.inputPolicy { + t.Fatal(tc.experimentName, ": expected", expectations.inputPolicy, "got", v) + } + + // make sure the interruptible value is the expected one + if v := factory.Interruptible(); v != expectations.interruptible { + t.Fatal(tc.experimentName, ": expected", expectations.interruptible, "got", v) + } + + // make sure we can create the measurer + measurer := factory.NewExperimentMeasurer() + if measurer == nil { + t.Fatal("expected non-nil measurer, got nil") + } + }) + } + + // make sure we create web_connectivity@v0.5 when the check-in says so + t.Run("we honor check-in flags for web_connectivity@v0.5", func(t *testing.T) { + // create a keyvalue store with the proper flags + store := &kvstore.Memory{} + checkincache.Store(store, &model.OOAPICheckInResult{ + Conf: model.OOAPICheckInResultConfig{ + Features: map[string]bool{ + "webconnectivity_0.5": true, + }, + }, + }) + + // get the experiment factory + factory, err := NewFactory("web_connectivity", store, model.DiscardLogger) + if err != nil { + t.Fatal(err) + } + + // make sure the enabled by default field is consistent with expectations + if !factory.enabledByDefault { + t.Fatal("expected enabledByDefault to be true") + } + + // make sure the input policy is the expected one + if factory.InputPolicy() != model.InputOrQueryBackend { + t.Fatal("expected inputPolicy to be InputOrQueryBackend") + } + + // make sure the interrupted value is the expected one + if factory.Interruptible() { + t.Fatal("expected interruptible to be false") + } + + // make sure we can create the measurer + measurer := factory.NewExperimentMeasurer() + if measurer == nil { + t.Fatal("expected non-nil measurer, got nil") + } + + // make sure the type we're creating is the correct one + if _, good := measurer.(*webconnectivitylte.Measurer); !good { + t.Fatalf("expected to see an instance of *webconnectivitylte.Measurer, got %T", measurer) + } + }) + + // add a test case for a nonexistent experiment + t.Run("we correctly return an error for a nonexistent experiment", func(t *testing.T) { + // the empty string is a nonexistent experiment + factory, err := NewFactory("", &kvstore.Memory{}, model.DiscardLogger) + if !errors.Is(err, ErrNoSuchExperiment) { + t.Fatal("unexpected err", err) + } + if factory != nil { + t.Fatal("expected nil factory here") + } + }) +} diff --git a/internal/registry/fbmessenger.go b/internal/registry/fbmessenger.go index 73bee4fe3c..0e3be67df0 100644 --- a/internal/registry/fbmessenger.go +++ b/internal/registry/fbmessenger.go @@ -16,7 +16,8 @@ func init() { *config.(*fbmessenger.Config), ) }, - config: &fbmessenger.Config{}, - inputPolicy: model.InputNone, + config: &fbmessenger.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/hhfm.go b/internal/registry/hhfm.go index 3b1038ad64..44eb426e92 100644 --- a/internal/registry/hhfm.go +++ b/internal/registry/hhfm.go @@ -16,7 +16,8 @@ func init() { *config.(*hhfm.Config), ) }, - config: &hhfm.Config{}, - inputPolicy: model.InputNone, + config: &hhfm.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/hirl.go b/internal/registry/hirl.go index 2c55b53b74..1209052fc1 100644 --- a/internal/registry/hirl.go +++ b/internal/registry/hirl.go @@ -16,7 +16,8 @@ func init() { *config.(*hirl.Config), ) }, - config: &hirl.Config{}, - inputPolicy: model.InputNone, + config: &hirl.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/httphostheader.go b/internal/registry/httphostheader.go index 7c7ff890fd..0eaa98c654 100644 --- a/internal/registry/httphostheader.go +++ b/internal/registry/httphostheader.go @@ -16,7 +16,8 @@ func init() { *config.(*httphostheader.Config), ) }, - config: &httphostheader.Config{}, - inputPolicy: model.InputOrQueryBackend, + config: &httphostheader.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/ndt.go b/internal/registry/ndt.go index ded014a4c0..c07d5ff7ff 100644 --- a/internal/registry/ndt.go +++ b/internal/registry/ndt.go @@ -16,8 +16,9 @@ func init() { *config.(*ndt7.Config), ) }, - config: &ndt7.Config{}, - interruptible: true, - inputPolicy: model.InputNone, + config: &ndt7.Config{}, + enabledByDefault: true, + interruptible: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/portfiltering.go b/internal/registry/portfiltering.go index 2c175638b4..36e152f38b 100644 --- a/internal/registry/portfiltering.go +++ b/internal/registry/portfiltering.go @@ -16,8 +16,9 @@ func init() { config.(portfiltering.Config), ) }, - config: portfiltering.Config{}, - interruptible: false, - inputPolicy: model.InputNone, + config: portfiltering.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/psiphon.go b/internal/registry/psiphon.go index 18aae3d784..46ff7e06ca 100644 --- a/internal/registry/psiphon.go +++ b/internal/registry/psiphon.go @@ -16,7 +16,8 @@ func init() { *config.(*psiphon.Config), ) }, - config: &psiphon.Config{}, - inputPolicy: model.InputOptional, + config: &psiphon.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOptional, } } diff --git a/internal/registry/quicping.go b/internal/registry/quicping.go index 7ada9c3e0b..67a87870c9 100644 --- a/internal/registry/quicping.go +++ b/internal/registry/quicping.go @@ -16,7 +16,8 @@ func init() { *config.(*quicping.Config), ) }, - config: &quicping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &quicping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/run.go b/internal/registry/run.go index 2310670c15..371aac43bb 100644 --- a/internal/registry/run.go +++ b/internal/registry/run.go @@ -16,7 +16,8 @@ func init() { *config.(*run.Config), ) }, - config: &run.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &run.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/signal.go b/internal/registry/signal.go index 9c568ce077..927630ff00 100644 --- a/internal/registry/signal.go +++ b/internal/registry/signal.go @@ -16,7 +16,8 @@ func init() { *config.(*signal.Config), ) }, - config: &signal.Config{}, - inputPolicy: model.InputNone, + config: &signal.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/simplequicping.go b/internal/registry/simplequicping.go index 33bf29b65d..6b56ad4ed7 100644 --- a/internal/registry/simplequicping.go +++ b/internal/registry/simplequicping.go @@ -16,7 +16,8 @@ func init() { *config.(*simplequicping.Config), ) }, - config: &simplequicping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &simplequicping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/sniblocking.go b/internal/registry/sniblocking.go index bda66d1805..a7f5fb7897 100644 --- a/internal/registry/sniblocking.go +++ b/internal/registry/sniblocking.go @@ -16,7 +16,8 @@ func init() { *config.(*sniblocking.Config), ) }, - config: &sniblocking.Config{}, - inputPolicy: model.InputOrQueryBackend, + config: &sniblocking.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/stunreachability.go b/internal/registry/stunreachability.go index 38ab7cf16f..291f5bb6ac 100644 --- a/internal/registry/stunreachability.go +++ b/internal/registry/stunreachability.go @@ -16,7 +16,8 @@ func init() { *config.(*stunreachability.Config), ) }, - config: &stunreachability.Config{}, - inputPolicy: model.InputOrStaticDefault, + config: &stunreachability.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrStaticDefault, } } diff --git a/internal/registry/tcpping.go b/internal/registry/tcpping.go index 0be3933832..89d01189dc 100644 --- a/internal/registry/tcpping.go +++ b/internal/registry/tcpping.go @@ -16,7 +16,8 @@ func init() { *config.(*tcpping.Config), ) }, - config: &tcpping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &tcpping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/telegram.go b/internal/registry/telegram.go index 2218dfd248..d68ffea468 100644 --- a/internal/registry/telegram.go +++ b/internal/registry/telegram.go @@ -16,8 +16,9 @@ func init() { config.(telegram.Config), ) }, - config: telegram.Config{}, - interruptible: false, - inputPolicy: model.InputNone, + config: telegram.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/tlsmiddlebox.go b/internal/registry/tlsmiddlebox.go index 4ca5733b12..132df65027 100644 --- a/internal/registry/tlsmiddlebox.go +++ b/internal/registry/tlsmiddlebox.go @@ -16,7 +16,8 @@ func init() { *config.(*tlsmiddlebox.Config), ) }, - config: &tlsmiddlebox.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &tlsmiddlebox.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/tlsping.go b/internal/registry/tlsping.go index 862b3186d0..4d49fc59dc 100644 --- a/internal/registry/tlsping.go +++ b/internal/registry/tlsping.go @@ -16,7 +16,8 @@ func init() { *config.(*tlsping.Config), ) }, - config: &tlsping.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &tlsping.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/tlstool.go b/internal/registry/tlstool.go index ed2047ddbf..8327a4ad7e 100644 --- a/internal/registry/tlstool.go +++ b/internal/registry/tlstool.go @@ -16,7 +16,8 @@ func init() { *config.(*tlstool.Config), ) }, - config: &tlstool.Config{}, - inputPolicy: model.InputOrQueryBackend, + config: &tlstool.Config{}, + enabledByDefault: true, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/tor.go b/internal/registry/tor.go index e5dc81f212..02b6c9b2f9 100644 --- a/internal/registry/tor.go +++ b/internal/registry/tor.go @@ -16,7 +16,8 @@ func init() { *config.(*tor.Config), ) }, - config: &tor.Config{}, - inputPolicy: model.InputNone, + config: &tor.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/torsf.go b/internal/registry/torsf.go index 38ef135151..ad2d6051c3 100644 --- a/internal/registry/torsf.go +++ b/internal/registry/torsf.go @@ -16,7 +16,8 @@ func init() { *config.(*torsf.Config), ) }, - config: &torsf.Config{}, - inputPolicy: model.InputNone, + config: &torsf.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/urlgetter.go b/internal/registry/urlgetter.go index 8044af92ce..8f8c45985a 100644 --- a/internal/registry/urlgetter.go +++ b/internal/registry/urlgetter.go @@ -16,7 +16,8 @@ func init() { *config.(*urlgetter.Config), ) }, - config: &urlgetter.Config{}, - inputPolicy: model.InputStrictlyRequired, + config: &urlgetter.Config{}, + enabledByDefault: true, + inputPolicy: model.InputStrictlyRequired, } } diff --git a/internal/registry/vanillator.go b/internal/registry/vanillator.go index 5f9d4fa36d..cd8fae6583 100644 --- a/internal/registry/vanillator.go +++ b/internal/registry/vanillator.go @@ -16,7 +16,8 @@ func init() { *config.(*vanillator.Config), ) }, - config: &vanillator.Config{}, - inputPolicy: model.InputNone, + config: &vanillator.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } } diff --git a/internal/registry/webconnectivity.go b/internal/registry/webconnectivity.go index ada8767417..2e8b857861 100644 --- a/internal/registry/webconnectivity.go +++ b/internal/registry/webconnectivity.go @@ -16,8 +16,9 @@ func init() { config.(webconnectivity.Config), ) }, - config: webconnectivity.Config{}, - interruptible: false, - inputPolicy: model.InputOrQueryBackend, + config: webconnectivity.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/webconnectivityv05.go b/internal/registry/webconnectivityv05.go index 7697113f61..28a5d3c8de 100644 --- a/internal/registry/webconnectivityv05.go +++ b/internal/registry/webconnectivityv05.go @@ -18,8 +18,9 @@ func init() { config.(*webconnectivitylte.Config), ) }, - config: &webconnectivitylte.Config{}, - interruptible: false, - inputPolicy: model.InputOrQueryBackend, + config: &webconnectivitylte.Config{}, + enabledByDefault: true, + interruptible: false, + inputPolicy: model.InputOrQueryBackend, } } diff --git a/internal/registry/whatsapp.go b/internal/registry/whatsapp.go index 9d82598260..cac5efde0e 100644 --- a/internal/registry/whatsapp.go +++ b/internal/registry/whatsapp.go @@ -16,7 +16,8 @@ func init() { *config.(*whatsapp.Config), ) }, - config: &whatsapp.Config{}, - inputPolicy: model.InputNone, + config: &whatsapp.Config{}, + enabledByDefault: true, + inputPolicy: model.InputNone, } }