From f3c41bba67b0634a115d8e5faebe72e3c987f9ba Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 26 Jun 2024 07:02:59 +0200 Subject: [PATCH 01/16] feat(oonirun): allow true JSON richer input --- internal/engine/experimentbuilder.go | 25 ++++++++++++++-------- internal/mocks/experimentbuilder.go | 13 +++++++++++- internal/model/experiment.go | 6 ++++++ internal/oonirun/experiment.go | 27 +++++++++++++++++++----- internal/oonirun/experiment_test.go | 27 +++++++++++++++++++++++- internal/oonirun/v1_test.go | 4 ++++ internal/oonirun/v2.go | 5 +++-- internal/oonirun/v2_test.go | 31 ++++++++++++++-------------- internal/registry/factory.go | 15 ++++++++++++++ internal/registry/portfiltering.go | 4 ++-- internal/registry/telegram.go | 4 ++-- 11 files changed, 124 insertions(+), 37 deletions(-) diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 330777957b..591197f0c5 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -5,11 +5,13 @@ package engine // import ( + "encoding/json" + "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/registry" ) -// experimentBuilder implements ExperimentBuilder. +// experimentBuilder implements [model.ExperimentBuilder]. // // This type is now just a tiny wrapper around registry.Factory. type experimentBuilder struct { @@ -24,37 +26,42 @@ type experimentBuilder struct { var _ model.ExperimentBuilder = &experimentBuilder{} -// Interruptible implements ExperimentBuilder.Interruptible. +// Interruptible implements [model.ExperimentBuilder]. func (b *experimentBuilder) Interruptible() bool { return b.factory.Interruptible() } -// InputPolicy implements ExperimentBuilder.InputPolicy. +// InputPolicy implements [model.ExperimentBuilder]. func (b *experimentBuilder) InputPolicy() model.InputPolicy { return b.factory.InputPolicy() } -// Options implements ExperimentBuilder.Options. +// Options implements [model.ExperimentBuilder]. func (b *experimentBuilder) Options() (map[string]model.ExperimentOptionInfo, error) { return b.factory.Options() } -// SetOptionAny implements ExperimentBuilder.SetOptionAny. +// SetOptionAny implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetOptionAny(key string, value any) error { return b.factory.SetOptionAny(key, value) } -// SetOptionsAny implements ExperimentBuilder.SetOptionsAny. +// SetOptionsAny implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetOptionsAny(options map[string]any) error { return b.factory.SetOptionsAny(options) } -// SetCallbacks implements ExperimentBuilder.SetCallbacks. +// SetOptionsJSON implements [model.ExperimentBuilder]. +func (b *experimentBuilder) SetOptionsJSON(value json.RawMessage) error { + return b.factory.SetOptionsJSON(value) +} + +// SetCallbacks implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { b.callbacks = callbacks } -// NewExperiment creates the experiment +// NewExperiment creates a new [model.Experiment] instance. func (b *experimentBuilder) NewExperiment() model.Experiment { measurer := b.factory.NewExperimentMeasurer() experiment := newExperiment(b.session, measurer) @@ -67,7 +74,7 @@ func (b *experimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoader return b.factory.NewTargetLoader(config) } -// newExperimentBuilder creates a new experimentBuilder instance. +// newExperimentBuilder creates a new [*experimentBuilder] instance. func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) { factory, err := registry.NewFactory(name, session.kvStore, session.logger) if err != nil { diff --git a/internal/mocks/experimentbuilder.go b/internal/mocks/experimentbuilder.go index 1f6a27187f..a9cd880ba5 100644 --- a/internal/mocks/experimentbuilder.go +++ b/internal/mocks/experimentbuilder.go @@ -1,6 +1,10 @@ package mocks -import "github.com/ooni/probe-cli/v3/internal/model" +import ( + "encoding/json" + + "github.com/ooni/probe-cli/v3/internal/model" +) // ExperimentBuilder mocks model.ExperimentBuilder. type ExperimentBuilder struct { @@ -14,6 +18,8 @@ type ExperimentBuilder struct { MockSetOptionsAny func(options map[string]any) error + MockSetOptionsJSON func(value json.RawMessage) error + MockSetCallbacks func(callbacks model.ExperimentCallbacks) MockNewExperiment func() model.Experiment @@ -43,6 +49,11 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error { return eb.MockSetOptionsAny(options) } +func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error { + // TODO(bassosimone): write unit tests for this method + return eb.MockSetOptionsJSON(value) +} + func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { eb.MockSetCallbacks(callbacks) } diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 5aada21ff4..95848242c2 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -7,6 +7,7 @@ package model import ( "context" + "encoding/json" "errors" "fmt" ) @@ -235,6 +236,11 @@ type ExperimentBuilder interface { // the SetOptionAny method for more information. SetOptionsAny(options map[string]any) error + // SetOptionsJSON uses the given [json.RawMessage] to initialize fields + // of the configuration for running the experiment. The [json.RawMessage] + // MUST contain a serialization of the experiment config's type. + SetOptionsJSON(value json.RawMessage) error + // SetCallbacks sets the experiment's interactive callbacks. SetCallbacks(callbacks ExperimentCallbacks) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index c4613107d7..a660935244 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -6,6 +6,7 @@ package oonirun import ( "context" + "encoding/json" "fmt" "math/rand" "strings" @@ -25,9 +26,18 @@ type Experiment struct { // Annotations contains OPTIONAL Annotations for the experiment. Annotations map[string]string - // ExtraOptions contains OPTIONAL extra options for the experiment. + // ExtraOptions contains OPTIONAL extra options that modify the + // default-empty 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 + // configuration. After we have initialized the configuration + // as such, we then apply the changes described by the ExtraOptions. + InitialOptions json.RawMessage + // Inputs contains the OPTIONAL experiment Inputs Inputs []string @@ -82,15 +92,22 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // TODO(bassosimone,DecFox): when we're executed by OONI Run v2, it probably makes - // slightly more sense to set options from a json.RawMessage because the current - // command line limitation is that it's hard to set non scalar parameters and instead - // with using OONI Run v2 we can completely bypass such a limitation. + // TODO(bassosimone): we need another patch after the current one + // to correctly serialize the options as configured using InitialOptions + // and ExtraOptions otherwise the Measurement.Options field turns out + // to always be empty and this is highly suboptimal for us. // 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 { return err } diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 1fd38abb0b..2fe651119e 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -2,6 +2,7 @@ package oonirun import ( "context" + "encoding/json" "errors" "reflect" "sort" @@ -16,6 +17,7 @@ import ( func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { shuffledInputsPrev := experimentShuffledInputs.Load() var calledSetOptionsAny int + var calledSetOptionsJSON int var failedToSubmit int var calledKibiBytesReceived int var calledKibiBytesSent int @@ -44,6 +46,10 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { calledSetOptionsAny++ return nil }, + MockSetOptionsJSON: func(value json.RawMessage) error { + calledSetOptionsJSON++ + return nil + }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ MockMeasureWithContext: func( @@ -109,6 +115,9 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { if calledSetOptionsAny < 1 { t.Fatal("should have called SetOptionsAny") } + if calledSetOptionsJSON < 1 { + t.Fatal("should have called SetOptionsJSON") + } if calledKibiBytesReceived < 1 { t.Fatal("did not call KibiBytesReceived") } @@ -198,10 +207,14 @@ func TestExperimentRun(t *testing.T) { args: args{}, expectErr: errMocked, }, { - name: "cannot set options", + 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 { return errMocked }, @@ -226,6 +239,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -255,6 +271,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -298,6 +317,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -344,6 +366,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index 910d910b0c..8ea9b05139 100644 --- a/internal/oonirun/v1_test.go +++ b/internal/oonirun/v1_test.go @@ -2,6 +2,7 @@ package oonirun import ( "context" + "encoding/json" "errors" "net/http" "strings" @@ -26,6 +27,9 @@ func newMinimalFakeSession() *mocks.Session { MockSetOptionsAny: func(options map[string]any) error { return nil }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ MockMeasureWithContext: func( diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 4330b07b76..6552ad582e 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -54,7 +54,7 @@ type V2Nettest struct { // `Safe` will be available for the experiment run, but omitted from // the serialized Measurement that the experiment builder will submit // to the OONI backend. - Options map[string]any `json:"options"` + Options json.RawMessage `json:"options"` // TestName contains the nettest name. TestName string `json:"test_name"` @@ -183,7 +183,8 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri // construct an experiment from the current nettest exp := &Experiment{ Annotations: config.Annotations, - ExtraOptions: nettest.Options, + ExtraOptions: make(map[string]any), + InitialOptions: nettest.Options, Inputs: nettest.Inputs, InputFilePaths: nil, MaxRuntime: config.MaxRuntime, diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index d849c6d088..b77b9b7bf7 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -7,7 +7,6 @@ import ( "net/http" "net/http/httptest" "testing" - "time" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" @@ -27,9 +26,9 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -73,9 +72,9 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -132,9 +131,9 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -220,9 +219,9 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "", // empty! }}, } @@ -374,6 +373,9 @@ func TestV2MeasureDescriptor(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputNone }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -426,7 +428,7 @@ func TestV2MeasureDescriptor(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{}, + Options: json.RawMessage(`{}`), TestName: "example", }}, } @@ -532,5 +534,4 @@ func TestV2DescriptorCacheLoad(t *testing.T) { t.Fatal("expected nil cache") } }) - } diff --git a/internal/registry/factory.go b/internal/registry/factory.go index ee73e95a8f..9c5ffac30a 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -5,6 +5,7 @@ package registry // import ( + "encoding/json" "errors" "fmt" "math" @@ -228,6 +229,20 @@ func (b *Factory) SetOptionsAny(options map[string]any) error { return nil } +// SetOptionsJSON unmarshals the given [json.RawMessage] inside +// the experiment specific configuration. +func (b *Factory) SetOptionsJSON(value json.RawMessage) error { + // handle the case where the options are empty + if len(value) <= 0 { + return nil + } + + // 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) +} + // fieldbyname return v's field whose name is equal to the given key. func (b *Factory) fieldbyname(v interface{}, key string) (reflect.Value, error) { // See https://stackoverflow.com/a/6396678/4354461 diff --git a/internal/registry/portfiltering.go b/internal/registry/portfiltering.go index 8c6a857df8..2d7c67f250 100644 --- a/internal/registry/portfiltering.go +++ b/internal/registry/portfiltering.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return portfiltering.NewExperimentMeasurer( - config.(portfiltering.Config), + *config.(*portfiltering.Config), ) }, canonicalName: canonicalName, - config: portfiltering.Config{}, + config: &portfiltering.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputNone, diff --git a/internal/registry/telegram.go b/internal/registry/telegram.go index 987e640935..bf4f71aed1 100644 --- a/internal/registry/telegram.go +++ b/internal/registry/telegram.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return telegram.NewExperimentMeasurer( - config.(telegram.Config), + *config.(*telegram.Config), ) }, canonicalName: canonicalName, - config: telegram.Config{}, + config: &telegram.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputNone, From 4f183bda80a6edefbe8845f11618fd22b855b82d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 26 Jun 2024 07:07:11 +0200 Subject: [PATCH 02/16] x --- internal/model/experiment.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 95848242c2..ad4e0dfcfc 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -237,8 +237,9 @@ type ExperimentBuilder interface { SetOptionsAny(options map[string]any) error // SetOptionsJSON uses the given [json.RawMessage] to initialize fields - // of the configuration for running the experiment. The [json.RawMessage] - // MUST contain a serialization of the experiment config's type. + // of the configuration for running the experiment. The [json.RawMessage], if + // not empty, MUST contain a serialization of the experiment config's + // type. An empty [json.RawMessage] will silently be ignored. SetOptionsJSON(value json.RawMessage) error // SetCallbacks sets the experiment's interactive callbacks. From 50d1909cd84f74d6a85660a5dde2687f98c6f0de Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:01:15 +0400 Subject: [PATCH 03/16] chore: add more tests for new code I wrote --- internal/engine/experimentbuilder.go | 6 ++ internal/engine/experimentbuilder_test.go | 118 ++++++++++++++++++++++ internal/model/experiment.go | 3 + internal/registry/factory.go | 8 +- internal/registry/factory_test.go | 24 ++++- 5 files changed, 155 insertions(+), 4 deletions(-) diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 591197f0c5..7c9469a856 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 d81e0bb7bb..6d1c5e4f83 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/model/experiment.go b/internal/model/experiment.go index ad4e0dfcfc..b514df3dec 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -297,6 +297,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/registry/factory.go b/internal/registry/factory.go index 9c5ffac30a..5c54183250 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -111,15 +111,17 @@ func (b *Factory) Options() (map[string]model.ExperimentOptionInfo, error) { if ptrinfo.Kind() != reflect.Ptr { return nil, ErrConfigIsNotAStructPointer } - structinfo := ptrinfo.Elem().Type() + valueinfo := ptrinfo.Elem() + structinfo := valueinfo.Type() if structinfo.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(), + Doc: field.Tag.Get("ooni"), + Type: field.Type.String(), + Value: valueinfo.Field(i).Interface(), } } return result, nil diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index c92f031577..998d10f1ac 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -55,7 +55,12 @@ 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, + } b := &Factory{ config: config, } @@ -63,6 +68,7 @@ func TestExperimentBuilderOptions(t *testing.T) { if err != nil { t.Fatal(err) } + for name, value := range options { switch name { case "Chan": @@ -72,6 +78,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 +89,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 +100,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,6 +111,10 @@ 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) } From cf55bfb71ea1006747e74e0153c239df910aa9d1 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:04:45 +0400 Subject: [PATCH 04/16] x --- internal/mocks/experimentbuilder_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/mocks/experimentbuilder_test.go b/internal/mocks/experimentbuilder_test.go index 55ba1783f9..7e90fff66b 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{ From c6dacf7b40e3430609271edcde6f3f8f972eddb4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:05:17 +0400 Subject: [PATCH 05/16] x --- internal/mocks/experimentbuilder.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/mocks/experimentbuilder.go b/internal/mocks/experimentbuilder.go index a9cd880ba5..bcb9652745 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) } From 76216433a6c4c6291c60ab7840687c91a2a307cb Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:10:35 +0400 Subject: [PATCH 06/16] x --- internal/oonirun/experiment.go | 2 ++ internal/oonirun/experiment_test.go | 30 ++++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index a660935244..ddd1b36ca6 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -96,6 +96,8 @@ func (ed *Experiment) Run(ctx context.Context) error { // to correctly serialize the options as configured using InitialOptions // and ExtraOptions otherwise the Measurement.Options field turns out // to always be empty and this is highly suboptimal for us. + // + // The next patch is https://github.com/ooni/probe-cli/pull/1630. // 2. configure experiment's options // diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 2fe651119e..e29cb0bf70 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -42,14 +42,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( @@ -206,13 +206,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 { From 4575f5e15f6f94d778abc6bfd3e075e46fa96720 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:12:18 +0400 Subject: [PATCH 07/16] x --- internal/oonirun/v1_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index 8ea9b05139..b1109a8efa 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 { From d1f243db4b33af91eaa97ae81387156f9ce0a46c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:20:59 +0400 Subject: [PATCH 08/16] x --- internal/registry/factory.go | 39 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 5c54183250..879bfad478 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -106,24 +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 } + + // obtain information about the value and its type valueinfo := ptrinfo.Elem() - structinfo := valueinfo.Type() - if structinfo.Kind() != reflect.Struct { + 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(), - Value: valueinfo.Field(i).Interface(), + + // 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 } From 21c62dbec37afbc50448bba8d22e240b6284e051 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:25:38 +0400 Subject: [PATCH 09/16] fix: make sure with testing experiment config is always a struct --- internal/registry/factory.go | 1 - internal/registry/factory_test.go | 18 ++++++++++++++++++ internal/registry/webconnectivity.go | 4 ++-- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 879bfad478..813e9f7a44 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -264,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 998d10f1ac..bb089f7f50 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -6,6 +6,7 @@ import ( "fmt" "math" "os" + "reflect" "testing" "github.com/apex/log" @@ -964,3 +965,20 @@ func TestFactoryNewTargetLoader(t *testing.T) { } }) } + +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 470bad802b..0c2d79367d 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, From b8d219ba188ef8794431f0ecde21ed96c37d0d94 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:28:16 +0400 Subject: [PATCH 10/16] x --- internal/registry/factory_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index bb089f7f50..2071f387b2 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -21,10 +21,17 @@ import ( ) 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) { @@ -57,10 +64,12 @@ func TestExperimentBuilderOptions(t *testing.T) { t.Run("when config is a pointer to struct", func(t *testing.T) { config := &fakeExperimentConfig{ - Chan: make(chan any), - String: "foobar", - Truth: true, - Value: 177114, + Chan: make(chan any), + String: "foobar", + Truth: true, + Value: 177114, + private: 55, + Invisible: 9876, } b := &Factory{ config: config, @@ -117,7 +126,7 @@ func TestExperimentBuilderOptions(t *testing.T) { } default: - t.Fatal("unknown name", name) + t.Fatal("unexpected option name", name) } } }) From d49c4db04351e29d1dbb6bfe73b8c38603727d03 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:43:15 +0400 Subject: [PATCH 11/16] x --- internal/registry/factory_test.go | 109 +++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-) diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index 2071f387b2..ce03c15149 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -2,6 +2,7 @@ package registry import ( "context" + "encoding/json" "errors" "fmt" "math" @@ -34,7 +35,7 @@ type fakeExperimentConfig struct { Invisible int64 } -func TestExperimentBuilderOptions(t *testing.T) { +func TestFactoryOptions(t *testing.T) { t.Run("when config is not a pointer", func(t *testing.T) { b := &Factory{ config: 17, @@ -132,7 +133,7 @@ func TestExperimentBuilderOptions(t *testing.T) { }) } -func TestExperimentBuilderSetOptionAny(t *testing.T) { +func TestFactorySetOptionAny(t *testing.T) { var inputs = []struct { TestCaseName string InitialConfig any @@ -398,7 +399,7 @@ func TestExperimentBuilderSetOptionAny(t *testing.T) { } } -func TestExperimentBuilderSetOptionsAny(t *testing.T) { +func TestFactorySetOptionsAny(t *testing.T) { b := &Factory{config: &fakeExperimentConfig{}} t.Run("we correctly handle an empty map", func(t *testing.T) { @@ -441,6 +442,106 @@ 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) + } + + 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 { @@ -975,6 +1076,8 @@ 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) { From eed4d020fefa9d69856e01c08f926068d334834b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:44:55 +0400 Subject: [PATCH 12/16] x --- internal/registry/factory_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index ce03c15149..bd4feb84fc 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -528,6 +528,7 @@ func TestFactorySetOptionsJSON(t *testing.T) { 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() { From 0dd4e6735d56f37d26e286f15ab1f8f5b4b538ec Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 13:02:23 +0400 Subject: [PATCH 13/16] x --- internal/oonirun/experiment.go | 19 +++++---- internal/oonirun/experiment_test.go | 64 +++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index ddd1b36ca6..ff2af23c26 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -101,16 +101,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 } @@ -161,6 +154,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 e29cb0bf70..81fcdcba19 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" @@ -126,6 +128,68 @@ 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{ + "ReturnError": true, + "SleepTime": 500, + }, + InitialOptions: []byte(`{"Message": "foobar", "SleepTime": 100}`), + 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("foobar"), // set by InitialOptions + }, + "ReturnError": { + Doc: "Toogle to return a mocked error", + Type: "bool", + Value: bool(true), // set by ExtraOptions + }, + "SleepTime": { + Doc: "Amount of time to sleep for in nanosecond", + Type: "int64", + Value: int64(500), // set by InitialOptions, overriden by ExtraOptions + }, + } + + // 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 From 8f507ebb369cf6381911cc53fd2dfa95b7667ca4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 13:03:04 +0400 Subject: [PATCH 14/16] x --- internal/oonirun/experiment.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index ff2af23c26..1fe1c70795 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 From 168e9bfecf52a49bdf422f709b8348b2cec86d2d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 13:07:04 +0400 Subject: [PATCH 15/16] x --- internal/oonirun/experiment_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 81fcdcba19..cbe24d803b 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -134,10 +134,9 @@ func TestExperimentSetOptions(t *testing.T) { // create the Experiment we're using for this test exp := &Experiment{ ExtraOptions: map[string]any{ - "ReturnError": true, - "SleepTime": 500, + "Message": "jarjarbinks", }, - InitialOptions: []byte(`{"Message": "foobar", "SleepTime": 100}`), + InitialOptions: []byte(`{"Message": "foobar", "ReturnError": true}`), Name: "example", // TODO(bassosimone): A zero-value session works here. The proper change @@ -170,17 +169,17 @@ func TestExperimentSetOptions(t *testing.T) { "Message": { Doc: "Message to emit at test completion", Type: "string", - Value: string("foobar"), // set by InitialOptions + Value: string("jarjarbinks"), // set by ExtraOptions }, "ReturnError": { Doc: "Toogle to return a mocked error", Type: "bool", - Value: bool(true), // set by ExtraOptions + Value: bool(true), // set by InitialOptions }, "SleepTime": { Doc: "Amount of time to sleep for in nanosecond", Type: "int64", - Value: int64(500), // set by InitialOptions, overriden by ExtraOptions + Value: int64(1000000000), // still the default nonzero value }, } From bfc184f3674226650b9414e63df52e789a9ecb98 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 13:23:38 +0400 Subject: [PATCH 16/16] x --- internal/registry/factory_test.go | 49 ++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index bd4feb84fc..d4a3d8368c 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -21,21 +21,23 @@ import ( "github.com/ooni/probe-cli/v3/internal/targetloading" ) -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 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 + } + t.Run("when config is not a pointer", func(t *testing.T) { b := &Factory{ config: 17, @@ -134,6 +136,15 @@ func TestFactoryOptions(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 @@ -400,6 +411,16 @@ func TestFactorySetOptionAny(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) {