From 41a42826541921485e5d72e4fbd3b15da2e123a2 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 7 Jun 2024 09:18:12 +0200 Subject: [PATCH 1/2] feat(dnscheck): implement richer input (#1618) This diff: 1. modifies `./internal/registry` and its `dnscheck.go` file such that `dnscheck` has its own private target loader; 2. modifies `./internal/targetloading` to allow reusing the code to read static input from CLI and from files; 3. modifies `./internal/model` to pass optional richer input to experiments; 4. modifies `./internal/engine` to pass richer input to experiments; 5. modifies `./internal/experiment/dnscheck` to read and use the richer input it is passed; 6. implements a custom target loader for `./internal/experiment/dnscheck` returning some static entries; 7. modifies `./internal/oonirun` to make sure richer-input experiments honor their options by moving option loading before the loading of targets. Once this diff is merged, `miniooni` and `ooniprobe` will run `dnscheck` with richer input. Obviously, we are still lacking an API to fetch richer input, so for now this is static richer input, suitable as a starting point to land more diffs. Part of https://github.com/ooni/probe/issues/2607. --- internal/engine/experiment.go | 29 +- internal/engine/experiment_test.go | 2 +- internal/experiment/dnscheck/dnscheck.go | 50 +-- internal/experiment/dnscheck/dnscheck_test.go | 89 +++-- internal/experiment/dnscheck/richerinput.go | 123 +++++++ .../experiment/dnscheck/richerinput_test.go | 312 ++++++++++++++++++ .../experiment/dnscheck/testdata/input.txt | 2 + internal/model/experiment.go | 6 + internal/oonirun/experiment.go | 28 +- internal/oonirun/experiment_test.go | 27 +- internal/reflectx/reflectx.go | 31 ++ internal/reflectx/reflectx_test.go | 87 +++++ internal/registry/dnscheck.go | 7 +- internal/registry/example.go | 5 + internal/registry/factory.go | 15 +- internal/registry/factory_test.go | 82 +++++ internal/targetloading/targetloading.go | 56 ++-- internal/targetloading/targetloading_test.go | 5 +- internal/testingx/fakefill.go | 7 + internal/testingx/fakefill_test.go | 55 ++- 20 files changed, 909 insertions(+), 109 deletions(-) create mode 100644 internal/experiment/dnscheck/richerinput.go 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/engine/experiment.go b/internal/engine/experiment.go index 61197c0d8d..5f6ac5d874 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -109,11 +109,16 @@ 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): move here code that supports filling the options field + // when there is richer input, which currently is inside ./internal/oonirun. + // + // We MUST do this because the current solution only works for OONI Run and when + // there are command line options but does not work for API/static targets. m := &model.Measurement{ DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion, - Input: model.MeasurementInput(input), + Input: model.MeasurementInput(target.Input()), MeasurementStartTime: utctimenow.Format(model.MeasurementDateFormat), MeasurementStartTimeSaved: utctimenow, ProbeIP: model.DefaultProbeIP, @@ -204,19 +209,29 @@ 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 + // 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. + // + // 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() - // 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, 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..49eed47d5c 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 ( @@ -96,7 +97,6 @@ type TestKeys struct { // Measurer performs the measurement. type Measurer struct { - Config Endpoints *Endpoints } @@ -114,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 +125,14 @@ 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 + if args.Target == nil { + return ErrInputRequired + } + 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) tk.Lookups = make(map[string]urlgetter.TestKeys) @@ -133,20 +141,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 +198,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 +215,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 +251,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 +318,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..1deaf3d92a 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{ + URL: "", // 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{ + URL: "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{ + URL: "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{ + URL: "dot://one.one.one.one", + Options: &Config{ + DefaultAddrs: "1.1.1.1 1.0.0.1", + }, + }, } err := measurer.Run(ctx, args) if err != nil { @@ -111,6 +125,21 @@ func TestWithCancelledContext(t *testing.T) { } } +func TestDNSCheckFailsWithNilTarget(t *testing.T) { + 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(context.Background(), args) + if !errors.Is(err, ErrInputRequired) { + t.Fatal("unexpected err", err) + } +} + func TestMakeResolverURL(t *testing.T) { // test address substitution addr := "255.255.255.0" @@ -140,14 +169,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{ + URL: "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 +202,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 +224,10 @@ func TestDNSCheckWait(t *testing.T) { Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: &measurement, Session: newsession(), + Target: &Target{ + URL: 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..4d4a7ce528 --- /dev/null +++ b/internal/experiment/dnscheck/richerinput.go @@ -0,0 +1,123 @@ +package dnscheck + +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 { + // Options contains the configuration. + Options *Config + + // URL is the input URL. + URL string +} + +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.URL +} + +// String implements [model.ExperimentTarget]. +func (t *Target) String() string { + return t.URL +} + +// 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{ + defaultInput: defaultInput, + loader: loader, + options: options, + } +} + +// targetLoader loads targets for this experiment. +type targetLoader struct { + defaultInput []model.ExperimentTarget + loader *targetloading.Loader + options *Config +} + +// Load implements model.ExperimentTargetLoader. +func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { + // 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) + } + + // 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, + URL: 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{ + 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", + }, + }, + + // 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) { + // 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/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/model/experiment.go b/internal/model/experiment.go index 8bd309f326..c15bc3ee2d 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -123,6 +123,12 @@ type ExperimentArgs struct { // Session is the MANDATORY session the experiment can use. Session ExperimentSession + + // 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 } // ExperimentMeasurer is the interface that allows to run a diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 758db9e0c5..c4613107d7 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -82,27 +82,37 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // 2. create input loader and load input for this experiment + // 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 + // possibly be used to produce richer input targets. + if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { + return err + } + + // 3. create target loader and load targets 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) { + // 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) } - // 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/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 }, } }, diff --git a/internal/reflectx/reflectx.go b/internal/reflectx/reflectx.go new file mode 100644 index 0000000000..ee26907ae4 --- /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..f564536aa1 --- /dev/null +++ b/internal/reflectx/reflectx_test.go @@ -0,0 +1,87 @@ +package reflectx + +import ( + "testing" + + "github.com/ooni/probe-cli/v3/internal/testingx" +) + +type example struct { + 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 + +func init() { + ff := &testingx.FakeFiller{} + ff.Fill(&nonzero) + nonzero.namex = "foo" + nonzero.num = 123 +} + +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, + }, { + 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 { + 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) + } + }) + } +} diff --git a/internal/registry/dnscheck.go b/internal/registry/dnscheck.go index 2890ca19a5..009078aa6b 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,DecFox): 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/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.go b/internal/registry/factory.go index 0b37a79ee2..ee73e95a8f 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,8 @@ type Session = model.ExperimentTargetLoaderSession // NewTargetLoader creates a new [model.ExperimentTargetLoader] instance. func (b *Factory) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { - return &targetloading.Loader{ + // Construct the default loader used in the non-richer input case. + loader := &targetloading.Loader{ CheckInConfig: config.CheckInConfig, // OPTIONAL ExperimentName: b.canonicalName, InputPolicy: b.inputPolicy, @@ -53,6 +57,15 @@ 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 } // Interruptible returns whether the experiment is interruptible. diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index 60105d1354..c92f031577 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,84 @@ 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 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 + }, + }, + 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) + } + }) + + 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/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") } 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..4704deb68d 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. @@ -25,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) { @@ -34,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) { @@ -46,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") @@ -53,7 +58,7 @@ func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) { } } -func TestFakeFillAllocatesIntoAMapLikeWithNonStringKeys(t *testing.T) { +func TestFakeFillPanicsWithMapsWithNonStringKeys(t *testing.T) { var panicmsg string func() { defer func() { @@ -83,9 +88,57 @@ 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") } } } + +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 9c169e416eaaed982fa8004564220da3334ec861 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 7 Jun 2024 09:24:12 +0200 Subject: [PATCH 2/2] x --- pkg/oonimkall/taskmodel.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/oonimkall/taskmodel.go b/pkg/oonimkall/taskmodel.go index 3bd6ccfee5..e030cbc8f7 100644 --- a/pkg/oonimkall/taskmodel.go +++ b/pkg/oonimkall/taskmodel.go @@ -162,6 +162,12 @@ type event struct { // taskSession abstracts a OONI session. type taskSession interface { + // A session should be used by an experiment. + model.ExperimentSession + + // A session should be used when loading targets. + model.ExperimentTargetLoaderSession + // A session can be closed. io.Closer @@ -196,12 +202,6 @@ type taskSession interface { // ResolverNetworkName must be called after MaybeLookupLocationContext // and returns the resolved resolver's network name. ResolverNetworkName() string - - // A session should be used by an experiment. - model.ExperimentSession - - // A session should be used when loading targets. - model.ExperimentTargetLoaderSession } //