cleanup: SaveMeasurement doesn't belong to Experiment (#1541)

This cleanup may be a bit tangential, but it still makes sense in the
context of what I'm doing, i.e., cleaning up the engine API and ensuring
we mostly use engine functionality in clients.

While doing this, it occurred to me that I could avoid keeping the
SaveMeasurement method attached to an experiment, and I could instead
just make it a pure function.

This reduces the complexity of the *engine.Experiment type.

While there, slightly improve testing.

Part of ooni/probe#2700

cmd/ooniprobe/internal/nettests/nettests.go

@@ -231,7 +231,7 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error
 		}
 		// We only save the measurement to disk if we failed to upload the measurement
 		if saveToDisk {
-			if err := exp.SaveMeasurement(measurement, msmt.MeasurementFilePath.String); err != nil {
+			if err := engine.SaveMeasurement(measurement, msmt.MeasurementFilePath.String); err != nil {
 				return errors.Wrap(err, "failed to save measurement on disk")
 			}
 		}

internal/engine/experiment.go

@@ -6,10 +6,8 @@ package engine
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"net/http"
-	"os"
 	"runtime"
 	"time"
 
@@ -185,16 +183,6 @@ func (e *experiment) MeasureWithContext(
 	return
 }
 
-// SaveMeasurement implements Experiment.SaveMeasurement.
-func (e *experiment) SaveMeasurement(measurement *model.Measurement, filePath string) error {
-	return e.saveMeasurement(
-		measurement, filePath, json.Marshal, os.OpenFile,
-		func(fp *os.File, b []byte) (int, error) {
-			return fp.Write(b)
-		},
-	)
-}
-
 // SubmitAndUpdateMeasurementContext implements Experiment.SubmitAndUpdateMeasurementContext.
 func (e *experiment) SubmitAndUpdateMeasurementContext(
 	ctx context.Context, measurement *model.Measurement) error {
@@ -278,24 +266,3 @@ func (e *experiment) newReportTemplate() model.OOAPIReportTemplate {
 		TestVersion:       e.testVersion,
 	}
 }
-
-func (e *experiment) 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/experiment_integration_test.go

@@ -3,15 +3,13 @@ package engine
 import (
 	"context"
 	"encoding/json"
-	"errors"
-	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"os"
-	"path/filepath"
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/registry"
 )
@@ -264,18 +262,24 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string)
 	measurement.AddAnnotations(map[string]string{
 		"probe-engine-ci": "yes",
 	})
-	data, err := json.Marshal(measurement)
+	savedMeasurement, err := json.Marshal(measurement)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if data == nil {
+	if savedMeasurement == nil {
 		t.Fatal("data is nil")
 	}
+	tempfile, err := os.CreateTemp("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	filename := tempfile.Name()
+	tempfile.Close()
 	err = experiment.SubmitAndUpdateMeasurementContext(ctx, measurement)
 	if err != nil {
 		t.Fatal(err)
 	}
-	err = experiment.SaveMeasurement(measurement, "/tmp/experiment.jsonl")
+	err = SaveMeasurement(measurement, filename)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -289,54 +293,13 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string)
 	if sk == nil {
 		t.Fatal("got nil summary keys")
 	}
-}
-
-func TestSaveMeasurementErrors(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skip test in short mode")
-	}
-	sess := newSessionForTesting(t)
-	defer sess.Close()
-	builder, err := sess.NewExperimentBuilder("example")
+	loadedMeasurement, err := os.ReadFile(filename)
 	if err != nil {
 		t.Fatal(err)
 	}
-	exp := builder.NewExperiment().(*experiment)
-	dirname, err := ioutil.TempDir("", "ooniprobe-engine-save-measurement")
-	if err != nil {
-		t.Fatal(err)
-	}
-	filename := filepath.Join(dirname, "report.jsonl")
-	m := new(model.Measurement)
-	err = exp.saveMeasurement(
-		m, filename, func(v interface{}) ([]byte, error) {
-			return nil, errors.New("mocked error")
-		}, os.OpenFile, func(fp *os.File, b []byte) (int, error) {
-			return fp.Write(b)
-		},
-	)
-	if err == nil {
-		t.Fatal("expected an error here")
-	}
-	err = exp.saveMeasurement(
-		m, filename, json.Marshal,
-		func(name string, flag int, perm os.FileMode) (*os.File, error) {
-			return nil, errors.New("mocked error")
-		}, func(fp *os.File, b []byte) (int, error) {
-			return fp.Write(b)
-		},
-	)
-	if err == nil {
-		t.Fatal("expected an error here")
-	}
-	err = exp.saveMeasurement(
-		m, filename, json.Marshal, os.OpenFile,
-		func(fp *os.File, b []byte) (int, error) {
-			return 0, errors.New("mocked error")
-		},
-	)
-	if err == nil {
-		t.Fatal("expected an error here")
+	withFinalNewline := append(savedMeasurement, '\n')
+	if diff := cmp.Diff(withFinalNewline, loadedMeasurement); diff != "" {
+		t.Fatal(diff)
 	}
 }
 

internal/engine/saver.go

@@ -1,7 +1,9 @@
 package engine
 
 import (
+	"encoding/json"
 	"errors"
+	"os"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
@@ -14,9 +16,6 @@ type SaverConfig struct {
 	// Enabled is true if saving is enabled.
 	Enabled bool
 
-	// Experiment is the experiment we're currently running.
-	Experiment SaverExperiment
-
 	// FilePath is the filepath where to append the measurement as a
 	// serialized JSON followed by a newline character.
 	FilePath string
@@ -25,11 +24,6 @@ type SaverConfig struct {
 	Logger model.Logger
 }
 
-// SaverExperiment is an experiment according to the Saver.
-type SaverExperiment interface {
-	SaveMeasurement(m *model.Measurement, filepath string) error
-}
-
 // NewSaver creates a new instance of Saver.
 func NewSaver(config SaverConfig) (Saver, error) {
 	if !config.Enabled {
@@ -38,10 +32,10 @@ func NewSaver(config SaverConfig) (Saver, error) {
 	if config.FilePath == "" {
 		return nil, errors.New("saver: passed an empty filepath")
 	}
-	return realSaver{
-		Experiment: config.Experiment,
-		FilePath:   config.FilePath,
-		Logger:     config.Logger,
+	return &realSaver{
+		FilePath: config.FilePath,
+		Logger:   config.Logger,
+		savefunc: SaveMeasurement,
 	}, nil
 }
 
@@ -54,14 +48,45 @@ func (fs fakeSaver) SaveMeasurement(m *model.Measurement) error {
 var _ Saver = fakeSaver{}
 
 type realSaver struct {
-	Experiment SaverExperiment
-	FilePath   string
-	Logger     model.Logger
+	FilePath string
+	Logger   model.Logger
+	savefunc func(measurement *model.Measurement, filePath string) error
 }
 
-func (rs realSaver) SaveMeasurement(m *model.Measurement) error {
+func (rs *realSaver) SaveMeasurement(m *model.Measurement) error {
 	rs.Logger.Info("saving measurement to disk")
-	return rs.Experiment.SaveMeasurement(m, rs.FilePath)
+	return rs.savefunc(m, rs.FilePath)
 }
 
-var _ Saver = realSaver{}
+var _ Saver = &realSaver{}
+
+// 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()
+}