diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 591197f0c..7c9469a85 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -11,6 +11,12 @@ import ( "github.com/ooni/probe-cli/v3/internal/registry" ) +// TODO(bassosimone,DecFox): we should eventually finish merging the code in +// file with the code inside the ./internal/registry package. +// +// If there's time, this could happen at the end of the current (as of 2024-06-27) +// richer input work, otherwise any time in the future is actually fine. + // experimentBuilder implements [model.ExperimentBuilder]. // // This type is now just a tiny wrapper around registry.Factory. diff --git a/internal/engine/experimentbuilder_test.go b/internal/engine/experimentbuilder_test.go index d81e0bb7b..6d1c5e4f8 100644 --- a/internal/engine/experimentbuilder_test.go +++ b/internal/engine/experimentbuilder_test.go @@ -2,9 +2,11 @@ package engine import ( "context" + "encoding/json" "errors" "testing" + "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -49,3 +51,119 @@ func TestExperimentBuilderEngineWebConnectivity(t *testing.T) { t.Fatal("expected zero length targets") } } + +func TestExperimentBuilderBasicOperations(t *testing.T) { + // create a session for testing that does not use the network at all + sess := newSessionForTestingNoLookups(t) + + // create an experiment builder for example + builder, err := sess.NewExperimentBuilder("example") + if err != nil { + t.Fatal(err) + } + + // example should be interruptible + t.Run("Interruptible", func(t *testing.T) { + if !builder.Interruptible() { + t.Fatal("example should be interruptible") + } + }) + + // we expect to see the InputNone input policy + t.Run("InputPolicy", func(t *testing.T) { + if builder.InputPolicy() != model.InputNone { + t.Fatal("unexpectyed input policy") + } + }) + + // get the options and check whether they are what we expect + t.Run("Options", func(t *testing.T) { + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "Good day from the example experiment!"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: false}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // we can set a specific existing option + t.Run("SetOptionAny", func(t *testing.T) { + if err := builder.SetOptionAny("Message", "foobar"); err != nil { + t.Fatal(err) + } + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: false}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // we can set all options at the same time + t.Run("SetOptions", func(t *testing.T) { + inputs := map[string]any{ + "Message": "foobar", + "ReturnError": true, + } + if err := builder.SetOptionsAny(inputs); err != nil { + t.Fatal(err) + } + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: true}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // we can set all options using JSON + t.Run("SetOptionsJSON", func(t *testing.T) { + inputs := json.RawMessage(`{ + "Message": "foobar", + "ReturnError": true + }`) + if err := builder.SetOptionsJSON(inputs); err != nil { + t.Fatal(err) + } + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: true}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // TODO(bassosimone): we could possibly add more checks here. I am not doing this + // right now, because https://github.com/ooni/probe-cli/pull/1629 mostly cares about + // providing input and the rest of the codebase did not change. + // + // Also, it would make sense to eventually merge experimentbuilder.go with the + // ./internal/registry package, which also has coverage. + // + // In conclusion, our main objective for now is to make sure we don't screw the + // pooch when setting options using the experiment builder. +} diff --git a/internal/mocks/experimentbuilder.go b/internal/mocks/experimentbuilder.go index a9cd880ba..bcb965274 100644 --- a/internal/mocks/experimentbuilder.go +++ b/internal/mocks/experimentbuilder.go @@ -50,7 +50,6 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error { } func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error { - // TODO(bassosimone): write unit tests for this method return eb.MockSetOptionsJSON(value) } diff --git a/internal/mocks/experimentbuilder_test.go b/internal/mocks/experimentbuilder_test.go index 55ba1783f..7e90fff66 100644 --- a/internal/mocks/experimentbuilder_test.go +++ b/internal/mocks/experimentbuilder_test.go @@ -1,6 +1,7 @@ package mocks import ( + "encoding/json" "errors" "testing" @@ -72,6 +73,19 @@ func TestExperimentBuilder(t *testing.T) { } }) + t.Run("SetOptionsJSON", func(t *testing.T) { + expected := errors.New("mocked error") + eb := &ExperimentBuilder{ + MockSetOptionsJSON: func(value json.RawMessage) error { + return expected + }, + } + err := eb.SetOptionsJSON([]byte(`{}`)) + if !errors.Is(err, expected) { + t.Fatal("unexpected value") + } + }) + t.Run("SetCallbacks", func(t *testing.T) { var called bool eb := &ExperimentBuilder{ diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 666718c43..202158a73 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -309,6 +309,9 @@ type ExperimentOptionInfo struct { // Type contains the type. Type string + + // Value contains the current option value. + Value any } // ExperimentTargetLoader loads targets from local or remote sources. diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 505c943fc..329b57492 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -27,13 +27,13 @@ type Experiment struct { Annotations map[string]string // ExtraOptions contains OPTIONAL extra options that modify the - // default-empty experiment-specific configuration. We apply + // default experiment-specific configuration. We apply // the changes described by this field after using the InitialOptions // field to initialize the experiment-specific configuration. ExtraOptions map[string]any // InitialOptions contains an OPTIONAL [json.RawMessage] object - // used to initialize the default-empty experiment-specific + // used to initialize the default experiment-specific // configuration. After we have initialized the configuration // as such, we then apply the changes described by the ExtraOptions. InitialOptions json.RawMessage @@ -94,16 +94,9 @@ func (ed *Experiment) Run(ctx context.Context) error { // 2. configure experiment's options // - // We first unmarshal the InitialOptions into the experiment - // configuration and afterwards we modify the configuration using - // the values contained inside the ExtraOptions field. - // // This MUST happen before loading targets because the options will // possibly be used to produce richer input targets. - if err := builder.SetOptionsJSON(ed.InitialOptions); err != nil { - return err - } - if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { + if err := ed.setOptions(builder); err != nil { return err } @@ -154,6 +147,16 @@ func (ed *Experiment) Run(ctx context.Context) error { return inputProcessor.Run(ctx) } +func (ed *Experiment) setOptions(builder model.ExperimentBuilder) error { + // We first unmarshal the InitialOptions into the experiment + // configuration and afterwards we modify the configuration using + // the values contained inside the ExtraOptions field. + if err := builder.SetOptionsJSON(ed.InitialOptions); err != nil { + return err + } + return builder.SetOptionsAny(ed.ExtraOptions) +} + // inputProcessor is an alias for model.ExperimentInputProcessor type inputProcessor = model.ExperimentInputProcessor diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 2fe651119..cbe24d803 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -9,6 +9,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/engine" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/testingx" @@ -42,14 +44,14 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, - MockSetOptionsAny: func(options map[string]any) error { - calledSetOptionsAny++ - return nil - }, MockSetOptionsJSON: func(value json.RawMessage) error { calledSetOptionsJSON++ return nil }, + MockSetOptionsAny: func(options map[string]any) error { + calledSetOptionsAny++ + return nil + }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ MockMeasureWithContext: func( @@ -126,6 +128,67 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { } } +// This test ensures that we honour InitialOptions then ExtraOptions. +func TestExperimentSetOptions(t *testing.T) { + + // create the Experiment we're using for this test + exp := &Experiment{ + ExtraOptions: map[string]any{ + "Message": "jarjarbinks", + }, + InitialOptions: []byte(`{"Message": "foobar", "ReturnError": true}`), + Name: "example", + + // TODO(bassosimone): A zero-value session works here. The proper change + // however would be to write a engine.NewExperimentBuilder factory that takes + // as input an interface for the session. This would help testing. + Session: &engine.Session{}, + } + + // create the experiment builder manually + builder, err := exp.newExperimentBuilder(exp.Name) + if err != nil { + t.Fatal(err) + } + + // invoke the method we're testing + if err := exp.setOptions(builder); err != nil { + t.Fatal(err) + } + + // obtain the options + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + + // describe what we expect to happen + // + // we basically want ExtraOptions to override InitialOptions + expect := map[string]model.ExperimentOptionInfo{ + "Message": { + Doc: "Message to emit at test completion", + Type: "string", + Value: string("jarjarbinks"), // set by ExtraOptions + }, + "ReturnError": { + Doc: "Toogle to return a mocked error", + Type: "bool", + Value: bool(true), // set by InitialOptions + }, + "SleepTime": { + Doc: "Amount of time to sleep for in nanosecond", + Type: "int64", + Value: int64(1000000000), // still the default nonzero value + }, + } + + // make sure the result equals expectation + if diff := cmp.Diff(expect, options); diff != "" { + t.Fatal(diff) + } +} + func Test_experimentOptionsToStringList(t *testing.T) { type args struct { options map[string]any @@ -206,13 +269,33 @@ func TestExperimentRun(t *testing.T) { }, args: args{}, expectErr: errMocked, + }, { + name: "cannot set InitialOptions", + fields: fields{ + newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { + eb := &mocks.ExperimentBuilder{ + MockSetOptionsJSON: func(value json.RawMessage) error { + return errMocked + }, + } + return eb, nil + }, + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return []model.ExperimentTarget{}, nil + }, + } + }, + }, + args: args{}, + expectErr: errMocked, }, { name: "cannot set ExtraOptions", fields: fields{ newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ MockSetOptionsJSON: func(value json.RawMessage) error { - // TODO(bassosimone): need a test case before this one return nil }, MockSetOptionsAny: func(options map[string]any) error { diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index 8ea9b0513..b1109a8ef 100644 --- a/internal/oonirun/v1_test.go +++ b/internal/oonirun/v1_test.go @@ -24,10 +24,10 @@ func newMinimalFakeSession() *mocks.Session { MockInputPolicy: func() model.InputPolicy { return model.InputNone }, - MockSetOptionsAny: func(options map[string]any) error { + MockSetOptionsJSON: func(value json.RawMessage) error { return nil }, - MockSetOptionsJSON: func(value json.RawMessage) error { + MockSetOptionsAny: func(options map[string]any) error { return nil }, MockNewExperiment: func() model.Experiment { diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 9c5ffac30..813e9f7a4 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -106,22 +106,47 @@ var ( // Options returns the options exposed by this experiment. func (b *Factory) Options() (map[string]model.ExperimentOptionInfo, error) { + // create the result value result := make(map[string]model.ExperimentOptionInfo) + + // make sure we're dealing with a pointer ptrinfo := reflect.ValueOf(b.config) if ptrinfo.Kind() != reflect.Ptr { return nil, ErrConfigIsNotAStructPointer } - structinfo := ptrinfo.Elem().Type() - if structinfo.Kind() != reflect.Struct { + + // obtain information about the value and its type + valueinfo := ptrinfo.Elem() + typeinfo := valueinfo.Type() + + // make sure we're dealing with a struct + if typeinfo.Kind() != reflect.Struct { return nil, ErrConfigIsNotAStructPointer } - for i := 0; i < structinfo.NumField(); i++ { - field := structinfo.Field(i) - result[field.Name] = model.ExperimentOptionInfo{ - Doc: field.Tag.Get("ooni"), - Type: field.Type.String(), + + // cycle through the fields + for i := 0; i < typeinfo.NumField(); i++ { + fieldType, fieldValue := typeinfo.Field(i), valueinfo.Field(i) + + // do not include private fields into our list of fields + if !fieldType.IsExported() { + continue + } + + // skip fields that are missing an `ooni` tag + docs := fieldType.Tag.Get("ooni") + if docs == "" { + continue + } + + // create a description of this field + result[fieldType.Name] = model.ExperimentOptionInfo{ + Doc: docs, + Type: fieldType.Type.String(), + Value: fieldValue.Interface(), } } + return result, nil } @@ -239,7 +264,6 @@ func (b *Factory) SetOptionsJSON(value json.RawMessage) error { // otherwise unmarshal into the configuration, which we assume // to be a pointer to a structure. - // TODO(bassosimone): make sure with testing that b.config is always a pointer. return json.Unmarshal(value, b.config) } diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index c92f03157..d4a3d8368 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -2,10 +2,12 @@ package registry import ( "context" + "encoding/json" "errors" "fmt" "math" "os" + "reflect" "testing" "github.com/apex/log" @@ -19,14 +21,23 @@ import ( "github.com/ooni/probe-cli/v3/internal/targetloading" ) -type fakeExperimentConfig struct { - Chan chan any `ooni:"we cannot set this"` - String string `ooni:"a string"` - Truth bool `ooni:"something that no-one knows"` - Value int64 `ooni:"a number"` -} +func TestFactoryOptions(t *testing.T) { + + // the fake configuration we're using in this test + type fakeExperimentConfig struct { + // values that should be included into the Options return value + Chan chan any `ooni:"we cannot set this"` + String string `ooni:"a string"` + Truth bool `ooni:"something that no-one knows"` + Value int64 `ooni:"a number"` + + // values that should not be included because they're private + private int64 `ooni:"a private number"` + + // values that should not be included because they lack "ooni"'s tag + Invisible int64 + } -func TestExperimentBuilderOptions(t *testing.T) { t.Run("when config is not a pointer", func(t *testing.T) { b := &Factory{ config: 17, @@ -55,7 +66,14 @@ func TestExperimentBuilderOptions(t *testing.T) { }) t.Run("when config is a pointer to struct", func(t *testing.T) { - config := &fakeExperimentConfig{} + config := &fakeExperimentConfig{ + Chan: make(chan any), + String: "foobar", + Truth: true, + Value: 177114, + private: 55, + Invisible: 9876, + } b := &Factory{ config: config, } @@ -63,6 +81,7 @@ func TestExperimentBuilderOptions(t *testing.T) { if err != nil { t.Fatal(err) } + for name, value := range options { switch name { case "Chan": @@ -72,6 +91,10 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "chan interface {}" { t.Fatal("invalid type", value.Type) } + if value.Value.(chan any) == nil { + t.Fatal("expected non-nil channel here") + } + case "String": if value.Doc != "a string" { t.Fatal("invalid doc") @@ -79,6 +102,10 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "string" { t.Fatal("invalid type", value.Type) } + if v := value.Value.(string); v != "foobar" { + t.Fatal("unexpected string value", v) + } + case "Truth": if value.Doc != "something that no-one knows" { t.Fatal("invalid doc") @@ -86,6 +113,10 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "bool" { t.Fatal("invalid type", value.Type) } + if v := value.Value.(bool); !v { + t.Fatal("unexpected bool value", v) + } + case "Value": if value.Doc != "a number" { t.Fatal("invalid doc") @@ -93,14 +124,27 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "int64" { t.Fatal("invalid type", value.Type) } + if v := value.Value.(int64); v != 177114 { + t.Fatal("unexpected int64 value", v) + } + default: - t.Fatal("unknown name", name) + t.Fatal("unexpected option name", name) } } }) } -func TestExperimentBuilderSetOptionAny(t *testing.T) { +func TestFactorySetOptionAny(t *testing.T) { + + // the fake configuration we're using in this test + type fakeExperimentConfig struct { + Chan chan any `ooni:"we cannot set this"` + String string `ooni:"a string"` + Truth bool `ooni:"something that no-one knows"` + Value int64 `ooni:"a number"` + } + var inputs = []struct { TestCaseName string InitialConfig any @@ -366,7 +410,17 @@ func TestExperimentBuilderSetOptionAny(t *testing.T) { } } -func TestExperimentBuilderSetOptionsAny(t *testing.T) { +func TestFactorySetOptionsAny(t *testing.T) { + + // the fake configuration we're using in this test + type fakeExperimentConfig struct { + // values that should be included into the Options return value + Chan chan any `ooni:"we cannot set this"` + String string `ooni:"a string"` + Truth bool `ooni:"something that no-one knows"` + Value int64 `ooni:"a number"` + } + b := &Factory{config: &fakeExperimentConfig{}} t.Run("we correctly handle an empty map", func(t *testing.T) { @@ -409,6 +463,107 @@ func TestExperimentBuilderSetOptionsAny(t *testing.T) { }) } +func TestFactorySetOptionsJSON(t *testing.T) { + + // PersonRecord is a fake experiment configuration. + // + // Note how the `ooni` tag here is missing because we don't care + // about whether such a tag is present when using JSON. + type PersonRecord struct { + Name string + Age int64 + Friends []string + } + + // testcase is a test case for this function. + type testcase struct { + // name is the name of the test case + name string + + // mutableConfig is the config in which we should unmarshal the JSON + mutableConfig *PersonRecord + + // rawJSON contains the raw JSON to unmarshal into mutableConfig + rawJSON json.RawMessage + + // expectErr is the error we expect + expectErr error + + // expectRecord is what we expectRecord to see in the end + expectRecord *PersonRecord + } + + cases := []testcase{ + { + name: "we correctly accept zero-length options", + mutableConfig: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"bar", "baz"}, + }, + rawJSON: []byte{}, + expectRecord: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"bar", "baz"}, + }, + }, + + { + name: "we return an error on JSON parsing error", + mutableConfig: &PersonRecord{}, + rawJSON: []byte(`{`), + expectErr: errors.New("unexpected end of JSON input"), + expectRecord: &PersonRecord{}, + }, + + { + name: "we correctly unmarshal into the existing config", + mutableConfig: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"bar", "baz"}, + }, + rawJSON: []byte(`{"Friends":["foo","oof"]}`), + expectErr: nil, + expectRecord: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"foo", "oof"}, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + + // create the factory to use + factory := &Factory{config: tc.mutableConfig} + + // unmarshal into the mutableConfig + err := factory.SetOptionsJSON(tc.rawJSON) + + // make sure the error is the one we actually expect + switch { + case err == nil && tc.expectErr == nil: + if diff := cmp.Diff(tc.expectRecord, tc.mutableConfig); diff != "" { + t.Fatal(diff) + } + return + + case err != nil && tc.expectErr != nil: + if err.Error() != tc.expectErr.Error() { + t.Fatal("expected", tc.expectErr, "got", err) + } + return + + default: + t.Fatal("expected", tc.expectErr, "got", err) + } + }) + } +} + func TestNewFactory(t *testing.T) { // experimentSpecificExpectations contains expectations for an experiment type experimentSpecificExpectations struct { @@ -942,3 +1097,22 @@ func TestFactoryNewTargetLoader(t *testing.T) { } }) } + +// This test is important because SetOptionsJSON assumes that the experiment +// config is a struct pointer into which it is possible to write +func TestExperimentConfigIsAlwaysAPointerToStruct(t *testing.T) { + for name, ffunc := range AllExperiments { + t.Run(name, func(t *testing.T) { + factory := ffunc() + config := factory.config + ctype := reflect.TypeOf(config) + if ctype.Kind() != reflect.Pointer { + t.Fatal("expected a pointer") + } + ctype = ctype.Elem() + if ctype.Kind() != reflect.Struct { + t.Fatal("expected a struct") + } + }) + } +} diff --git a/internal/registry/webconnectivity.go b/internal/registry/webconnectivity.go index 470bad802..0c2d79367 100644 --- a/internal/registry/webconnectivity.go +++ b/internal/registry/webconnectivity.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return webconnectivity.NewExperimentMeasurer( - config.(webconnectivity.Config), + *config.(*webconnectivity.Config), ) }, canonicalName: canonicalName, - config: webconnectivity.Config{}, + config: &webconnectivity.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputOrQueryBackend,