From 5a98c815cc28e7e7f57371c4e44702ce529aa24e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 5 Jun 2024 21:26:36 +0200 Subject: [PATCH] feat: introduce richer input Most of the existing code is designed to move around lists of `model.OOAPIURLInfo` and measuring such URLs. This design originally suited Web Connectivity but it's not good enough for richer input because it does not contain options. With this diff, we move into the direction of richer input by replacing `model.OOAPIURLInfo` lists with lists of: ```Go // internal/model/experiment.go type ExperimentTarget struct { Category() string Country() string Input() string } ``` where `*model.OOAPIURLInfo` implements `model.ExperimentTarget` in a trivial way and where, additionally: 1. the `InputLoader` is modified to load `ExperimentTarget`; 2. the `Experiment` is modify to measure an `ExperimentTarget`. In addition to applying these changes, this diff also adapts the whole tree to use `ExperimentTarget` in all places and adds a trivial constructor to obtain `OOAPIURLInfo` when the category code and the country code are unknown. With this diff merged, implementing richer input for real is a matter of implementing the following changes: 1. the `*registry.Factory` has a new func field, defined by each experiment, that loads a list of `ExperimentTarget`; 2. we have a library for input loading containing the same code that we currently use for the input loader; 3. the `InputLoader` is gone and instead we use the factory (or its `*engine.experimentBuilder` wrapper for input loading; 4. we modify the `ExperimentArgs` passed to the `ExperimentMeasurer` to contain an additional field that is the `ExperimentTarget` we want to measure; 5. each experiment that needs richer input type-casts from the `ExperimentTarget` interface to the concrete type that the experiment richer input should have and accesses any option. Part of https://github.com/ooni/probe-cli/pull/1612. This implementation strategy emerged while discussing this matter with @ainghazal, thank you so much for that! --- cmd/ooniprobe/internal/nettests/dash.go | 4 +- cmd/ooniprobe/internal/nettests/dnscheck.go | 2 +- cmd/ooniprobe/internal/nettests/echcheck.go | 4 +- .../internal/nettests/facebook_messenger.go | 4 +- .../http_header_field_manipulation.go | 4 +- .../nettests/http_invalid_request_line.go | 4 +- cmd/ooniprobe/internal/nettests/ndt.go | 4 +- cmd/ooniprobe/internal/nettests/nettests.go | 14 ++-- cmd/ooniprobe/internal/nettests/psiphon.go | 4 +- cmd/ooniprobe/internal/nettests/riseupvpn.go | 5 +- cmd/ooniprobe/internal/nettests/signal.go | 4 +- .../internal/nettests/stunreachability.go | 2 +- cmd/ooniprobe/internal/nettests/telegram.go | 4 +- cmd/ooniprobe/internal/nettests/tor.go | 4 +- cmd/ooniprobe/internal/nettests/torsf.go | 4 +- cmd/ooniprobe/internal/nettests/vanillator.go | 4 +- .../internal/nettests/web_connectivity.go | 2 +- cmd/ooniprobe/internal/nettests/whatsapp.go | 4 +- internal/engine/experiment.go | 7 +- .../engine/experiment_integration_test.go | 12 ++-- internal/engine/inputloader.go | 64 +++++++++++-------- internal/engine/inputloader_test.go | 28 ++++---- internal/mocks/experiment.go | 6 +- internal/mocks/experiment_test.go | 8 ++- internal/mocks/experimentinputloader.go | 4 +- internal/mocks/experimentinputloader_test.go | 2 +- internal/model/experiment.go | 28 ++++++-- internal/model/ooapi.go | 37 +++++++++++ internal/oonirun/experiment.go | 12 ++-- internal/oonirun/experiment_test.go | 26 ++++---- internal/oonirun/inputprocessor.go | 15 ++--- internal/oonirun/inputprocessor_test.go | 40 ++++++------ internal/oonirun/v1_test.go | 3 +- internal/oonirun/v2_test.go | 3 +- pkg/oonimkall/experiment.go | 2 +- pkg/oonimkall/experiment_test.go | 2 +- pkg/oonimkall/taskmocks_test.go | 8 +-- pkg/oonimkall/taskmodel.go | 2 +- pkg/oonimkall/taskrunner.go | 2 +- pkg/oonimkall/taskrunner_test.go | 12 ++-- pkg/oonimkall/webconnectivity.go | 10 ++- 41 files changed, 259 insertions(+), 151 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/dash.go b/cmd/ooniprobe/internal/nettests/dash.go index 80f8618dff..89e073e120 100644 --- a/cmd/ooniprobe/internal/nettests/dash.go +++ b/cmd/ooniprobe/internal/nettests/dash.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // Dash test implementation type Dash struct { } @@ -10,5 +12,5 @@ func (d Dash) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/dnscheck.go b/cmd/ooniprobe/internal/nettests/dnscheck.go index beebcd306d..4fe7de5fbd 100644 --- a/cmd/ooniprobe/internal/nettests/dnscheck.go +++ b/cmd/ooniprobe/internal/nettests/dnscheck.go @@ -10,7 +10,7 @@ import ( // DNSCheck nettest implementation. type DNSCheck struct{} -func (n DNSCheck) lookupURLs(ctl *Controller) ([]string, error) { +func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { inputloader := &engine.InputLoader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine diff --git a/cmd/ooniprobe/internal/nettests/echcheck.go b/cmd/ooniprobe/internal/nettests/echcheck.go index 7d6ff526e8..15a45de713 100644 --- a/cmd/ooniprobe/internal/nettests/echcheck.go +++ b/cmd/ooniprobe/internal/nettests/echcheck.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // ECHCheck nettest implementation. type ECHCheck struct{} @@ -11,5 +13,5 @@ func (n ECHCheck) Run(ctl *Controller) error { } // providing an input containing an empty string causes the experiment // to recognize the empty string and use the default URL - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/facebook_messenger.go b/cmd/ooniprobe/internal/nettests/facebook_messenger.go index 1316babee5..7ecbd1e198 100644 --- a/cmd/ooniprobe/internal/nettests/facebook_messenger.go +++ b/cmd/ooniprobe/internal/nettests/facebook_messenger.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // FacebookMessenger test implementation type FacebookMessenger struct { } @@ -12,5 +14,5 @@ func (h FacebookMessenger) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/http_header_field_manipulation.go b/cmd/ooniprobe/internal/nettests/http_header_field_manipulation.go index 6fdd39688f..09332c20a4 100644 --- a/cmd/ooniprobe/internal/nettests/http_header_field_manipulation.go +++ b/cmd/ooniprobe/internal/nettests/http_header_field_manipulation.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // HTTPHeaderFieldManipulation test implementation type HTTPHeaderFieldManipulation struct { } @@ -12,5 +14,5 @@ func (h HTTPHeaderFieldManipulation) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/http_invalid_request_line.go b/cmd/ooniprobe/internal/nettests/http_invalid_request_line.go index fb87e462d6..d107d2e8b2 100644 --- a/cmd/ooniprobe/internal/nettests/http_invalid_request_line.go +++ b/cmd/ooniprobe/internal/nettests/http_invalid_request_line.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // HTTPInvalidRequestLine test implementation type HTTPInvalidRequestLine struct { } @@ -12,5 +14,5 @@ func (h HTTPInvalidRequestLine) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/ndt.go b/cmd/ooniprobe/internal/nettests/ndt.go index b8848b0056..a862dde106 100644 --- a/cmd/ooniprobe/internal/nettests/ndt.go +++ b/cmd/ooniprobe/internal/nettests/ndt.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // NDT test implementation. We use v7 of NDT since 2020-03-12. type NDT struct { } @@ -11,5 +13,5 @@ func (n NDT) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index 15122ab416..e3cf272510 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -88,24 +88,22 @@ type Controller struct { // - on success, a list of strings containing URLs to test; // // - on failure, an error. -func (c *Controller) BuildAndSetInputIdxMap(testlist []model.OOAPIURLInfo) ([]string, error) { - var urls []string +func (c *Controller) BuildAndSetInputIdxMap(testlist []model.ExperimentTarget) ([]model.ExperimentTarget, error) { urlIDMap := make(map[int64]int64) for idx, url := range testlist { log.Debugf("Going over URL %d", idx) urlID, err := c.Probe.DB().CreateOrUpdateURL( - url.URL, url.CategoryCode, url.CountryCode, + url.Input(), url.Category(), url.Country(), ) if err != nil { log.Error("failed to add to the URL table") return nil, err } - log.Debugf("Mapped URL %s to idx %d and urlID %d", url.URL, idx, urlID) + log.Debugf("Mapped URL %s to idx %d and urlID %d", url.Input(), idx, urlID) urlIDMap[int64(idx)] = urlID - urls = append(urls, url.URL) } c.inputIdxMap = urlIDMap - return urls, nil + return testlist, nil } // SetNettestIndex is used to set the current nettest index and total nettest @@ -120,7 +118,7 @@ func (c *Controller) SetNettestIndex(i, n int) { // // This function will continue to run in most cases but will // immediately halt if something's wrong with the file system. -func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error { +func (c *Controller) Run(builder model.ExperimentBuilder, inputs []model.ExperimentTarget) error { db := c.Probe.DB() // This will configure the controller as handler for the callbacks // called by ooni/probe-engine/experiment.Experiment. @@ -193,7 +191,7 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error } c.msmts[idx64] = msmt - if input != "" { + if input.Input() != "" { c.OnProgress(0, fmt.Sprintf("processing input: %s", input)) } measurement, err := exp.MeasureWithContext(context.Background(), input) diff --git a/cmd/ooniprobe/internal/nettests/psiphon.go b/cmd/ooniprobe/internal/nettests/psiphon.go index 940340cc4e..ffba2bbf33 100644 --- a/cmd/ooniprobe/internal/nettests/psiphon.go +++ b/cmd/ooniprobe/internal/nettests/psiphon.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // Psiphon test implementation type Psiphon struct { } @@ -12,5 +14,5 @@ func (h Psiphon) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/riseupvpn.go b/cmd/ooniprobe/internal/nettests/riseupvpn.go index 185fbcefe4..0f3566df34 100644 --- a/cmd/ooniprobe/internal/nettests/riseupvpn.go +++ b/cmd/ooniprobe/internal/nettests/riseupvpn.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // RiseupVPN test implementation type RiseupVPN struct { } @@ -12,6 +14,5 @@ func (h RiseupVPN) Run(ctl *Controller) error { if err != nil { return err } - - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/signal.go b/cmd/ooniprobe/internal/nettests/signal.go index 3a2df6b874..ceca5f311d 100644 --- a/cmd/ooniprobe/internal/nettests/signal.go +++ b/cmd/ooniprobe/internal/nettests/signal.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // Signal nettest implementation. type Signal struct{} @@ -11,5 +13,5 @@ func (h Signal) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/stunreachability.go b/cmd/ooniprobe/internal/nettests/stunreachability.go index 186bb7bb98..c09ae23cb2 100644 --- a/cmd/ooniprobe/internal/nettests/stunreachability.go +++ b/cmd/ooniprobe/internal/nettests/stunreachability.go @@ -10,7 +10,7 @@ import ( // STUNReachability nettest implementation. type STUNReachability struct{} -func (n STUNReachability) lookupURLs(ctl *Controller) ([]string, error) { +func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { inputloader := &engine.InputLoader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine diff --git a/cmd/ooniprobe/internal/nettests/telegram.go b/cmd/ooniprobe/internal/nettests/telegram.go index 82d75d82c2..87c6099f89 100644 --- a/cmd/ooniprobe/internal/nettests/telegram.go +++ b/cmd/ooniprobe/internal/nettests/telegram.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // Telegram test implementation type Telegram struct { } @@ -12,5 +14,5 @@ func (h Telegram) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/tor.go b/cmd/ooniprobe/internal/nettests/tor.go index 96bfbb7d2f..9f229c31ce 100644 --- a/cmd/ooniprobe/internal/nettests/tor.go +++ b/cmd/ooniprobe/internal/nettests/tor.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // Tor test implementation type Tor struct { } @@ -12,5 +14,5 @@ func (h Tor) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/cmd/ooniprobe/internal/nettests/torsf.go b/cmd/ooniprobe/internal/nettests/torsf.go index 494dae4743..ef3b6425fb 100644 --- a/cmd/ooniprobe/internal/nettests/torsf.go +++ b/cmd/ooniprobe/internal/nettests/torsf.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // TorSf test implementation type TorSf struct { } @@ -10,7 +12,7 @@ func (h TorSf) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } func (h TorSf) onlyBackground() {} diff --git a/cmd/ooniprobe/internal/nettests/vanillator.go b/cmd/ooniprobe/internal/nettests/vanillator.go index 11d7266549..1e154181c7 100644 --- a/cmd/ooniprobe/internal/nettests/vanillator.go +++ b/cmd/ooniprobe/internal/nettests/vanillator.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // VanillaTor test implementation type VanillaTor struct { } @@ -10,7 +12,7 @@ func (h VanillaTor) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } func (h VanillaTor) onlyBackground() {} diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index fc643c0091..5d2fc939ea 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -8,7 +8,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]string, error) { +func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) { inputloader := &engine.InputLoader{ CheckInConfig: &model.OOAPICheckInConfig{ // Setting Charging and OnWiFi to true causes the CheckIn diff --git a/cmd/ooniprobe/internal/nettests/whatsapp.go b/cmd/ooniprobe/internal/nettests/whatsapp.go index 4660abe006..d46953b262 100644 --- a/cmd/ooniprobe/internal/nettests/whatsapp.go +++ b/cmd/ooniprobe/internal/nettests/whatsapp.go @@ -1,5 +1,7 @@ package nettests +import "github.com/ooni/probe-cli/v3/internal/model" + // WhatsApp test implementation type WhatsApp struct { } @@ -12,5 +14,5 @@ func (h WhatsApp) Run(ctl *Controller) error { if err != nil { return err } - return ctl.Run(builder, []string{""}) + return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")}) } diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 41afdddebf..61197c0d8d 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -58,6 +58,8 @@ type experiment struct { testVersion string } +var _ model.Experiment = &experiment{} + // newExperiment creates a new [*experiment] given a [model.ExperimentMeasurer]. func newExperiment(sess *Session, measurer model.ExperimentMeasurer) *experiment { return &experiment{ @@ -180,7 +182,8 @@ func (e *experiment) OpenReportContext(ctx context.Context) error { } // MeasureWithContext implements [model.Experiment]. -func (e *experiment) MeasureWithContext(ctx context.Context, input string) (*model.Measurement, error) { +func (e *experiment) MeasureWithContext( + ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) { // Here we ensure that we have already looked up the probe location // information such that we correctly populate the measurement and also // VERY IMPORTANTLY to scrub the IP address from the measurement. @@ -204,7 +207,7 @@ func (e *experiment) MeasureWithContext(ctx context.Context, input string) (*mod // 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(input) + measurement := e.newMeasurement(target.Input()) // Record when we started the experiment, to compute the runtime. start := time.Now() diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index a9906b6d84..1d9a7f91a3 100644 --- a/internal/engine/experiment_integration_test.go +++ b/internal/engine/experiment_integration_test.go @@ -177,7 +177,8 @@ func TestSetCallbacks(t *testing.T) { } register := ®isterCallbacksCalled{} builder.SetCallbacks(register) - if _, err := builder.NewExperiment().MeasureWithContext(context.Background(), ""); err != nil { + target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("") + if _, err := builder.NewExperiment().MeasureWithContext(context.Background(), target); err != nil { t.Fatal(err) } if register.onProgressCalled == false { @@ -221,7 +222,8 @@ func TestMeasurementFailure(t *testing.T) { if err := builder.SetOptionAny("ReturnError", true); err != nil { t.Fatal(err) } - measurement, err := builder.NewExperiment().MeasureWithContext(context.Background(), "") + target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("") + measurement, err := builder.NewExperiment().MeasureWithContext(context.Background(), target) if err == nil { t.Fatal("expected an error here") } @@ -255,7 +257,8 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string) if experiment.ReportID() == "" { t.Fatal("reportID should not be empty here") } - measurement, err := experiment.MeasureWithContext(ctx, input) + target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input) + measurement, err := experiment.MeasureWithContext(ctx, target) if err != nil { t.Fatal(err) } @@ -413,7 +416,8 @@ func TestMeasureLookupLocationFailure(t *testing.T) { exp := newExperiment(sess, new(antaniMeasurer)) ctx, cancel := context.WithCancel(context.Background()) cancel() // so we fail immediately - if _, err := exp.MeasureWithContext(ctx, "xx"); err == nil { + target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("xx") + if _, err := exp.MeasureWithContext(ctx, target); err == nil { t.Fatal("expected an error here") } } diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 4db0834dc0..9e3737fa51 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -110,7 +110,7 @@ type InputLoader struct { // Load attempts to load input using the specified input loader. We will // return a list of URLs because this is the only input we support. -func (il *InputLoader) Load(ctx context.Context) ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { switch il.InputPolicy { case model.InputOptional: return il.loadOptional() @@ -126,26 +126,29 @@ func (il *InputLoader) Load(ctx context.Context) ([]model.OOAPIURLInfo, error) { } // loadNone implements the InputNone policy. -func (il *InputLoader) loadNone() ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) { if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 { return nil, ErrNoInputExpected } - // Note that we need to return a single empty entry. - return []model.OOAPIURLInfo{{}}, nil + // 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 } // loadOptional implements the InputOptional policy. -func (il *InputLoader) loadOptional() ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err == nil && len(inputs) <= 0 { - // Note that we need to return a single empty entry. - inputs = []model.OOAPIURLInfo{{}} + // Implementation note: the convention for input-less experiments is that + // they require a single entry containing an empty input. + inputs = []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")} } return inputs, err } // loadStrictlyRequired implements the InputStrictlyRequired policy. -func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -154,7 +157,7 @@ func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.OOAPIURL } // loadOrQueryBackend implements the InputOrQueryBackend policy. -func (il *InputLoader) loadOrQueryBackend(ctx context.Context) ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -236,12 +239,12 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // staticInputForExperiment returns the static input for the given experiment // or an error if there's no static input for the experiment. -func staticInputForExperiment(name string) ([]model.OOAPIURLInfo, error) { - return stringListToModelURLInfo(StaticBareInputForExperiment(name)) +func staticInputForExperiment(name string) ([]model.ExperimentTarget, error) { + return inputLoaderStringListToModelExperimentTarget(StaticBareInputForExperiment(name)) } // loadOrStaticDefault implements the InputOrStaticDefault policy. -func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -250,10 +253,10 @@ func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.OOAPIURLI } // loadLocal loads inputs from StaticInputs and SourceFiles. -func (il *InputLoader) loadLocal() ([]model.OOAPIURLInfo, error) { - inputs := []model.OOAPIURLInfo{} +func (il *InputLoader) loadLocal() ([]model.ExperimentTarget, error) { + inputs := []model.ExperimentTarget{} for _, input := range il.StaticInputs { - inputs = append(inputs, model.OOAPIURLInfo{URL: input}) + inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) } for _, filepath := range il.SourceFiles { extra, err := il.readfile(filepath, fsx.OpenFile) @@ -274,8 +277,8 @@ type inputLoaderOpenFn 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 *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.OOAPIURLInfo, error) { - inputs := []model.OOAPIURLInfo{} +func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) { + inputs := []model.ExperimentTarget{} filep, err := open(filepath) if err != nil { return nil, err @@ -288,7 +291,7 @@ func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]mode for scanner.Scan() { line := scanner.Text() if line != "" { - inputs = append(inputs, model.OOAPIURLInfo{URL: line}) + inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(line)) } } if scanner.Err() != nil { @@ -298,7 +301,7 @@ func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]mode } // loadRemote loads inputs from a remote source. -func (il *InputLoader) loadRemote(ctx context.Context) ([]model.OOAPIURLInfo, error) { +func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { config := il.CheckInConfig if config == nil { // Note: Session.CheckIn documentation says it will fill in @@ -314,7 +317,16 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.OOAPIURLInfo, er if reply.WebConnectivity == nil || len(reply.WebConnectivity.URLs) <= 0 { return nil, ErrNoURLsReturned } - return reply.WebConnectivity.URLs, nil + output := inputLoaderWebConnectivityURLsToModelExperimentTarget(reply.WebConnectivity.URLs) + return output, nil +} + +func inputLoaderWebConnectivityURLsToModelExperimentTarget( + inputs []model.OOAPIURLInfo) (outputs []model.ExperimentTarget) { + for _, input := range inputs { + outputs = append(outputs, &input) + } + return } // checkIn executes the check-in and filters the returned URLs to exclude @@ -367,25 +379,21 @@ func (il *InputLoader) logger() InputLoaderLogger { return log.Log } -// stringListToModelURLInfo is an utility function to convert +// inputLoaderStringListToModelExperimentTarget is an utility function to convert // a list of strings containing URLs into a list of model.URLInfo // which would have been returned by an hypothetical backend // API serving input for a test for which we don't have an API // yet (e.g., stunreachability and dnscheck). -func stringListToModelURLInfo(input []string, err error) ([]model.OOAPIURLInfo, error) { +func inputLoaderStringListToModelExperimentTarget(input []string, err error) ([]model.ExperimentTarget, error) { if err != nil { return nil, err } - var output []model.OOAPIURLInfo + var output []model.ExperimentTarget for _, URL := range input { if _, err := url.Parse(URL); err != nil { return nil, err } - output = append(output, model.OOAPIURLInfo{ - CategoryCode: "MISC", // hard to find a category - CountryCode: "XX", // representing no country - URL: URL, - }) + output = append(output, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(URL)) } return output, nil } diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go index 68e1ab5489..0f06e3946e 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/engine/inputloader_test.go @@ -77,7 +77,7 @@ func TestInputLoaderInputNoneWithNoInput(t *testing.T) { if err != nil { t.Fatal(err) } - if len(out) != 1 || out[0].URL != "" { + if len(out) != 1 || out[0].Input() != "" { t.Fatal("not the output we expected") } } @@ -91,7 +91,7 @@ func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { if err != nil { t.Fatal(err) } - if len(out) != 1 || out[0].URL != "" { + if len(out) != 1 || out[0].Input() != "" { t.Fatal("not the output we expected") } } @@ -272,13 +272,13 @@ func TestInputLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) { } for idx := 0; idx < len(dnsCheckDefaultInput); idx++ { e := out[idx] - if e.CategoryCode != "MISC" { + if e.Category() != "MISC" { t.Fatal("invalid category code") } - if e.CountryCode != "XX" { + if e.Country() != "XX" { t.Fatal("invalid country code") } - if e.URL != dnsCheckDefaultInput[idx] { + if e.Input() != dnsCheckDefaultInput[idx] { t.Fatal("invalid URL") } } @@ -299,13 +299,13 @@ func TestInputLoaderInputOrStaticDefaultWithoutInputStunReachability(t *testing. } for idx := 0; idx < len(stunReachabilityDefaultInput); idx++ { e := out[idx] - if e.CategoryCode != "MISC" { + if e.Category() != "MISC" { t.Fatal("invalid category code") } - if e.CountryCode != "XX" { + if e.Country() != "XX" { t.Fatal("invalid country code") } - if e.URL != stunReachabilityDefaultInput[idx] { + if e.Input() != stunReachabilityDefaultInput[idx] { t.Fatal("invalid URL") } } @@ -635,7 +635,7 @@ func TestStringListToModelURLInfoWithValidInput(t *testing.T) { "stun://stun.voip.blackberry.com:3478", "stun://stun.altar.com.pl:3478", } - output, err := stringListToModelURLInfo(input, nil) + output, err := inputLoaderStringListToModelExperimentTarget(input, nil) if err != nil { t.Fatal(err) } @@ -643,13 +643,13 @@ func TestStringListToModelURLInfoWithValidInput(t *testing.T) { t.Fatal("unexpected output length") } for idx := 0; idx < len(input); idx++ { - if input[idx] != output[idx].URL { + if input[idx] != output[idx].Input() { t.Fatal("unexpected entry") } - if output[idx].CategoryCode != "MISC" { + if output[idx].Category() != "MISC" { t.Fatal("unexpected category") } - if output[idx].CountryCode != "XX" { + if output[idx].Country() != "XX" { t.Fatal("unexpected country") } } @@ -661,7 +661,7 @@ func TestStringListToModelURLInfoWithInvalidInput(t *testing.T) { "\t", // <- not a valid URL "stun://stun.altar.com.pl:3478", } - output, err := stringListToModelURLInfo(input, nil) + output, err := inputLoaderStringListToModelExperimentTarget(input, nil) if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { t.Fatal("no the error we expected", err) } @@ -677,7 +677,7 @@ func TestStringListToModelURLInfoWithError(t *testing.T) { "stun://stun.altar.com.pl:3478", } expected := errors.New("mocked error") - output, err := stringListToModelURLInfo(input, expected) + output, err := inputLoaderStringListToModelExperimentTarget(input, expected) if !errors.Is(err, expected) { t.Fatal("no the error we expected", err) } diff --git a/internal/mocks/experiment.go b/internal/mocks/experiment.go index 3c833b8cb8..3063ab3ae1 100644 --- a/internal/mocks/experiment.go +++ b/internal/mocks/experiment.go @@ -17,7 +17,7 @@ type Experiment struct { MockReportID func() string MockMeasureWithContext func( - ctx context.Context, input string) (measurement *model.Measurement, err error) + ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) MockSaveMeasurement func(measurement *model.Measurement, filePath string) error @@ -44,8 +44,8 @@ func (e *Experiment) ReportID() string { } func (e *Experiment) MeasureWithContext( - ctx context.Context, input string) (measurement *model.Measurement, err error) { - return e.MockMeasureWithContext(ctx, input) + ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { + return e.MockMeasureWithContext(ctx, target) } func (e *Experiment) SaveMeasurement(measurement *model.Measurement, filePath string) error { diff --git a/internal/mocks/experiment_test.go b/internal/mocks/experiment_test.go index 017752d178..ecd1f157b9 100644 --- a/internal/mocks/experiment_test.go +++ b/internal/mocks/experiment_test.go @@ -60,11 +60,15 @@ func TestExperiment(t *testing.T) { t.Run("MeasureWithContext", func(t *testing.T) { expected := errors.New("mocked err") e := &Experiment{ - MockMeasureWithContext: func(ctx context.Context, input string) (measurement *model.Measurement, err error) { + MockMeasureWithContext: func( + ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { return nil, expected }, } - out, err := e.MeasureWithContext(context.Background(), "xo") + out, err := e.MeasureWithContext( + context.Background(), + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.example.com/"), + ) if !errors.Is(err, expected) { t.Fatal("unexpected err", err) } diff --git a/internal/mocks/experimentinputloader.go b/internal/mocks/experimentinputloader.go index 5c7234348d..c46db13bef 100644 --- a/internal/mocks/experimentinputloader.go +++ b/internal/mocks/experimentinputloader.go @@ -8,12 +8,12 @@ import ( // ExperimentInputLoader mocks model.ExperimentInputLoader type ExperimentInputLoader struct { - MockLoad func(ctx context.Context) ([]model.OOAPIURLInfo, error) + MockLoad func(ctx context.Context) ([]model.ExperimentTarget, error) } var _ model.ExperimentInputLoader = &ExperimentInputLoader{} // Load calls MockLoad -func (eil *ExperimentInputLoader) Load(ctx context.Context) ([]model.OOAPIURLInfo, error) { +func (eil *ExperimentInputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { return eil.MockLoad(ctx) } diff --git a/internal/mocks/experimentinputloader_test.go b/internal/mocks/experimentinputloader_test.go index a22f456c47..a9d872e4bd 100644 --- a/internal/mocks/experimentinputloader_test.go +++ b/internal/mocks/experimentinputloader_test.go @@ -12,7 +12,7 @@ func TestExperimentInputLoader(t *testing.T) { t.Run("Load", func(t *testing.T) { expected := errors.New("mocked error") eil := &ExperimentInputLoader{ - MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) { + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return nil, expected }, } diff --git a/internal/model/experiment.go b/internal/model/experiment.go index f070cddf21..b725032b79 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -77,6 +77,24 @@ func (d PrinterCallbacks) OnProgress(percentage float64, message string) { d.Logger.Infof("[%5.1f%%] %s", percentage*100, message) } +// ExperimentTarget contains a target for the experiment to measure. +type ExperimentTarget interface { + // Category returns the github.com/citizenlab/test-lists category + // code for this piece of richer input. + // + // Return "MISC" if there's no applicable category code. + Category() string + + // Country returns the country code for this + // piece of richer input. + // + // Return "ZZ" if there's not applicable country code. + Country() string + + // Input returns the experiment input, which typically is a URL. + Input() string +} + // ExperimentArgs contains the arguments passed to an experiment. type ExperimentArgs struct { // Callbacks contains MANDATORY experiment callbacks. @@ -124,11 +142,11 @@ type Experiment interface { // successfully before, or an empty string, otherwise. ReportID() string - // MeasureWithContext performs a synchronous measurement. + // MeasureWithContext measures the given experiment target. // - // Return value: strictly either a non-nil measurement and - // a nil error or a nil measurement and a non-nil error. - MeasureWithContext(ctx context.Context, input string) (measurement *Measurement, err error) + // Return value: either a non-nil measurement and a nil error + // or a nil measurement and a non-nil error. + MeasureWithContext(ctx context.Context, target ExperimentTarget) (measurement *Measurement, err error) // SubmitAndUpdateMeasurementContext submits a measurement and updates the // fields whose value has changed as part of the submission. @@ -212,7 +230,7 @@ type ExperimentOptionInfo struct { // ExperimentInputLoader loads inputs from local or remote sources. type ExperimentInputLoader interface { - Load(ctx context.Context) ([]OOAPIURLInfo, error) + Load(ctx context.Context) ([]ExperimentTarget, error) } // Submitter submits a measurement to the OONI collector. diff --git a/internal/model/ooapi.go b/internal/model/ooapi.go index 940bc4ce5a..ad4bf18a4d 100644 --- a/internal/model/ooapi.go +++ b/internal/model/ooapi.go @@ -147,6 +147,43 @@ type OOAPIURLInfo struct { URL string `json:"url"` } +const ( + // DefaultCategoryCode is the default category code to use + // when a URL's category code is unknown. + DefaultCategoryCode = "MISC" + + // DefaultCountryCode is the default country code to use + // when a URL's country code is unknown. + DefaultCountryCode = "ZZ" +) + +// NewOOAPIURLInfoWithDefaultCategoryAndCountry constructs a new instance +// of [*OOAPIURLInfo] with default category and country code. +func NewOOAPIURLInfoWithDefaultCategoryAndCountry(URL string) *OOAPIURLInfo { + return &OOAPIURLInfo{ + CategoryCode: DefaultCategoryCode, + CountryCode: DefaultCountryCode, + URL: URL, + } +} + +var _ ExperimentTarget = &OOAPIURLInfo{} + +// Category implements ExperimentTarget. +func (o *OOAPIURLInfo) Category() string { + return o.CategoryCode +} + +// Country implements ExperimentTarget. +func (o *OOAPIURLInfo) Country() string { + return o.CountryCode +} + +// Input implements ExperimentTarget. +func (o *OOAPIURLInfo) Input() string { + return o.URL +} + const ( // OOAPIReportDefaultDataFormatVersion is the default data format version. // diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 611dd23fbe..016f94e523 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -70,7 +70,7 @@ type Experiment struct { newSaverFn func() (model.Saver, error) // newInputProcessorFn is OPTIONAL and used for testing. - newInputProcessorFn func(experiment model.Experiment, inputList []model.OOAPIURLInfo, + newInputProcessorFn func(experiment model.Experiment, inputList []model.ExperimentTarget, saver model.Saver, submitter model.Submitter) inputProcessor } @@ -138,7 +138,7 @@ type inputProcessor = model.ExperimentInputProcessor // newInputProcessor creates a new inputProcessor instance. func (ed *Experiment) newInputProcessor(experiment model.Experiment, - inputList []model.OOAPIURLInfo, saver model.Saver, submitter model.Submitter) inputProcessor { + inputList []model.ExperimentTarget, saver model.Saver, submitter model.Submitter) inputProcessor { if ed.newInputProcessorFn != nil { return ed.newInputProcessorFn(experiment, inputList, saver, submitter) } @@ -243,11 +243,11 @@ type experimentWrapper struct { } func (ew *experimentWrapper) MeasureWithContext( - ctx context.Context, input string, idx int) (*model.Measurement, error) { - if input != "" { - ew.logger.Infof("[%d/%d] running with input: %s", idx+1, ew.total, input) + ctx context.Context, target model.ExperimentTarget, idx int) (*model.Measurement, error) { + if target.Input() != "" { + ew.logger.Infof("[%d/%d] running with input: %s", idx+1, ew.total, target.Input()) } - return ew.child.MeasureWithContext(ctx, input, idx) + return ew.child.MeasureWithContext(ctx, target, idx) } // experimentSubmitterWrapper implements a submission policy where we don't diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index cd84e89b23..16d6d16bb4 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -49,7 +49,8 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ - MockMeasureWithContext: func(ctx context.Context, input string) (*model.Measurement, error) { + MockMeasureWithContext: func( + ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) { ff := &testingx.FakeFiller{} var meas model.Measurement ff.Fill(&meas) @@ -167,7 +168,8 @@ func TestExperimentRun(t *testing.T) { newInputLoaderFn func(inputPolicy model.InputPolicy) inputLoader newSubmitterFn func(ctx context.Context) (model.Submitter, error) newSaverFn func() (model.Saver, error) - newInputProcessorFn func(experiment model.Experiment, inputList []model.OOAPIURLInfo, saver model.Saver, submitter model.Submitter) inputProcessor + newInputProcessorFn func(experiment model.Experiment, + inputList []model.ExperimentTarget, saver model.Saver, submitter model.Submitter) inputProcessor } type args struct { ctx context.Context @@ -199,7 +201,7 @@ func TestExperimentRun(t *testing.T) { }, newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { return &mocks.ExperimentInputLoader{ - MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) { + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return nil, errMocked }, } @@ -223,8 +225,8 @@ func TestExperimentRun(t *testing.T) { }, newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { return &mocks.ExperimentInputLoader{ - MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) { - return []model.OOAPIURLInfo{}, nil + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return []model.ExperimentTarget{}, nil }, } }, @@ -263,8 +265,8 @@ func TestExperimentRun(t *testing.T) { }, newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { return &mocks.ExperimentInputLoader{ - MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) { - return []model.OOAPIURLInfo{}, nil + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return []model.ExperimentTarget{}, nil }, } }, @@ -306,8 +308,8 @@ func TestExperimentRun(t *testing.T) { }, newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { return &mocks.ExperimentInputLoader{ - MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) { - return []model.OOAPIURLInfo{}, nil + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return []model.ExperimentTarget{}, nil }, } }, @@ -352,8 +354,8 @@ func TestExperimentRun(t *testing.T) { }, newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { return &mocks.ExperimentInputLoader{ - MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) { - return []model.OOAPIURLInfo{}, nil + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return []model.ExperimentTarget{}, nil }, } }, @@ -363,7 +365,7 @@ func TestExperimentRun(t *testing.T) { newSaverFn: func() (model.Saver, error) { return &mocks.Saver{}, nil }, - newInputProcessorFn: func(experiment model.Experiment, inputList []model.OOAPIURLInfo, + newInputProcessorFn: func(experiment model.Experiment, inputList []model.ExperimentTarget, saver model.Saver, submitter model.Submitter) inputProcessor { return &mocks.ExperimentInputProcessor{ MockRun: func(ctx context.Context) error { diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go index 08e0574b18..d4e292fd63 100644 --- a/internal/oonirun/inputprocessor.go +++ b/internal/oonirun/inputprocessor.go @@ -10,13 +10,13 @@ import ( // InputProcessorExperiment is the Experiment // according to InputProcessor. type InputProcessorExperiment interface { - MeasureWithContext(ctx context.Context, input string) (*model.Measurement, error) + MeasureWithContext(ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) } // InputProcessorExperimentWrapper is a wrapper for an // Experiment that also allow to pass around the input index. type InputProcessorExperimentWrapper interface { - MeasureWithContext(ctx context.Context, input string, idx int) (*model.Measurement, error) + MeasureWithContext(ctx context.Context, target model.ExperimentTarget, idx int) (*model.Measurement, error) } // NewInputProcessorExperimentWrapper creates a new @@ -31,8 +31,8 @@ type inputProcessorExperimentWrapper struct { } func (ipew inputProcessorExperimentWrapper) MeasureWithContext( - ctx context.Context, input string, idx int) (*model.Measurement, error) { - return ipew.exp.MeasureWithContext(ctx, input) + ctx context.Context, target model.ExperimentTarget, idx int) (*model.Measurement, error) { + return ipew.exp.MeasureWithContext(ctx, target) } var _ InputProcessorExperimentWrapper = inputProcessorExperimentWrapper{} @@ -47,7 +47,7 @@ type InputProcessor struct { Experiment InputProcessorExperimentWrapper // Inputs is the list of inputs to measure. - Inputs []model.OOAPIURLInfo + Inputs []model.ExperimentTarget // MaxRuntime is the optional maximum runtime // when looping over a list of inputs (e.g. when @@ -135,12 +135,11 @@ const ( // also returns the reason why we stopped. func (ip *InputProcessor) run(ctx context.Context) (int, error) { start := time.Now() - for idx, url := range ip.Inputs { + for idx, target := range ip.Inputs { if ip.MaxRuntime > 0 && time.Since(start) > ip.MaxRuntime { return stopMaxRuntime, nil } - input := url.URL - meas, err := ip.Experiment.MeasureWithContext(ctx, input, idx) + meas, err := ip.Experiment.MeasureWithContext(ctx, target, idx) if err != nil { return 0, err } diff --git a/internal/oonirun/inputprocessor_test.go b/internal/oonirun/inputprocessor_test.go index 83d32e9f09..a427b5126f 100644 --- a/internal/oonirun/inputprocessor_test.go +++ b/internal/oonirun/inputprocessor_test.go @@ -16,7 +16,7 @@ type FakeInputProcessorExperiment struct { } func (fipe *FakeInputProcessorExperiment) MeasureWithContext( - ctx context.Context, input string) (*model.Measurement, error) { + ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) { if fipe.Err != nil { return nil, fipe.Err } @@ -28,7 +28,7 @@ func (fipe *FakeInputProcessorExperiment) MeasureWithContext( // is MERGING annotations as opposed to overwriting them. m.AddAnnotation("antani", "antani") m.AddAnnotation("foo", "baz") // would be bar below - m.Input = model.MeasurementInput(input) + m.Input = model.MeasurementInput(target.Input()) fipe.M = append(fipe.M, m) return m, nil } @@ -39,9 +39,9 @@ func TestInputProcessorMeasurementFailed(t *testing.T) { Experiment: NewInputProcessorExperimentWrapper( &FakeInputProcessorExperiment{Err: expected}, ), - Inputs: []model.OOAPIURLInfo{{ - URL: "https://www.kernel.org/", - }}, + Inputs: []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), + }, } ctx := context.Background() if err := ip.Run(ctx); !errors.Is(err, expected) { @@ -68,9 +68,9 @@ func TestInputProcessorSubmissionFailed(t *testing.T) { "foo": "bar", }, Experiment: NewInputProcessorExperimentWrapper(fipe), - Inputs: []model.OOAPIURLInfo{{ - URL: "https://www.kernel.org/", - }}, + Inputs: []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), + }, Options: []string{"fake=true"}, Submitter: NewInputProcessorSubmitterWrapper( &FakeInputProcessorSubmitter{Err: expected}, @@ -117,9 +117,9 @@ func TestInputProcessorSaveOnDiskFailed(t *testing.T) { Experiment: NewInputProcessorExperimentWrapper( &FakeInputProcessorExperiment{}, ), - Inputs: []model.OOAPIURLInfo{{ - URL: "https://www.kernel.org/", - }}, + Inputs: []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), + }, Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper( &FakeInputProcessorSaver{Err: expected}, @@ -140,11 +140,10 @@ func TestInputProcessorGood(t *testing.T) { submitter := &FakeInputProcessorSubmitter{Err: nil} ip := &InputProcessor{ Experiment: NewInputProcessorExperimentWrapper(fipe), - Inputs: []model.OOAPIURLInfo{{ - URL: "https://www.kernel.org/", - }, { - URL: "https://www.slashdot.org/", - }}, + Inputs: []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.slashdot.org/"), + }, Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper(saver), Submitter: NewInputProcessorSubmitterWrapper(submitter), @@ -182,11 +181,10 @@ func TestInputProcessorMaxRuntime(t *testing.T) { submitter := &FakeInputProcessorSubmitter{Err: nil} ip := &InputProcessor{ Experiment: NewInputProcessorExperimentWrapper(fipe), - Inputs: []model.OOAPIURLInfo{{ - URL: "https://www.kernel.org/", - }, { - URL: "https://www.slashdot.org/", - }}, + Inputs: []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.kernel.org/"), + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("https://www.slashdot.org/"), + }, MaxRuntime: 1 * time.Nanosecond, Options: []string{"fake=true"}, Saver: NewInputProcessorSaverWrapper(saver), diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index 6e6e473e25..c23455a9fc 100644 --- a/internal/oonirun/v1_test.go +++ b/internal/oonirun/v1_test.go @@ -28,7 +28,8 @@ func newMinimalFakeSession() *mocks.Session { }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ - MockMeasureWithContext: func(ctx context.Context, input string) (*model.Measurement, error) { + MockMeasureWithContext: func( + ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) { ff := &testingx.FakeFiller{} var meas model.Measurement ff.Fill(&meas) diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index f2eab58f5b..c4afa60e96 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -379,7 +379,8 @@ func TestV2MeasureDescriptor(t *testing.T) { }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ - MockMeasureWithContext: func(ctx context.Context, input string) (*model.Measurement, error) { + MockMeasureWithContext: func( + ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) { return nil, expected }, MockKibiBytesReceived: func() float64 { diff --git a/pkg/oonimkall/experiment.go b/pkg/oonimkall/experiment.go index b8c58f5a20..71250c5221 100644 --- a/pkg/oonimkall/experiment.go +++ b/pkg/oonimkall/experiment.go @@ -84,7 +84,7 @@ func (eb *experimentBuilderWrapper) setCallbacks(cb ExperimentCallbacks) { type experiment interface { // MeasureWithContext runs the measurement with the given input // and context. It returns a measurement or an error. - MeasureWithContext(ctx context.Context, input string) ( + MeasureWithContext(ctx context.Context, target model.ExperimentTarget) ( measurement *model.Measurement, err error) // KibiBytesSent returns the number of KiB sent. diff --git a/pkg/oonimkall/experiment_test.go b/pkg/oonimkall/experiment_test.go index f6fa6a4f68..ecb0936306 100644 --- a/pkg/oonimkall/experiment_test.go +++ b/pkg/oonimkall/experiment_test.go @@ -81,7 +81,7 @@ type FakeExperiment struct { } // MeasureWithContext implements experiment.MeasureWithContext. -func (e *FakeExperiment) MeasureWithContext(ctx context.Context, input string) ( +func (e *FakeExperiment) MeasureWithContext(ctx context.Context, target model.ExperimentTarget) ( measurement *model.Measurement, err error) { return e.Measurement, e.Err } diff --git a/pkg/oonimkall/taskmocks_test.go b/pkg/oonimkall/taskmocks_test.go index af58a82dd9..839f519e36 100644 --- a/pkg/oonimkall/taskmocks_test.go +++ b/pkg/oonimkall/taskmocks_test.go @@ -97,7 +97,7 @@ type MockableTaskRunnerDependencies struct { MockableKibiBytesSent func() float64 MockableOpenReportContext func(ctx context.Context) error MockableReportID func() string - MockableMeasureWithContext func(ctx context.Context, input string) ( + MockableMeasureWithContext func(ctx context.Context, target model.ExperimentTarget) ( measurement *model.Measurement, err error) MockableSubmitAndUpdateMeasurementContext func( ctx context.Context, measurement *model.Measurement) error @@ -200,9 +200,9 @@ func (dep *MockableTaskRunnerDependencies) ReportID() string { return dep.MockableReportID() } -func (dep *MockableTaskRunnerDependencies) MeasureWithContext(ctx context.Context, input string) ( - measurement *model.Measurement, err error) { - return dep.MockableMeasureWithContext(ctx, input) +func (dep *MockableTaskRunnerDependencies) MeasureWithContext( + ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { + return dep.MockableMeasureWithContext(ctx, target) } func (dep *MockableTaskRunnerDependencies) SubmitAndUpdateMeasurementContext( diff --git a/pkg/oonimkall/taskmodel.go b/pkg/oonimkall/taskmodel.go index cd60ee9b4f..86f534e01b 100644 --- a/pkg/oonimkall/taskmodel.go +++ b/pkg/oonimkall/taskmodel.go @@ -252,7 +252,7 @@ type taskExperiment interface { ReportID() string // MeasureWithContext runs the measurement. - MeasureWithContext(ctx context.Context, input string) ( + MeasureWithContext(ctx context.Context, target model.ExperimentTarget) ( measurement *model.Measurement, err error) // SubmitAndUpdateMeasurementContext submits the measurement diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go index 161d23ad87..b9a4367dc9 100644 --- a/pkg/oonimkall/taskrunner.go +++ b/pkg/oonimkall/taskrunner.go @@ -284,7 +284,7 @@ func (r *runnerForTask) Run(rootCtx context.Context) { } m, err := experiment.MeasureWithContext( r.contextForExperiment(measCtx, builder), - input, + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input), ) if builder.Interruptible() && measCtx.Err() != nil { // We want to stop here only if interruptible otherwise we want to diff --git a/pkg/oonimkall/taskrunner_test.go b/pkg/oonimkall/taskrunner_test.go index e9f8f4920d..1a3e732062 100644 --- a/pkg/oonimkall/taskrunner_test.go +++ b/pkg/oonimkall/taskrunner_test.go @@ -196,7 +196,7 @@ func TestTaskRunnerRun(t *testing.T) { MockableReportID: func() string { return "20211202T074907Z_example_IT_30722_n1_axDLHNUfJaV1IbuU" }, - MockableMeasureWithContext: func(ctx context.Context, input string) (*model.Measurement, error) { + MockableMeasureWithContext: func(ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) { return &model.Measurement{}, nil }, MockableSubmitAndUpdateMeasurementContext: func(ctx context.Context, measurement *model.Measurement) error { @@ -447,7 +447,7 @@ func TestTaskRunnerRun(t *testing.T) { fake.MockableInputPolicy = func() model.InputPolicy { return model.InputNone } - fake.MockableMeasureWithContext = func(ctx context.Context, input string) (measurement *model.Measurement, err error) { + fake.MockableMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { return nil, errors.New("preconditions error") } runner.sessionBuilder = fake @@ -477,7 +477,7 @@ func TestTaskRunnerRun(t *testing.T) { fake.MockableInputPolicy = func() model.InputPolicy { return model.InputNone } - fake.MockableMeasureWithContext = func(ctx context.Context, input string) (measurement *model.Measurement, err error) { + fake.MockableMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { return nil, errors.New("preconditions error") } runner.sessionBuilder = fake @@ -669,7 +669,7 @@ func TestTaskRunnerRun(t *testing.T) { fake.MockableInputPolicy = func() model.InputPolicy { return model.InputStrictlyRequired } - fake.MockableMeasureWithContext = func(ctx context.Context, input string) (measurement *model.Measurement, err error) { + fake.MockableMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { time.Sleep(1 * time.Second) return &model.Measurement{}, nil } @@ -714,7 +714,7 @@ func TestTaskRunnerRun(t *testing.T) { return true } ctx, cancel := context.WithCancel(context.Background()) - fake.MockableMeasureWithContext = func(ctx context.Context, input string) (measurement *model.Measurement, err error) { + fake.MockableMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { cancel() return &model.Measurement{}, nil } @@ -778,7 +778,7 @@ func TestTaskRunnerRun(t *testing.T) { fake.MockableSetCallbacks = func(cbs model.ExperimentCallbacks) { callbacks = cbs } - fake.MockableMeasureWithContext = func(ctx context.Context, input string) (measurement *model.Measurement, err error) { + fake.MockableMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { callbacks.OnProgress(1, "hello from the fake experiment") return &model.Measurement{}, nil } diff --git a/pkg/oonimkall/webconnectivity.go b/pkg/oonimkall/webconnectivity.go index e96df38d4c..b1c44691d5 100644 --- a/pkg/oonimkall/webconnectivity.go +++ b/pkg/oonimkall/webconnectivity.go @@ -4,10 +4,13 @@ import ( "context" "encoding/json" + "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" ) // WebConnectivityConfig contains settings for WebConnectivity. +// +// Deprecated: this code was just an experiment that we never ended up using. type WebConnectivityConfig struct { // Callbacks contains the experiment callbacks. This field is // optional. Leave it empty and we'll use a default set of @@ -20,6 +23,8 @@ type WebConnectivityConfig struct { } // WebConnectivityResults contains the results of WebConnectivity. +// +// Deprecated: this code was just an experiment that we never ended up using. type WebConnectivityResults struct { // KibiBytesReceived contains the KiB received. KibiBytesReceived float64 @@ -65,7 +70,8 @@ func (r *webConnectivityRunner) run(ctx context.Context, config *WebConnectivity builder.setCallbacks(config.Callbacks) } exp := builder.newExperiment() - measurement, err := exp.MeasureWithContext(ctx, config.Input) + target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(config.Input) + measurement, err := exp.MeasureWithContext(ctx, target) if err != nil { return nil, err } @@ -86,6 +92,8 @@ func (r *webConnectivityRunner) run(ctx context.Context, config *WebConnectivity // // This API is currently experimental. We do not promise that we will bump // the major version number when changing it. +// +// Deprecated: this code was just an experiment that we never ended up using. func (sess *Session) WebConnectivity(ctx *Context, config *WebConnectivityConfig) (*WebConnectivityResults, error) { return (&webConnectivityRunner{sess: sess}).run(ctx.ctx, config) }