From 244dd1efcfa9b27c9d03acc458eb547861767141 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 3 Apr 2024 10:16:49 +0200 Subject: [PATCH] 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 https://github.com/ooni/probe/issues/2700 --- cmd/ooniprobe/internal/nettests/nettests.go | 2 +- internal/engine/experiment.go | 33 ----- .../engine/experiment_integration_test.go | 65 +++------- internal/engine/saver.go | 61 ++++++--- internal/engine/saver_test.go | 116 ++++++++++++++---- internal/model/experiment.go | 5 - internal/oonirun/experiment.go | 7 +- 7 files changed, 155 insertions(+), 134 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index ecea88c07..01e82de2b 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/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") } } diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go index 3b1f72773..cca379161 100644 --- a/internal/engine/experiment.go +++ b/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() -} diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index a1578f859..a9906b6d8 100644 --- a/internal/engine/experiment_integration_test.go +++ b/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) } } diff --git a/internal/engine/saver.go b/internal/engine/saver.go index 1ae104b52..bc5c9e4c5 100644 --- a/internal/engine/saver.go +++ b/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() +} diff --git a/internal/engine/saver_test.go b/internal/engine/saver_test.go index afb5247de..18f655eaa 100644 --- a/internal/engine/saver_test.go +++ b/internal/engine/saver_test.go @@ -1,12 +1,18 @@ package engine import ( + "encoding/json" "errors" + "os" + "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) { @@ -38,43 +44,109 @@ func TestNewSaverWithEmptyFilePath(t *testing.T) { } } -type FakeSaverExperiment struct { - M *model.Measurement - Error error - FilePath string -} - -func (fse *FakeSaverExperiment) SaveMeasurement(m *model.Measurement, filepath string) error { - fse.M = m - fse.FilePath = filepath - return fse.Error -} - -var _ SaverExperiment = &FakeSaverExperiment{} - func TestNewSaverWithFailureWhenSaving(t *testing.T) { + filep := runtimex.Try1(os.CreateTemp("", "")) + filename := filep.Name() + filep.Close() expected := errors.New("mocked error") - fse := &FakeSaverExperiment{Error: expected} saver, err := NewSaver(SaverConfig{ - Enabled: true, - FilePath: "report.jsonl", - Experiment: fse, - Logger: log.Log, + Enabled: true, + FilePath: filename, + Logger: log.Log, }) if err != nil { t.Fatal(err) } - if _, ok := saver.(realSaver); !ok { + 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(fse.M, m); diff != "" { + if diff := cmp.Diff(m, gotMeasurement); diff != "" { t.Fatal(diff) } - if fse.FilePath != "report.jsonl" { + 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("", "") + if err != nil { + t.Fatal(err) + } + filename := filep.Name() + filep.Close() + + // create and fake-fill the measurement + m := &model.Measurement{} + ff := &testingx.FakeFiller{} + ff.Fill(m) + + // write the measurement to disk + if err := SaveMeasurement(m, filename); err != nil { + t.Fatal(err) + } + + // marshal the measurement to JSON with extra \n at the end + expect := append(must.MarshalJSON(m), '\n') + + // read the measurement from file + got := runtimex.Try1(os.ReadFile(filename)) + + // make sure what we read matches what we expect + if diff := cmp.Diff(expect, got); diff != "" { + t.Fatal(diff) + } +} + +func TestSaveMeasurementErrors(t *testing.T) { + dirname, err := os.MkdirTemp("", "ooniprobe-engine-save-measurement") + if err != nil { + t.Fatal(err) + } + filename := filepath.Join(dirname, "report.jsonl") + m := new(model.Measurement) + err = 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 = 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 = 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") + } +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 29385fa2e..df3499a2a 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -196,11 +196,6 @@ type Experiment interface { // when used with an asynchronous experiment. MeasureWithContext(ctx context.Context, input string) (measurement *Measurement, err error) - // SaveMeasurement saves a measurement on the specified file path. - // - // Deprecated: new code should use a Saver. - SaveMeasurement(measurement *Measurement, filePath string) error - // SubmitAndUpdateMeasurementContext submits a measurement and updates the // fields whose value has changed as part of the submission. // diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 3bd40e2f2..0227002ee 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -166,10 +166,9 @@ func (ed *Experiment) newSaver(experiment model.Experiment) (engine.Saver, error return ed.newSaverFn(experiment) } return engine.NewSaver(engine.SaverConfig{ - Enabled: !ed.NoJSON, - Experiment: experiment, - FilePath: ed.ReportFile, - Logger: ed.Session.Logger(), + Enabled: !ed.NoJSON, + FilePath: ed.ReportFile, + Logger: ed.Session.Logger(), }) }