From f3c41bba67b0634a115d8e5faebe72e3c987f9ba Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 26 Jun 2024 07:02:59 +0200 Subject: [PATCH 01/30] 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 330777957..591197f0c 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 1f6a27187..a9cd880ba 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 5aada21ff..95848242c 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 c4613107d..a66093524 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 1fd38abb0..2fe651119 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 910d910b0..8ea9b0513 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 4330b07b7..6552ad582 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 d849c6d08..b77b9b7bf 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 ee73e95a8..9c5ffac30 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 8c6a857df..2d7c67f25 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 987e64093..bf4f71aed 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/30] 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 95848242c..ad4e0dfcf 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 ca1cdc7b18c48892d4dbd08b5d1bb9eea080dcae Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 26 Jun 2024 09:35:30 +0200 Subject: [PATCH 03/30] feat: correctly set options from richer input --- internal/engine/experiment.go | 9 +-- internal/experiment/dnscheck/dnscheck.go | 2 +- internal/experiment/dnscheck/dnscheck_test.go | 18 ++--- internal/experiment/dnscheck/richerinput.go | 18 +++-- .../experiment/dnscheck/richerinput_test.go | 30 ++++---- internal/experiment/openvpn/openvpn.go | 2 +- internal/experiment/openvpn/openvpn_test.go | 8 +- internal/experiment/openvpn/richerinput.go | 18 +++-- .../experiment/openvpn/richerinput_test.go | 4 +- internal/experimentconfig/experimentconfig.go | 76 +++++++++++++++++++ internal/model/experiment.go | 12 +++ internal/model/ooapi.go | 13 ++++ internal/oonirun/experiment.go | 8 +- internal/oonirun/inputprocessor.go | 6 +- internal/oonirun/inputprocessor_test.go | 4 - 15 files changed, 164 insertions(+), 64 deletions(-) create mode 100644 internal/experimentconfig/experimentconfig.go diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 5f6ac5d87..5061ec966 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -111,13 +111,10 @@ func (e *experiment) SubmitAndUpdateMeasurementContext( // newMeasurement creates a new measurement for this experiment with the given input. func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measurement { utctimenow := time.Now().UTC() - // TODO(bassosimone,DecFox): move here code that supports filling the options field - // when there is richer input, which currently is inside ./internal/oonirun. - // - // We MUST do this because the current solution only works for OONI Run and when - // there are command line options but does not work for API/static targets. + m := &model.Measurement{ DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion, + Options: target.Options(), Input: model.MeasurementInput(target.Input()), MeasurementStartTime: utctimenow.Format(model.MeasurementDateFormat), MeasurementStartTimeSaved: utctimenow, @@ -135,6 +132,7 @@ func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measur TestStartTime: e.testStartTime, TestVersion: e.testVersion, } + m.AddAnnotation("architecture", runtime.GOARCH) m.AddAnnotation("engine_name", "ooniprobe-engine") m.AddAnnotation("engine_version", version.Version) @@ -144,6 +142,7 @@ func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measur m.AddAnnotation("vcs_revision", runtimex.BuildInfo.VcsRevision) m.AddAnnotation("vcs_time", runtimex.BuildInfo.VcsTime) m.AddAnnotation("vcs_tool", runtimex.BuildInfo.VcsTool) + return m } diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index b5c1583b2..815ab2511 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -134,7 +134,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if !ok { return ErrInvalidInputType } - config, input := target.Options, target.URL + config, input := target.Config, target.URL sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input) // 1. fill the measurement with test keys diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index bf8f89555..c7055930c 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -72,7 +72,7 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) { Session: newsession(), Target: &Target{ URL: "", // explicitly empty - Options: &Config{ + Config: &Config{ Domain: "example.com", }, }, @@ -90,8 +90,8 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) { Measurement: &model.Measurement{Input: "Not a valid URL \x7f"}, Session: newsession(), Target: &Target{ - URL: "Not a valid URL \x7f", - Options: &Config{}, + URL: "Not a valid URL \x7f", + Config: &Config{}, }, } err := measurer.Run(context.Background(), args) @@ -107,8 +107,8 @@ func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) { Measurement: &model.Measurement{Input: "file://1.1.1.1"}, Session: newsession(), Target: &Target{ - URL: "file://1.1.1.1", - Options: &Config{}, + URL: "file://1.1.1.1", + Config: &Config{}, }, } err := measurer.Run(context.Background(), args) @@ -128,7 +128,7 @@ func TestWithCancelledContext(t *testing.T) { Session: newsession(), Target: &Target{ URL: "dot://one.one.one.one", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -191,7 +191,7 @@ func TestDNSCheckValid(t *testing.T) { Session: newsession(), Target: &Target{ URL: "dot://one.one.one.one:853", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -239,8 +239,8 @@ func TestDNSCheckWait(t *testing.T) { Measurement: &measurement, Session: newsession(), Target: &Target{ - URL: input, - Options: &Config{}, + URL: input, + Config: &Config{}, }, } err := measurer.Run(context.Background(), args) diff --git a/internal/experiment/dnscheck/richerinput.go b/internal/experiment/dnscheck/richerinput.go index 4d4a7ce52..f42d077e2 100644 --- a/internal/experiment/dnscheck/richerinput.go +++ b/internal/experiment/dnscheck/richerinput.go @@ -3,6 +3,7 @@ package dnscheck import ( "context" + "github.com/ooni/probe-cli/v3/internal/experimentconfig" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/reflectx" "github.com/ooni/probe-cli/v3/internal/targetloading" @@ -10,8 +11,8 @@ import ( // Target is a richer-input target that this experiment should measure. type Target struct { - // Options contains the configuration. - Options *Config + // Config contains the configuration. + Config *Config // URL is the input URL. URL string @@ -34,6 +35,11 @@ func (t *Target) Input() string { return t.URL } +// Options implements [model.ExperimentTarget]. +func (t *Target) Options() []string { + return experimentconfig.DefaultOptionsSerializer(t.Config) +} + // String implements [model.ExperimentTarget]. func (t *Target) String() string { return t.URL @@ -83,8 +89,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err var targets []model.ExperimentTarget for _, input := range inputs { targets = append(targets, &Target{ - Options: tl.options, - URL: input, + Config: tl.options, + URL: input, }) } return targets, nil @@ -100,14 +106,14 @@ var defaultInput = []model.ExperimentTarget{ // &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ HTTP3Enabled: true, DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, diff --git a/internal/experiment/dnscheck/richerinput_test.go b/internal/experiment/dnscheck/richerinput_test.go index 2d9e5dd53..7d5b70ed5 100644 --- a/internal/experiment/dnscheck/richerinput_test.go +++ b/internal/experiment/dnscheck/richerinput_test.go @@ -16,7 +16,7 @@ import ( func TestTarget(t *testing.T) { target := &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "8.8.8.8 8.8.4.4", Domain: "example.com", HTTP3Enabled: false, @@ -79,14 +79,14 @@ func TestNewLoader(t *testing.T) { var testDefaultInput = []model.ExperimentTarget{ &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ HTTP3Enabled: true, DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, &Target{ URL: "https://dns.google/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, @@ -136,25 +136,25 @@ func TestTargetLoaderLoad(t *testing.T) { expectTargets: []model.ExperimentTarget{ &Target{ URL: "https://dns.cloudflare.com/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, &Target{ URL: "https://one.one.one.one/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, &Target{ URL: "https://1dot1dot1dot1dot.com/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, &Target{ URL: "https://dns.cloudflare/dns-query", - Options: &Config{ + Config: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -202,12 +202,12 @@ func TestTargetLoaderLoad(t *testing.T) { expectErr: nil, expectTargets: []model.ExperimentTarget{ &Target{ - URL: "https://dns.cloudflare.com/dns-query", - Options: &Config{}, + URL: "https://dns.cloudflare.com/dns-query", + Config: &Config{}, }, &Target{ - URL: "https://one.one.one.one/dns-query", - Options: &Config{}, + URL: "https://one.one.one.one/dns-query", + Config: &Config{}, }, }, }, @@ -229,12 +229,12 @@ func TestTargetLoaderLoad(t *testing.T) { expectErr: nil, expectTargets: []model.ExperimentTarget{ &Target{ - URL: "https://1dot1dot1dot1dot.com/dns-query", - Options: &Config{}, + URL: "https://1dot1dot1dot1dot.com/dns-query", + Config: &Config{}, }, &Target{ - URL: "https://dns.cloudflare/dns-query", - Options: &Config{}, + URL: "https://dns.cloudflare/dns-query", + Config: &Config{}, }, }, }, diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index 0cf8255f4..23e904a9e 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -227,7 +227,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if !ok { return targetloading.ErrInvalidInputType } - config, input := target.Options, target.URL + config, input := target.Config, target.URL // 2. obtain the endpoint representation from the input URL endpoint, err := newEndpointFromInputString(input) diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index e45b52ae7..f050fab3a 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -202,8 +202,8 @@ func TestBadTargetURLFailure(t *testing.T) { Measurement: measurement, Session: sess, Target: &openvpn.Target{ - URL: "openvpn://badprovider/?address=aa", - Options: &openvpn.Config{}, + URL: "openvpn://badprovider/?address=aa", + Config: &openvpn.Config{}, }, } err := m.Run(ctx, args) @@ -260,8 +260,8 @@ func TestSuccess(t *testing.T) { Measurement: measurement, Session: sess, Target: &openvpn.Target{ - URL: "openvpn://riseupvpn.corp/?address=127.0.0.1:9989&transport=tcp", - Options: &openvpn.Config{}, + URL: "openvpn://riseupvpn.corp/?address=127.0.0.1:9989&transport=tcp", + Config: &openvpn.Config{}, }, } err := m.Run(ctx, args) diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go index 050673b35..5743865e9 100644 --- a/internal/experiment/openvpn/richerinput.go +++ b/internal/experiment/openvpn/richerinput.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/ooni/probe-cli/v3/internal/experimentconfig" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/reflectx" "github.com/ooni/probe-cli/v3/internal/targetloading" @@ -23,8 +24,8 @@ var providerAuthentication = map[string]AuthMethod{ // Target is a richer-input target that this experiment should measure. type Target struct { - // Options contains the configuration. - Options *Config + // Config contains the configuration. + Config *Config // URL is the input URL. URL string @@ -47,6 +48,11 @@ func (t *Target) Input() string { return t.URL } +// Options implements [model.ExperimentTarget]. +func (t *Target) Options() (options []string) { + return experimentconfig.DefaultOptionsSerializer(t.Config) +} + // String implements [model.ExperimentTarget]. func (t *Target) String() string { return t.URL @@ -96,8 +102,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err var targets []model.ExperimentTarget for _, input := range inputs { targets = append(targets, &Target{ - Options: tl.options, - URL: input, + Config: tl.options, + URL: input, }) } return targets, nil @@ -140,8 +146,8 @@ func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.Experiment // TODO(ainghazal): implement (surfshark, etc) } targets = append(targets, &Target{ - URL: input, - Options: config, + URL: input, + Config: config, }) } diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index 4c0469663..3d4d74ac3 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -16,7 +16,7 @@ import ( func TestTarget(t *testing.T) { target := &Target{ URL: "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp", - Options: &Config{ + Config: &Config{ Auth: "SHA512", Cipher: "AES-256-GCM", Provider: "unknown", @@ -112,7 +112,7 @@ func TestTargetLoaderLoad(t *testing.T) { expectTargets: []model.ExperimentTarget{ &Target{ URL: "openvpn://unknown.corp/1.1.1.1", - Options: &Config{ + Config: &Config{ Provider: "unknown", SafeCA: "aa", SafeCert: "bb", diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go new file mode 100644 index 000000000..503378142 --- /dev/null +++ b/internal/experimentconfig/experimentconfig.go @@ -0,0 +1,76 @@ +// Package experimentconfig contains code to manage experiments configuration. +package experimentconfig + +import ( + "fmt" + "reflect" + "strings" +) + +// TODO(bassosimone): we should probably move here all the code inside +// of registry used to serialize existing options and to set values from +// generic map[string]any types. + +// DefaultOptionsSerializer serializes options for [model.ExperimentTarget] +// honouring its Options method contract: +// +// 1. we do not serialize options whose name starts with "Safe"; +// +// 2. we do not serialize scalar values. +// +// This method MUST be passed a pointer to a struct. Otherwise, the return +// value will be a zero-length list (either nil or empty). +func DefaultOptionsSerializer(config any) (options []string) { + // as documented, this method MUST be passed a struct pointer + stval := reflect.ValueOf(config) + if stval.Kind() != reflect.Pointer { + return + } + stval = stval.Elem() + if stval.Kind() != reflect.Struct { + return + } + + // obtain the structure type + stt := stval.Type() + + // cycle through the struct fields + for idx := 0; idx < stval.NumField(); idx++ { + // obtain the field type and value + fieldval, fieldtype := stval.Field(idx), stt.Field(idx) + + // make sure the field is public + if !fieldtype.IsExported() { + continue + } + + // make sure the file name does not start with Safe + if strings.HasPrefix(fieldtype.Name, "Safe") { + continue + } + + // add the field iff it's a scalar + switch fieldval.Kind() { + case reflect.Bool, + reflect.Int, + reflect.Int8, + reflect.Int16, + reflect.Int32, + reflect.Int64, + reflect.Uint, + reflect.Uint8, + reflect.Uint16, + reflect.Uint32, + reflect.Uint64, + reflect.Float32, + reflect.Float64, + reflect.String: + options = append(options, fmt.Sprintf("%s=%s", fieldtype.Name, fieldval.Interface())) + + default: + // nothing + } + } + + return +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index ad4e0dfcf..666718c43 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -99,6 +99,18 @@ type ExperimentTarget interface { // Input returns the experiment input, which is typically a URL. Input() string + // Options transforms the options contained by this target + // into a []string containing options as they were provided + // using the command line `-O option=value` syntax. + // + // This method MUST NOT serialize all the options whose name + // starts with the "Safe" prefix. This method MAY skip serializing + // sensitive options and options we cannot serialize into a list + // of strings (e.g., objects and lists). + // + // Consider using the [experimentconfig] package to serialize. + Options() []string + // String MUST return the experiment input. // // Implementation note: previously existing code often times treated diff --git a/internal/model/ooapi.go b/internal/model/ooapi.go index 3750cece1..51cc49af5 100644 --- a/internal/model/ooapi.go +++ b/internal/model/ooapi.go @@ -224,6 +224,19 @@ func (o *OOAPIURLInfo) Input() string { return o.URL } +// Options implements ExperimentTarget. +func (o *OOAPIURLInfo) Options() []string { + // Implementation note: we're not serializing any options for now. If/when + // we do that, remember the Options contract: + // + // 1. skip options whose name begins with "Safe"; + // + // 2. skip options that are not scalars. + // + // Consider using the [experimentconfig] package to serialize. + return nil +} + // String implements [ExperimentTarget]. func (o *OOAPIURLInfo) String() string { return o.URL diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index a66093524..505c943fc 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -92,11 +92,6 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // 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 @@ -121,7 +116,7 @@ func (ed *Experiment) Run(ctx context.Context) error { // 4. randomize input, if needed if ed.Random { - // Note: since go1.20 the default random generated is random seeded + // Note: since go1.20 the default random generator is randomly seeded // // See https://tip.golang.org/doc/go1.20 rand.Shuffle(len(targetList), func(i, j int) { @@ -177,7 +172,6 @@ func (ed *Experiment) newInputProcessor(experiment model.Experiment, }, Inputs: inputList, MaxRuntime: time.Duration(ed.MaxRuntime) * time.Second, - Options: experimentOptionsToStringList(ed.ExtraOptions), Saver: NewInputProcessorSaverWrapper(saver), Submitter: &experimentSubmitterWrapper{ child: NewInputProcessorSubmitterWrapper(submitter), diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go index d4e292fd6..79ef4380e 100644 --- a/internal/oonirun/inputprocessor.go +++ b/internal/oonirun/inputprocessor.go @@ -55,9 +55,6 @@ type InputProcessor struct { // there will be no MaxRuntime limit. MaxRuntime time.Duration - // Options contains command line options for this experiment. - Options []string - // Saver is the code that will save measurement results // on persistent storage (e.g. the file system). Saver InputProcessorSaverWrapper @@ -144,9 +141,10 @@ func (ip *InputProcessor) run(ctx context.Context) (int, error) { return 0, err } meas.AddAnnotations(ip.Annotations) - meas.Options = ip.Options err = ip.Submitter.Submit(ctx, idx, meas) if err != nil { + // TODO(bassosimone): the default submitter used by + // miniooni ignores errors. Should we make it the default? return 0, err } // Note: must be after submission because submission modifies diff --git a/internal/oonirun/inputprocessor_test.go b/internal/oonirun/inputprocessor_test.go index a427b5126..b6f97f453 100644 --- a/internal/oonirun/inputprocessor_test.go +++ b/internal/oonirun/inputprocessor_test.go @@ -71,7 +71,6 @@ func TestInputProcessorSubmissionFailed(t *testing.T) { Inputs: []model.ExperimentTarget{ model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), }, - Options: []string{"fake=true"}, Submitter: NewInputProcessorSubmitterWrapper( &FakeInputProcessorSubmitter{Err: expected}, ), @@ -120,7 +119,6 @@ func TestInputProcessorSaveOnDiskFailed(t *testing.T) { Inputs: []model.ExperimentTarget{ model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), }, - Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper( &FakeInputProcessorSaver{Err: expected}, ), @@ -144,7 +142,6 @@ func TestInputProcessorGood(t *testing.T) { model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.slashdot.org/"), }, - Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper(saver), Submitter: NewInputProcessorSubmitterWrapper(submitter), } @@ -186,7 +183,6 @@ func TestInputProcessorMaxRuntime(t *testing.T) { model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.slashdot.org/"), }, MaxRuntime: 1 * time.Nanosecond, - Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper(saver), Submitter: NewInputProcessorSubmitterWrapper(submitter), } From 45a18b75a4a3b7163b664bbad1f067b4c09d6460 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 26 Jun 2024 09:38:40 +0200 Subject: [PATCH 04/30] x --- internal/experimentconfig/experimentconfig.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go index 503378142..eb0f9b91e 100644 --- a/internal/experimentconfig/experimentconfig.go +++ b/internal/experimentconfig/experimentconfig.go @@ -16,7 +16,7 @@ import ( // // 1. we do not serialize options whose name starts with "Safe"; // -// 2. we do not serialize scalar values. +// 2. we only serialize scalar values. // // This method MUST be passed a pointer to a struct. Otherwise, the return // value will be a zero-length list (either nil or empty). From 50d1909cd84f74d6a85660a5dde2687f98c6f0de Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:01:15 +0400 Subject: [PATCH 05/30] 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 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/model/experiment.go b/internal/model/experiment.go index ad4e0dfcf..b514df3de 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 9c5ffac30..5c5418325 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 c92f03157..998d10f1a 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 06/30] 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 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{ From c6dacf7b40e3430609271edcde6f3f8f972eddb4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:05:17 +0400 Subject: [PATCH 07/30] x --- internal/mocks/experimentbuilder.go | 1 - 1 file changed, 1 deletion(-) 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) } From 76216433a6c4c6291c60ab7840687c91a2a307cb Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:10:35 +0400 Subject: [PATCH 08/30] 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 a66093524..ddd1b36ca 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 2fe651119..e29cb0bf7 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 09/30] 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 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 { From d1f243db4b33af91eaa97ae81387156f9ce0a46c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:20:59 +0400 Subject: [PATCH 10/30] 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 5c5418325..879bfad47 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 11/30] 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 879bfad47..813e9f7a4 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 998d10f1a..bb089f7f5 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 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, From b8d219ba188ef8794431f0ecde21ed96c37d0d94 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:28:16 +0400 Subject: [PATCH 12/30] 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 bb089f7f5..2071f387b 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 13/30] 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 2071f387b..ce03c1514 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 14/30] 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 ce03c1514..bd4feb84f 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 15/30] 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 ddd1b36ca..ff2af23c2 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 e29cb0bf7..81fcdcba1 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 16/30] 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 ff2af23c2..1fe1c7079 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 17/30] 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 81fcdcba1..cbe24d803 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 18/30] 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 bd4feb84f..d4a3d8368 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) { From 43ef9bfa87e3b1704c77591c0ca54d16f367f637 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 14:46:28 +0400 Subject: [PATCH 19/30] x --- internal/engine/experiment.go | 4 + internal/engine/experiment_test.go | 86 +++++++++++++++++++ internal/experimentconfig/experimentconfig.go | 11 ++- internal/model/experiment.go | 5 +- 4 files changed, 100 insertions(+), 6 deletions(-) diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 5061ec966..42a237357 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -46,6 +46,10 @@ func (emr *experimentMutableReport) Get() (report probeservices.ReportChannel) { return } +// TODO(bassosimone,DecFox): it would be nice if `*experiment` depended +// on an interface rather than depending on the concrete session, because +// that will allow us to write tests using mocks much more easily. + // experiment implements [model.Experiment]. type experiment struct { byteCounter *bytecounter.Counter diff --git a/internal/engine/experiment_test.go b/internal/engine/experiment_test.go index ceab79d7b..c9be288df 100644 --- a/internal/engine/experiment_test.go +++ b/internal/engine/experiment_test.go @@ -1,9 +1,14 @@ package engine import ( + "sync" "testing" + "time" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/enginelocate" + "github.com/ooni/probe-cli/v3/internal/experiment/dnscheck" "github.com/ooni/probe-cli/v3/internal/experiment/example" "github.com/ooni/probe-cli/v3/internal/experiment/signal" "github.com/ooni/probe-cli/v3/internal/model" @@ -105,3 +110,84 @@ func TestExperimentMeasurementSummaryKeys(t *testing.T) { } }) } + +// This test ensures that (*experiment).newMeasurement is working as intended. +func TestExperimentNewMeasurement(t *testing.T) { + // create a session for testing that does not use the network at all + sess := newSessionForTestingNoLookups(t) + + // create a conventional time for starting the experiment + t0 := time.Date(2024, 6, 27, 10, 33, 0, 0, time.UTC) + + // create the experiment + exp := &experiment{ + byteCounter: bytecounter.New(), + callbacks: model.NewPrinterCallbacks(model.DiscardLogger), + measurer: &dnscheck.Measurer{}, + mrep: &experimentMutableReport{ + mu: sync.Mutex{}, + report: nil, + }, + session: sess, + testName: "dnscheck", + testStartTime: t0.Format(model.MeasurementDateFormat), + testVersion: "0.1.0", + } + + // create the richer input target + target := &dnscheck.Target{ + Config: &dnscheck.Config{ + DefaultAddrs: "8.8.8.8 2001:4860:4860::8888", + HTTP3Enabled: true, + }, + URL: "https://dns.google/dns-query", + } + + // create measurement + meas := exp.newMeasurement(target) + + // make sure the input is correctly serialized + t.Run("Input", func(t *testing.T) { + if meas.Input != "https://dns.google/dns-query" { + t.Fatal("unexpected meas.Input") + } + }) + + // make sure the options are correctly serialized + t.Run("Options", func(t *testing.T) { + expectOptions := []string{`DefaultAddrs=8.8.8.8 2001:4860:4860::8888`, `HTTP3Enabled=true`} + if diff := cmp.Diff(expectOptions, meas.Options); diff != "" { + t.Fatal(diff) + } + }) + + // make sure we've got the expected annotation keys + t.Run("Annotations", func(t *testing.T) { + const ( + expected = 1 << iota + got + ) + m := map[string]int{ + "architecture": expected, + "engine_name": expected, + "engine_version": expected, + "go_version": expected, + "platform": expected, + "vcs_modified": expected, + "vcs_revision": expected, + "vcs_time": expected, + "vcs_tool": expected, + } + for key := range meas.Annotations { + m[key] |= got + } + for key, value := range m { + if value != expected|got { + t.Fatal("expected", expected|got, "for", key, "got", value) + } + } + }) + + // TODO(bassosimone,DecFox): this is the correct place where to + // add more tests regarding how we create measurements. +} diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go index eb0f9b91e..a2e321381 100644 --- a/internal/experimentconfig/experimentconfig.go +++ b/internal/experimentconfig/experimentconfig.go @@ -16,7 +16,9 @@ import ( // // 1. we do not serialize options whose name starts with "Safe"; // -// 2. we only serialize scalar values. +// 2. we only serialize scalar values; +// +// 3. we never serialize any zero values. // // This method MUST be passed a pointer to a struct. Otherwise, the return // value will be a zero-length list (either nil or empty). @@ -49,7 +51,7 @@ func DefaultOptionsSerializer(config any) (options []string) { continue } - // add the field iff it's a scalar + // add the field iff it's a nonzero scalar switch fieldval.Kind() { case reflect.Bool, reflect.Int, @@ -65,7 +67,10 @@ func DefaultOptionsSerializer(config any) (options []string) { reflect.Float32, reflect.Float64, reflect.String: - options = append(options, fmt.Sprintf("%s=%s", fieldtype.Name, fieldval.Interface())) + if fieldval.IsZero() { + continue + } + options = append(options, fmt.Sprintf("%s=%v", fieldtype.Name, fieldval.Interface())) default: // nothing diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 202158a73..31b80f01f 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -104,9 +104,8 @@ type ExperimentTarget interface { // using the command line `-O option=value` syntax. // // This method MUST NOT serialize all the options whose name - // starts with the "Safe" prefix. This method MAY skip serializing - // sensitive options and options we cannot serialize into a list - // of strings (e.g., objects and lists). + // starts with the "Safe" prefix. This method MUST skip serializing + // sensitive options, non-scalar options, and zero value options. // // Consider using the [experimentconfig] package to serialize. Options() []string From 6ca260c883d8453480938071d6e6f2952d8e8a2e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 14:49:21 +0400 Subject: [PATCH 20/30] x --- internal/experiment/dnscheck/richerinput_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/internal/experiment/dnscheck/richerinput_test.go b/internal/experiment/dnscheck/richerinput_test.go index 7d5b70ed5..46f361cf9 100644 --- a/internal/experiment/dnscheck/richerinput_test.go +++ b/internal/experiment/dnscheck/richerinput_test.go @@ -44,6 +44,19 @@ func TestTarget(t *testing.T) { } }) + t.Run("Options", func(t *testing.T) { + expect := []string{ + "DefaultAddrs=8.8.8.8 8.8.4.4", + "Domain=example.com", + "HTTPHost=dns.google", + "TLSServerName=dns.google.com", + "TLSVersion=TLSv1.3", + } + if diff := cmp.Diff(expect, target.Options()); diff != "" { + t.Fatal(diff) + } + }) + t.Run("String", func(t *testing.T) { if target.String() != "https://dns.google/dns-query" { t.Fatal("invalid String") From 653e54b8583a59b65c836928a6856c7e376cc7a5 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 16:33:16 +0400 Subject: [PATCH 21/30] x --- internal/experiment/openvpn/richerinput_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index 3d4d74ac3..6c8097f1a 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -44,6 +44,17 @@ func TestTarget(t *testing.T) { } }) + t.Run("Options", func(t *testing.T) { + expect := []string{ + "Auth=SHA512", + "Cipher=AES-256-GCM", + "Provider=unknown", + } + if diff := cmp.Diff(expect, target.Options()); diff != "" { + t.Fatal(diff) + } + }) + t.Run("String", func(t *testing.T) { if target.String() != "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp" { t.Fatal("invalid String") From c5a70e8e458c1b67ee6da37ce71597792c1f3f80 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:05:40 +0400 Subject: [PATCH 22/30] x --- .../experimentconfig/experimentconfig_test.go | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 internal/experimentconfig/experimentconfig_test.go diff --git a/internal/experimentconfig/experimentconfig_test.go b/internal/experimentconfig/experimentconfig_test.go new file mode 100644 index 000000000..ad5090bcb --- /dev/null +++ b/internal/experimentconfig/experimentconfig_test.go @@ -0,0 +1,175 @@ +package experimentconfig + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestDefaultOptionsSerializer(t *testing.T) { + // configuration is the configuration we're testing the serialization of. + // + // Note that there's no `ooni:"..."` annotation here because we have changed + // our model in https://github.com/ooni/probe-cli/pull/1629, and now this kind + // of annotations are only command-line related. + type configuration struct { + // booleans + ValBool bool + + // integers + ValInt int + ValInt8 int8 + ValInt16 int16 + ValInt32 int32 + ValInt64 int64 + + // unsigned integers + ValUint uint + ValUint8 uint8 + ValUint16 uint16 + ValUint32 uint32 + ValUint64 uint64 + + // floats + ValFloat32 float32 + ValFloat64 float64 + + // strings + ValString string + + // unexported fields we should ignore + privateInt int + privateString string + privateList []int16 + + // safe fields we should ignore + SafeBool bool + SafeInt int + SafeString string + + // non-scalar fields we should ignore + NSList []int64 + NSMap map[string]string + } + + // testcase is a test case implemented by this function + type testcase struct { + // name is the test case name + name string + + // config is the config to transform into a list of options + config any + + // expectConfigType is an extra check to make sure we're actually + // passing the correct type for the config, which is here to ensure + // that, with a nil pointer to struct, we're not crashing. We need + // some extra case here because of how the Go type system work, + // and specifically we want to be sure we're passing an any containing + // a tuple like (type=*configuration,value=nil). + // + // See https://codefibershq.com/blog/golang-why-nil-is-not-always-nil + expectConfigType string + + // expect is the expected result + expect []string + } + + cases := []testcase{ + { + name: "we return a nil list for zero values", + expectConfigType: "*experimentconfig.configuration", + config: &configuration{}, + expect: nil, + }, + + { + name: "we return a nil list for non-pointers", + expectConfigType: "experimentconfig.configuration", + config: configuration{}, + expect: nil, + }, + + { + name: "we return a nil list for non-struct pointers", + expectConfigType: "*int64", + config: func() *int64 { + v := int64(12345) + return &v + }(), + expect: nil, + }, + + { + name: "we return a nil list for a nil struct pointer", + expectConfigType: "*experimentconfig.configuration", + config: func() *configuration { + return (*configuration)(nil) + }(), + expect: nil, + }, + + { + name: "we only serialize the fields that should be exported", + expectConfigType: "*experimentconfig.configuration", + config: &configuration{ + ValBool: true, + ValInt: 1, + ValInt8: 2, + ValInt16: 3, + ValInt32: 4, + ValInt64: 5, + ValUint: 6, + ValUint8: 7, + ValUint16: 8, + ValUint32: 9, + ValUint64: 10, + ValFloat32: 11, + ValFloat64: 12, + ValString: "tredici", + privateInt: 14, + privateString: "quindici", + privateList: []int16{16}, + SafeBool: true, + SafeInt: 18, + SafeString: "diciannove", + NSList: []int64{20}, + NSMap: map[string]string{"21": "22"}, + }, + expect: []string{ + "ValBool=true", + "ValInt=1", + "ValInt8=2", + "ValInt16=3", + "ValInt32=4", + "ValInt64=5", + "ValUint=6", + "ValUint8=7", + "ValUint16=8", + "ValUint32=9", + "ValUint64=10", + "ValFloat32=11", + "ValFloat64=12", + "ValString=tredici", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // first make sure that tc.config has really the expected + // type for the reason explained in its docstring + if actual := fmt.Sprintf("%T", tc.config); actual != tc.expectConfigType { + t.Fatal("expected", tc.expectConfigType, "got", actual) + } + + // then serialize the content of the config to a list of strings + got := DefaultOptionsSerializer(tc.config) + + // finally, make sure that the result matches expectations + if diff := cmp.Diff(tc.expect, got); diff != "" { + t.Fatal(diff) + } + }) + } +} From 7f1a0c7e2266d100036fd212c6514ed67615433d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:07:29 +0400 Subject: [PATCH 23/30] x --- internal/experimentconfig/experimentconfig.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go index a2e321381..617429100 100644 --- a/internal/experimentconfig/experimentconfig.go +++ b/internal/experimentconfig/experimentconfig.go @@ -24,6 +24,9 @@ import ( // value will be a zero-length list (either nil or empty). func DefaultOptionsSerializer(config any) (options []string) { // as documented, this method MUST be passed a struct pointer + // + // Implementation note: the .Elem method converts a nil + // pointer to a zero value pointee type. stval := reflect.ValueOf(config) if stval.Kind() != reflect.Pointer { return From bae507658c7fdc32f3bbc5c675230ffc25f32c6a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:25:01 +0400 Subject: [PATCH 24/30] x --- internal/experimentconfig/experimentconfig.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go index 617429100..58932b0e9 100644 --- a/internal/experimentconfig/experimentconfig.go +++ b/internal/experimentconfig/experimentconfig.go @@ -26,7 +26,7 @@ func DefaultOptionsSerializer(config any) (options []string) { // as documented, this method MUST be passed a struct pointer // // Implementation note: the .Elem method converts a nil - // pointer to a zero value pointee type. + // pointer to a zero-value pointee type. stval := reflect.ValueOf(config) if stval.Kind() != reflect.Pointer { return @@ -49,7 +49,7 @@ func DefaultOptionsSerializer(config any) (options []string) { continue } - // make sure the file name does not start with Safe + // make sure the field name does not start with "Safe" if strings.HasPrefix(fieldtype.Name, "Safe") { continue } From 9eeb1aaf2e30e12da23957431712c943102087c8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:29:25 +0400 Subject: [PATCH 25/30] x --- internal/model/ooapi_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/model/ooapi_test.go b/internal/model/ooapi_test.go index aee509e85..a02958183 100644 --- a/internal/model/ooapi_test.go +++ b/internal/model/ooapi_test.go @@ -128,6 +128,10 @@ func TestOOAPIURLInfo(t *testing.T) { t.Fatal("invalid Input") } + if info.Options() != nil { + t.Fatal("invalid Options") + } + if info.String() != "https://www.facebook.com/" { t.Fatal("invalid String") } From 0005f1ec9d42e6c4d3b7783773d1058ed19fac1b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:31:48 +0400 Subject: [PATCH 26/30] x --- internal/model/ooapi.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/model/ooapi.go b/internal/model/ooapi.go index 51cc49af5..8a0449568 100644 --- a/internal/model/ooapi.go +++ b/internal/model/ooapi.go @@ -231,7 +231,9 @@ func (o *OOAPIURLInfo) Options() []string { // // 1. skip options whose name begins with "Safe"; // - // 2. skip options that are not scalars. + // 2. skip options that are not scalars; + // + // 3. avoid serializing zero values. // // Consider using the [experimentconfig] package to serialize. return nil From 620d4f9605dcd7976e9e2c7c26d776f6f61d8894 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:39:54 +0400 Subject: [PATCH 27/30] x --- internal/oonirun/experiment.go | 18 ------------ internal/oonirun/experiment_test.go | 44 ----------------------------- internal/oonirun/inputprocessor.go | 7 +++-- 3 files changed, 5 insertions(+), 64 deletions(-) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 329b57492..0435b21d1 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -7,9 +7,7 @@ package oonirun import ( "context" "encoding/json" - "fmt" "math/rand" - "strings" "sync/atomic" "time" @@ -235,22 +233,6 @@ func (ed *Experiment) newTargetLoader(builder model.ExperimentBuilder) targetLoa }) } -// experimentOptionsToStringList convers the options to []string, which is -// the format with which we include them into a OONI Measurement. The resulting -// []string will skip any option that is named with a `Safe` prefix (case -// sensitive). -func experimentOptionsToStringList(options map[string]any) (out []string) { - // the prefix to skip inclusion in the string list - safeOptionPrefix := "Safe" - for key, value := range options { - if strings.HasPrefix(key, safeOptionPrefix) { - continue - } - out = append(out, fmt.Sprintf("%s=%v", key, value)) - } - return -} - // experimentWrapper wraps an experiment and logs progress type experimentWrapper struct { // child is the child experiment wrapper diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index cbe24d803..c840d7a17 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -4,8 +4,6 @@ import ( "context" "encoding/json" "errors" - "reflect" - "sort" "testing" "time" @@ -189,48 +187,6 @@ func TestExperimentSetOptions(t *testing.T) { } } -func Test_experimentOptionsToStringList(t *testing.T) { - type args struct { - options map[string]any - } - tests := []struct { - name string - args args - wantOut []string - }{ - { - name: "happy path: a map with three entries returns three items", - args: args{ - map[string]any{ - "foo": 1, - "bar": 2, - "baaz": 3, - }, - }, - wantOut: []string{"baaz=3", "bar=2", "foo=1"}, - }, - { - name: "an option beginning with `Safe` is skipped from the output", - args: args{ - map[string]any{ - "foo": 1, - "Safefoo": 42, - }, - }, - wantOut: []string{"foo=1"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotOut := experimentOptionsToStringList(tt.args.options) - sort.Strings(gotOut) - if !reflect.DeepEqual(gotOut, tt.wantOut) { - t.Errorf("experimentOptionsToStringList() = %v, want %v", gotOut, tt.wantOut) - } - }) - } -} - func TestExperimentRun(t *testing.T) { errMocked := errors.New("mocked error") type fields struct { diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go index 79ef4380e..e8f86e958 100644 --- a/internal/oonirun/inputprocessor.go +++ b/internal/oonirun/inputprocessor.go @@ -143,8 +143,11 @@ func (ip *InputProcessor) run(ctx context.Context) (int, error) { meas.AddAnnotations(ip.Annotations) err = ip.Submitter.Submit(ctx, idx, meas) if err != nil { - // TODO(bassosimone): the default submitter used by - // miniooni ignores errors. Should we make it the default? + // TODO(bassosimone): when re-reading this code, I find it confusing that + // we return on error because I am always like "wait, this is not the right + // thing to do here". Then, I remember that the experimentSubmitterWrapper + // ignores this error and so it's like it does not exist. Maybe we should + // rewrite the code to do the right thing here. return 0, err } // Note: must be after submission because submission modifies From 7b8200083dff0793e2d0f187be8cebf8efdc3191 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 17:52:42 +0400 Subject: [PATCH 28/30] x --- internal/oonirun/inputprocessor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go index e8f86e958..57b4f4489 100644 --- a/internal/oonirun/inputprocessor.go +++ b/internal/oonirun/inputprocessor.go @@ -145,9 +145,9 @@ func (ip *InputProcessor) run(ctx context.Context) (int, error) { if err != nil { // TODO(bassosimone): when re-reading this code, I find it confusing that // we return on error because I am always like "wait, this is not the right - // thing to do here". Then, I remember that the experimentSubmitterWrapper + // thing to do here". Then, I remember that the experimentSubmitterWrapper{} // ignores this error and so it's like it does not exist. Maybe we should - // rewrite the code to do the right thing here. + // rewrite the code to do the right thing here 😬😬😬. return 0, err } // Note: must be after submission because submission modifies From ddf00a7f1e83c9df8d045ee69ba8f88a92bb7770 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 18:10:08 +0400 Subject: [PATCH 29/30] x --- internal/oonirun/inputprocessor_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/oonirun/inputprocessor_test.go b/internal/oonirun/inputprocessor_test.go index b6f97f453..1c8ee6ca6 100644 --- a/internal/oonirun/inputprocessor_test.go +++ b/internal/oonirun/inputprocessor_test.go @@ -95,9 +95,6 @@ func TestInputProcessorSubmissionFailed(t *testing.T) { if m.Annotations["antani"] != "antani" { t.Fatal("invalid annotation: antani") } - if len(m.Options) != 1 || m.Options[0] != "fake=true" { - t.Fatal("options not set") - } } type FakeInputProcessorSaver struct { From 418ebbda37ed772f2a1137722dcb7f26724c6043 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 28 Jun 2024 07:59:46 +0400 Subject: [PATCH 30/30] chore: update the living design document --- docs/design/dd-008-richer-input.md | 93 +++++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 9 deletions(-) diff --git a/docs/design/dd-008-richer-input.md b/docs/design/dd-008-richer-input.md index 7927d7974..730616747 100644 --- a/docs/design/dd-008-richer-input.md +++ b/docs/design/dd-008-richer-input.md @@ -199,6 +199,7 @@ type ExperimentTarget struct { Category() string // equivalent to OOAPIURLInfo.CategoryCode Country() string // equivalent to OOAPIURLInfo.CountryCode Input() string // equivalent to OOAPIURLInfo.URL + String() string // serializes to the input } type InputLoader = TargetLoader // we renamed to reflect the change of purpose @@ -212,6 +213,10 @@ type Experiment interface { } ``` +The `String` method is used to reduce the `ExperimentTarget` to the input string, which +allows for backwards compatibility. We can obtain a string representation of the target's +input and use it every time where previous we used the `input` string. + Note that we also renamed the `InputLoader` to `TargetLoader` to reflect the fact that we're not loading bare input anymore, rather we're loading richer input targets. @@ -361,14 +366,90 @@ wouldn't be able to cast such an interface to the `*target` type. Therefore, unconditionally casting could lead to crashes when integrating new code and generally makes for a less robust codebase. +## Implementation: add OpenVPN + +Pull request [#1625](https://github.com/ooni/probe-cli/pull/1625) added richer +input support for the `openvpn` experiment. Because this experiment already +supports richer input through the `api.dev.ooni.io` backend, we now have the +first experiment capable of using richer input. + +## Implementation: fix serializing options + +Pull request [#1630](https://github.com/ooni/probe-cli/pull/1630) adds +support for correctly serializing options. We extend the model of a richer +input target to include the following function: + +```Go +type ExperimentTarget struct { + // ... + Options() []string +} +``` + +Then we implement `Options` for every possible experiment target. There is +a default implementation in the `experimentconfig` package implementing the +default semantics that was also available before: + +1. skip fields whose name starts with `Safe`; + +2. only serialize scalar values; + +3. do not serializes any zero value. + +Additionally, we now serialize the options inside the `newMeasurement` +constructor typical of each experiment. + +## Implementation: improve passing options to experiments + +Pull request [#1629](https://github.com/ooni/probe-cli/pull/1629) modifies +the way in which the `./internal/oonirun` package loads data for experiments +such that, when using OONI Run v2, we load its `options` field as a +`json.RawMessage` rather than using a `map[string]any`. This fact is +significant because, previously, we could only unmarshal options provided +by command line, which were always scalar. With this change, instead, we +can keep backwards compatibility with respect to the command line but it's +now also possible for experiments options specified via OONI Run v2 to +provide non-scalar options. + +The key change to enable this is to modify a `*registry.Factory` type to add: + +```Go +type Factory struct { /* ... */ } + +func (fx *Factory) SetOptionsJSON(value json.RawMessage) error +``` + +In this way, we can directly assign the raw JSON to the experiment config +that is kept inside of the `*Factory` itself. + +Additionally, constructing an experiment using `*oonirun.Experiment` now +includes two options related field: + +```Go +type Experiment struct { + InitialOptions json.RawMessage // new addition + ExtraOptions map[string]any // also present before +} +``` + +Initialization of experiment options will work as follows: + +1. the per-experiment `*Factory` constructor initializes fields to their +default value, which, in most cases, SHOULD be the zero value; + +2. we update the config using `InitialOptions` unless it is empty; + +3. we update the config using `ExtraOptions` unless it is empty. + +In practice, the code would always use either `InitialOptions` or +`ExtraOptions`, but we also wanted to specify priority in case both +of them were available. + ## Next steps This is a rough sequence of next steps that we should expand as we implement additional bits of richer input and for which we need reference issues. -* setting `(*Measurement).Options` inside `(*engine.experiment).newMeasurement` -rather than inside the `oonirun` package. - * fully convert `dnscheck`'s static list to live inside `dnscheck` instead of `targetloading` and to use the proper richer input. @@ -378,15 +459,9 @@ rather than inside the `oonirun` package. * implement backend API for serving `stunreachability` richer input. - * implement backend API for serving `openvpn` richer input. - * deliver feature flags using experiment-specific richer input rather than using the check-in API (and maybe keep the caching support?). -* deliver OONI Run v2 options as a `json.RawMessage` rather than passing -through a `map[string]any` such that we can support non-scalar options when -using OONI Run v2. - * try to eliminate `InputPolicy` and instead have each experiment define its own constructor for the proper target loader, and split the implementation inside of the `targetloader` package to have multiple target loaders.