diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index a6d82a5c7..54b583bf3 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -60,9 +60,6 @@ type V2Nettest struct { TestName string `json:"test_name"` } -// ErrHTTPRequestFailed indicates that an HTTP request failed. -var ErrHTTPRequestFailed = errors.New("oonirun: HTTP request failed") - // getV2DescriptorFromHTTPSURL GETs a v2Descriptor instance from // a static URL (e.g., from a GitHub repo or from a Gist). func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient, @@ -96,7 +93,11 @@ const v2DescriptorCacheKey = "oonirun-v2.state" // v2DescriptorCacheLoad loads the v2DescriptorCache. func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, error) { + // attempt to access the cache data, err := fsstore.Get(v2DescriptorCacheKey) + + // if there's a miss either create a new descriptor or return the + // error if it's something I/O related if err != nil { if errors.Is(err, kvstore.ErrNoSuchKey) { cache := &v2DescriptorCache{ @@ -106,13 +107,19 @@ func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, err } return nil, err } + + // transform the raw descriptor into a struct var cache v2DescriptorCache if err := json.Unmarshal(data, &cache); err != nil { return nil, err } + + // handle the case where there are no entries inside the on-disk cache + // by properly initializing to a non-nil map if cache.Entries == nil { cache.Entries = make(map[string]*V2Descriptor) } + return &cache, nil } @@ -168,13 +175,18 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri // more robust in terms of the implementation. return ErrNilDescriptor } + logger := config.Session.Logger() + for _, nettest := range desc.Nettests { + // early handling of the case where the test name is empty if nettest.TestName == "" { logger.Warn("oonirun: nettest name cannot be empty") v2CountEmptyNettestNames.Add(1) continue } + + // construct an experiment from the current nettest exp := &Experiment{ Annotations: config.Annotations, ExtraOptions: nettest.Options, @@ -193,12 +205,15 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri newSaverFn: nil, newInputProcessorFn: nil, } + + // actually run the experiment if err := exp.Run(ctx); err != nil { logger.Warnf("cannot run experiment: %s", err.Error()) v2CountFailedExperiments.Add(1) continue } } + return nil } @@ -209,14 +224,25 @@ var ErrNeedToAcceptChanges = errors.New("oonirun: need to accept changes") // v2DescriptorDiff shows what changed between the old and the new descriptors. func v2DescriptorDiff(oldValue, newValue *V2Descriptor, URL string) string { + // JSON serialize old descriptor oldData, err := json.MarshalIndent(oldValue, "", " ") runtimex.PanicOnError(err, "json.MarshalIndent failed unexpectedly") + + // JSON serialize new descriptor newData, err := json.MarshalIndent(newValue, "", " ") runtimex.PanicOnError(err, "json.MarshalIndent failed unexpectedly") + + // make sure the serializations are newline-terminated oldString, newString := string(oldData)+"\n", string(newData)+"\n" + + // generate names for the final diff oldFile := "OLD " + URL newFile := "NEW " + URL + + // compute the edits to update from the old to the new descriptor edits := myers.ComputeEdits(span.URIFromPath(oldFile), oldString, newString) + + // transform the edits and obtain an unified diff return fmt.Sprint(gotextdiff.ToUnified(oldFile, newFile, oldString, edits)) } @@ -233,25 +259,39 @@ func v2DescriptorDiff(oldValue, newValue *V2Descriptor, URL string) string { func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error { logger := config.Session.Logger() logger.Infof("oonirun/v2: running %s", URL) + + // load the descriptor from the cache cache, err := v2DescriptorCacheLoad(config.KVStore) if err != nil { return err } + + // pull a possibly new descriptor without updating the old descriptor clnt := config.Session.DefaultHTTPClient() oldValue, newValue, err := cache.PullChangesWithoutSideEffects(ctx, clnt, logger, URL) if err != nil { return err } + + // compare the new descriptor to the old descriptor diff := v2DescriptorDiff(oldValue, newValue, URL) + + // possibly stop if configured to ask for permission when accepting changes if !config.AcceptChanges && diff != "" { logger.Warnf("oonirun: %s changed as follows:\n\n%s", URL, diff) logger.Warnf("oonirun: we are not going to run this link until you accept changes") return ErrNeedToAcceptChanges } + + // in case there are changes, update the descriptor if diff != "" { if err := cache.Update(config.KVStore, URL, newValue); err != nil { return err } } - return V2MeasureDescriptor(ctx, config, newValue) // handles nil newValue gracefully + + // measure using the possibly-new descriptor + // + // note: this function gracefully handles nil values + return V2MeasureDescriptor(ctx, config, newValue) } diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index a802b6069..931428737 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -12,10 +12,13 @@ import ( "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingx" ) func TestOONIRunV2LinkCommonCase(t *testing.T) { + // make a local server that returns a reasonable descriptor for the example experiment server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -33,8 +36,10 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -42,19 +47,24 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { }, KVStore: &kvstore.Memory{}, MaxRuntime: 0, - NoCollector: true, + NoCollector: true, // disable collector so we don't submit NoJSON: true, Random: false, ReportFile: "", Session: newMinimalFakeSession(), } + + // create a link runner for the local server URL r := NewLinkRunner(config, server.URL) + + // run and verify that we could run without getting errors if err := r.Run(ctx); err != nil { t.Fatal(err) } } func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { + // make a server that returns a minimal descriptor for the example experiment server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -72,8 +82,12 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + + // create with a key value store that returns an empty cache and fails to update + // the cache afterwards such that we can see if we detect such an error expected := errors.New("mocked") config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error @@ -95,14 +109,21 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // create new runner for the local server URL r := NewLinkRunner(config, server.URL) + + // attempt to run the link err := r.Run(ctx) + + // make sure we exactly got the cache updating error if !errors.Is(err, expected) { t.Fatal("unexpected err", err) } } func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { + // make a local server that would return a reasonable descriptor server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -120,8 +141,11 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + + // create a minimal link configuration config := &LinkConfig{ AcceptChanges: false, // should see "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -135,19 +159,29 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // create a new runner for the local server URL r := NewLinkRunner(config, server.URL) + + // attempt to run the link err := r.Run(ctx) + + // make sure the error indicates we need to accept changes if !errors.Is(err, ErrNeedToAcceptChanges) { t.Fatal("unexpected err", err) } } func TestOONIRunV2LinkNilDescriptor(t *testing.T) { + // create a local server that returns a literal "null" as the JSON descriptor server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("null")) })) + defer server.Close() ctx := context.Background() + + // create a minimal link configuration config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -161,14 +195,24 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // attempt to run the link at the local server r := NewLinkRunner(config, server.URL) + + // make sure we correctly handled an invalid "null" descriptor if err := r.Run(ctx); err != nil { t.Fatal(err) + t.Fatal("unexpected error", err) } } func TestOONIRunV2LinkEmptyTestName(t *testing.T) { + // load the count of the number of cases where the test name was empty so we can + // later on check whether this count has increased due to running this test emptyTestNamesPrev := v2CountEmptyNettestNames.Load() + + // create a local server that will respond with a minimal descriptor that + // actually contains an empty test name, which is what we want to test server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { descriptor := &V2Descriptor{ Name: "", @@ -186,8 +230,11 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { runtimex.PanicOnError(err, "json.Marshal failed") w.Write(data) })) + defer server.Close() ctx := context.Background() + + // create a minimal link configuration config := &LinkConfig{ AcceptChanges: true, // avoid "oonirun: need to accept changes" error Annotations: map[string]string{ @@ -201,30 +248,116 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // construct a link runner relative to the local server URL r := NewLinkRunner(config, server.URL) + + // attempt to run and verify there's no error (the code only emits a warning in this case) if err := r.Run(ctx); err != nil { t.Fatal(err) } + + // make sure the loop for running nettests continued where we expected it to do so if v2CountEmptyNettestNames.Load() != emptyTestNamesPrev+1 { t.Fatal("expected to see 1 more instance of empty nettest names") } } +func TestOONIRunV2LinkConnectionResetByPeer(t *testing.T) { + // create a local server that will reset the connection immediately. + // actually contains an empty test name, which is what we want to test + server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) + + defer server.Close() + ctx := context.Background() + + // create a minimal link configuration + config := &LinkConfig{ + AcceptChanges: true, // avoid "oonirun: need to accept changes" error + Annotations: map[string]string{ + "platform": "linux", + }, + KVStore: &kvstore.Memory{}, + MaxRuntime: 0, + NoCollector: true, + NoJSON: true, + Random: false, + ReportFile: "", + Session: newMinimalFakeSession(), + } + + // construct a link runner relative to the local server URL + r := NewLinkRunner(config, server.URL) + + // attempt to run and verify we got ECONNRESET + if err := r.Run(ctx); !errors.Is(err, netxlite.ECONNRESET) { + t.Fatal("unexpected error", err) + } +} + +func TestOONIRunV2LinkNonParseableJSON(t *testing.T) { + // create a local server that will respond with a non-parseable JSON. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{`)) + })) + + defer server.Close() + ctx := context.Background() + + // create a minimal link configuration + config := &LinkConfig{ + AcceptChanges: true, // avoid "oonirun: need to accept changes" error + Annotations: map[string]string{ + "platform": "linux", + }, + KVStore: &kvstore.Memory{}, + MaxRuntime: 0, + NoCollector: true, + NoJSON: true, + Random: false, + ReportFile: "", + Session: newMinimalFakeSession(), + } + + // construct a link runner relative to the local server URL + r := NewLinkRunner(config, server.URL) + + // attempt to run and verify there's a JSON parsing error + if err := r.Run(ctx); err == nil || err.Error() != "unexpected end of JSON input" { + t.Fatal("unexpected error", err) + } +} + func TestV2MeasureDescriptor(t *testing.T) { + t.Run("with nil descriptor", func(t *testing.T) { ctx := context.Background() config := &LinkConfig{} + + // invoke the function with a nil descriptor and make sure the code + // is correctly handling this specific case by returnning error err := V2MeasureDescriptor(ctx, config, nil) + if !errors.Is(err, ErrNilDescriptor) { t.Fatal("unexpected err", err) } }) t.Run("with failing experiment", func(t *testing.T) { + // load the previous count of failed experiments so we can check that it increased later previousFailedExperiments := v2CountFailedExperiments.Load() + expected := errors.New("mocked error") + ctx := context.Background() sess := newMinimalFakeSession() + + // create a mocked submitter that will panic in case we try to submit, such that + // this test fails with a panic if we go as far as attempting to submit + // + // Note: the convention is that we do not submit experiment results when the + // experiment measurement function returns a non-nil error, since such an error + // represents a fundamental failure in setting up the experiment sess.MockNewSubmitter = func(ctx context.Context) (model.Submitter, error) { subm := &mocks.Submitter{ MockSubmit: func(ctx context.Context, m *model.Measurement) error { @@ -233,6 +366,9 @@ func TestV2MeasureDescriptor(t *testing.T) { } return subm, nil } + + // mock an experiment builder where we have the measurement function fail by returning + // an error, which has the meaning indicated in the previous comment sess.MockNewExperimentBuilder = func(name string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ MockInputPolicy: func() model.InputPolicy { @@ -258,6 +394,8 @@ func TestV2MeasureDescriptor(t *testing.T) { } return eb, nil } + + // create a mostly empty config referring to the session config := &LinkConfig{ AcceptChanges: false, Annotations: map[string]string{}, @@ -269,6 +407,8 @@ func TestV2MeasureDescriptor(t *testing.T) { ReportFile: "", Session: sess, } + + // create a mostly empty descriptor referring to the example experiment descr := &V2Descriptor{ Name: "", Description: "", @@ -279,10 +419,18 @@ func TestV2MeasureDescriptor(t *testing.T) { TestName: "example", }}, } + + // attempt to measure this descriptor err := V2MeasureDescriptor(ctx, config, descr) + + // here we do not expect to see an error because the implementation continues + // until it has run all experiments and just emits warning messages if err != nil { t.Fatal(err) } + + // however there's also a count of the number of times we failed to load + // an experiment and we use that to make sure the code failed where we expected if v2CountFailedExperiments.Load() != previousFailedExperiments+1 { t.Fatal("expected to see a failed experiment") } @@ -290,9 +438,13 @@ func TestV2MeasureDescriptor(t *testing.T) { } func TestV2MeasureHTTPS(t *testing.T) { + t.Run("when we cannot load from cache", func(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() + + // construct the link configuration with a key-value store that fails + // with a well-know error when attempting to load. config := &LinkConfig{ AcceptChanges: false, Annotations: map[string]string{}, @@ -308,15 +460,22 @@ func TestV2MeasureHTTPS(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } + + // attempt to measure with the given config (there's no need to pass an URL + // here because we should fail to load from the cache first) err := v2MeasureHTTPS(ctx, config, "") + + // verify that we've actually got the expected error if !errors.Is(err, expected) { t.Fatal("unexpected err", err) } }) t.Run("when we cannot pull changes", func(t *testing.T) { + // create and immediately cancel a context so that HTTP would fail ctx, cancel := context.WithCancel(context.Background()) cancel() // fail immediately + config := &LinkConfig{ AcceptChanges: false, Annotations: map[string]string{}, @@ -328,25 +487,39 @@ func TestV2MeasureHTTPS(t *testing.T) { ReportFile: "", Session: newMinimalFakeSession(), } - err := v2MeasureHTTPS(ctx, config, "https://example.com") // should not use URL + + // attempt to measure with a random URL (which is fine since we shouldn't use it) + err := v2MeasureHTTPS(ctx, config, "https://example.com") + + // make sure that we've actually go the expected error if !errors.Is(err, context.Canceled) { t.Fatal("unexpected err", err) } }) + } func TestV2DescriptorCacheLoad(t *testing.T) { - t.Run("cannot unmarshal cache content", func(t *testing.T) { + + t.Run("handle the case where we cannot unmarshal the cache content", func(t *testing.T) { + // write an invalid serialized JSON into the cache fsstore := &kvstore.Memory{} if err := fsstore.Set(v2DescriptorCacheKey, []byte("{")); err != nil { t.Fatal(err) } + + // attempt to load descriptors cache, err := v2DescriptorCacheLoad(fsstore) + + // make sure we cannot unmarshal if err == nil || err.Error() != "unexpected end of JSON input" { t.Fatal("unexpected err", err) } + + // make sure the returned cache is nil if cache != nil { t.Fatal("expected nil cache") } }) + }