feat(dnscheck): implement richer input

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

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

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
 	}
 }

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,21 +106,40 @@ 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 {
 		t.Fatal(err)
 	}
 }
 
+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 {