refactor: move Saver, Submitter, and InputProcessor to oonirun (#1543)

My current objective is to cleanup and rationalize the engine package
such that it is more explicit how we're using probeservices APIs.

In performing this task, I noticed that Saver and InputProcessor are
only ever used by the oonirun package.

This happens because:

1. the Saver is just a tiny wrapper around SaveMeasurement that
constructs either a model.Saver or a fake one depending on the
configuration, which seems a pretty specific use case of oonirun given
the current API we're using;

2. we can same basically the same for the Submitter;

3. the InputProcessor, albeit nice, is probably not flexible enough to
be used elsewhere unless we hammer it a lot.

Additionally, in the future, ./cmd/ooniprobe should probably be based on
oonirun, and possibly also ./pkg/oonimkall.

Also, ./internal/cmd/miniooni is already based on oonirun.

To conclude, oonirun should be the focus of future work and changes,
while the functionality I'm moving here, could either be in engine or
oonirun. But my current goal is to focus on the engine, so it's fine to
move these types.

Part of ooni/probe#2700

internal/engine/savemeasurement.go

@@ -0,0 +1,39 @@
+package engine
+
+import (
+	"encoding/json"
+	"os"
+
+	"github.com/ooni/probe-cli/v3/internal/model"
+)
+
+// SaveMeasurement saves a measurement on the specified file path.
+func SaveMeasurement(measurement *model.Measurement, filePath string) error {
+	return saveMeasurement(
+		measurement, filePath, json.Marshal, os.OpenFile,
+		func(fp *os.File, b []byte) (int, error) {
+			return fp.Write(b)
+		},
+	)
+}
+
+func saveMeasurement(
+	measurement *model.Measurement, filePath string,
+	marshal func(v interface{}) ([]byte, error),
+	openFile func(name string, flag int, perm os.FileMode) (*os.File, error),
+	write func(fp *os.File, b []byte) (n int, err error),
+) error {
+	data, err := marshal(measurement)
+	if err != nil {
+		return err
+	}
+	data = append(data, byte('\n'))
+	filep, err := openFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600)
+	if err != nil {
+		return err
+	}
+	if _, err := write(filep, data); err != nil {
+		return err
+	}
+	return filep.Close()
+}

internal/engine/saver_test.go → internal/engine/savemeasurement_test.go

@@ -7,80 +7,13 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/apex/log"
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/must"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-func TestNewSaverDisabled(t *testing.T) {
-	saver, err := NewSaver(SaverConfig{
-		Enabled: false,
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if _, ok := saver.(fakeSaver); !ok {
-		t.Fatal("not the type of Saver we expected")
-	}
-	m := new(model.Measurement)
-	if err := saver.SaveMeasurement(m); err != nil {
-		t.Fatal(err)
-	}
-}
-
-func TestNewSaverWithEmptyFilePath(t *testing.T) {
-	saver, err := NewSaver(SaverConfig{
-		Enabled:  true,
-		FilePath: "",
-	})
-	if err == nil || err.Error() != "saver: passed an empty filepath" {
-		t.Fatalf("not the error we expected: %+v", err)
-	}
-	if saver != nil {
-		t.Fatal("saver should be nil here")
-	}
-}
-
-func TestNewSaverWithFailureWhenSaving(t *testing.T) {
-	filep := runtimex.Try1(os.CreateTemp("", ""))
-	filename := filep.Name()
-	filep.Close()
-	expected := errors.New("mocked error")
-	saver, err := NewSaver(SaverConfig{
-		Enabled:  true,
-		FilePath: filename,
-		Logger:   log.Log,
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	realSaver, ok := saver.(*realSaver)
-	if !ok {
-		t.Fatal("not the type of saver we expected")
-	}
-	var (
-		gotMeasurement *model.Measurement
-		gotFilePath    string
-	)
-	realSaver.savefunc = func(measurement *model.Measurement, filePath string) error {
-		gotMeasurement, gotFilePath = measurement, filePath
-		return expected
-	}
-	m := &model.Measurement{Input: "www.kernel.org"}
-	if err := saver.SaveMeasurement(m); !errors.Is(err, expected) {
-		t.Fatalf("not the error we expected: %+v", err)
-	}
-	if diff := cmp.Diff(m, gotMeasurement); diff != "" {
-		t.Fatal(diff)
-	}
-	if gotFilePath != filename {
-		t.Fatal("passed invalid filepath")
-	}
-}
-
 func TestSaveMeasurementSuccess(t *testing.T) {
 	// get temporary file where to write the measurement
 	filep, err := os.CreateTemp("", "")

internal/engine/session.go

@@ -420,7 +420,7 @@ func (s *Session) newProbeServicesClient(ctx context.Context) (*probeservices.Cl
 }
 
 // NewSubmitter creates a new submitter instance.
-func (s *Session) NewSubmitter(ctx context.Context) (Submitter, error) {
+func (s *Session) NewSubmitter(ctx context.Context) (model.Submitter, error) {
 	psc, err := s.newProbeServicesClient(ctx)
 	if err != nil {
 		return nil, err

internal/oonirun/experiment.go

@@ -64,14 +64,14 @@ type Experiment struct {
 	newInputLoaderFn func(inputPolicy model.InputPolicy) inputLoader
 
 	// newSubmitterFn is OPTIONAL and used for testing.
-	newSubmitterFn func(ctx context.Context) (engine.Submitter, error)
+	newSubmitterFn func(ctx context.Context) (model.Submitter, error)
 
 	// newSaverFn is OPTIONAL and used for testing.
-	newSaverFn func(experiment model.Experiment) (engine.Saver, error)
+	newSaverFn func(experiment model.Experiment) (model.Saver, error)
 
 	// newInputProcessorFn is OPTIONAL and used for testing.
 	newInputProcessorFn func(experiment model.Experiment, inputList []model.OOAPIURLInfo,
-		saver engine.Saver, submitter engine.Submitter) inputProcessor
+		saver model.Saver, submitter model.Submitter) inputProcessor
 }
 
 // Run runs the given experiment.
@@ -138,46 +138,46 @@ type inputProcessor = model.ExperimentInputProcessor
 
 // newInputProcessor creates a new inputProcessor instance.
 func (ed *Experiment) newInputProcessor(experiment model.Experiment,
-	inputList []model.OOAPIURLInfo, saver engine.Saver, submitter engine.Submitter) inputProcessor {
+	inputList []model.OOAPIURLInfo, saver model.Saver, submitter model.Submitter) inputProcessor {
 	if ed.newInputProcessorFn != nil {
 		return ed.newInputProcessorFn(experiment, inputList, saver, submitter)
 	}
-	return &engine.InputProcessor{
+	return &InputProcessor{
 		Annotations: ed.Annotations,
 		Experiment: &experimentWrapper{
-			child:  engine.NewInputProcessorExperimentWrapper(experiment),
+			child:  NewInputProcessorExperimentWrapper(experiment),
 			logger: ed.Session.Logger(),
 			total:  len(inputList),
 		},
 		Inputs:     inputList,
 		MaxRuntime: time.Duration(ed.MaxRuntime) * time.Second,
 		Options:    experimentOptionsToStringList(ed.ExtraOptions),
-		Saver:      engine.NewInputProcessorSaverWrapper(saver),
+		Saver:      NewInputProcessorSaverWrapper(saver),
 		Submitter: &experimentSubmitterWrapper{
-			child:  engine.NewInputProcessorSubmitterWrapper(submitter),
+			child:  NewInputProcessorSubmitterWrapper(submitter),
 			logger: ed.Session.Logger(),
 		},
 	}
 }
 
 // newSaver creates a new engine.Saver instance.
-func (ed *Experiment) newSaver(experiment model.Experiment) (engine.Saver, error) {
+func (ed *Experiment) newSaver(experiment model.Experiment) (model.Saver, error) {
 	if ed.newSaverFn != nil {
 		return ed.newSaverFn(experiment)
 	}
-	return engine.NewSaver(engine.SaverConfig{
+	return NewSaver(SaverConfig{
 		Enabled:  !ed.NoJSON,
 		FilePath: ed.ReportFile,
 		Logger:   ed.Session.Logger(),
 	})
 }
 
 // newSubmitter creates a new engine.Submitter instance.
-func (ed *Experiment) newSubmitter(ctx context.Context) (engine.Submitter, error) {
+func (ed *Experiment) newSubmitter(ctx context.Context) (model.Submitter, error) {
 	if ed.newSubmitterFn != nil {
 		return ed.newSubmitterFn(ctx)
 	}
-	return engine.NewSubmitter(ctx, engine.SubmitterConfig{
+	return NewSubmitter(ctx, SubmitterConfig{
 		Enabled: !ed.NoCollector,
 		Session: ed.Session,
 		Logger:  ed.Session.Logger(),
@@ -233,7 +233,7 @@ func experimentOptionsToStringList(options map[string]any) (out []string) {
 // experimentWrapper wraps an experiment and logs progress
 type experimentWrapper struct {
 	// child is the child experiment wrapper
-	child engine.InputProcessorExperimentWrapper
+	child InputProcessorExperimentWrapper
 
 	// logger is the logger to use
 	logger model.Logger
@@ -254,7 +254,7 @@ func (ew *experimentWrapper) MeasureAsync(
 // fail if we cannot submit a measurement
 type experimentSubmitterWrapper struct {
 	// child is the child submitter wrapper
-	child engine.InputProcessorSubmitterWrapper
+	child InputProcessorSubmitterWrapper
 
 	// logger is the logger to use
 	logger model.Logger

internal/oonirun/experiment_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/testingx"
@@ -81,7 +80,7 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 		},
 		newExperimentBuilderFn: nil,
 		newInputLoaderFn:       nil,
-		newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+		newSubmitterFn: func(ctx context.Context) (model.Submitter, error) {
 			subm := &mocks.Submitter{
 				MockSubmit: func(ctx context.Context, m *model.Measurement) error {
 					failedToSubmit++
@@ -171,9 +170,9 @@ func TestExperimentRun(t *testing.T) {
 		Session                Session
 		newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error)
 		newInputLoaderFn       func(inputPolicy model.InputPolicy) inputLoader
-		newSubmitterFn         func(ctx context.Context) (engine.Submitter, error)
-		newSaverFn             func(experiment model.Experiment) (engine.Saver, error)
-		newInputProcessorFn    func(experiment model.Experiment, inputList []model.OOAPIURLInfo, saver engine.Saver, submitter engine.Submitter) inputProcessor
+		newSubmitterFn         func(ctx context.Context) (model.Submitter, error)
+		newSaverFn             func(experiment model.Experiment) (model.Saver, error)
+		newInputProcessorFn    func(experiment model.Experiment, inputList []model.OOAPIURLInfo, saver model.Saver, submitter model.Submitter) inputProcessor
 	}
 	type args struct {
 		ctx context.Context
@@ -274,7 +273,7 @@ func TestExperimentRun(t *testing.T) {
 					},
 				}
 			},
-			newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+			newSubmitterFn: func(ctx context.Context) (model.Submitter, error) {
 				return nil, errMocked
 			},
 		},
@@ -317,10 +316,10 @@ func TestExperimentRun(t *testing.T) {
 					},
 				}
 			},
-			newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+			newSubmitterFn: func(ctx context.Context) (model.Submitter, error) {
 				return &mocks.Submitter{}, nil
 			},
-			newSaverFn: func(experiment model.Experiment) (engine.Saver, error) {
+			newSaverFn: func(experiment model.Experiment) (model.Saver, error) {
 				return nil, errMocked
 			},
 		},
@@ -363,14 +362,14 @@ func TestExperimentRun(t *testing.T) {
 					},
 				}
 			},
-			newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+			newSubmitterFn: func(ctx context.Context) (model.Submitter, error) {
 				return &mocks.Submitter{}, nil
 			},
-			newSaverFn: func(experiment model.Experiment) (engine.Saver, error) {
+			newSaverFn: func(experiment model.Experiment) (model.Saver, error) {
 				return &mocks.Saver{}, nil
 			},
 			newInputProcessorFn: func(experiment model.Experiment, inputList []model.OOAPIURLInfo,
-				saver engine.Saver, submitter engine.Submitter) inputProcessor {
+				saver model.Saver, submitter model.Submitter) inputProcessor {
 				return &mocks.ExperimentInputProcessor{
 					MockRun: func(ctx context.Context) error {
 						return errMocked