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(), }) }