refactor(inputloader): better docs and naming (#265)

* refactor(inputloader): better docs and naming

Work done as part of https://github.com/ooni/probe/issues/1299.

* fix: correct a typo

cmd/ooniprobe/internal/nettests/web_connectivity.go

@@ -10,7 +10,7 @@ import (
 
 func lookupURLs(ctl *Controller, limit int64, categories []string) ([]string, map[int64]int64, error) {
 	inputloader := engine.NewInputLoader(engine.InputLoaderConfig{
-		InputPolicy:   engine.InputOrQueryTestLists,
+		InputPolicy:   engine.InputOrQueryBackend,
 		Session:       ctl.Session,
 		SourceFiles:   ctl.InputFiles,
 		StaticInputs:  ctl.Inputs,

internal/engine/allexperiments.go

@@ -152,7 +152,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 				))
 			},
 			config:      &httphostheader.Config{},
-			inputPolicy: InputOrQueryTestLists,
+			inputPolicy: InputOrQueryBackend,
 		}
 	},
 
@@ -237,7 +237,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 				))
 			},
 			config:      &sniblocking.Config{},
-			inputPolicy: InputOrQueryTestLists,
+			inputPolicy: InputOrQueryBackend,
 		}
 	},
 
@@ -273,7 +273,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 				))
 			},
 			config:      &tlstool.Config{},
-			inputPolicy: InputOrQueryTestLists,
+			inputPolicy: InputOrQueryBackend,
 		}
 	},
 
@@ -309,7 +309,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 				))
 			},
 			config:      &webconnectivity.Config{},
-			inputPolicy: InputOrQueryTestLists,
+			inputPolicy: InputOrQueryBackend,
 		}
 	},
 

internal/engine/experiment_integration_test.go

@@ -142,7 +142,7 @@ func TestNeedsInput(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if builder.InputPolicy() != InputOrQueryTestLists {
+	if builder.InputPolicy() != InputOrQueryBackend {
 		t.Fatal("web_connectivity certainly needs input")
 	}
 }

internal/engine/experimentbuilder.go

@@ -16,12 +16,12 @@ import (
 type InputPolicy string
 
 const (
-	// InputOrQueryTestLists indicates that the experiment requires
+	// InputOrQueryBackend indicates that the experiment requires
 	// external input to run and that this kind of input is URLs
 	// from the citizenlab/test-lists repository. If this input
 	// not provided to the experiment, then the code that runs the
 	// experiment is supposed to fetch from URLs from OONI's backends.
-	InputOrQueryTestLists = InputPolicy("or_query_test_lists")
+	InputOrQueryBackend = InputPolicy("or_query_backend")
 
 	// InputStrictlyRequired indicates that the experiment
 	// requires input and we currently don't have an API for

internal/engine/inputloader.go

@@ -10,14 +10,15 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
 )
 
-// The following errors are returned by the InputLoader.
+// These errors are returned by the InputLoader.
 var (
-	ErrNoInputExpected   = errors.New("we did not expect any input")
-	ErrInputRequired     = errors.New("no input provided")
 	ErrDetectedEmptyFile = errors.New("file did not contain any input")
+	ErrInputRequired     = errors.New("no input provided")
+	ErrNoInputExpected   = errors.New("we did not expect any input")
 )
 
-// InputLoaderSession is the session according to an InputLoader.
+// InputLoaderSession is the session according to an InputLoader. We
+// introduce this abstraction because it helps us with testing.
 type InputLoaderSession interface {
 	MaybeLookupLocationContext(ctx context.Context) error
 	NewOrchestraClient(ctx context.Context) (model.ExperimentOrchestraClient, error)
@@ -25,8 +26,8 @@ type InputLoaderSession interface {
 }
 
 // InputLoader loads input according to the specified policy
-// from the specified sources and OONI services. The behaviour
-// depends on the input policy as described below.
+// either from command line and input files or from OONI services. The
+// behaviour depends on the input policy as described below.
 //
 // InputNone
 //
@@ -40,16 +41,16 @@ type InputLoaderSession interface {
 // input, we return it. Otherwise we return a single, empty entry
 // that causes experiments that don't require input to run once.
 //
-// InputOrQueryTestLists
+// InputOrQueryBackend
 //
 // We gather input from StaticInput and SourceFiles. If there is
 // input, we return it. Otherwise, we use OONI's probe services
-// to gather input using the test lists API.
+// to gather input using the best API for the task.
 //
 // InputStrictlyRequired
 //
-// Like InputOrQueryTestLists but, if there is no input, it's an
-// user error and we just abort running the experiment.
+// We gather input from StaticInput and SourceFiles. If there is
+// input, we return it. Otherwise, we return an error.
 type InputLoader interface {
 	// 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.
@@ -64,7 +65,8 @@ type InputLoaderConfig struct {
 
 	// 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.
+	// per line. We will fail if any file is unreadable
+	// as well as if any file is empty.
 	SourceFiles []string
 
 	// InputPolicy specifies the input policy for the
@@ -94,10 +96,17 @@ func NewInputLoader(config InputLoaderConfig) InputLoader {
 	return inputLoader{InputLoaderConfig: config}
 }
 
+// TODO(bassosimone): it seems there's no reason to return an
+// interface from the constructor. Generally, "Effective Go"
+// recommends that an interface is used by the receiver rather
+// than by the sender. We should follow that rule of thumb.
+
+// inputLoader is the concrete implementation of InputLoader.
 type inputLoader struct {
 	InputLoaderConfig
 }
 
+// verifies that inputLoader is an InputLoader.
 var _ InputLoader = inputLoader{}
 
 // Load attempts to load input using the specified input loader. We will
@@ -106,8 +115,8 @@ func (il inputLoader) Load(ctx context.Context) ([]model.URLInfo, error) {
 	switch il.InputPolicy {
 	case InputOptional:
 		return il.loadOptional()
-	case InputOrQueryTestLists:
-		return il.loadOrQueryTestList(ctx)
+	case InputOrQueryBackend:
+		return il.loadOrQueryBackend(ctx)
 	case InputStrictlyRequired:
 		return il.loadStrictlyRequired(ctx)
 	default:
@@ -115,21 +124,26 @@ func (il inputLoader) Load(ctx context.Context) ([]model.URLInfo, error) {
 	}
 }
 
+// loadNone implements the InputNone policy.
 func (il inputLoader) loadNone() ([]model.URLInfo, error) {
 	if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 {
 		return nil, ErrNoInputExpected
 	}
+	// Note that we need to return a single empty entry.
 	return []model.URLInfo{{}}, nil
 }
 
+// loadOptional implements the InputOptional policy.
 func (il inputLoader) loadOptional() ([]model.URLInfo, error) {
 	inputs, err := il.loadLocal()
 	if err == nil && len(inputs) <= 0 {
+		// Note that we need to return a single empty entry.
 		inputs = []model.URLInfo{{}}
 	}
 	return inputs, err
 }
 
+// loadStrictlyRequired implements the InputStrictlyRequired policy.
 func (il inputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo, error) {
 	inputs, err := il.loadLocal()
 	if err != nil || len(inputs) > 0 {
@@ -138,14 +152,16 @@ func (il inputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo
 	return nil, ErrInputRequired
 }
 
-func (il inputLoader) loadOrQueryTestList(ctx context.Context) ([]model.URLInfo, error) {
+// loadOrQueryBackend implements the InputOrQueryBackend policy.
+func (il inputLoader) loadOrQueryBackend(ctx context.Context) ([]model.URLInfo, error) {
 	inputs, err := il.loadLocal()
 	if err != nil || len(inputs) > 0 {
 		return inputs, err
 	}
-	return il.loadRemote(loadRemoteConfig{ctx: ctx, session: il.Session})
+	return il.loadRemote(inputLoaderLoadRemoteConfig{ctx: ctx, session: il.Session})
 }
 
+// loadLocal loads inputs from StaticInputs and SourceFiles.
 func (il inputLoader) loadLocal() ([]model.URLInfo, error) {
 	inputs := []model.URLInfo{}
 	for _, input := range il.StaticInputs {
@@ -165,7 +181,12 @@ func (il inputLoader) loadLocal() ([]model.URLInfo, error) {
 	return inputs, nil
 }
 
-func (il inputLoader) readfile(filepath string, open func(string) (fsx.File, error)) ([]model.URLInfo, error) {
+// inputLoaderOpenFn is the type of the function to open a file.
+type inputLoaderOpenFn func(filepath string) (fsx.File, error)
+
+// readfile reads inputs from the specified file. The open argument should be
+// compatibile with stdlib's fs.Open and helps us with unit testing.
+func (il inputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.URLInfo, error) {
 	inputs := []model.URLInfo{}
 	filep, err := open(filepath)
 	if err != nil {
@@ -188,12 +209,15 @@ func (il inputLoader) readfile(filepath string, open func(string) (fsx.File, err
 	return inputs, nil
 }
 
-type loadRemoteConfig struct {
+// inputLoaderLoadRemoteConfig contains configuration for loading the input from
+// a remote source (which currently is _only_ the OONI backend).
+type inputLoaderLoadRemoteConfig struct {
 	ctx     context.Context
 	session InputLoaderSession
 }
 
-func (il inputLoader) loadRemote(conf loadRemoteConfig) ([]model.URLInfo, error) {
+// loadRemote loads inputs from a remote source.
+func (il inputLoader) loadRemote(conf inputLoaderLoadRemoteConfig) ([]model.URLInfo, error) {
 	if err := conf.session.MaybeLookupLocationContext(conf.ctx); err != nil {
 		return nil, err
 	}

internal/engine/inputloader_integration_test.go

@@ -211,7 +211,7 @@ func TestInputLoaderInputOrQueryTestListsWithInput(t *testing.T) {
 			"testdata/inputloader1.txt",
 			"testdata/inputloader2.txt",
 		},
-		InputPolicy: engine.InputOrQueryTestLists,
+		InputPolicy: engine.InputOrQueryBackend,
 	})
 	ctx := context.Background()
 	out, err := il.Load(ctx)
@@ -247,7 +247,7 @@ func TestInputLoaderInputOrQueryTestListsWithNoInputAndCancelledContext(t *testi
 	}
 	defer sess.Close()
 	il := engine.NewInputLoader(engine.InputLoaderConfig{
-		InputPolicy: engine.InputOrQueryTestLists,
+		InputPolicy: engine.InputOrQueryBackend,
 		Session:     sess,
 	})
 	ctx, cancel := context.WithCancel(context.Background())
@@ -282,7 +282,7 @@ func TestInputLoaderInputOrQueryTestListsWithNoInput(t *testing.T) {
 	}
 	defer sess.Close()
 	il := engine.NewInputLoader(engine.InputLoaderConfig{
-		InputPolicy: engine.InputOrQueryTestLists,
+		InputPolicy: engine.InputOrQueryBackend,
 		Session:     sess,
 		URLLimit:    30,
 	})
@@ -298,7 +298,7 @@ func TestInputLoaderInputOrQueryTestListsWithNoInput(t *testing.T) {
 
 func TestInputLoaderInputOrQueryTestListsWithEmptyFile(t *testing.T) {
 	il := engine.NewInputLoader(engine.InputLoaderConfig{
-		InputPolicy: engine.InputOrQueryTestLists,
+		InputPolicy: engine.InputOrQueryBackend,
 		SourceFiles: []string{
 			"testdata/inputloader1.txt",
 			"testdata/inputloader3.txt", // we want it before inputloader2.txt

internal/engine/inputloader_test.go

@@ -65,7 +65,7 @@ func (InputLoaderBrokenSession) ProbeCC() string {
 
 func TestInputLoaderNewOrchestraClientFailure(t *testing.T) {
 	il := inputLoader{}
-	lrc := loadRemoteConfig{
+	lrc := inputLoaderLoadRemoteConfig{
 		ctx:     context.Background(),
 		session: InputLoaderBrokenSession{},
 	}
@@ -98,7 +98,7 @@ func (InputLoaderBrokenOrchestraClient) FetchURLList(ctx context.Context, config
 
 func TestInputLoaderFetchURLListFailure(t *testing.T) {
 	il := inputLoader{}
-	lrc := loadRemoteConfig{
+	lrc := inputLoaderLoadRemoteConfig{
 		ctx: context.Background(),
 		session: InputLoaderBrokenSession{
 			OrchestraClient: InputLoaderBrokenOrchestraClient{},

internal/engine/internal/sessionresolver/resolvermaker.go

@@ -21,7 +21,7 @@ const systemResolverURL = "system:///"
 
 // allmakers contains all the makers in a list. We use the http3
 // prefix to indicate we wanna use http3. The code will translate
-// this to https and set the proper next options.
+// this to https and set the proper netx options.
 var allmakers = []*resolvermaker{{
 	url: "https://cloudflare-dns.com/dns-query",
 }, {

pkg/oonimkall/internal/tasks/runner.go

@@ -176,7 +176,7 @@ func (r *Runner) Run(ctx context.Context) {
 	builder.SetCallbacks(&runnerCallbacks{emitter: r.emitter})
 	if len(r.settings.Inputs) <= 0 {
 		switch builder.InputPolicy() {
-		case engine.InputOrQueryTestLists, engine.InputStrictlyRequired:
+		case engine.InputOrQueryBackend, engine.InputStrictlyRequired:
 			r.emitter.EmitFailureStartup("no input provided")
 			return
 		}
@@ -209,7 +209,7 @@ func (r *Runner) Run(ctx context.Context) {
 		// this policy in the future, but for now this covers in a
 		// reasonable way web connectivity, so we should be ok.
 		switch builder.InputPolicy() {
-		case engine.InputOrQueryTestLists, engine.InputStrictlyRequired:
+		case engine.InputOrQueryBackend, engine.InputStrictlyRequired:
 			var cancel context.CancelFunc
 			ctx, cancel = context.WithTimeout(
 				ctx, time.Duration(r.settings.Options.MaxRuntime)*time.Second,