From 4ea29ff336e489eda529744a9f1eeabef56c45ec Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 13:51:37 +0200 Subject: [PATCH 01/18] refactor: make TargetLoader using ExperimentBuilder This diff completes the set of preliminary richer input diffs. We build the TargetLoader using the ExperimentBuilder, which in turn uses a registry.Factory under the hood. This means that we can load targets for each experiment. Part of https://github.com/ooni/probe/issues/2607 --- cmd/ooniprobe/internal/nettests/dnscheck.go | 16 ++++---- .../internal/nettests/stunreachability.go | 16 ++++---- .../internal/nettests/web_connectivity.go | 21 +++++----- internal/engine/experimentbuilder.go | 7 ++++ internal/mocks/experimentbuilder.go | 8 ++++ internal/model/experiment.go | 40 ++++++++++++++++++- internal/oonirun/experiment.go | 21 +++++----- internal/oonirun/experiment_test.go | 12 +++--- internal/registry/dash.go | 4 +- internal/registry/dnscheck.go | 4 +- internal/registry/dnsping.go | 4 +- internal/registry/dslxtutorial.go | 8 ++-- internal/registry/echcheck.go | 8 ++-- internal/registry/example.go | 4 +- internal/registry/factory.go | 20 ++++++++++ internal/registry/fbmessenger.go | 4 +- internal/registry/hhfm.go | 4 +- internal/registry/hirl.go | 4 +- internal/registry/httphostheader.go | 4 +- internal/registry/ndt.go | 4 +- internal/registry/openvpn.go | 4 +- internal/registry/portfiltering.go | 4 +- internal/registry/psiphon.go | 4 +- internal/registry/quicping.go | 4 +- internal/registry/riseupvpn.go | 8 ++-- internal/registry/signal.go | 4 +- internal/registry/simplequicping.go | 4 +- internal/registry/sniblocking.go | 4 +- internal/registry/stunreachability.go | 4 +- internal/registry/tcpping.go | 4 +- internal/registry/telegram.go | 4 +- internal/registry/tlsmiddlebox.go | 4 +- internal/registry/tlsping.go | 4 +- internal/registry/tlstool.go | 4 +- internal/registry/tor.go | 4 +- internal/registry/torsf.go | 4 +- internal/registry/urlgetter.go | 4 +- internal/registry/vanillator.go | 6 ++- internal/registry/webconnectivity.go | 4 +- internal/registry/webconnectivityv05.go | 4 +- internal/registry/whatsapp.go | 4 +- internal/targetloading/targetloading.go | 13 ++---- internal/targetloading/targetloading_test.go | 11 ++++- 43 files changed, 229 insertions(+), 98 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/dnscheck.go b/cmd/ooniprobe/internal/nettests/dnscheck.go index cb53560155..2c8e798622 100644 --- a/cmd/ooniprobe/internal/nettests/dnscheck.go +++ b/cmd/ooniprobe/internal/nettests/dnscheck.go @@ -4,23 +4,21 @@ import ( "context" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/targetloading" ) // DNSCheck nettest implementation. type DNSCheck struct{} -func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - targetloader := &targetloading.Loader{ +func (n DNSCheck) lookupURLs(ctl *Controller, builder model.ExperimentBuilder) ([]model.ExperimentTarget, error) { + config := &model.ExperimentTargetLoaderConfig{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, - ExperimentName: "dnscheck", - InputPolicy: model.InputOrStaticDefault, - Session: ctl.Session, - SourceFiles: ctl.InputFiles, - StaticInputs: ctl.Inputs, + Session: ctl.Session, + SourceFiles: ctl.InputFiles, + StaticInputs: ctl.Inputs, } + targetloader := builder.NewTargetLoader(config) testlist, err := targetloader.Load(context.Background()) if err != nil { return nil, err @@ -34,7 +32,7 @@ func (n DNSCheck) Run(ctl *Controller) error { if err != nil { return err } - urls, err := n.lookupURLs(ctl) + urls, err := n.lookupURLs(ctl, builder) if err != nil { return err } diff --git a/cmd/ooniprobe/internal/nettests/stunreachability.go b/cmd/ooniprobe/internal/nettests/stunreachability.go index 743d53e8f2..c6313d96c8 100644 --- a/cmd/ooniprobe/internal/nettests/stunreachability.go +++ b/cmd/ooniprobe/internal/nettests/stunreachability.go @@ -4,23 +4,21 @@ import ( "context" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/targetloading" ) // STUNReachability nettest implementation. type STUNReachability struct{} -func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - targetloader := &targetloading.Loader{ +func (n STUNReachability) lookupURLs(ctl *Controller, builder model.ExperimentBuilder) ([]model.ExperimentTarget, error) { + config := &model.ExperimentTargetLoaderConfig{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, - ExperimentName: "stunreachability", - InputPolicy: model.InputOrStaticDefault, - Session: ctl.Session, - SourceFiles: ctl.InputFiles, - StaticInputs: ctl.Inputs, + Session: ctl.Session, + SourceFiles: ctl.InputFiles, + StaticInputs: ctl.Inputs, } + targetloader := builder.NewTargetLoader(config) testlist, err := targetloader.Load(context.Background()) if err != nil { return nil, err @@ -34,7 +32,7 @@ func (n STUNReachability) Run(ctl *Controller) error { if err != nil { return err } - urls, err := n.lookupURLs(ctl) + urls, err := n.lookupURLs(ctl, builder) if err != nil { return err } diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index 7e4bcad996..0da3be56d1 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -5,11 +5,11 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/targetloading" ) -func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) { - targetloader := &targetloading.Loader{ +func (n WebConnectivity) lookupURLs( + ctl *Controller, builder model.ExperimentBuilder, categories []string) ([]model.ExperimentTarget, error) { + config := &model.ExperimentTargetLoaderConfig{ CheckInConfig: &model.OOAPICheckInConfig{ // Setting Charging and OnWiFi to true causes the CheckIn // API to return to us as much URL as possible with the @@ -21,12 +21,11 @@ func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]mod CategoryCodes: categories, }, }, - ExperimentName: "web_connectivity", - InputPolicy: model.InputOrQueryBackend, - Session: ctl.Session, - SourceFiles: ctl.InputFiles, - StaticInputs: ctl.Inputs, + Session: ctl.Session, + SourceFiles: ctl.InputFiles, + StaticInputs: ctl.Inputs, } + targetloader := builder.NewTargetLoader(config) testlist, err := targetloader.Load(context.Background()) if err != nil { return nil, err @@ -39,12 +38,12 @@ type WebConnectivity struct{} // Run starts the test func (n WebConnectivity) Run(ctl *Controller) error { - log.Debugf("Enabled category codes are the following %v", ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes) - urls, err := n.lookupURLs(ctl, ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes) + builder, err := ctl.Session.NewExperimentBuilder("web_connectivity") if err != nil { return err } - builder, err := ctl.Session.NewExperimentBuilder("web_connectivity") + log.Debugf("Enabled category codes are the following %v", ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes) + urls, err := n.lookupURLs(ctl, builder, ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes) if err != nil { return err } diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index a60d0a50d5..330777957b 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -22,6 +22,8 @@ type experimentBuilder struct { session *Session } +var _ model.ExperimentBuilder = &experimentBuilder{} + // Interruptible implements ExperimentBuilder.Interruptible. func (b *experimentBuilder) Interruptible() bool { return b.factory.Interruptible() @@ -60,6 +62,11 @@ func (b *experimentBuilder) NewExperiment() model.Experiment { return experiment } +// NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. +func (b *experimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return b.factory.NewTargetLoader(config) +} + // newExperimentBuilder creates a new experimentBuilder instance. func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) { factory, err := registry.NewFactory(name, session.kvStore, session.logger) diff --git a/internal/mocks/experimentbuilder.go b/internal/mocks/experimentbuilder.go index 16763d471d..1f6a27187f 100644 --- a/internal/mocks/experimentbuilder.go +++ b/internal/mocks/experimentbuilder.go @@ -17,8 +17,12 @@ type ExperimentBuilder struct { MockSetCallbacks func(callbacks model.ExperimentCallbacks) MockNewExperiment func() model.Experiment + + MockNewTargetLoader func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader } +var _ model.ExperimentBuilder = &ExperimentBuilder{} + func (eb *ExperimentBuilder) Interruptible() bool { return eb.MockInterruptible() } @@ -46,3 +50,7 @@ func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { func (eb *ExperimentBuilder) NewExperiment() model.Experiment { return eb.MockNewExperiment() } + +func (eb *ExperimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return eb.MockNewTargetLoader(config) +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 9ae3dbcce6..8bd309f326 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -232,8 +232,46 @@ type ExperimentBuilder interface { // SetCallbacks sets the experiment's interactive callbacks. SetCallbacks(callbacks ExperimentCallbacks) - // NewExperiment creates the experiment instance. + // NewExperiment creates the [Experiment] instance. NewExperiment() Experiment + + // NewTargetLoader creates the [ExperimentTargetLoader] instance. + NewTargetLoader(config *ExperimentTargetLoaderConfig) ExperimentTargetLoader +} + +// ExperimentTargetLoaderConfig is the configuration to create a new [ExperimentTargetLoader]. +// +// The zero value is not ready to use; please, init the MANDATORY fields. +type ExperimentTargetLoaderConfig struct { + // CheckInConfig contains OPTIONAL options for the CheckIn API. If not set, then we'll create a + // default config. If set but there are fields inside it that are not set, then we will set them + // to a default value. + CheckInConfig *OOAPICheckInConfig + + // Session is the MANDATORY current measurement session. + Session ExperimentTargetLoaderSession + + // StaticInputs contains OPTIONAL input to be added + // to the resulting input list if possible. + StaticInputs []string + + // SourceFiles contains OPTIONAL files to read input + // from. Each file should contain a single input string + // per line. We will fail if any file is unreadable + // as well as if any file is empty. + SourceFiles []string +} + +// ExperimentTargetLoaderSession is the session according to [ExperimentTargetLoader]. +type ExperimentTargetLoaderSession interface { + // CheckIn invokes the check-in API. + CheckIn(ctx context.Context, config *OOAPICheckInConfig) (*OOAPICheckInResult, error) + + // FetchOpenVPNConfig fetches the OpenVPN experiment configuration. + FetchOpenVPNConfig(ctx context.Context, provider, cc string) (*OOAPIVPNProviderConfig, error) + + // Logger returns the logger to use. + Logger() Logger } // ExperimentOptionInfo contains info about an experiment option. diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 09deedaedc..758db9e0c5 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -14,7 +14,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/humanize" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/targetloading" ) // experimentShuffledInputs counts how many times we shuffled inputs @@ -61,7 +60,7 @@ type Experiment struct { newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error) // newTargetLoaderFn is OPTIONAL and used for testing. - newTargetLoaderFn func(inputPolicy model.InputPolicy) targetLoader + newTargetLoaderFn func(builder model.ExperimentBuilder) targetLoader // newSubmitterFn is OPTIONAL and used for testing. newSubmitterFn func(ctx context.Context) (model.Submitter, error) @@ -84,7 +83,7 @@ func (ed *Experiment) Run(ctx context.Context) error { } // 2. create input loader and load input for this experiment - targetLoader := ed.newTargetLoader(builder.InputPolicy()) + targetLoader := ed.newTargetLoader(builder) targetList, err := targetLoader.Load(ctx) if err != nil { return err @@ -196,22 +195,20 @@ func (ed *Experiment) newExperimentBuilder(experimentName string) (model.Experim type targetLoader = model.ExperimentTargetLoader // newTargetLoader creates a new [model.ExperimentTargetLoader]. -func (ed *Experiment) newTargetLoader(inputPolicy model.InputPolicy) targetLoader { +func (ed *Experiment) newTargetLoader(builder model.ExperimentBuilder) targetLoader { if ed.newTargetLoaderFn != nil { - return ed.newTargetLoaderFn(inputPolicy) + return ed.newTargetLoaderFn(builder) } - return &targetloading.Loader{ + return builder.NewTargetLoader(&model.ExperimentTargetLoaderConfig{ CheckInConfig: &model.OOAPICheckInConfig{ RunType: model.RunTypeManual, OnWiFi: true, // meaning: not on 4G Charging: true, }, - ExperimentName: ed.Name, - InputPolicy: inputPolicy, - StaticInputs: ed.Inputs, - SourceFiles: ed.InputFilePaths, - Session: ed.Session, - } + StaticInputs: ed.Inputs, + SourceFiles: ed.InputFilePaths, + Session: ed.Session, + }) } // experimentOptionsToStringList convers the options to []string, which is diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 29764d9c8c..b93fd25ae2 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -165,7 +165,7 @@ func TestExperimentRun(t *testing.T) { ReportFile string Session Session newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error) - newTargetLoaderFn func(inputPolicy model.InputPolicy) targetLoader + newTargetLoaderFn func(builder model.ExperimentBuilder) targetLoader newSubmitterFn func(ctx context.Context) (model.Submitter, error) newSaverFn func() (model.Saver, error) newInputProcessorFn func(experiment model.Experiment, @@ -199,7 +199,7 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return nil, errMocked @@ -223,7 +223,7 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil @@ -263,7 +263,7 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil @@ -306,7 +306,7 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil @@ -352,7 +352,7 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil diff --git a/internal/registry/dash.go b/internal/registry/dash.go index 812619361b..e8a2ad1165 100644 --- a/internal/registry/dash.go +++ b/internal/registry/dash.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["dash"] = func() *Factory { + const canonicalName = "dash" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return dash.NewExperimentMeasurer( *config.(*dash.Config), ) }, + canonicalName: canonicalName, config: &dash.Config{}, enabledByDefault: true, interruptible: true, diff --git a/internal/registry/dnscheck.go b/internal/registry/dnscheck.go index b9355773fc..2890ca19a5 100644 --- a/internal/registry/dnscheck.go +++ b/internal/registry/dnscheck.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["dnscheck"] = func() *Factory { + const canonicalName = "dnscheck" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return dnscheck.NewExperimentMeasurer( *config.(*dnscheck.Config), ) }, + canonicalName: canonicalName, config: &dnscheck.Config{}, enabledByDefault: true, inputPolicy: model.InputOrStaticDefault, diff --git a/internal/registry/dnsping.go b/internal/registry/dnsping.go index 92d52456ed..fb01d1ab80 100644 --- a/internal/registry/dnsping.go +++ b/internal/registry/dnsping.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["dnsping"] = func() *Factory { + const canonicalName = "dnsping" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return dnsping.NewExperimentMeasurer( *config.(*dnsping.Config), ) }, + canonicalName: canonicalName, config: &dnsping.Config{}, enabledByDefault: true, inputPolicy: model.InputOrStaticDefault, diff --git a/internal/registry/dslxtutorial.go b/internal/registry/dslxtutorial.go index 41aef7d40c..199e36077c 100644 --- a/internal/registry/dslxtutorial.go +++ b/internal/registry/dslxtutorial.go @@ -10,15 +10,17 @@ import ( ) func init() { - AllExperiments["simple_sni"] = func() *Factory { + const canonicalName = "simple_sni" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return chapter02.NewExperimentMeasurer( *config.(*chapter02.Config), ) }, - config: &chapter02.Config{}, - inputPolicy: model.InputOrQueryBackend, + canonicalName: canonicalName, + config: &chapter02.Config{}, + inputPolicy: model.InputOrQueryBackend, } } } diff --git a/internal/registry/echcheck.go b/internal/registry/echcheck.go index b091d58af9..939cf3d15d 100644 --- a/internal/registry/echcheck.go +++ b/internal/registry/echcheck.go @@ -10,15 +10,17 @@ import ( ) func init() { - AllExperiments["echcheck"] = func() *Factory { + const canonicalName = "echcheck" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return echcheck.NewExperimentMeasurer( *config.(*echcheck.Config), ) }, - config: &echcheck.Config{}, - inputPolicy: model.InputOptional, + canonicalName: canonicalName, + config: &echcheck.Config{}, + inputPolicy: model.InputOptional, } } } diff --git a/internal/registry/example.go b/internal/registry/example.go index aa2d13ee11..94c55ca59f 100644 --- a/internal/registry/example.go +++ b/internal/registry/example.go @@ -12,13 +12,15 @@ import ( ) func init() { - AllExperiments["example"] = func() *Factory { + const canonicalName = "example" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return example.NewExperimentMeasurer( *config.(*example.Config), "example", ) }, + canonicalName: canonicalName, config: &example.Config{ Message: "Good day from the example experiment!", SleepTime: int64(time.Second), diff --git a/internal/registry/factory.go b/internal/registry/factory.go index f0baa34806..aba0946d64 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -15,6 +15,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/checkincache" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/strcasex" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // Factory allows to construct an experiment measurer. @@ -22,6 +23,9 @@ type Factory struct { // build is the constructor that build an experiment with the given config. build func(config interface{}) model.ExperimentMeasurer + // canonicalName is the canonical name of the experiment. + canonicalName string + // config contains the experiment's config. config any @@ -218,6 +222,22 @@ func (b *Factory) NewExperimentMeasurer() model.ExperimentMeasurer { return b.build(b.config) } +// Session is the session definition according to this package. +type Session = model.ExperimentTargetLoaderSession + +// NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. +func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &targetloading.Loader{ + CheckInConfig: config.CheckInConfig, // OPTIONAL + ExperimentName: b.canonicalName, + InputPolicy: b.inputPolicy, + Logger: config.Session.Logger(), + Session: config.Session, + StaticInputs: config.StaticInputs, + SourceFiles: config.SourceFiles, + } +} + // CanonicalizeExperimentName allows code to provide experiment names // in a more flexible way, where we have aliases. // diff --git a/internal/registry/fbmessenger.go b/internal/registry/fbmessenger.go index d3f3233e3a..cd46519826 100644 --- a/internal/registry/fbmessenger.go +++ b/internal/registry/fbmessenger.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["facebook_messenger"] = func() *Factory { + const canonicalName = "facebook_messenger" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return fbmessenger.NewExperimentMeasurer( *config.(*fbmessenger.Config), ) }, + canonicalName: canonicalName, config: &fbmessenger.Config{}, enabledByDefault: true, inputPolicy: model.InputNone, diff --git a/internal/registry/hhfm.go b/internal/registry/hhfm.go index 0820a86476..a86ce98192 100644 --- a/internal/registry/hhfm.go +++ b/internal/registry/hhfm.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["http_header_field_manipulation"] = func() *Factory { + const canonicalName = "http_header_field_manipulation" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return hhfm.NewExperimentMeasurer( *config.(*hhfm.Config), ) }, + canonicalName: canonicalName, config: &hhfm.Config{}, enabledByDefault: true, inputPolicy: model.InputNone, diff --git a/internal/registry/hirl.go b/internal/registry/hirl.go index 846493bc8f..84dbea7ccd 100644 --- a/internal/registry/hirl.go +++ b/internal/registry/hirl.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["http_invalid_request_line"] = func() *Factory { + const canonicalName = "http_invalid_request_line" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return hirl.NewExperimentMeasurer( *config.(*hirl.Config), ) }, + canonicalName: canonicalName, config: &hirl.Config{}, enabledByDefault: true, inputPolicy: model.InputNone, diff --git a/internal/registry/httphostheader.go b/internal/registry/httphostheader.go index c1ecffd76f..c3a02b655c 100644 --- a/internal/registry/httphostheader.go +++ b/internal/registry/httphostheader.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["http_host_header"] = func() *Factory { + const canonicalName = "http_host_header" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return httphostheader.NewExperimentMeasurer( *config.(*httphostheader.Config), ) }, + canonicalName: canonicalName, config: &httphostheader.Config{}, enabledByDefault: true, inputPolicy: model.InputOrQueryBackend, diff --git a/internal/registry/ndt.go b/internal/registry/ndt.go index e6891c7c7b..71ea5562a6 100644 --- a/internal/registry/ndt.go +++ b/internal/registry/ndt.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["ndt"] = func() *Factory { + const canonicalName = "ndt" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return ndt7.NewExperimentMeasurer( *config.(*ndt7.Config), ) }, + canonicalName: canonicalName, config: &ndt7.Config{}, enabledByDefault: true, interruptible: true, diff --git a/internal/registry/openvpn.go b/internal/registry/openvpn.go index cf9244786f..8bf107630c 100644 --- a/internal/registry/openvpn.go +++ b/internal/registry/openvpn.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["openvpn"] = func() *Factory { + const canonicalName = "openvpn" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return openvpn.NewExperimentMeasurer( *config.(*openvpn.Config), "openvpn", ) }, + canonicalName: canonicalName, config: &openvpn.Config{}, enabledByDefault: true, interruptible: true, diff --git a/internal/registry/portfiltering.go b/internal/registry/portfiltering.go index 56937eaca1..8c6a857df8 100644 --- a/internal/registry/portfiltering.go +++ b/internal/registry/portfiltering.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["portfiltering"] = func() *Factory { + const canonicalName = "portfiltering" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config any) model.ExperimentMeasurer { return portfiltering.NewExperimentMeasurer( config.(portfiltering.Config), ) }, + canonicalName: canonicalName, config: portfiltering.Config{}, enabledByDefault: true, interruptible: false, diff --git a/internal/registry/psiphon.go b/internal/registry/psiphon.go index 48dc7888a2..517bb22053 100644 --- a/internal/registry/psiphon.go +++ b/internal/registry/psiphon.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["psiphon"] = func() *Factory { + const canonicalName = "psiphon" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return psiphon.NewExperimentMeasurer( *config.(*psiphon.Config), ) }, + canonicalName: canonicalName, config: &psiphon.Config{}, enabledByDefault: true, inputPolicy: model.InputOptional, diff --git a/internal/registry/quicping.go b/internal/registry/quicping.go index 090037ad78..77a1d3ebf2 100644 --- a/internal/registry/quicping.go +++ b/internal/registry/quicping.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["quicping"] = func() *Factory { + const canonicalName = "quicping" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return quicping.NewExperimentMeasurer( *config.(*quicping.Config), ) }, + canonicalName: canonicalName, config: &quicping.Config{}, enabledByDefault: true, inputPolicy: model.InputStrictlyRequired, diff --git a/internal/registry/riseupvpn.go b/internal/registry/riseupvpn.go index 950a86b79e..6527f3a04f 100644 --- a/internal/registry/riseupvpn.go +++ b/internal/registry/riseupvpn.go @@ -10,15 +10,17 @@ import ( ) func init() { - AllExperiments["riseupvpn"] = func() *Factory { + const canonicalName = "riseupvpn" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return riseupvpn.NewExperimentMeasurer( *config.(*riseupvpn.Config), ) }, - config: &riseupvpn.Config{}, - inputPolicy: model.InputNone, + canonicalName: canonicalName, + config: &riseupvpn.Config{}, + inputPolicy: model.InputNone, } } } diff --git a/internal/registry/signal.go b/internal/registry/signal.go index 615d44b732..5890936b31 100644 --- a/internal/registry/signal.go +++ b/internal/registry/signal.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["signal"] = func() *Factory { + const canonicalName = "signal" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return signal.NewExperimentMeasurer( *config.(*signal.Config), ) }, + canonicalName: canonicalName, config: &signal.Config{}, enabledByDefault: true, inputPolicy: model.InputNone, diff --git a/internal/registry/simplequicping.go b/internal/registry/simplequicping.go index 8eb3e7c54e..c83b8cec88 100644 --- a/internal/registry/simplequicping.go +++ b/internal/registry/simplequicping.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["simplequicping"] = func() *Factory { + const canonicalName = "simplequicping" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return simplequicping.NewExperimentMeasurer( *config.(*simplequicping.Config), ) }, + canonicalName: canonicalName, config: &simplequicping.Config{}, enabledByDefault: true, inputPolicy: model.InputStrictlyRequired, diff --git a/internal/registry/sniblocking.go b/internal/registry/sniblocking.go index 8af8214d68..a433de3dc2 100644 --- a/internal/registry/sniblocking.go +++ b/internal/registry/sniblocking.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["sni_blocking"] = func() *Factory { + const canonicalName = "sni_blocking" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return sniblocking.NewExperimentMeasurer( *config.(*sniblocking.Config), ) }, + canonicalName: canonicalName, config: &sniblocking.Config{}, enabledByDefault: true, inputPolicy: model.InputOrQueryBackend, diff --git a/internal/registry/stunreachability.go b/internal/registry/stunreachability.go index dfa331c3fc..db35b59845 100644 --- a/internal/registry/stunreachability.go +++ b/internal/registry/stunreachability.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["stunreachability"] = func() *Factory { + const canonicalName = "stunreachability" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return stunreachability.NewExperimentMeasurer( *config.(*stunreachability.Config), ) }, + canonicalName: canonicalName, config: &stunreachability.Config{}, enabledByDefault: true, inputPolicy: model.InputOrStaticDefault, diff --git a/internal/registry/tcpping.go b/internal/registry/tcpping.go index d0ca932494..029e6e2d1c 100644 --- a/internal/registry/tcpping.go +++ b/internal/registry/tcpping.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["tcpping"] = func() *Factory { + const canonicalName = "tcpping" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return tcpping.NewExperimentMeasurer( *config.(*tcpping.Config), ) }, + canonicalName: canonicalName, config: &tcpping.Config{}, enabledByDefault: true, inputPolicy: model.InputStrictlyRequired, diff --git a/internal/registry/telegram.go b/internal/registry/telegram.go index 8f61d78baa..987e640935 100644 --- a/internal/registry/telegram.go +++ b/internal/registry/telegram.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["telegram"] = func() *Factory { + const canonicalName = "telegram" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config any) model.ExperimentMeasurer { return telegram.NewExperimentMeasurer( config.(telegram.Config), ) }, + canonicalName: canonicalName, config: telegram.Config{}, enabledByDefault: true, interruptible: false, diff --git a/internal/registry/tlsmiddlebox.go b/internal/registry/tlsmiddlebox.go index a97de82c89..73e1ecfa33 100644 --- a/internal/registry/tlsmiddlebox.go +++ b/internal/registry/tlsmiddlebox.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["tlsmiddlebox"] = func() *Factory { + const canonicalName = "tlsmiddlebox" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return tlsmiddlebox.NewExperimentMeasurer( *config.(*tlsmiddlebox.Config), ) }, + canonicalName: canonicalName, config: &tlsmiddlebox.Config{}, enabledByDefault: true, inputPolicy: model.InputStrictlyRequired, diff --git a/internal/registry/tlsping.go b/internal/registry/tlsping.go index a40723d20b..6ec5048d52 100644 --- a/internal/registry/tlsping.go +++ b/internal/registry/tlsping.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["tlsping"] = func() *Factory { + const canonicalName = "tlsping" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return tlsping.NewExperimentMeasurer( *config.(*tlsping.Config), ) }, + canonicalName: canonicalName, config: &tlsping.Config{}, enabledByDefault: true, inputPolicy: model.InputStrictlyRequired, diff --git a/internal/registry/tlstool.go b/internal/registry/tlstool.go index 8bb70983f2..0fe2625a55 100644 --- a/internal/registry/tlstool.go +++ b/internal/registry/tlstool.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["tlstool"] = func() *Factory { + const canonicalName = "tlstool" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return tlstool.NewExperimentMeasurer( *config.(*tlstool.Config), ) }, + canonicalName: canonicalName, config: &tlstool.Config{}, enabledByDefault: true, inputPolicy: model.InputOrQueryBackend, diff --git a/internal/registry/tor.go b/internal/registry/tor.go index 73098bf770..2e2a613265 100644 --- a/internal/registry/tor.go +++ b/internal/registry/tor.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["tor"] = func() *Factory { + const canonicalName = "tor" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return tor.NewExperimentMeasurer( *config.(*tor.Config), ) }, + canonicalName: canonicalName, config: &tor.Config{}, enabledByDefault: true, inputPolicy: model.InputNone, diff --git a/internal/registry/torsf.go b/internal/registry/torsf.go index 178f01d5e4..3090900445 100644 --- a/internal/registry/torsf.go +++ b/internal/registry/torsf.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["torsf"] = func() *Factory { + const canonicalName = "torsf" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return torsf.NewExperimentMeasurer( *config.(*torsf.Config), ) }, + canonicalName: canonicalName, config: &torsf.Config{}, enabledByDefault: false, inputPolicy: model.InputNone, diff --git a/internal/registry/urlgetter.go b/internal/registry/urlgetter.go index 63dba20f9e..ae3391bd67 100644 --- a/internal/registry/urlgetter.go +++ b/internal/registry/urlgetter.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["urlgetter"] = func() *Factory { + const canonicalName = "urlgetter" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return urlgetter.NewExperimentMeasurer( *config.(*urlgetter.Config), ) }, + canonicalName: canonicalName, config: &urlgetter.Config{}, enabledByDefault: true, inputPolicy: model.InputStrictlyRequired, diff --git a/internal/registry/vanillator.go b/internal/registry/vanillator.go index 1af548660a..a1835e22ce 100644 --- a/internal/registry/vanillator.go +++ b/internal/registry/vanillator.go @@ -10,14 +10,16 @@ import ( ) func init() { - AllExperiments["vanilla_tor"] = func() *Factory { + const canonicalName = "vanilla_tor" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return vanillator.NewExperimentMeasurer( *config.(*vanillator.Config), ) }, - config: &vanillator.Config{}, + canonicalName: canonicalName, + config: &vanillator.Config{}, // We discussed this topic with @aanorbel. On Android this experiment crashes // frequently because of https://github.com/ooni/probe/issues/2406. So, it seems // more cautious to disable it by default and let the check-in API decide. diff --git a/internal/registry/webconnectivity.go b/internal/registry/webconnectivity.go index 1ea720d581..470bad802b 100644 --- a/internal/registry/webconnectivity.go +++ b/internal/registry/webconnectivity.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["web_connectivity"] = func() *Factory { + const canonicalName = "web_connectivity" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config any) model.ExperimentMeasurer { return webconnectivity.NewExperimentMeasurer( config.(webconnectivity.Config), ) }, + canonicalName: canonicalName, config: webconnectivity.Config{}, enabledByDefault: true, interruptible: false, diff --git a/internal/registry/webconnectivityv05.go b/internal/registry/webconnectivityv05.go index 0881b4448f..3a56511a69 100644 --- a/internal/registry/webconnectivityv05.go +++ b/internal/registry/webconnectivityv05.go @@ -12,13 +12,15 @@ import ( ) func init() { - AllExperiments["web_connectivity@v0.5"] = func() *Factory { + const canonicalName = "web_connectivity@v0.5" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config any) model.ExperimentMeasurer { return webconnectivitylte.NewExperimentMeasurer( config.(*webconnectivitylte.Config), ) }, + canonicalName: canonicalName, config: &webconnectivitylte.Config{}, enabledByDefault: true, interruptible: false, diff --git a/internal/registry/whatsapp.go b/internal/registry/whatsapp.go index 84acb57898..24a27a2acc 100644 --- a/internal/registry/whatsapp.go +++ b/internal/registry/whatsapp.go @@ -10,13 +10,15 @@ import ( ) func init() { - AllExperiments["whatsapp"] = func() *Factory { + const canonicalName = "whatsapp" + AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return whatsapp.NewExperimentMeasurer( *config.(*whatsapp.Config), ) }, + canonicalName: canonicalName, config: &whatsapp.Config{}, enabledByDefault: true, inputPolicy: model.InputNone, diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index 6590c2c4a7..66a22bc88e 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -13,7 +13,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/experiment/openvpn" "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/registry" "github.com/ooni/probe-cli/v3/internal/stuninput" ) @@ -26,12 +25,8 @@ var ( ErrNoStaticInput = errors.New("no static input for this experiment") ) -// Session is the session according to a [*Loader]. -type Session interface { - CheckIn(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) - FetchOpenVPNConfig(ctx context.Context, - provider, cc string) (*model.OOAPIVPNProviderConfig, error) -} +// Session is the session according to a [*Loader] instance. +type Session = model.ExperimentTargetLoaderSession // Logger is the [model.Logger] according to a [*Loader]. type Logger interface { @@ -230,7 +225,7 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // // TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability // inputs using richer input (aka check-in v2). - switch registry.CanonicalizeExperimentName(name) { + switch name { case "dnscheck": return dnsCheckDefaultInput, nil case "stunreachability": @@ -305,7 +300,7 @@ func (il *Loader) readfile(filepath string, open openFunc) ([]model.ExperimentTa // loadRemote loads inputs from a remote source. func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { - switch registry.CanonicalizeExperimentName(il.ExperimentName) { + switch il.ExperimentName { case "openvpn": // TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass // the desired provider here, if known. We only have one provider for now, so I'm acting like YAGNI. Another diff --git a/internal/targetloading/targetloading_test.go b/internal/targetloading/targetloading_test.go index 9710ce1b34..82684a8205 100644 --- a/internal/targetloading/targetloading_test.go +++ b/internal/targetloading/targetloading_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/apex/log" "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" @@ -535,7 +536,7 @@ type TargetLoaderMockableSession struct { Error error } -// CheckIn implements TargetLoaderSession.CheckIn. +// CheckIn implements [Session]. func (sess *TargetLoaderMockableSession) CheckIn( ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { if sess.Output == nil && sess.Error == nil { @@ -544,13 +545,19 @@ func (sess *TargetLoaderMockableSession) CheckIn( return sess.Output, sess.Error } -// FetchOpenVPNConfig implements TargetLoaderSession.FetchOpenVPNConfig. +// FetchOpenVPNConfig implements [Session]. func (sess *TargetLoaderMockableSession) FetchOpenVPNConfig( ctx context.Context, provider, cc string) (*model.OOAPIVPNProviderConfig, error) { runtimex.Assert(!(sess.Error == nil && sess.FetchOpenVPNConfigOutput == nil), "both FetchOpenVPNConfig and Error are nil") return sess.FetchOpenVPNConfigOutput, sess.Error } +// Logger implements [Session]. +func (sess *TargetLoaderMockableSession) Logger() model.Logger { + // Such that we see some logs when running tests + return log.Log +} + func TestTargetLoaderCheckInFailure(t *testing.T) { il := &Loader{ Session: &TargetLoaderMockableSession{ From 4beba3721aa446c0475894fb127f458e2524397f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 15:34:27 +0200 Subject: [PATCH 02/18] x --- internal/experimentname/experimentname.go | 25 +++++++++++++++++++++++ internal/registry/factory.go | 25 ++--------------------- internal/registry/factory_test.go | 3 ++- internal/targetloading/targetloading.go | 5 +++-- 4 files changed, 32 insertions(+), 26 deletions(-) create mode 100644 internal/experimentname/experimentname.go diff --git a/internal/experimentname/experimentname.go b/internal/experimentname/experimentname.go new file mode 100644 index 0000000000..193edc6135 --- /dev/null +++ b/internal/experimentname/experimentname.go @@ -0,0 +1,25 @@ +// Package experimentname contains code to manipulate experiment names. +package experimentname + +import "github.com/ooni/probe-cli/v3/internal/strcasex" + +// Canonicalize allows code to provide experiment names +// in a more flexible way, where we have aliases. +// +// Because we allow for uppercase experiment names for backwards +// compatibility with MK, we need to add some exceptions here when +// mapping (e.g., DNSCheck => dnscheck). +func Canonicalize(name string) string { + switch name = strcasex.ToSnake(name); name { + case "ndt_7": + name = "ndt" // since 2020-03-18, we use ndt7 to implement ndt by default + case "dns_check": + name = "dnscheck" + case "stun_reachability": + name = "stunreachability" + case "web_connectivity@v_0_5": + name = "web_connectivity@v0.5" + default: + } + return name +} diff --git a/internal/registry/factory.go b/internal/registry/factory.go index aba0946d64..eca08b2094 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -13,8 +13,8 @@ import ( "strconv" "github.com/ooni/probe-cli/v3/internal/checkincache" + "github.com/ooni/probe-cli/v3/internal/experimentname" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/strcasex" "github.com/ooni/probe-cli/v3/internal/targetloading" ) @@ -238,27 +238,6 @@ func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) mo } } -// CanonicalizeExperimentName allows code to provide experiment names -// in a more flexible way, where we have aliases. -// -// Because we allow for uppercase experiment names for backwards -// compatibility with MK, we need to add some exceptions here when -// mapping (e.g., DNSCheck => dnscheck). -func CanonicalizeExperimentName(name string) string { - switch name = strcasex.ToSnake(name); name { - case "ndt_7": - name = "ndt" // since 2020-03-18, we use ndt7 to implement ndt by default - case "dns_check": - name = "dnscheck" - case "stun_reachability": - name = "stunreachability" - case "web_connectivity@v_0_5": - name = "web_connectivity@v0.5" - default: - } - return name -} - // ErrNoSuchExperiment indicates a given experiment does not exist. var ErrNoSuchExperiment = errors.New("no such experiment") @@ -305,7 +284,7 @@ const OONI_FORCE_ENABLE_EXPERIMENT = "OONI_FORCE_ENABLE_EXPERIMENT" func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (*Factory, error) { // Make sure we are deadling with the canonical experiment name. Historically MK used // names such as WebConnectivity and we want to continue supporting this use case. - name = CanonicalizeExperimentName(name) + name = experimentname.Canonicalize(name) // Handle A/B testing where we dynamically choose LTE for some users. The current policy // only relates to a few users to collect data. diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index dd32ddafb3..a309400945 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/checkincache" "github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte" + "github.com/ooni/probe-cli/v3/internal/experimentname" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -691,7 +692,7 @@ func TestNewFactory(t *testing.T) { // get experiment expectations -- note that here we must canonicalize the // experiment name otherwise we won't find it into the map when testing non-canonical names - expectations := expectationsMap[CanonicalizeExperimentName(tc.experimentName)] + expectations := expectationsMap[experimentname.Canonicalize(tc.experimentName)] if expectations == nil { t.Fatal("no expectations for", tc.experimentName) } diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index 66a22bc88e..92e6a3b99c 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -11,6 +11,7 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/experiment/openvpn" + "github.com/ooni/probe-cli/v3/internal/experimentname" "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/stuninput" @@ -225,7 +226,7 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // // TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability // inputs using richer input (aka check-in v2). - switch name { + switch experimentname.Canonicalize(name) { case "dnscheck": return dnsCheckDefaultInput, nil case "stunreachability": @@ -300,7 +301,7 @@ func (il *Loader) readfile(filepath string, open openFunc) ([]model.ExperimentTa // loadRemote loads inputs from a remote source. func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { - switch il.ExperimentName { + switch experimentname.Canonicalize(il.ExperimentName) { case "openvpn": // TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass // the desired provider here, if known. We only have one provider for now, so I'm acting like YAGNI. Another From 775c836f1660c65fbcaee7e7b3ed85d5b00f4860 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 15:50:34 +0200 Subject: [PATCH 03/18] x --- .../experimentname/experimentname_test.go | 55 +++++++++++++++++++ internal/mocks/experimentbuilder_test.go | 12 ++++ 2 files changed, 67 insertions(+) create mode 100644 internal/experimentname/experimentname_test.go diff --git a/internal/experimentname/experimentname_test.go b/internal/experimentname/experimentname_test.go new file mode 100644 index 0000000000..df191d768d --- /dev/null +++ b/internal/experimentname/experimentname_test.go @@ -0,0 +1,55 @@ +// Package experimentname contains code to manipulate experiment names. +package experimentname + +import "testing" + +func TestCanonicalize(t *testing.T) { + tests := []struct { + input string + expect string + }{ + { + input: "example", + expect: "example", + }, + { + input: "Example", + expect: "example", + }, + { + input: "ndt7", + expect: "ndt", + }, + { + input: "Ndt7", + expect: "ndt", + }, + { + input: "DNSCheck", + expect: "dnscheck", + }, + { + input: "dns_check", + expect: "dnscheck", + }, + { + input: "STUNReachability", + expect: "stunreachability", + }, + { + input: "stun_reachability", + expect: "stunreachability", + }, + { + input: "WebConnectivity@v0.5", + expect: "web_connectivity@v0.5", + }, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + if got := Canonicalize(tt.input); got != tt.expect { + t.Errorf("Canonicalize() = %v, want %v", got, tt.expect) + } + }) + } +} diff --git a/internal/mocks/experimentbuilder_test.go b/internal/mocks/experimentbuilder_test.go index 6fd1ce532c..55ba1783f9 100644 --- a/internal/mocks/experimentbuilder_test.go +++ b/internal/mocks/experimentbuilder_test.go @@ -96,4 +96,16 @@ func TestExperimentBuilder(t *testing.T) { t.Fatal("invalid result") } }) + + t.Run("NewTargetLoader", func(t *testing.T) { + tloader := &ExperimentTargetLoader{} + eb := &ExperimentBuilder{ + MockNewTargetLoader: func(*model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return tloader + }, + } + if out := eb.NewTargetLoader(&model.ExperimentTargetLoaderConfig{}); out != tloader { + t.Fatal("invalid result") + } + }) } From 4eb8147518a7c169907a17dbcd666baa234e91d3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 15:52:40 +0200 Subject: [PATCH 04/18] x --- internal/registry/factory.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/internal/registry/factory.go b/internal/registry/factory.go index eca08b2094..0b37a79ee2 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -39,6 +39,22 @@ type Factory struct { interruptible bool } +// Session is the session definition according to this package. +type Session = model.ExperimentTargetLoaderSession + +// NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. +func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &targetloading.Loader{ + CheckInConfig: config.CheckInConfig, // OPTIONAL + ExperimentName: b.canonicalName, + InputPolicy: b.inputPolicy, + Logger: config.Session.Logger(), + Session: config.Session, + StaticInputs: config.StaticInputs, + SourceFiles: config.SourceFiles, + } +} + // Interruptible returns whether the experiment is interruptible. func (b *Factory) Interruptible() bool { return b.interruptible @@ -222,22 +238,6 @@ func (b *Factory) NewExperimentMeasurer() model.ExperimentMeasurer { return b.build(b.config) } -// Session is the session definition according to this package. -type Session = model.ExperimentTargetLoaderSession - -// NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. -func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { - return &targetloading.Loader{ - CheckInConfig: config.CheckInConfig, // OPTIONAL - ExperimentName: b.canonicalName, - InputPolicy: b.inputPolicy, - Logger: config.Session.Logger(), - Session: config.Session, - StaticInputs: config.StaticInputs, - SourceFiles: config.SourceFiles, - } -} - // ErrNoSuchExperiment indicates a given experiment does not exist. var ErrNoSuchExperiment = errors.New("no such experiment") From e38678d6be8f0138025816e55f82d73498169422 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 16:02:38 +0200 Subject: [PATCH 05/18] x --- internal/registry/factory_test.go | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index a309400945..60105d1354 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -1,17 +1,20 @@ package registry import ( + "context" "errors" "fmt" "math" "os" "testing" + "github.com/apex/log" "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/checkincache" "github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte" "github.com/ooni/probe-cli/v3/internal/experimentname" "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -295,6 +298,15 @@ func TestExperimentBuilderSetOptionAny(t *testing.T) { FieldValue: 1.11, ExpectErr: ErrCannotSetIntegerOption, ExpectConfig: &fakeExperimentConfig{}, + }, { + TestCaseName: "[int] for float64 with zero fractional value", + InitialConfig: &fakeExperimentConfig{}, + FieldName: "Value", + FieldValue: float64(16.0), + ExpectErr: nil, + ExpectConfig: &fakeExperimentConfig{ + Value: 16, + }, }, { TestCaseName: "[string] for serialized bool value while setting a string value", InitialConfig: &fakeExperimentConfig{}, @@ -802,3 +814,49 @@ func TestNewFactory(t *testing.T) { } }) } + +// Make sure the target loader for web connectivity is WAI when using no static inputs. +func TestFactoryNewTargetLoaderWebConnectivity(t *testing.T) { + // construct the proper factory instance + store := &kvstore.Memory{} + factory, err := NewFactory("web_connectivity", store, log.Log) + if err != nil { + t.Fatal(err) + } + + // define the expected error. + expected := errors.New("antani") + + // create suitable loader config. + config := &model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ + // nothing + }, + Session: &mocks.Session{ + MockCheckIn: func(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { + return nil, expected + }, + MockLogger: func() model.Logger { + return log.Log + }, + }, + StaticInputs: nil, + SourceFiles: nil, + } + + // obtain the loader + loader := factory.NewTargetLoader(config) + + // attempt to load targets + targets, err := loader.Load(context.Background()) + + // make sure we've got the expected error + if !errors.Is(err, expected) { + t.Fatal("unexpected error", err) + } + + // make sure there are no targets + if len(targets) != 0 { + t.Fatal("expected zero length targets") + } +} From 06d300f927172a4d4634093e778a1d4c470dbbba Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 16:08:50 +0200 Subject: [PATCH 06/18] x --- internal/engine/experimentbuilder_test.go | 50 +++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/internal/engine/experimentbuilder_test.go b/internal/engine/experimentbuilder_test.go index 00a22ef62d..d81e0bb7bb 100644 --- a/internal/engine/experimentbuilder_test.go +++ b/internal/engine/experimentbuilder_test.go @@ -1 +1,51 @@ package engine + +import ( + "context" + "errors" + "testing" + + "github.com/ooni/probe-cli/v3/internal/model" +) + +func TestExperimentBuilderEngineWebConnectivity(t *testing.T) { + // create a session for testing that does not use the network at all + sess := newSessionForTestingNoLookups(t) + + // create an experiment builder for Web Connectivity + builder, err := sess.NewExperimentBuilder("WebConnectivity") + if err != nil { + t.Fatal(err) + } + + // create suitable loader config + config := &model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ + // nothing + }, + Session: sess, + StaticInputs: nil, + SourceFiles: nil, + } + + // create the loader + loader := builder.NewTargetLoader(config) + + // create cancelled context to interrupt immediately so that we + // don't use the network when running this test + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // attempt to load targets + targets, err := loader.Load(ctx) + + // make sure we've got the expected error + if !errors.Is(err, context.Canceled) { + t.Fatal("unexpected err", err) + } + + // make sure there are no targets + if len(targets) != 0 { + t.Fatal("expected zero length targets") + } +} From d4904486b5b73931a254995eca9f1defd53ce7b2 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 16:14:58 +0200 Subject: [PATCH 07/18] x --- internal/oonirun/experiment_test.go | 10 ++++++++++ internal/oonirun/v1_test.go | 10 ++++++++++ internal/oonirun/v2_test.go | 10 ++++++++++ 3 files changed, 30 insertions(+) diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index b93fd25ae2..d438f47ce1 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -67,6 +67,16 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { } return exp }, + MockNewTargetLoader: func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + // Implementation note: the convention for input-less experiments is that + // they require a single entry containing an empty input. + entry := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("") + return []model.ExperimentTarget{entry}, nil + }, + } + }, } return eb, nil }, diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index c23455a9fc..910d910b0c 100644 --- a/internal/oonirun/v1_test.go +++ b/internal/oonirun/v1_test.go @@ -44,6 +44,16 @@ func newMinimalFakeSession() *mocks.Session { } return exp }, + MockNewTargetLoader: func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + // Implementation note: the convention for input-less experiments is that + // they require a single entry containing an empty input. + entry := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("") + return []model.ExperimentTarget{entry}, nil + }, + } + }, } return eb, nil }, diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index c4afa60e96..d849c6d088 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -392,6 +392,16 @@ func TestV2MeasureDescriptor(t *testing.T) { } return exp }, + MockNewTargetLoader: func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + // Implementation note: the convention for input-less experiments is that + // they require a single entry containing an empty input. + entry := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("") + return []model.ExperimentTarget{entry}, nil + }, + } + }, } return eb, nil } From a97bcd8f2a6b4dcf975659b21f1e34362538f418 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 18:54:39 +0200 Subject: [PATCH 08/18] dnscheck --- internal/engine/experiment.go | 19 ++- internal/engine/experiment_test.go | 2 +- internal/experiment/dnscheck/dnscheck.go | 43 ++++--- internal/experiment/dnscheck/dnscheck_test.go | 74 +++++++---- internal/experiment/dnscheck/richerinput.go | 120 ++++++++++++++++++ internal/model/experiment.go | 3 + internal/oonirun/experiment.go | 14 +- internal/registry/dnscheck.go | 7 +- internal/registry/factory.go | 9 +- internal/targetloading/targetloading.go | 56 ++++---- internal/targetloading/targetloading_test.go | 5 +- 11 files changed, 262 insertions(+), 90 deletions(-) create mode 100644 internal/experiment/dnscheck/richerinput.go diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 61197c0d8d..f2934ce4fa 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -109,11 +109,12 @@ func (e *experiment) SubmitAndUpdateMeasurementContext( } // newMeasurement creates a new measurement for this experiment with the given input. -func (e *experiment) newMeasurement(input string) *model.Measurement { +func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measurement { utctimenow := time.Now().UTC() + // TODO(bassosimone,DecFox): add support for unmarshaling options. m := &model.Measurement{ DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion, - Input: model.MeasurementInput(input), + Input: model.MeasurementInput(target.Input()), MeasurementStartTime: utctimenow.Format(model.MeasurementDateFormat), MeasurementStartTimeSaved: utctimenow, ProbeIP: model.DefaultProbeIP, @@ -204,10 +205,15 @@ func (e *experiment) MeasureWithContext( ctx = bytecounter.WithExperimentByteCounter(ctx, e.byteCounter) // Create a new measurement that the experiment measurer will finish filling - // by adding the test keys etc. Please, note that, as of 2024-06-05, we're using - // the measurement Input to provide input to an experiment. We'll probably - // change this, when we'll have finished implementing richer input. - measurement := e.newMeasurement(target.Input()) + // by adding the test keys etc. Please, note that, as of 2024-06-06: + // + // 1. experiments using richer input receive input via the Target field; + // + // 2. other experiments use (*Measurement).Input. + // + // Here we're passing the whole target to newMeasurement such that we're able + // to record options values in addition to the input value. + measurement := e.newMeasurement(target) // Record when we started the experiment, to compute the runtime. start := time.Now() @@ -217,6 +223,7 @@ func (e *experiment) MeasureWithContext( Callbacks: e.callbacks, Measurement: measurement, Session: e.session, + Target: target, } // Invoke the measurer. Conventionally, an error being returned here diff --git a/internal/engine/experiment_test.go b/internal/engine/experiment_test.go index e2444306af..ceab79d7bb 100644 --- a/internal/engine/experiment_test.go +++ b/internal/engine/experiment_test.go @@ -17,7 +17,7 @@ func TestExperimentHonoursSharingDefaults(t *testing.T) { t.Fatal(err) } exp := builder.NewExperiment().(*experiment) - return exp.newMeasurement("") + return exp.newMeasurement(model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")) } type spec struct { name string diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index 4c9d5fd17c..3b6c93787b 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -96,7 +96,6 @@ type TestKeys struct { // Measurer performs the measurement. type Measurer struct { - Config Endpoints *Endpoints } @@ -125,6 +124,10 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { measurement := args.Measurement sess := args.Session + // 0. obtain the richer input target, config, and input or panic + target := args.Target.(*Target) + config, input := target.options, target.input + // 1. fill the measurement with test keys tk := new(TestKeys) tk.Lookups = make(map[string]urlgetter.TestKeys) @@ -133,20 +136,19 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { // 2. select the domain to resolve or use default and, while there, also // ensure that we register all the other options we're using. - domain := m.Config.Domain + domain := config.Domain if domain == "" { domain = defaultDomain } - tk.DefaultAddrs = m.Config.DefaultAddrs + tk.DefaultAddrs = config.DefaultAddrs tk.Domain = domain - tk.HTTP3Enabled = m.Config.HTTP3Enabled - tk.HTTPHost = m.Config.HTTPHost - tk.TLSServerName = m.Config.TLSServerName - tk.TLSVersion = m.Config.TLSVersion + tk.HTTP3Enabled = config.HTTP3Enabled + tk.HTTPHost = config.HTTPHost + tk.TLSServerName = config.TLSServerName + tk.TLSVersion = config.TLSVersion tk.Residual = m.Endpoints != nil // 3. parse the input URL describing the resolver to use - input := string(measurement.Input) if input == "" { return ErrInputRequired } @@ -191,7 +193,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { for _, addr := range addrs { allAddrs[addr] = true } - for _, addr := range strings.Split(m.Config.DefaultAddrs, " ") { + for _, addr := range strings.Split(config.DefaultAddrs, " ") { if addr != "" { allAddrs[addr] = true } @@ -208,10 +210,10 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { for addr := range allAddrs { inputs = append(inputs, urlgetter.MultiInput{ Config: urlgetter.Config{ - DNSHTTPHost: m.httpHost(URL.Host), - DNSTLSServerName: m.tlsServerName(URL.Hostname()), - DNSTLSVersion: m.Config.TLSVersion, - HTTP3Enabled: m.Config.HTTP3Enabled, + DNSHTTPHost: config.httpHost(URL.Host), + DNSTLSServerName: config.tlsServerName(URL.Hostname()), + DNSTLSVersion: config.TLSVersion, + HTTP3Enabled: config.HTTP3Enabled, RejectDNSBogons: true, // bogons are errors in this context ResolverURL: makeResolverURL(URL, addr), Timeout: 15 * time.Second, @@ -244,17 +246,17 @@ func (m *Measurer) lookupHost(ctx context.Context, hostname string, r model.Reso // httpHost returns the configured HTTP host, if set, otherwise // it will return the host provide as argument. -func (m *Measurer) httpHost(httpHost string) string { - if m.Config.HTTPHost != "" { - return m.Config.HTTPHost +func (c *Config) httpHost(httpHost string) string { + if c.HTTPHost != "" { + return c.HTTPHost } return httpHost } // tlsServerName is like httpHost for the TLS server name. -func (m *Measurer) tlsServerName(tlsServerName string) string { - if m.Config.TLSServerName != "" { - return m.Config.TLSServerName +func (c *Config) tlsServerName(tlsServerName string) string { + if c.TLSServerName != "" { + return c.TLSServerName } return tlsServerName } @@ -311,9 +313,8 @@ func makeResolverURL(URL *url.URL, addr string) string { } // NewExperimentMeasurer creates a new ExperimentMeasurer. -func NewExperimentMeasurer(config Config) model.ExperimentMeasurer { +func NewExperimentMeasurer() model.ExperimentMeasurer { return &Measurer{ - Config: config, Endpoints: nil, // disabled by default } } diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index 2fd2c295c8..bfe91eb767 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -8,44 +8,40 @@ import ( "time" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/legacy/mockable" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" ) func TestHTTPHostWithOverride(t *testing.T) { - m := Measurer{Config: Config{HTTPHost: "antani"}} - result := m.httpHost("mascetti") - if result != "antani" { + c := &Config{HTTPHost: "antani"} + if result := c.httpHost("mascetti"); result != "antani" { t.Fatal("not the result we expected") } } func TestHTTPHostWithoutOverride(t *testing.T) { - m := Measurer{Config: Config{}} - result := m.httpHost("mascetti") - if result != "mascetti" { + c := &Config{} + if result := c.httpHost("mascetti"); result != "mascetti" { t.Fatal("not the result we expected") } } func TestTLSServerNameWithOverride(t *testing.T) { - m := Measurer{Config: Config{TLSServerName: "antani"}} - result := m.tlsServerName("mascetti") - if result != "antani" { + c := &Config{TLSServerName: "antani"} + if result := c.tlsServerName("mascetti"); result != "antani" { t.Fatal("not the result we expected") } } func TestTLSServerNameWithoutOverride(t *testing.T) { - m := Measurer{Config: Config{}} - result := m.tlsServerName("mascetti") - if result != "mascetti" { + c := &Config{} + if result := c.tlsServerName("mascetti"); result != "mascetti" { t.Fatal("not the result we expected") } } func TestExperimentNameAndVersion(t *testing.T) { - measurer := NewExperimentMeasurer(Config{Domain: "example.com"}) + measurer := NewExperimentMeasurer() if measurer.ExperimentName() != "dnscheck" { t.Error("unexpected experiment name") } @@ -55,11 +51,17 @@ func TestExperimentNameAndVersion(t *testing.T) { } func TestDNSCheckFailsWithoutInput(t *testing.T) { - measurer := NewExperimentMeasurer(Config{Domain: "example.com"}) + measurer := NewExperimentMeasurer() args := &model.ExperimentArgs{ Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: new(model.Measurement), Session: newsession(), + Target: &Target{ + input: "", // explicitly empty + options: &Config{ + Domain: "example.com", + }, + }, } err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrInputRequired) { @@ -68,11 +70,15 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) { } func TestDNSCheckFailsWithInvalidURL(t *testing.T) { - measurer := NewExperimentMeasurer(Config{}) + measurer := NewExperimentMeasurer() args := &model.ExperimentArgs{ Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: &model.Measurement{Input: "Not a valid URL \x7f"}, Session: newsession(), + Target: &Target{ + input: "Not a valid URL \x7f", + options: &Config{}, + }, } err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrInvalidURL) { @@ -81,11 +87,15 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) { } func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) { - measurer := NewExperimentMeasurer(Config{}) + measurer := NewExperimentMeasurer() args := &model.ExperimentArgs{ Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: &model.Measurement{Input: "file://1.1.1.1"}, Session: newsession(), + Target: &Target{ + input: "file://1.1.1.1", + options: &Config{}, + }, } err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrUnsupportedURLScheme) { @@ -96,14 +106,18 @@ func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) { func TestWithCancelledContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // immediately cancel the context - measurer := NewExperimentMeasurer(Config{ - DefaultAddrs: "1.1.1.1 1.0.0.1", - }) + measurer := NewExperimentMeasurer() measurement := &model.Measurement{Input: "dot://one.one.one.one"} args := &model.ExperimentArgs{ Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: measurement, Session: newsession(), + Target: &Target{ + input: "dot://one.one.one.one", + options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, } err := measurer.Run(ctx, args) if err != nil { @@ -140,14 +154,18 @@ func TestDNSCheckValid(t *testing.T) { t.Skip("skip test in short mode") } - measurer := NewExperimentMeasurer(Config{ - DefaultAddrs: "1.1.1.1 1.0.0.1", - }) + measurer := NewExperimentMeasurer() measurement := model.Measurement{Input: "dot://one.one.one.one:853"} args := &model.ExperimentArgs{ Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: &measurement, Session: newsession(), + Target: &Target{ + input: "dot://one.one.one.one:853", + options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, } err := measurer.Run(context.Background(), args) if err != nil { @@ -169,7 +187,11 @@ func TestDNSCheckValid(t *testing.T) { } func newsession() model.ExperimentSession { - return &mockable.Session{MockableLogger: log.Log} + return &mocks.Session{ + MockLogger: func() model.Logger { + return log.Log + }, + } } func TestDNSCheckWait(t *testing.T) { @@ -187,6 +209,10 @@ func TestDNSCheckWait(t *testing.T) { Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: &measurement, Session: newsession(), + Target: &Target{ + input: input, + options: &Config{}, + }, } err := measurer.Run(context.Background(), args) if err != nil { diff --git a/internal/experiment/dnscheck/richerinput.go b/internal/experiment/dnscheck/richerinput.go new file mode 100644 index 0000000000..758b014319 --- /dev/null +++ b/internal/experiment/dnscheck/richerinput.go @@ -0,0 +1,120 @@ +package dnscheck + +import ( + "context" + + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" +) + +// Target is a richer-input target that this experiment should measure. +type Target struct { + // input is the input. + input string + + // options is the configuration. + options *Config +} + +var _ model.ExperimentTarget = &Target{} + +// Category implements [model.ExperimentTarget]. +func (t *Target) Category() string { + return model.DefaultCategoryCode +} + +// Country implements [model.ExperimentTarget]. +func (t *Target) Country() string { + return model.DefaultCountryCode +} + +// Input implements [model.ExperimentTarget]. +func (t *Target) Input() string { + return t.input +} + +// String implements [model.ExperimentTarget]. +func (t *Target) String() string { + return t.input +} + +// NewLoader constructs a new [model.ExperimentTargerLoader] instance. +// +// This function PANICS if options is not an instance of [*dnscheck.Config]. +func NewLoader(loader *targetloading.Loader, gopts any) model.ExperimentTargetLoader { + // Panic if we cannot convert the options to the expected type. + // + // We do not expect a panic here because the type is managed by the registry package. + options := gopts.(*Config) + + // Construct the proper loader instance. + return &targetLoader{ + loader: loader, + options: options, + } +} + +type targetLoader struct { + loader *targetloading.Loader + options *Config +} + +// Load implements model.ExperimentTargetLoader. +func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { + // TODO(bassosimone): we need a way to know whether the options are empty!!! + + // If there's nothing to statically load fallback to the API + if len(tl.loader.StaticInputs) <= 0 && len(tl.loader.SourceFiles) <= 0 { + return tl.loadFromBackend(ctx) + } + + // Otherwise, attempt to load the static inputs from CLI and files + inputs, err := targetloading.LoadStatic(tl.loader) + + // Handle the case where we couldn't + if err != nil { + return nil, err + } + + // Build the list of targets that we should measure. + var targets []model.ExperimentTarget + for _, input := range inputs { + targets = append(targets, &Target{ + options: tl.options, + input: input, + }) + } + return targets, nil +} + +var defaultInput = []model.ExperimentTarget{ + // + // https://dns.google/dns-query + // + // Measure HTTP/3 first and then HTTP/2 (see https://github.com/ooni/probe/issues/2675). + // + // Make sure we include the typical IP addresses for the domain. + // + &Target{ + input: "https://dns.google/dns-query", + options: &Config{ + HTTP3Enabled: true, + DefaultAddrs: "8.8.8.8 8.8.4.4", + }, + }, + &Target{ + input: "https://dns.google/dns-query", + options: &Config{ + DefaultAddrs: "8.8.8.8 8.8.4.4", + }, + }, + + // TODO(bassosimone): before merging, we need to reinstate the + // whole list that we previously had in tree +} + +func (tl *targetLoader) loadFromBackend(_ context.Context) ([]model.ExperimentTarget, error) { + // TODO(https://github.com/ooni/probe/issues/1390): serve DNSCheck + // inputs using richer input (aka check-in v2). + return defaultInput, nil +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 8bd309f326..4aac4a0147 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -123,6 +123,9 @@ type ExperimentArgs struct { // Session is the MANDATORY session the experiment can use. Session ExperimentSession + + // Target is the MANDATORY target we're measuring. + Target ExperimentTarget } // ExperimentMeasurer is the interface that allows to run a diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 758db9e0c5..3136e32c66 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -82,14 +82,19 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // 2. create input loader and load input for this experiment + // 2. configure experiment's options + if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { + return err + } + + // 3. create input loader and load input for this experiment targetLoader := ed.newTargetLoader(builder) targetList, err := targetLoader.Load(ctx) if err != nil { return err } - // 3. randomize input, if needed + // 4. randomize input, if needed if ed.Random { rnd := rand.New(rand.NewSource(time.Now().UnixNano())) // #nosec G404 -- not really important rnd.Shuffle(len(targetList), func(i, j int) { @@ -98,11 +103,6 @@ func (ed *Experiment) Run(ctx context.Context) error { experimentShuffledInputs.Add(1) } - // 4. configure experiment's options - if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { - return err - } - // 5. construct the experiment instance experiment := builder.NewExperiment() logger := ed.Session.Logger() diff --git a/internal/registry/dnscheck.go b/internal/registry/dnscheck.go index 2890ca19a5..2ed902c38e 100644 --- a/internal/registry/dnscheck.go +++ b/internal/registry/dnscheck.go @@ -12,16 +12,17 @@ import ( func init() { const canonicalName = "dnscheck" AllExperiments[canonicalName] = func() *Factory { + // TODO(bassosimone): for now, we MUST keep the InputOrStaticDefault + // policy because otherwise ./pkg/oonimkall should break. return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { - return dnscheck.NewExperimentMeasurer( - *config.(*dnscheck.Config), - ) + return dnscheck.NewExperimentMeasurer() }, canonicalName: canonicalName, config: &dnscheck.Config{}, enabledByDefault: true, inputPolicy: model.InputOrStaticDefault, + newLoader: dnscheck.NewLoader, } } } diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 0b37a79ee2..2bb6dd3fe2 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -37,6 +37,9 @@ type Factory struct { // interruptible indicates whether the experiment is interruptible. interruptible bool + + // newLoader is the OPTIONAL function to create a new loader. + newLoader func(config *targetloading.Loader, options any) model.ExperimentTargetLoader } // Session is the session definition according to this package. @@ -44,7 +47,7 @@ type Session = model.ExperimentTargetLoaderSession // NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { - return &targetloading.Loader{ + loader := &targetloading.Loader{ CheckInConfig: config.CheckInConfig, // OPTIONAL ExperimentName: b.canonicalName, InputPolicy: b.inputPolicy, @@ -53,6 +56,10 @@ func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) mo StaticInputs: config.StaticInputs, SourceFiles: config.SourceFiles, } + if b.newLoader != nil { + return b.newLoader(loader, b.config) + } + return loader } // Interruptible returns whether the experiment is interruptible. diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index 92e6a3b99c..c6c91162c5 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -220,16 +220,14 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // Implementation note: we may be called from pkg/oonimkall // with a non-canonical experiment name, so we need to convert // the experiment name to be canonical before proceeding. - // - // TODO(https://github.com/ooni/probe/issues/1390): serve DNSCheck - // inputs using richer input (aka check-in v2). - // - // TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability - // inputs using richer input (aka check-in v2). switch experimentname.Canonicalize(name) { case "dnscheck": + // TODO(https://github.com/ooni/probe/issues/1390): serve DNSCheck + // inputs using richer input (aka check-in v2). return dnsCheckDefaultInput, nil case "stunreachability": + // TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability + // inputs using richer input (aka check-in v2). return stunReachabilityDefaultInput, nil default: return nil, ErrNoStaticInput @@ -251,24 +249,17 @@ func (il *Loader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarg return staticInputForExperiment(il.ExperimentName) } -// loadLocal loads inputs from StaticInputs and SourceFiles. +// loadLocal loads inputs from the [*Loader] StaticInputs and SourceFiles. func (il *Loader) loadLocal() ([]model.ExperimentTarget, error) { - inputs := []model.ExperimentTarget{} - for _, input := range il.StaticInputs { - inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) + inputs, err := LoadStatic(il) + if err != nil { + return nil, err } - for _, filepath := range il.SourceFiles { - extra, err := il.readfile(filepath, fsx.OpenFile) - if err != nil { - return nil, err - } - // See https://github.com/ooni/probe-engine/issues/1123. - if len(extra) <= 0 { - return nil, fmt.Errorf("%w: %s", ErrDetectedEmptyFile, filepath) - } - inputs = append(inputs, extra...) + var targets []model.ExperimentTarget + for _, input := range inputs { + targets = append(targets, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) } - return inputs, nil + return targets, nil } // openFunc is the type of the function to open a file. @@ -276,8 +267,8 @@ type openFunc func(filepath string) (fs.File, error) // readfile reads inputs from the specified file. The open argument should be // compatible with stdlib's fs.Open and helps us with unit testing. -func (il *Loader) readfile(filepath string, open openFunc) ([]model.ExperimentTarget, error) { - inputs := []model.ExperimentTarget{} +func readfile(filepath string, open openFunc) ([]string, error) { + inputs := []string{} filep, err := open(filepath) if err != nil { return nil, err @@ -290,7 +281,7 @@ func (il *Loader) readfile(filepath string, open openFunc) ([]model.ExperimentTa for scanner.Scan() { line := scanner.Text() if line != "" { - inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(line)) + inputs = append(inputs, line) } } if scanner.Err() != nil { @@ -299,6 +290,23 @@ func (il *Loader) readfile(filepath string, open openFunc) ([]model.ExperimentTa return inputs, nil } +// LoadStatic loads inputs from the [*Loader] StaticInputs and SourceFiles. +func LoadStatic(config *Loader) ([]string, error) { + inputs := append([]string{}, config.StaticInputs...) + for _, filepath := range config.SourceFiles { + extra, err := readfile(filepath, fsx.OpenFile) + if err != nil { + return nil, err + } + // See https://github.com/ooni/probe-engine/issues/1123. + if len(extra) <= 0 { + return nil, fmt.Errorf("%w: %s", ErrDetectedEmptyFile, filepath) + } + inputs = append(inputs, extra...) + } + return inputs, nil +} + // loadRemote loads inputs from a remote source. func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { switch experimentname.Canonicalize(il.ExperimentName) { diff --git a/internal/targetloading/targetloading_test.go b/internal/targetloading/targetloading_test.go index 82684a8205..0147ddb5ba 100644 --- a/internal/targetloading/targetloading_test.go +++ b/internal/targetloading/targetloading_test.go @@ -509,9 +509,8 @@ func (TargetLoaderBrokenFile) Close() error { return nil } -func TestTargetLoaderReadfileScannerFailure(t *testing.T) { - il := &Loader{} - out, err := il.readfile("", TargetLoaderBrokenFS{}.Open) +func TestReadfileScannerFailure(t *testing.T) { + out, err := readfile("", TargetLoaderBrokenFS{}.Open) if !errors.Is(err, syscall.EFAULT) { t.Fatal("not the error we expected") } From 1a57fd330fea0067de389a19cb8d919ba4067e5f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 19:22:15 +0200 Subject: [PATCH 09/18] x --- internal/engine/experiment.go | 13 ++++++++++--- internal/experiment/dnscheck/dnscheck.go | 6 +++++- internal/model/experiment.go | 5 ++++- internal/oonirun/experiment.go | 5 ++++- internal/registry/dnscheck.go | 2 +- internal/registry/factory.go | 6 ++++++ 6 files changed, 30 insertions(+), 7 deletions(-) diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index f2934ce4fa..9183a48d99 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -111,7 +111,11 @@ 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): add support for unmarshaling options. + // TODO(bassosimone,DecFox): move here code that supports unmarshaling options + // 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, Input: model.MeasurementInput(target.Input()), @@ -207,7 +211,8 @@ func (e *experiment) MeasureWithContext( // Create a new measurement that the experiment measurer will finish filling // by adding the test keys etc. Please, note that, as of 2024-06-06: // - // 1. experiments using richer input receive input via the Target field; + // 1. experiments using richer input receive input via the Target field + // and ignore (*Measurement).Input field. // // 2. other experiments use (*Measurement).Input. // @@ -218,7 +223,9 @@ func (e *experiment) MeasureWithContext( // Record when we started the experiment, to compute the runtime. start := time.Now() - // Prepare the arguments for the experiment measurer + // Prepare the arguments for the experiment measurer. + // + // Only richer-input-aware experiments honour the Target field. args := &model.ExperimentArgs{ Callbacks: e.callbacks, Measurement: measurement, diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index 3b6c93787b..af788cea02 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -19,6 +19,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/legacy/tracex" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) const ( @@ -113,7 +114,7 @@ func (m *Measurer) ExperimentVersion() string { // errors are in addition to any other errors returned by the low level packages // that are used by this experiment to implement its functionality. var ( - ErrInputRequired = errors.New("this experiment needs input") + ErrInputRequired = targetloading.ErrInputRequired ErrInvalidURL = errors.New("the input URL is invalid") ErrUnsupportedURLScheme = errors.New("unsupported URL scheme") ) @@ -125,6 +126,9 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { sess := args.Session // 0. obtain the richer input target, config, and input or panic + if args.Target == nil { + return ErrInputRequired + } target := args.Target.(*Target) config, input := target.options, target.input diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 4aac4a0147..c15bc3ee2d 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -124,7 +124,10 @@ type ExperimentArgs struct { // Session is the MANDATORY session the experiment can use. Session ExperimentSession - // Target is the MANDATORY target we're measuring. + // Target is the OPTIONAL target we're measuring. + // + // Only richer-input-aware experiments use this field. These experiments + // SHOULD be defensive and handle the case where this field is nil. Target ExperimentTarget } diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 3136e32c66..5665a13fb0 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -83,11 +83,14 @@ func (ed *Experiment) Run(ctx context.Context) error { } // 2. configure experiment's options + // + // This MUST happen before loading targets because the options will + // possibly be used to produce richer input targets. if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { return err } - // 3. create input loader and load input for this experiment + // 3. create target loader and load targets for this experiment targetLoader := ed.newTargetLoader(builder) targetList, err := targetLoader.Load(ctx) if err != nil { diff --git a/internal/registry/dnscheck.go b/internal/registry/dnscheck.go index 2ed902c38e..009078aa6b 100644 --- a/internal/registry/dnscheck.go +++ b/internal/registry/dnscheck.go @@ -12,7 +12,7 @@ import ( func init() { const canonicalName = "dnscheck" AllExperiments[canonicalName] = func() *Factory { - // TODO(bassosimone): for now, we MUST keep the InputOrStaticDefault + // TODO(bassosimone,DecFox): for now, we MUST keep the InputOrStaticDefault // policy because otherwise ./pkg/oonimkall should break. return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { diff --git a/internal/registry/factory.go b/internal/registry/factory.go index 2bb6dd3fe2..ee73e95a8f 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -47,6 +47,7 @@ type Session = model.ExperimentTargetLoaderSession // NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + // Construct the default loader used in the non-richer input case. loader := &targetloading.Loader{ CheckInConfig: config.CheckInConfig, // OPTIONAL ExperimentName: b.canonicalName, @@ -56,9 +57,14 @@ func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) mo StaticInputs: config.StaticInputs, SourceFiles: config.SourceFiles, } + + // If an experiment implements richer input, it will use its custom loader + // that will use experiment specific policy for loading targets. if b.newLoader != nil { return b.newLoader(loader, b.config) } + + // Otherwise just return the default loader. return loader } From 399cf788a9e817a4ccfdb63ea67d34524e5ae8a8 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 19:26:17 +0200 Subject: [PATCH 10/18] x --- internal/oonirun/experiment.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 5665a13fb0..428bf19ba2 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -82,6 +82,11 @@ 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. + // 2. configure experiment's options // // This MUST happen before loading targets because the options will From 34fe88f861324f9d73b5d3a94c9226743dd13134 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 19:45:31 +0200 Subject: [PATCH 11/18] x --- internal/engine/experiment.go | 9 +++++---- internal/oonirun/experiment.go | 6 ++++-- internal/oonirun/experiment_test.go | 27 +++++++++++++-------------- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 9183a48d99..5f6ac5d874 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -111,7 +111,7 @@ 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 unmarshaling options + // 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 @@ -211,10 +211,11 @@ func (e *experiment) MeasureWithContext( // Create a new measurement that the experiment measurer will finish filling // by adding the test keys etc. Please, note that, as of 2024-06-06: // - // 1. experiments using richer input receive input via the Target field - // and ignore (*Measurement).Input field. + // 1. Experiments using richer input receive input via the Target field + // and ignore (*Measurement).Input, which however contains the same value + // that would be returned by the Target.Input method. // - // 2. other experiments use (*Measurement).Input. + // 2. Other experiments use (*Measurement).Input. // // Here we're passing the whole target to newMeasurement such that we're able // to record options values in addition to the input value. diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 428bf19ba2..c4613107d7 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -104,8 +104,10 @@ func (ed *Experiment) Run(ctx context.Context) error { // 4. randomize input, if needed if ed.Random { - rnd := rand.New(rand.NewSource(time.Now().UnixNano())) // #nosec G404 -- not really important - rnd.Shuffle(len(targetList), func(i, j int) { + // Note: since go1.20 the default random generated is random seeded + // + // See https://tip.golang.org/doc/go1.20 + rand.Shuffle(len(targetList), func(i, j int) { targetList[i], targetList[j] = targetList[j], targetList[i] }) experimentShuffledInputs.Add(1) diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index d438f47ce1..1fd38abb0b 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -27,9 +27,6 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { ExtraOptions: map[string]any{ "SleepTime": int64(10 * time.Millisecond), }, - Inputs: []string{ - "a", "b", "c", - }, InputFilePaths: []string{}, MaxRuntime: 0, Name: "example", @@ -70,10 +67,12 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { MockNewTargetLoader: func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { - // Implementation note: the convention for input-less experiments is that - // they require a single entry containing an empty input. - entry := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("") - return []model.ExperimentTarget{entry}, nil + results := []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("a"), + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("b"), + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("c"), + } + return results, nil }, } }, @@ -199,12 +198,12 @@ func TestExperimentRun(t *testing.T) { args: args{}, expectErr: errMocked, }, { - name: "cannot load input", + name: "cannot set options", fields: fields{ newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ - MockInputPolicy: func() model.InputPolicy { - return model.InputOptional + MockSetOptionsAny: func(options map[string]any) error { + return errMocked }, } return eb, nil @@ -212,7 +211,7 @@ func TestExperimentRun(t *testing.T) { newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { - return nil, errMocked + return []model.ExperimentTarget{}, nil }, } }, @@ -220,7 +219,7 @@ func TestExperimentRun(t *testing.T) { args: args{}, expectErr: errMocked, }, { - name: "cannot set options", + name: "cannot load input", fields: fields{ newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ @@ -228,7 +227,7 @@ func TestExperimentRun(t *testing.T) { return model.InputOptional }, MockSetOptionsAny: func(options map[string]any) error { - return errMocked + return nil }, } return eb, nil @@ -236,7 +235,7 @@ func TestExperimentRun(t *testing.T) { newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { - return []model.ExperimentTarget{}, nil + return nil, errMocked }, } }, From ed895617f8afa408c3aca1855fb657dabe0a3271 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 19:57:59 +0200 Subject: [PATCH 12/18] x --- internal/registry/example.go | 5 ++++ internal/registry/factory_test.go | 47 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/internal/registry/example.go b/internal/registry/example.go index 94c55ca59f..9bcb5459bb 100644 --- a/internal/registry/example.go +++ b/internal/registry/example.go @@ -14,6 +14,11 @@ import ( func init() { const canonicalName = "example" AllExperiments[canonicalName] = func() *Factory { + // TODO(bassosimone,DecFox): as pointed out by @ainghazal, this experiment + // should be the one that people modify to start out new experiments, so it's + // kind of suboptimal that it has a constructor with explicit experiment + // name to ease writing some tests that ./pkg/oonimkall needs given that no + // other experiment ever sets the experiment name externally! return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { return example.NewExperimentMeasurer( diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index 60105d1354..20a7ffe74b 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -16,6 +16,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) type fakeExperimentConfig struct { @@ -860,3 +861,49 @@ func TestFactoryNewTargetLoaderWebConnectivity(t *testing.T) { t.Fatal("expected zero length targets") } } + +// customConfig is a custom config for [TestFactoryCustomTargetLoaderForRicherInput]. +type customConfig struct{} + +// customTargetLoader is a custom target loader for [TestFactoryCustomTargetLoaderForRicherInput]. +type customTargetLoader struct{} + +// Load implements [model.ExperimentTargetLoader]. +func (c *customTargetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { + panic("should not be called") +} + +func TestFactoryCustomTargetLoaderForRicherInput(t *testing.T) { + // create factory creating a custom target loader + factory := &Factory{ + build: nil, + canonicalName: "", + config: &customConfig{}, + enabledByDefault: false, + inputPolicy: "", + interruptible: false, + newLoader: func(config *targetloading.Loader, options any) model.ExperimentTargetLoader { + return &customTargetLoader{} + }, + } + + // create config for creating a new target loader + config := &model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + Session: &mocks.Session{ + MockLogger: func() model.Logger { + return model.DiscardLogger + }, + }, + StaticInputs: []string{}, + SourceFiles: []string{}, + } + + // create the loader + loader := factory.NewTargetLoader(config) + + // make sure the type is the one we expected + if _, good := loader.(*customTargetLoader); !good { + t.Fatalf("expected a *customTargetLoader, got %T", loader) + } +} From c9e55e7eded48ae666abd3e7c341cc9042129cba Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 21:55:18 +0200 Subject: [PATCH 13/18] x --- internal/experiment/dnscheck/dnscheck.go | 2 +- internal/experiment/dnscheck/dnscheck_test.go | 41 ++- internal/experiment/dnscheck/richerinput.go | 43 +-- .../experiment/dnscheck/richerinput_test.go | 312 ++++++++++++++++++ .../experiment/dnscheck/testdata/input.txt | 2 + internal/reflectx/reflectx.go | 31 ++ internal/reflectx/reflectx_test.go | 70 ++++ 7 files changed, 468 insertions(+), 33 deletions(-) create mode 100644 internal/experiment/dnscheck/richerinput_test.go create mode 100644 internal/experiment/dnscheck/testdata/input.txt create mode 100644 internal/reflectx/reflectx.go create mode 100644 internal/reflectx/reflectx_test.go diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index af788cea02..e932e7c61f 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -130,7 +130,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { return ErrInputRequired } target := args.Target.(*Target) - config, input := target.options, target.input + config, input := target.Options, target.URL // 1. fill the measurement with test keys tk := new(TestKeys) diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index bfe91eb767..75d7a3f37c 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -57,8 +57,8 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) { Measurement: new(model.Measurement), Session: newsession(), Target: &Target{ - input: "", // explicitly empty - options: &Config{ + URL: "", // explicitly empty + Options: &Config{ Domain: "example.com", }, }, @@ -76,8 +76,8 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) { Measurement: &model.Measurement{Input: "Not a valid URL \x7f"}, Session: newsession(), Target: &Target{ - input: "Not a valid URL \x7f", - options: &Config{}, + URL: "Not a valid URL \x7f", + Options: &Config{}, }, } err := measurer.Run(context.Background(), args) @@ -93,8 +93,8 @@ func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) { Measurement: &model.Measurement{Input: "file://1.1.1.1"}, Session: newsession(), Target: &Target{ - input: "file://1.1.1.1", - options: &Config{}, + URL: "file://1.1.1.1", + Options: &Config{}, }, } err := measurer.Run(context.Background(), args) @@ -113,8 +113,8 @@ func TestWithCancelledContext(t *testing.T) { Measurement: measurement, Session: newsession(), Target: &Target{ - input: "dot://one.one.one.one", - options: &Config{ + URL: "dot://one.one.one.one", + Options: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -125,6 +125,23 @@ func TestWithCancelledContext(t *testing.T) { } } +func TestWithNilTarget(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // immediately cancel the context + measurer := NewExperimentMeasurer() + measurement := &model.Measurement{Input: "dot://one.one.one.one"} + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: measurement, + Session: newsession(), + Target: nil, // explicitly nil + } + err := measurer.Run(ctx, args) + if !errors.Is(err, ErrInputRequired) { + t.Fatal("unexpected err", err) + } +} + func TestMakeResolverURL(t *testing.T) { // test address substitution addr := "255.255.255.0" @@ -161,8 +178,8 @@ func TestDNSCheckValid(t *testing.T) { Measurement: &measurement, Session: newsession(), Target: &Target{ - input: "dot://one.one.one.one:853", - options: &Config{ + URL: "dot://one.one.one.one:853", + Options: &Config{ DefaultAddrs: "1.1.1.1 1.0.0.1", }, }, @@ -210,8 +227,8 @@ func TestDNSCheckWait(t *testing.T) { Measurement: &measurement, Session: newsession(), Target: &Target{ - input: input, - options: &Config{}, + URL: input, + Options: &Config{}, }, } err := measurer.Run(context.Background(), args) diff --git a/internal/experiment/dnscheck/richerinput.go b/internal/experiment/dnscheck/richerinput.go index 758b014319..d512a511a9 100644 --- a/internal/experiment/dnscheck/richerinput.go +++ b/internal/experiment/dnscheck/richerinput.go @@ -4,16 +4,17 @@ import ( "context" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/reflectx" "github.com/ooni/probe-cli/v3/internal/targetloading" ) // Target is a richer-input target that this experiment should measure. type Target struct { - // input is the input. - input string + // Options contains the configuration. + Options *Config - // options is the configuration. - options *Config + // URL is the input URL. + URL string } var _ model.ExperimentTarget = &Target{} @@ -30,12 +31,12 @@ func (t *Target) Country() string { // Input implements [model.ExperimentTarget]. func (t *Target) Input() string { - return t.input + return t.URL } // String implements [model.ExperimentTarget]. func (t *Target) String() string { - return t.input + return t.URL } // NewLoader constructs a new [model.ExperimentTargerLoader] instance. @@ -49,22 +50,24 @@ func NewLoader(loader *targetloading.Loader, gopts any) model.ExperimentTargetLo // Construct the proper loader instance. return &targetLoader{ - loader: loader, - options: options, + defaultInput: defaultInput, + loader: loader, + options: options, } } +// targetLoader loads targets for this experiment. type targetLoader struct { - loader *targetloading.Loader - options *Config + defaultInput []model.ExperimentTarget + loader *targetloading.Loader + options *Config } // Load implements model.ExperimentTargetLoader. func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { - // TODO(bassosimone): we need a way to know whether the options are empty!!! - - // If there's nothing to statically load fallback to the API - if len(tl.loader.StaticInputs) <= 0 && len(tl.loader.SourceFiles) <= 0 { + // If inputs and files are all empty and there are no options, let's use the backend + if len(tl.loader.StaticInputs) <= 0 && len(tl.loader.SourceFiles) <= 0 && + reflectx.StructOrStructPtrIsZero(tl.options) { return tl.loadFromBackend(ctx) } @@ -80,8 +83,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, - input: input, + Options: tl.options, + URL: input, }) } return targets, nil @@ -96,15 +99,15 @@ var defaultInput = []model.ExperimentTarget{ // Make sure we include the typical IP addresses for the domain. // &Target{ - input: "https://dns.google/dns-query", - options: &Config{ + URL: "https://dns.google/dns-query", + Options: &Config{ HTTP3Enabled: true, DefaultAddrs: "8.8.8.8 8.8.4.4", }, }, &Target{ - input: "https://dns.google/dns-query", - options: &Config{ + URL: "https://dns.google/dns-query", + Options: &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 new file mode 100644 index 0000000000..2d9e5dd53d --- /dev/null +++ b/internal/experiment/dnscheck/richerinput_test.go @@ -0,0 +1,312 @@ +package dnscheck + +import ( + "context" + "errors" + "io/fs" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" +) + +func TestTarget(t *testing.T) { + target := &Target{ + URL: "https://dns.google/dns-query", + Options: &Config{ + DefaultAddrs: "8.8.8.8 8.8.4.4", + Domain: "example.com", + HTTP3Enabled: false, + HTTPHost: "dns.google", + TLSServerName: "dns.google.com", + TLSVersion: "TLSv1.3", + }, + } + + t.Run("Category", func(t *testing.T) { + if target.Category() != model.DefaultCategoryCode { + t.Fatal("invalid Category") + } + }) + + t.Run("Country", func(t *testing.T) { + if target.Country() != model.DefaultCountryCode { + t.Fatal("invalid Country") + } + }) + + t.Run("Input", func(t *testing.T) { + if target.Input() != "https://dns.google/dns-query" { + t.Fatal("invalid Input") + } + }) + + t.Run("String", func(t *testing.T) { + if target.String() != "https://dns.google/dns-query" { + t.Fatal("invalid String") + } + }) +} + +func TestNewLoader(t *testing.T) { + // create the pointers we expect to see + child := &targetloading.Loader{} + options := &Config{} + + // create the loader and cast it to its private type + loader := NewLoader(child, options).(*targetLoader) + + // make sure the default input is okay + if diff := cmp.Diff(defaultInput, loader.defaultInput); diff != "" { + t.Fatal(diff) + } + + // make sure the loader is okay + if child != loader.loader { + t.Fatal("invalid loader pointer") + } + + // make sure the options are okay + if options != loader.options { + t.Fatal("invalid options pointer") + } +} + +// testDefaultInput is the default input used by [TestTargetLoaderLoad]. +var testDefaultInput = []model.ExperimentTarget{ + &Target{ + URL: "https://dns.google/dns-query", + Options: &Config{ + HTTP3Enabled: true, + DefaultAddrs: "8.8.8.8 8.8.4.4", + }, + }, + &Target{ + URL: "https://dns.google/dns-query", + Options: &Config{ + DefaultAddrs: "8.8.8.8 8.8.4.4", + }, + }, +} + +func TestTargetLoaderLoad(t *testing.T) { + // testcase is a test case implemented by this function + type testcase struct { + // name is the test case name + name string + + // options contains the options to use + options *Config + + // loader is the loader to use + loader *targetloading.Loader + + // expectErr is the error we expect + expectErr error + + // expectResults contains the expected results + expectTargets []model.ExperimentTarget + } + + cases := []testcase{ + + { + name: "with options, inputs, and files", + options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + loader: &targetloading.Loader{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + ExperimentName: "dnscheck", + InputPolicy: model.InputOrStaticDefault, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{ + "https://dns.cloudflare.com/dns-query", + "https://one.one.one.one/dns-query", + }, + SourceFiles: []string{ + filepath.Join("testdata", "input.txt"), + }, + }, + expectErr: nil, + expectTargets: []model.ExperimentTarget{ + &Target{ + URL: "https://dns.cloudflare.com/dns-query", + Options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, + &Target{ + URL: "https://one.one.one.one/dns-query", + Options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, + &Target{ + URL: "https://1dot1dot1dot1dot.com/dns-query", + Options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, + &Target{ + URL: "https://dns.cloudflare/dns-query", + Options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, + }, + }, + + { + name: "with an unreadable file", + options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + loader: &targetloading.Loader{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + ExperimentName: "dnscheck", + InputPolicy: model.InputOrStaticDefault, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{ + "https://dns.cloudflare.com/dns-query", + "https://one.one.one.one/dns-query", + }, + SourceFiles: []string{ + filepath.Join("testdata", "nonexistent.txt"), + }, + }, + expectErr: fs.ErrNotExist, + expectTargets: nil, + }, + + { + name: "with just inputs", + options: &Config{}, + loader: &targetloading.Loader{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + ExperimentName: "dnscheck", + InputPolicy: model.InputOrStaticDefault, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{ + "https://dns.cloudflare.com/dns-query", + "https://one.one.one.one/dns-query", + }, + SourceFiles: []string{}, + }, + expectErr: nil, + expectTargets: []model.ExperimentTarget{ + &Target{ + URL: "https://dns.cloudflare.com/dns-query", + Options: &Config{}, + }, + &Target{ + URL: "https://one.one.one.one/dns-query", + Options: &Config{}, + }, + }, + }, + + { + name: "with just files", + options: &Config{}, + loader: &targetloading.Loader{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + ExperimentName: "dnscheck", + InputPolicy: model.InputOrStaticDefault, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{}, + SourceFiles: []string{ + filepath.Join("testdata", "input.txt"), + }, + }, + expectErr: nil, + expectTargets: []model.ExperimentTarget{ + &Target{ + URL: "https://1dot1dot1dot1dot.com/dns-query", + Options: &Config{}, + }, + &Target{ + URL: "https://dns.cloudflare/dns-query", + Options: &Config{}, + }, + }, + }, + + { + name: "with just options", + options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + loader: &targetloading.Loader{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + ExperimentName: "dnscheck", + InputPolicy: model.InputOrStaticDefault, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{}, + SourceFiles: []string{}, + }, + expectErr: nil, + expectTargets: nil, + }, + + { + name: "with no options, not inputs, no files", + options: &Config{}, + loader: &targetloading.Loader{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + ExperimentName: "dnscheck", + InputPolicy: model.InputOrStaticDefault, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{}, + SourceFiles: []string{}, + }, + expectErr: nil, + expectTargets: testDefaultInput, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // create a target loader using the given config + // + // note that we use a default test input for results predictability + // since the static list may change over time + tl := &targetLoader{ + defaultInput: testDefaultInput, + loader: tc.loader, + options: tc.options, + } + + // load targets + targets, err := tl.Load(context.Background()) + + // make sure error is consistent + switch { + case err == nil && tc.expectErr == nil: + // fallthrough + + case err != nil && tc.expectErr != nil: + if !errors.Is(err, tc.expectErr) { + t.Fatal("unexpected error", err) + } + // fallthrough + + default: + t.Fatal("expected", tc.expectErr, "got", err) + } + + // make sure the targets are consistent + if diff := cmp.Diff(tc.expectTargets, targets); diff != "" { + t.Fatal(diff) + } + }) + } +} diff --git a/internal/experiment/dnscheck/testdata/input.txt b/internal/experiment/dnscheck/testdata/input.txt new file mode 100644 index 0000000000..42a1896f10 --- /dev/null +++ b/internal/experiment/dnscheck/testdata/input.txt @@ -0,0 +1,2 @@ +https://1dot1dot1dot1dot.com/dns-query +https://dns.cloudflare/dns-query diff --git a/internal/reflectx/reflectx.go b/internal/reflectx/reflectx.go new file mode 100644 index 0000000000..ace8b464d9 --- /dev/null +++ b/internal/reflectx/reflectx.go @@ -0,0 +1,31 @@ +// Package reflectx contains [reflect] extensions. +package reflectx + +import ( + "reflect" + + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +// StructOrStructPtrIsZero returns whether a given struct or struct pointer +// only contains zero value public fields. This function panic if passed a value +// that is neither a pointer to struct nor a struct. This function panics if +// passed a nil struct pointer. +func StructOrStructPtrIsZero(vop any) bool { + vx := reflect.ValueOf(vop) + if vx.Kind() == reflect.Pointer { + vx = vx.Elem() + } + runtimex.Assert(vx.Kind() == reflect.Struct, "not a struct") + tx := vx.Type() + for idx := 0; idx < tx.NumField(); idx++ { + fvx, ftx := vx.Field(idx), tx.Field(idx) + if !ftx.IsExported() { + continue + } + if !fvx.IsZero() { + return false + } + } + return true +} diff --git a/internal/reflectx/reflectx_test.go b/internal/reflectx/reflectx_test.go new file mode 100644 index 0000000000..c8b74c5bbd --- /dev/null +++ b/internal/reflectx/reflectx_test.go @@ -0,0 +1,70 @@ +package reflectx + +import ( + "testing" + + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +type example struct { + _ struct{} + Age int64 + Ch chan int64 + F bool + Fmp map[string]*bool + Fp *bool + Fv []bool + Fvp []*bool + Name string + Ptr *int64 + V []int64 +} + +var nonzero example + +func init() { + ff := &testingx.FakeFiller{} + ff.Fill(&nonzero) +} + +func TestStructOrStructPtrIsZero(t *testing.T) { + + // testcase is a test case implemented by this function + type testcase struct { + // name is the name of the test case + name string + + // input is the input + input any + + // expect is the expected result + expect bool + } + + cases := []testcase{{ + name: "[struct] with zero value", + input: example{}, + expect: true, + }, { + name: "[ptr] with zero value", + input: &example{}, + expect: true, + }, { + name: "[struct] with nonzero value", + input: nonzero, + expect: false, + }, { + name: "[ptr] with nonzero value", + input: &nonzero, + expect: false, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Logf("input: %#v", tc.input) + if got := StructOrStructPtrIsZero(tc.input); got != tc.expect { + t.Fatal("expected", tc.expect, "got", got) + } + }) + } +} From 68f806ddcf402b63811d18162a655fd910e21507 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 22:01:41 +0200 Subject: [PATCH 14/18] x --- internal/experiment/dnscheck/dnscheck_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index 75d7a3f37c..1deaf3d92a 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -125,9 +125,7 @@ func TestWithCancelledContext(t *testing.T) { } } -func TestWithNilTarget(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // immediately cancel the context +func TestDNSCheckFailsWithNilTarget(t *testing.T) { measurer := NewExperimentMeasurer() measurement := &model.Measurement{Input: "dot://one.one.one.one"} args := &model.ExperimentArgs{ @@ -136,7 +134,7 @@ func TestWithNilTarget(t *testing.T) { Session: newsession(), Target: nil, // explicitly nil } - err := measurer.Run(ctx, args) + err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrInputRequired) { t.Fatal("unexpected err", err) } From 21baba002fa42d165c88427dd0aae784d272fd3a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 22:05:20 +0200 Subject: [PATCH 15/18] x --- internal/experiment/dnscheck/richerinput.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/experiment/dnscheck/richerinput.go b/internal/experiment/dnscheck/richerinput.go index d512a511a9..4d4a7ce528 100644 --- a/internal/experiment/dnscheck/richerinput.go +++ b/internal/experiment/dnscheck/richerinput.go @@ -112,8 +112,8 @@ var defaultInput = []model.ExperimentTarget{ }, }, - // TODO(bassosimone): before merging, we need to reinstate the - // whole list that we previously had in tree + // TODO(bassosimone,DecFox): before releasing, we need to either sync up + // this list with ./internal/targetloader or implement a backend API. } func (tl *targetLoader) loadFromBackend(_ context.Context) ([]model.ExperimentTarget, error) { From 6a86d436dce2cae80deb7c40402882be6d09bd86 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 22:19:57 +0200 Subject: [PATCH 16/18] x --- internal/experiment/dnscheck/dnscheck.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index e932e7c61f..49eed47d5c 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -131,6 +131,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { } target := args.Target.(*Target) config, input := target.Options, target.URL + sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input) // 1. fill the measurement with test keys tk := new(TestKeys) From 78ddc811e33d406af0c0b48d6253e7cc3935718c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 7 Jun 2024 09:04:10 +0200 Subject: [PATCH 17/18] x --- internal/reflectx/reflectx.go | 2 +- internal/reflectx/reflectx_test.go | 39 +++++++++---- internal/registry/factory_test.go | 93 ++++++++++++++++++++---------- internal/testingx/fakefill.go | 7 +++ internal/testingx/fakefill_test.go | 49 ++++++++++++++++ 5 files changed, 149 insertions(+), 41 deletions(-) diff --git a/internal/reflectx/reflectx.go b/internal/reflectx/reflectx.go index ace8b464d9..ee26907ae4 100644 --- a/internal/reflectx/reflectx.go +++ b/internal/reflectx/reflectx.go @@ -8,7 +8,7 @@ import ( ) // StructOrStructPtrIsZero returns whether a given struct or struct pointer -// only contains zero value public fields. This function panic if passed a value +// only contains zero-value public fields. This function panic if passed a value // that is neither a pointer to struct nor a struct. This function panics if // passed a nil struct pointer. func StructOrStructPtrIsZero(vop any) bool { diff --git a/internal/reflectx/reflectx_test.go b/internal/reflectx/reflectx_test.go index c8b74c5bbd..f564536aa1 100644 --- a/internal/reflectx/reflectx_test.go +++ b/internal/reflectx/reflectx_test.go @@ -7,17 +7,18 @@ import ( ) type example struct { - _ struct{} - Age int64 - Ch chan int64 - F bool - Fmp map[string]*bool - Fp *bool - Fv []bool - Fvp []*bool - Name string - Ptr *int64 - V []int64 + Age int64 + Ch chan int64 + F bool + Fmp map[string]*bool + Fp *bool + Fv []bool + Fvp []*bool + Name string + Ptr *int64 + V []int64 + namex string + num int64 } var nonzero example @@ -25,6 +26,8 @@ var nonzero example func init() { ff := &testingx.FakeFiller{} ff.Fill(&nonzero) + nonzero.namex = "foo" + nonzero.num = 123 } func TestStructOrStructPtrIsZero(t *testing.T) { @@ -57,6 +60,20 @@ func TestStructOrStructPtrIsZero(t *testing.T) { name: "[ptr] with nonzero value", input: &nonzero, expect: false, + }, { + name: "[struct] with only private fields being nonzero", + input: example{ + namex: "abc", + num: 128, + }, + expect: true, + }, { + name: "[ptr] with only private fields being nonzero", + input: &example{ + namex: "abc", + num: 128, + }, + expect: true, }} for _, tc := range cases { diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index 20a7ffe74b..c92f031577 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -873,37 +873,72 @@ func (c *customTargetLoader) Load(ctx context.Context) ([]model.ExperimentTarget panic("should not be called") } -func TestFactoryCustomTargetLoaderForRicherInput(t *testing.T) { - // create factory creating a custom target loader - factory := &Factory{ - build: nil, - canonicalName: "", - config: &customConfig{}, - enabledByDefault: false, - inputPolicy: "", - interruptible: false, - newLoader: func(config *targetloading.Loader, options any) model.ExperimentTargetLoader { - return &customTargetLoader{} - }, - } +func TestFactoryNewTargetLoader(t *testing.T) { + t.Run("with custom target loader", func(t *testing.T) { + // create factory creating a custom target loader + factory := &Factory{ + build: nil, + canonicalName: "", + config: &customConfig{}, + enabledByDefault: false, + inputPolicy: "", + interruptible: false, + newLoader: func(config *targetloading.Loader, options any) model.ExperimentTargetLoader { + return &customTargetLoader{} + }, + } - // create config for creating a new target loader - config := &model.ExperimentTargetLoaderConfig{ - CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, - Session: &mocks.Session{ - MockLogger: func() model.Logger { - return model.DiscardLogger + // create config for creating a new target loader + config := &model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + Session: &mocks.Session{ + MockLogger: func() model.Logger { + return model.DiscardLogger + }, }, - }, - StaticInputs: []string{}, - SourceFiles: []string{}, - } + StaticInputs: []string{}, + SourceFiles: []string{}, + } - // create the loader - loader := factory.NewTargetLoader(config) + // create the loader + loader := factory.NewTargetLoader(config) - // make sure the type is the one we expected - if _, good := loader.(*customTargetLoader); !good { - t.Fatalf("expected a *customTargetLoader, got %T", loader) - } + // make sure the type is the one we expected + if _, good := loader.(*customTargetLoader); !good { + t.Fatalf("expected a *customTargetLoader, got %T", loader) + } + }) + + t.Run("with default target loader", func(t *testing.T) { + // create factory creating a default target loader + factory := &Factory{ + build: nil, + canonicalName: "", + config: &customConfig{}, + enabledByDefault: false, + inputPolicy: "", + interruptible: false, + newLoader: nil, // explicitly nil + } + + // create config for creating a new target loader + config := &model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ }, + Session: &mocks.Session{ + MockLogger: func() model.Logger { + return model.DiscardLogger + }, + }, + StaticInputs: []string{}, + SourceFiles: []string{}, + } + + // create the loader + loader := factory.NewTargetLoader(config) + + // make sure the type is the one we expected + if _, good := loader.(*targetloading.Loader); !good { + t.Fatalf("expected a *targetloading.Loader, got %T", loader) + } + }) } diff --git a/internal/testingx/fakefill.go b/internal/testingx/fakefill.go index fdd7261d01..d756d74762 100644 --- a/internal/testingx/fakefill.go +++ b/internal/testingx/fakefill.go @@ -91,6 +91,13 @@ func (ff *FakeFiller) doFill(v reflect.Value) { // switch to the element v = v.Elem() } + + // make sure we skip initialization of fields we cannot initialize + // anyway because they're private or immutable + if !v.CanSet() { + return + } + switch v.Type().Kind() { case reflect.String: v.SetString(ff.getRandomString()) diff --git a/internal/testingx/fakefill_test.go b/internal/testingx/fakefill_test.go index 7e3f7f1974..dafe9e10b9 100644 --- a/internal/testingx/fakefill_test.go +++ b/internal/testingx/fakefill_test.go @@ -3,6 +3,8 @@ package testingx import ( "testing" "time" + + "github.com/google/go-cmp/cmp" ) // exampleStructure is an example structure we fill. @@ -89,3 +91,50 @@ func TestFakeFillAllocatesIntoASlice(t *testing.T) { } } } + +func TestFakeFillSkipsPrivateTypes(t *testing.T) { + t.Run("with private struct fields", func(t *testing.T) { + // define structure with mixed private and public fields + type employee struct { + ID int64 + age int64 + name string + } + + // create empty employee + var person employee + + // fake-fill the employee + ff := &FakeFiller{} + ff.Fill(&person) + + // define what we expect to see + expect := employee{ + ID: person.ID, + age: 0, + name: "", + } + + // make sure we've got what we expected + // + // Note: we cannot use cmp.Diff directly because it cannot + // access private fields, so we need to write manual comparison + if person != expect { + t.Fatal("expected", expect, "got", person) + } + }) + + t.Run("make sure we cannot initialize a non-addressable type", func(t *testing.T) { + // create a zero struct + shouldRemainZero := exampleStructure{} + + // attempt to fake fill w/o taking the address + ff := &FakeFiller{} + ff.Fill(shouldRemainZero) + + // make sure it's still zero + if diff := cmp.Diff(exampleStructure{}, shouldRemainZero); diff != "" { + t.Fatal(diff) + } + }) +} From 1533c5bde01130022e91e626a8dde780597e5ae0 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 7 Jun 2024 09:09:59 +0200 Subject: [PATCH 18/18] chore(fakefill): log the structs we fill --- internal/testingx/fakefill_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/testingx/fakefill_test.go b/internal/testingx/fakefill_test.go index dafe9e10b9..4704deb68d 100644 --- a/internal/testingx/fakefill_test.go +++ b/internal/testingx/fakefill_test.go @@ -27,6 +27,7 @@ func TestFakeFillWorksWithCustomTime(t *testing.T) { if req == nil { t.Fatal("we expected non nil here") } + t.Log(req) } func TestFakeFillAllocatesIntoAPointerToPointer(t *testing.T) { @@ -36,6 +37,7 @@ func TestFakeFillAllocatesIntoAPointerToPointer(t *testing.T) { if req == nil { t.Fatal("we expected non nil here") } + t.Log(req) } func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) { @@ -48,6 +50,7 @@ func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) { if len(resp) < 1 { t.Fatal("we expected some data here") } + t.Log(resp) for _, value := range resp { if value == nil { t.Fatal("expected non-nil here") @@ -55,7 +58,7 @@ func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) { } } -func TestFakeFillAllocatesIntoAMapLikeWithNonStringKeys(t *testing.T) { +func TestFakeFillPanicsWithMapsWithNonStringKeys(t *testing.T) { var panicmsg string func() { defer func() { @@ -85,6 +88,7 @@ func TestFakeFillAllocatesIntoASlice(t *testing.T) { if len(*resp) < 1 { t.Fatal("we expected some data here") } + t.Log(resp) for _, entry := range *resp { if entry == nil { t.Fatal("expected non-nil here")