refactor: make TargetLoader using ExperimentBuilder

This diff completes the set of preliminary richer input diffs.

We build the TargetLoader using the ExperimentBuilder, which in turn
uses a registry.Factory under the hood.

This means that we can load targets for each experiment.

Part of ooni/probe#2607

cmd/ooniprobe/internal/nettests/dnscheck.go

@@ -4,23 +4,21 @@ import (
 	"context"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
-	"github.com/ooni/probe-cli/v3/internal/targetloading"
 )
 
 // DNSCheck nettest implementation.
 type DNSCheck struct{}
 
-func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
-	targetloader := &targetloading.Loader{
+func (n DNSCheck) lookupURLs(ctl *Controller, builder model.ExperimentBuilder) ([]model.ExperimentTarget, error) {
+	config := &model.ExperimentTargetLoaderConfig{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			// not needed because we have default static input in the engine
 		},
-		ExperimentName: "dnscheck",
-		InputPolicy:    model.InputOrStaticDefault,
-		Session:        ctl.Session,
-		SourceFiles:    ctl.InputFiles,
-		StaticInputs:   ctl.Inputs,
+		Session:      ctl.Session,
+		SourceFiles:  ctl.InputFiles,
+		StaticInputs: ctl.Inputs,
 	}
+	targetloader := builder.NewTargetLoader(config)
 	testlist, err := targetloader.Load(context.Background())
 	if err != nil {
 		return nil, err
@@ -34,7 +32,7 @@ func (n DNSCheck) Run(ctl *Controller) error {
 	if err != nil {
 		return err
 	}
-	urls, err := n.lookupURLs(ctl)
+	urls, err := n.lookupURLs(ctl, builder)
 	if err != nil {
 		return err
 	}

cmd/ooniprobe/internal/nettests/stunreachability.go

@@ -4,23 +4,21 @@ import (
 	"context"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
-	"github.com/ooni/probe-cli/v3/internal/targetloading"
 )
 
 // STUNReachability nettest implementation.
 type STUNReachability struct{}
 
-func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
-	targetloader := &targetloading.Loader{
+func (n STUNReachability) lookupURLs(ctl *Controller, builder model.ExperimentBuilder) ([]model.ExperimentTarget, error) {
+	config := &model.ExperimentTargetLoaderConfig{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			// not needed because we have default static input in the engine
 		},
-		ExperimentName: "stunreachability",
-		InputPolicy:    model.InputOrStaticDefault,
-		Session:        ctl.Session,
-		SourceFiles:    ctl.InputFiles,
-		StaticInputs:   ctl.Inputs,
+		Session:      ctl.Session,
+		SourceFiles:  ctl.InputFiles,
+		StaticInputs: ctl.Inputs,
 	}
+	targetloader := builder.NewTargetLoader(config)
 	testlist, err := targetloader.Load(context.Background())
 	if err != nil {
 		return nil, err
@@ -34,7 +32,7 @@ func (n STUNReachability) Run(ctl *Controller) error {
 	if err != nil {
 		return err
 	}
-	urls, err := n.lookupURLs(ctl)
+	urls, err := n.lookupURLs(ctl, builder)
 	if err != nil {
 		return err
 	}

cmd/ooniprobe/internal/nettests/web_connectivity.go

@@ -5,11 +5,11 @@ import (
 
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/model"
-	"github.com/ooni/probe-cli/v3/internal/targetloading"
 )
 
-func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) {
-	targetloader := &targetloading.Loader{
+func (n WebConnectivity) lookupURLs(
+	ctl *Controller, builder model.ExperimentBuilder, categories []string) ([]model.ExperimentTarget, error) {
+	config := &model.ExperimentTargetLoaderConfig{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			// Setting Charging and OnWiFi to true causes the CheckIn
 			// API to return to us as much URL as possible with the
@@ -21,12 +21,11 @@ func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]mod
 				CategoryCodes: categories,
 			},
 		},
-		ExperimentName: "web_connectivity",
-		InputPolicy:    model.InputOrQueryBackend,
-		Session:        ctl.Session,
-		SourceFiles:    ctl.InputFiles,
-		StaticInputs:   ctl.Inputs,
+		Session:      ctl.Session,
+		SourceFiles:  ctl.InputFiles,
+		StaticInputs: ctl.Inputs,
 	}
+	targetloader := builder.NewTargetLoader(config)
 	testlist, err := targetloader.Load(context.Background())
 	if err != nil {
 		return nil, err
@@ -39,12 +38,12 @@ type WebConnectivity struct{}
 
 // Run starts the test
 func (n WebConnectivity) Run(ctl *Controller) error {
-	log.Debugf("Enabled category codes are the following %v", ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
-	urls, err := n.lookupURLs(ctl, ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
+	builder, err := ctl.Session.NewExperimentBuilder("web_connectivity")
 	if err != nil {
 		return err
 	}
-	builder, err := ctl.Session.NewExperimentBuilder("web_connectivity")
+	log.Debugf("Enabled category codes are the following %v", ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
+	urls, err := n.lookupURLs(ctl, builder, ctl.Probe.Config().Nettests.WebsitesEnabledCategoryCodes)
 	if err != nil {
 		return err
 	}

internal/engine/experimentbuilder.go

@@ -22,6 +22,8 @@ type experimentBuilder struct {
 	session *Session
 }
 
+var _ model.ExperimentBuilder = &experimentBuilder{}
+
 // Interruptible implements ExperimentBuilder.Interruptible.
 func (b *experimentBuilder) Interruptible() bool {
 	return b.factory.Interruptible()
@@ -60,6 +62,11 @@ func (b *experimentBuilder) NewExperiment() model.Experiment {
 	return experiment
 }
 
+// NewTargetLoader creates a new [model.ExperimentTargetLoader] instance.
+func (b *experimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader {
+	return b.factory.NewTargetLoader(config)
+}
+
 // newExperimentBuilder creates a new experimentBuilder instance.
 func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) {
 	factory, err := registry.NewFactory(name, session.kvStore, session.logger)

internal/mocks/experimentbuilder.go

@@ -17,8 +17,12 @@ type ExperimentBuilder struct {
 	MockSetCallbacks func(callbacks model.ExperimentCallbacks)
 
 	MockNewExperiment func() model.Experiment
+
+	MockNewTargetLoader func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader
 }
 
+var _ model.ExperimentBuilder = &ExperimentBuilder{}
+
 func (eb *ExperimentBuilder) Interruptible() bool {
 	return eb.MockInterruptible()
 }
@@ -46,3 +50,7 @@ func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) {
 func (eb *ExperimentBuilder) NewExperiment() model.Experiment {
 	return eb.MockNewExperiment()
 }
+
+func (eb *ExperimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader {
+	return eb.MockNewTargetLoader(config)
+}

internal/model/experiment.go

@@ -232,8 +232,46 @@ type ExperimentBuilder interface {
 	// SetCallbacks sets the experiment's interactive callbacks.
 	SetCallbacks(callbacks ExperimentCallbacks)
 
-	// NewExperiment creates the experiment instance.
+	// NewExperiment creates the [Experiment] instance.
 	NewExperiment() Experiment
+
+	// NewTargetLoader creates the [ExperimentTargetLoader] instance.
+	NewTargetLoader(config *ExperimentTargetLoaderConfig) ExperimentTargetLoader
+}
+
+// ExperimentTargetLoaderConfig is the configuration to create a new [ExperimentTargetLoader].
+//
+// The zero value is not ready to use; please, init the MANDATORY fields.
+type ExperimentTargetLoaderConfig struct {
+	// CheckInConfig contains OPTIONAL options for the CheckIn API. If not set, then we'll create a
+	// default config. If set but there are fields inside it that are not set, then we will set them
+	// to a default value.
+	CheckInConfig *OOAPICheckInConfig
+
+	// Session is the MANDATORY current measurement session.
+	Session ExperimentTargetLoaderSession
+
+	// StaticInputs contains OPTIONAL input to be added
+	// to the resulting input list if possible.
+	StaticInputs []string
+
+	// SourceFiles contains OPTIONAL files to read input
+	// from. Each file should contain a single input string
+	// per line. We will fail if any file is unreadable
+	// as well as if any file is empty.
+	SourceFiles []string
+}
+
+// ExperimentTargetLoaderSession is the session according to [ExperimentTargetLoader].
+type ExperimentTargetLoaderSession interface {
+	// CheckIn invokes the check-in API.
+	CheckIn(ctx context.Context, config *OOAPICheckInConfig) (*OOAPICheckInResult, error)
+
+	// FetchOpenVPNConfig fetches the OpenVPN experiment configuration.
+	FetchOpenVPNConfig(ctx context.Context, provider, cc string) (*OOAPIVPNProviderConfig, error)
+
+	// Logger returns the logger to use.
+	Logger() Logger
 }
 
 // ExperimentOptionInfo contains info about an experiment option.

internal/oonirun/experiment.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/ooni/probe-cli/v3/internal/humanize"
 	"github.com/ooni/probe-cli/v3/internal/model"
-	"github.com/ooni/probe-cli/v3/internal/targetloading"
 )
 
 // experimentShuffledInputs counts how many times we shuffled inputs
@@ -61,7 +60,7 @@ type Experiment struct {
 	newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error)
 
 	// newTargetLoaderFn is OPTIONAL and used for testing.
-	newTargetLoaderFn func(inputPolicy model.InputPolicy) targetLoader
+	newTargetLoaderFn func(builder model.ExperimentBuilder) targetLoader
 
 	// newSubmitterFn is OPTIONAL and used for testing.
 	newSubmitterFn func(ctx context.Context) (model.Submitter, error)
@@ -84,7 +83,7 @@ func (ed *Experiment) Run(ctx context.Context) error {
 	}
 
 	// 2. create input loader and load input for this experiment
-	targetLoader := ed.newTargetLoader(builder.InputPolicy())
+	targetLoader := ed.newTargetLoader(builder)
 	targetList, err := targetLoader.Load(ctx)
 	if err != nil {
 		return err
@@ -196,22 +195,20 @@ func (ed *Experiment) newExperimentBuilder(experimentName string) (model.Experim
 type targetLoader = model.ExperimentTargetLoader
 
 // newTargetLoader creates a new [model.ExperimentTargetLoader].
-func (ed *Experiment) newTargetLoader(inputPolicy model.InputPolicy) targetLoader {
+func (ed *Experiment) newTargetLoader(builder model.ExperimentBuilder) targetLoader {
 	if ed.newTargetLoaderFn != nil {
-		return ed.newTargetLoaderFn(inputPolicy)
+		return ed.newTargetLoaderFn(builder)
 	}
-	return &targetloading.Loader{
+	return builder.NewTargetLoader(&model.ExperimentTargetLoaderConfig{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			RunType:  model.RunTypeManual,
 			OnWiFi:   true, // meaning: not on 4G
 			Charging: true,
 		},
-		ExperimentName: ed.Name,
-		InputPolicy:    inputPolicy,
-		StaticInputs:   ed.Inputs,
-		SourceFiles:    ed.InputFilePaths,
-		Session:        ed.Session,
-	}
+		StaticInputs: ed.Inputs,
+		SourceFiles:  ed.InputFilePaths,
+		Session:      ed.Session,
+	})
 }
 
 // experimentOptionsToStringList convers the options to []string, which is

internal/oonirun/experiment_test.go

@@ -165,7 +165,7 @@ func TestExperimentRun(t *testing.T) {
 		ReportFile             string
 		Session                Session
 		newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error)
-		newTargetLoaderFn      func(inputPolicy model.InputPolicy) targetLoader
+		newTargetLoaderFn      func(builder model.ExperimentBuilder) targetLoader
 		newSubmitterFn         func(ctx context.Context) (model.Submitter, error)
 		newSaverFn             func() (model.Saver, error)
 		newInputProcessorFn    func(experiment model.Experiment,
@@ -199,7 +199,7 @@ func TestExperimentRun(t *testing.T) {
 				}
 				return eb, nil
 			},
-			newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader {
+			newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader {
 				return &mocks.ExperimentTargetLoader{
 					MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) {
 						return nil, errMocked
@@ -223,7 +223,7 @@ func TestExperimentRun(t *testing.T) {
 				}
 				return eb, nil
 			},
-			newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader {
+			newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader {
 				return &mocks.ExperimentTargetLoader{
 					MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) {
 						return []model.ExperimentTarget{}, nil
@@ -263,7 +263,7 @@ func TestExperimentRun(t *testing.T) {
 				}
 				return eb, nil
 			},
-			newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader {
+			newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader {
 				return &mocks.ExperimentTargetLoader{
 					MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) {
 						return []model.ExperimentTarget{}, nil
@@ -306,7 +306,7 @@ func TestExperimentRun(t *testing.T) {
 				}
 				return eb, nil
 			},
-			newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader {
+			newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader {
 				return &mocks.ExperimentTargetLoader{
 					MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) {
 						return []model.ExperimentTarget{}, nil
@@ -352,7 +352,7 @@ func TestExperimentRun(t *testing.T) {
 				}
 				return eb, nil
 			},
-			newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader {
+			newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader {
 				return &mocks.ExperimentTargetLoader{
 					MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) {
 						return []model.ExperimentTarget{}, nil

internal/registry/dash.go

@@ -10,13 +10,15 @@ import (
 )
 
 func init() {
-	AllExperiments["dash"] = func() *Factory {
+	const canonicalName = "dash"
+	AllExperiments[canonicalName] = func() *Factory {
 		return &Factory{
 			build: func(config interface{}) model.ExperimentMeasurer {
 				return dash.NewExperimentMeasurer(
 					*config.(*dash.Config),
 				)
 			},
+			canonicalName:    canonicalName,
 			config:           &dash.Config{},
 			enabledByDefault: true,
 			interruptible:    true,

internal/registry/dnscheck.go

@@ -10,13 +10,15 @@ import (
 )
 
 func init() {
-	AllExperiments["dnscheck"] = func() *Factory {
+	const canonicalName = "dnscheck"
+	AllExperiments[canonicalName] = func() *Factory {
 		return &Factory{
 			build: func(config interface{}) model.ExperimentMeasurer {
 				return dnscheck.NewExperimentMeasurer(
 					*config.(*dnscheck.Config),
 				)
 			},
+			canonicalName:    canonicalName,
 			config:           &dnscheck.Config{},
 			enabledByDefault: true,
 			inputPolicy:      model.InputOrStaticDefault,

internal/registry/dnsping.go

@@ -10,13 +10,15 @@ import (
 )
 
 func init() {
-	AllExperiments["dnsping"] = func() *Factory {
+	const canonicalName = "dnsping"
+	AllExperiments[canonicalName] = func() *Factory {
 		return &Factory{
 			build: func(config interface{}) model.ExperimentMeasurer {
 				return dnsping.NewExperimentMeasurer(
 					*config.(*dnsping.Config),
 				)
 			},
+			canonicalName:    canonicalName,
 			config:           &dnsping.Config{},
 			enabledByDefault: true,
 			inputPolicy:      model.InputOrStaticDefault,

internal/registry/dslxtutorial.go

@@ -10,15 +10,17 @@ import (
 )
 
 func init() {
-	AllExperiments["simple_sni"] = func() *Factory {
+	const canonicalName = "simple_sni"
+	AllExperiments[canonicalName] = func() *Factory {
 		return &Factory{
 			build: func(config interface{}) model.ExperimentMeasurer {
 				return chapter02.NewExperimentMeasurer(
 					*config.(*chapter02.Config),
 				)
 			},
-			config:      &chapter02.Config{},
-			inputPolicy: model.InputOrQueryBackend,
+			canonicalName: canonicalName,
+			config:        &chapter02.Config{},
+			inputPolicy:   model.InputOrQueryBackend,
 		}
 	}
 }