refactor: move engine.InputLoader to inputloading

This commit moves the engine.InputLoader type to a new package called
inputloading and adapts the naming to avoid stuttering.

The reason for moving InputLoader is that the engine package depends on
registry, and, per the plan described by the first richer input PR,
#1615, we want to move input loading
directly inside the registry. To this end, we need to move the input
loading feature outside of engine to avoid creating import loops.

We keep an integration test inside the engine package because it seems
such an integration test was checking both engine and the InputLoader
together. We may further refactor this test in the future.

Part of #1612

cmd/ooniprobe/internal/nettests/dnscheck.go

@@ -3,15 +3,15 @@ package nettests
 import (
 	"context"
 
-	engine "github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/inputloading"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 // DNSCheck nettest implementation.
 type DNSCheck struct{}
 
 func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
-	inputloader := &engine.InputLoader{
+	inputloader := &inputloading.Loader{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			// not needed because we have default static input in the engine
 		},

cmd/ooniprobe/internal/nettests/stunreachability.go

@@ -3,15 +3,15 @@ package nettests
 import (
 	"context"
 
-	engine "github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/inputloading"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 // STUNReachability nettest implementation.
 type STUNReachability struct{}
 
 func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
-	inputloader := &engine.InputLoader{
+	inputloader := &inputloading.Loader{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			// not needed because we have default static input in the engine
 		},

cmd/ooniprobe/internal/nettests/web_connectivity.go

@@ -4,12 +4,12 @@ import (
 	"context"
 
 	"github.com/apex/log"
-	engine "github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/inputloading"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) {
-	inputloader := &engine.InputLoader{
+	inputloader := &inputloading.Loader{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			// Setting Charging and OnWiFi to true causes the CheckIn
 			// API to return to us as much URL as possible with the

internal/engine/inputloader_network_test.go → internal/engine/inputloader_integration_test.go

@@ -6,10 +6,17 @@ import (
 
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/inputloading"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
+// This historical integration test ensures that we're able to fetch URLs from
+// the dev infrastructure. We say this test's historical because the inputloading.Loader
+// belonged to the engine package before we introduced richer input. It kind of feels
+// good to keep this integration test here since we want to use a real session and a real
+// Loader and double check whether we can get inputs. In a more distant future it would
+// kind of make sense to have a broader package with this kind of integration tests.
 func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
@@ -29,7 +36,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer sess.Close()
-	il := &engine.InputLoader{
+	il := &inputloading.Loader{
 		InputPolicy: model.InputOrQueryBackend,
 		Session:     sess,
 	}

internal/engine/inputloader.go → internal/inputloading/inputloading.go

@@ -1,4 +1,5 @@
-package engine
+// Package inputloading contains common code to load input.
+package inputloading
 
 import (
 	"bufio"
@@ -16,7 +17,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/stuninput"
 )
 
-// These errors are returned by the InputLoader.
+// These errors are returned by the [*Loader].
 var (
 	ErrNoURLsReturned    = errors.New("no URLs returned")
 	ErrDetectedEmptyFile = errors.New("file did not contain any input")
@@ -25,21 +26,20 @@ var (
 	ErrNoStaticInput     = errors.New("no static input for this experiment")
 )
 
-// InputLoaderSession is the session according to an InputLoader. We
-// introduce this abstraction because it helps us with testing.
-type InputLoaderSession interface {
+// Session is the session according to a [*Loader].
+type Session interface {
 	CheckIn(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error)
 	FetchOpenVPNConfig(ctx context.Context,
 		provider, cc string) (*model.OOAPIVPNProviderConfig, error)
 }
 
-// InputLoaderLogger is the logger according to an InputLoader.
-type InputLoaderLogger interface {
+// Logger is the [model.Logger] according to a [*Loader].
+type Logger interface {
 	// Warnf formats and emits a warning message.
 	Warnf(format string, v ...interface{})
 }
 
-// InputLoader loads input according to the specified policy
+// Loader loads input according to the specified policy
 // either from command line and input files or from OONI services. The
 // behaviour depends on the input policy as described below.
 //
@@ -74,7 +74,7 @@ type InputLoaderLogger interface {
 //
 // We gather input from StaticInput and SourceFiles. If there is
 // input, we return it. Otherwise, we return an error.
-type InputLoader struct {
+type Loader struct {
 	// CheckInConfig contains 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
@@ -91,14 +91,14 @@ type InputLoader struct {
 	// this field.
 	InputPolicy model.InputPolicy
 
-	// Logger is the optional logger that the InputLoader
+	// Logger is the optional logger that the [*Loader]
 	// should be using. If not set, we will use the default
 	// logger of github.com/apex/log.
-	Logger InputLoaderLogger
+	Logger Logger
 
 	// Session is the current measurement session. You
 	// MUST fill in this field.
-	Session InputLoaderSession
+	Session Session
 
 	// StaticInputs contains optional input to be added
 	// to the resulting input list if possible.
@@ -113,7 +113,7 @@ type InputLoader struct {
 
 // Load attempts to load input using the specified input loader. We will
 // return a list of URLs because this is the only input we support.
-func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) Load(ctx context.Context) ([]model.ExperimentTarget, error) {
 	switch il.InputPolicy {
 	case model.InputOptional:
 		return il.loadOptional()
@@ -129,7 +129,7 @@ func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, erro
 }
 
 // loadNone implements the InputNone policy.
-func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) {
+func (il *Loader) loadNone() ([]model.ExperimentTarget, error) {
 	if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 {
 		return nil, ErrNoInputExpected
 	}
@@ -140,7 +140,7 @@ func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) {
 }
 
 // loadOptional implements the InputOptional policy.
-func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) {
+func (il *Loader) loadOptional() ([]model.ExperimentTarget, error) {
 	inputs, err := il.loadLocal()
 	if err == nil && len(inputs) <= 0 {
 		// Implementation note: the convention for input-less experiments is that
@@ -151,7 +151,7 @@ func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) {
 }
 
 // loadStrictlyRequired implements the InputStrictlyRequired policy.
-func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) {
 	inputs, err := il.loadLocal()
 	if err != nil || len(inputs) > 0 {
 		return inputs, err
@@ -160,7 +160,7 @@ func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.Experime
 }
 
 // loadOrQueryBackend implements the InputOrQueryBackend policy.
-func (il *InputLoader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) {
 	inputs, err := il.loadLocal()
 	if err != nil || len(inputs) > 0 {
 		return inputs, err
@@ -247,7 +247,7 @@ func staticInputForExperiment(name string) ([]model.ExperimentTarget, error) {
 }
 
 // loadOrStaticDefault implements the InputOrStaticDefault policy.
-func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) {
 	inputs, err := il.loadLocal()
 	if err != nil || len(inputs) > 0 {
 		return inputs, err
@@ -256,7 +256,7 @@ func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.Experimen
 }
 
 // loadLocal loads inputs from StaticInputs and SourceFiles.
-func (il *InputLoader) loadLocal() ([]model.ExperimentTarget, error) {
+func (il *Loader) loadLocal() ([]model.ExperimentTarget, error) {
 	inputs := []model.ExperimentTarget{}
 	for _, input := range il.StaticInputs {
 		inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input))
@@ -280,7 +280,7 @@ type inputLoaderOpenFn 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 *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) {
+func (il *Loader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) {
 	inputs := []model.ExperimentTarget{}
 	filep, err := open(filepath)
 	if err != nil {
@@ -304,7 +304,7 @@ func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]mode
 }
 
 // loadRemote loads inputs from a remote source.
-func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) {
 	switch registry.CanonicalizeExperimentName(il.ExperimentName) {
 	case "openvpn":
 		// TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass
@@ -319,7 +319,7 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget
 }
 
 // loadRemoteWebConnectivity loads webconnectivity inputs from a remote source.
-func (il *InputLoader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) {
 	config := il.CheckInConfig
 	if config == nil {
 		// Note: Session.CheckIn documentation says it will fill in
@@ -356,7 +356,7 @@ func inputLoaderModelOOAPIURLInfoToModelExperimentTarget(
 }
 
 // loadRemoteOpenVPN loads openvpn inputs from a remote source.
-func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) {
+func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) {
 	// VPN Inputs do not match exactly the semantics expected from [model.OOAPIURLInfo],
 	// since OOAPIURLInfo is oriented towards webconnectivity,
 	// but we force VPN targets in the URL and ignore all the other fields.
@@ -393,7 +393,7 @@ func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.Experimen
 // checkIn executes the check-in and filters the returned URLs to exclude
 // the URLs that are not part of the requested categories. This is done for
 // robustness, just in case we or the API do something wrong.
-func (il *InputLoader) checkIn(
+func (il *Loader) checkIn(
 	ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
 	reply, err := il.Session.CheckIn(ctx, config)
 	if err != nil {
@@ -409,7 +409,7 @@ func (il *InputLoader) checkIn(
 }
 
 // fetchOpenVPNConfig fetches vpn information for the configured providers
-func (il *InputLoader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) {
+func (il *Loader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) {
 	reply, err := il.Session.FetchOpenVPNConfig(ctx, provider, "XX")
 	if err != nil {
 		return nil, err
@@ -420,7 +420,7 @@ func (il *InputLoader) fetchOpenVPNConfig(ctx context.Context, provider string)
 // preventMistakes makes the code more robust with respect to any possible
 // integration issue where the backend returns to us URLs that don't
 // belong to the category codes we requested.
-func (il *InputLoader) preventMistakes(input []model.OOAPIURLInfo, categories []string) (output []model.OOAPIURLInfo) {
+func (il *Loader) preventMistakes(input []model.OOAPIURLInfo, categories []string) (output []model.OOAPIURLInfo) {
 	if len(categories) <= 0 {
 		return input
 	}
@@ -442,7 +442,7 @@ func (il *InputLoader) preventMistakes(input []model.OOAPIURLInfo, categories []
 }
 
 // logger returns the configured logger or apex/log's default.
-func (il *InputLoader) logger() InputLoaderLogger {
+func (il *Loader) logger() Logger {
 	if il.Logger != nil {
 		return il.Logger
 	}