refactor: construct TargetLoader using ExperimentBuilder (#1617)

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, because the actual implementation of the TargetLoader
can be experiment dependent.

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/engine/experimentbuilder_test.go

@@ -1 +1,51 @@
 package engine
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/ooni/probe-cli/v3/internal/model"
+)
+
+func TestExperimentBuilderEngineWebConnectivity(t *testing.T) {
+	// create a session for testing that does not use the network at all
+	sess := newSessionForTestingNoLookups(t)
+
+	// create an experiment builder for Web Connectivity
+	builder, err := sess.NewExperimentBuilder("WebConnectivity")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// create suitable loader config
+	config := &model.ExperimentTargetLoaderConfig{
+		CheckInConfig: &model.OOAPICheckInConfig{
+			// nothing
+		},
+		Session:      sess,
+		StaticInputs: nil,
+		SourceFiles:  nil,
+	}
+
+	// create the loader
+	loader := builder.NewTargetLoader(config)
+
+	// create cancelled context to interrupt immediately so that we
+	// don't use the network when running this test
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	// attempt to load targets
+	targets, err := loader.Load(ctx)
+
+	// make sure we've got the expected error
+	if !errors.Is(err, context.Canceled) {
+		t.Fatal("unexpected err", err)
+	}
+
+	// make sure there are no targets
+	if len(targets) != 0 {
+		t.Fatal("expected zero length targets")
+	}
+}

internal/experimentname/experimentname.go

@@ -0,0 +1,25 @@
+// Package experimentname contains code to manipulate experiment names.
+package experimentname
+
+import "github.com/ooni/probe-cli/v3/internal/strcasex"
+
+// Canonicalize allows code to provide experiment names
+// in a more flexible way, where we have aliases.
+//
+// Because we allow for uppercase experiment names for backwards
+// compatibility with MK, we need to add some exceptions here when
+// mapping (e.g., DNSCheck => dnscheck).
+func Canonicalize(name string) string {
+	switch name = strcasex.ToSnake(name); name {
+	case "ndt_7":
+		name = "ndt" // since 2020-03-18, we use ndt7 to implement ndt by default
+	case "dns_check":
+		name = "dnscheck"
+	case "stun_reachability":
+		name = "stunreachability"
+	case "web_connectivity@v_0_5":
+		name = "[email protected]"
+	default:
+	}
+	return name
+}

internal/experimentname/experimentname_test.go

@@ -0,0 +1,55 @@
+// Package experimentname contains code to manipulate experiment names.
+package experimentname
+
+import "testing"
+
+func TestCanonicalize(t *testing.T) {
+	tests := []struct {
+		input  string
+		expect string
+	}{
+		{
+			input:  "example",
+			expect: "example",
+		},
+		{
+			input:  "Example",
+			expect: "example",
+		},
+		{
+			input:  "ndt7",
+			expect: "ndt",
+		},
+		{
+			input:  "Ndt7",
+			expect: "ndt",
+		},
+		{
+			input:  "DNSCheck",
+			expect: "dnscheck",
+		},
+		{
+			input:  "dns_check",
+			expect: "dnscheck",
+		},
+		{
+			input:  "STUNReachability",
+			expect: "stunreachability",
+		},
+		{
+			input:  "stun_reachability",
+			expect: "stunreachability",
+		},
+		{
+			input:  "[email protected]",
+			expect: "[email protected]",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			if got := Canonicalize(tt.input); got != tt.expect {
+				t.Errorf("Canonicalize() = %v, want %v", got, tt.expect)
+			}
+		})
+	}
+}

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/mocks/experimentbuilder_test.go

@@ -96,4 +96,16 @@ func TestExperimentBuilder(t *testing.T) {
 			t.Fatal("invalid result")
 		}
 	})
+
+	t.Run("NewTargetLoader", func(t *testing.T) {
+		tloader := &ExperimentTargetLoader{}
+		eb := &ExperimentBuilder{
+			MockNewTargetLoader: func(*model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader {
+				return tloader
+			},
+		}
+		if out := eb.NewTargetLoader(&model.ExperimentTargetLoaderConfig{}); out != tloader {
+			t.Fatal("invalid result")
+		}
+	})
 }

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.