diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 5061ec966..42a237357 100644 --- a/internal/engine/experiment.go +++ b/internal/engine/experiment.go @@ -46,6 +46,10 @@ func (emr *experimentMutableReport) Get() (report probeservices.ReportChannel) { return } +// TODO(bassosimone,DecFox): it would be nice if `*experiment` depended +// on an interface rather than depending on the concrete session, because +// that will allow us to write tests using mocks much more easily. + // experiment implements [model.Experiment]. type experiment struct { byteCounter *bytecounter.Counter diff --git a/internal/engine/experiment_test.go b/internal/engine/experiment_test.go index ceab79d7b..c9be288df 100644 --- a/internal/engine/experiment_test.go +++ b/internal/engine/experiment_test.go @@ -1,9 +1,14 @@ package engine import ( + "sync" "testing" + "time" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/bytecounter" "github.com/ooni/probe-cli/v3/internal/enginelocate" + "github.com/ooni/probe-cli/v3/internal/experiment/dnscheck" "github.com/ooni/probe-cli/v3/internal/experiment/example" "github.com/ooni/probe-cli/v3/internal/experiment/signal" "github.com/ooni/probe-cli/v3/internal/model" @@ -105,3 +110,84 @@ func TestExperimentMeasurementSummaryKeys(t *testing.T) { } }) } + +// This test ensures that (*experiment).newMeasurement is working as intended. +func TestExperimentNewMeasurement(t *testing.T) { + // create a session for testing that does not use the network at all + sess := newSessionForTestingNoLookups(t) + + // create a conventional time for starting the experiment + t0 := time.Date(2024, 6, 27, 10, 33, 0, 0, time.UTC) + + // create the experiment + exp := &experiment{ + byteCounter: bytecounter.New(), + callbacks: model.NewPrinterCallbacks(model.DiscardLogger), + measurer: &dnscheck.Measurer{}, + mrep: &experimentMutableReport{ + mu: sync.Mutex{}, + report: nil, + }, + session: sess, + testName: "dnscheck", + testStartTime: t0.Format(model.MeasurementDateFormat), + testVersion: "0.1.0", + } + + // create the richer input target + target := &dnscheck.Target{ + Config: &dnscheck.Config{ + DefaultAddrs: "8.8.8.8 2001:4860:4860::8888", + HTTP3Enabled: true, + }, + URL: "https://dns.google/dns-query", + } + + // create measurement + meas := exp.newMeasurement(target) + + // make sure the input is correctly serialized + t.Run("Input", func(t *testing.T) { + if meas.Input != "https://dns.google/dns-query" { + t.Fatal("unexpected meas.Input") + } + }) + + // make sure the options are correctly serialized + t.Run("Options", func(t *testing.T) { + expectOptions := []string{`DefaultAddrs=8.8.8.8 2001:4860:4860::8888`, `HTTP3Enabled=true`} + if diff := cmp.Diff(expectOptions, meas.Options); diff != "" { + t.Fatal(diff) + } + }) + + // make sure we've got the expected annotation keys + t.Run("Annotations", func(t *testing.T) { + const ( + expected = 1 << iota + got + ) + m := map[string]int{ + "architecture": expected, + "engine_name": expected, + "engine_version": expected, + "go_version": expected, + "platform": expected, + "vcs_modified": expected, + "vcs_revision": expected, + "vcs_time": expected, + "vcs_tool": expected, + } + for key := range meas.Annotations { + m[key] |= got + } + for key, value := range m { + if value != expected|got { + t.Fatal("expected", expected|got, "for", key, "got", value) + } + } + }) + + // TODO(bassosimone,DecFox): this is the correct place where to + // add more tests regarding how we create measurements. +} diff --git a/internal/experimentconfig/experimentconfig.go b/internal/experimentconfig/experimentconfig.go index eb0f9b91e..a2e321381 100644 --- a/internal/experimentconfig/experimentconfig.go +++ b/internal/experimentconfig/experimentconfig.go @@ -16,7 +16,9 @@ import ( // // 1. we do not serialize options whose name starts with "Safe"; // -// 2. we only serialize scalar values. +// 2. we only serialize scalar values; +// +// 3. we never serialize any zero values. // // This method MUST be passed a pointer to a struct. Otherwise, the return // value will be a zero-length list (either nil or empty). @@ -49,7 +51,7 @@ func DefaultOptionsSerializer(config any) (options []string) { continue } - // add the field iff it's a scalar + // add the field iff it's a nonzero scalar switch fieldval.Kind() { case reflect.Bool, reflect.Int, @@ -65,7 +67,10 @@ func DefaultOptionsSerializer(config any) (options []string) { reflect.Float32, reflect.Float64, reflect.String: - options = append(options, fmt.Sprintf("%s=%s", fieldtype.Name, fieldval.Interface())) + if fieldval.IsZero() { + continue + } + options = append(options, fmt.Sprintf("%s=%v", fieldtype.Name, fieldval.Interface())) default: // nothing diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 202158a73..31b80f01f 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -104,9 +104,8 @@ type ExperimentTarget interface { // using the command line `-O option=value` syntax. // // This method MUST NOT serialize all the options whose name - // starts with the "Safe" prefix. This method MAY skip serializing - // sensitive options and options we cannot serialize into a list - // of strings (e.g., objects and lists). + // starts with the "Safe" prefix. This method MUST skip serializing + // sensitive options, non-scalar options, and zero value options. // // Consider using the [experimentconfig] package to serialize. Options() []string