feat(oonirun): improve tests (#915)

See https://github.com/ooni/probe/issues/2184

While there, rename `runtimex.PanicIfFalse` to `runtimex.Assert` (it was about time...)

internal/oonirun/experiment.go

@@ -11,11 +11,15 @@ import (
 	"strings"
 	"time"
 
+	"github.com/ooni/probe-cli/v3/internal/atomicx"
 	"github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/humanize"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
+// experimentShuffledInputs counts how many times we shuffled inputs
+var experimentShuffledInputs = &atomicx.Int64{}
+
 // Experiment describes an experiment to run. You MUST fill all the fields that
 // are marked as MANDATORY, otherwise Experiment.Run will cause panics.
 type Experiment struct {
@@ -52,6 +56,22 @@ type Experiment struct {
 
 	// Session is the MANDATORY session.
 	Session Session
+
+	// newExperimentBuilderFn is OPTIONAL and used for testing.
+	newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error)
+
+	// newInputLoaderFn is OPTIONAL and used for testing.
+	newInputLoaderFn func(inputPolicy model.InputPolicy) inputLoader
+
+	// newSubmitterFn is OPTIONAL and used for testing.
+	newSubmitterFn func(ctx context.Context) (engine.Submitter, error)
+
+	// newSaverFn is OPTIONAL and used for testing.
+	newSaverFn func(experiment model.Experiment) (engine.Saver, error)
+
+	// newInputProcessorFn is OPTIONAL and used for testing.
+	newInputProcessorFn func(experiment model.Experiment, inputList []model.OOAPIURLInfo,
+		saver engine.Saver, submitter engine.Submitter) inputProcessor
 }
 
 // Run runs the given experiment.
@@ -76,6 +96,7 @@ func (ed *Experiment) Run(ctx context.Context) error {
 		rnd.Shuffle(len(inputList), func(i, j int) {
 			inputList[i], inputList[j] = inputList[j], inputList[i]
 		})
+		experimentShuffledInputs.Add(1)
 	}
 
 	// 4. configure experiment's options
@@ -112,14 +133,15 @@ func (ed *Experiment) Run(ctx context.Context) error {
 	return inputProcessor.Run(ctx)
 }
 
-// inputProcessor processes inputs running the given experiment.
-type inputProcessor interface {
-	Run(ctx context.Context) error
-}
+// inputProcessor is an alias for model.ExperimentInputProcessor
+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 {
+	if ed.newInputProcessorFn != nil {
+		return ed.newInputProcessorFn(experiment, inputList, saver, submitter)
+	}
 	return &engine.InputProcessor{
 		Annotations: ed.Annotations,
 		Experiment: &experimentWrapper{
@@ -140,6 +162,9 @@ func (ed *Experiment) newInputProcessor(experiment model.Experiment,
 
 // newSaver creates a new engine.Saver instance.
 func (ed *Experiment) newSaver(experiment model.Experiment) (engine.Saver, error) {
+	if ed.newSaverFn != nil {
+		return ed.newSaverFn(experiment)
+	}
 	return engine.NewSaver(engine.SaverConfig{
 		Enabled:    !ed.NoJSON,
 		Experiment: experiment,
@@ -150,6 +175,9 @@ func (ed *Experiment) newSaver(experiment model.Experiment) (engine.Saver, error
 
 // newSubmitter creates a new engine.Submitter instance.
 func (ed *Experiment) newSubmitter(ctx context.Context) (engine.Submitter, error) {
+	if ed.newSubmitterFn != nil {
+		return ed.newSubmitterFn(ctx)
+	}
 	return engine.NewSubmitter(ctx, engine.SubmitterConfig{
 		Enabled: !ed.NoCollector,
 		Session: ed.Session,
@@ -159,16 +187,20 @@ func (ed *Experiment) newSubmitter(ctx context.Context) (engine.Submitter, error
 
 // newExperimentBuilder creates a new engine.ExperimentBuilder for the given experimentName.
 func (ed *Experiment) newExperimentBuilder(experimentName string) (model.ExperimentBuilder, error) {
+	if ed.newExperimentBuilderFn != nil {
+		return ed.newExperimentBuilderFn(experimentName)
+	}
 	return ed.Session.NewExperimentBuilder(ed.Name)
 }
 
-// inputLoader loads inputs from local or remote sources.
-type inputLoader interface {
-	Load(ctx context.Context) ([]model.OOAPIURLInfo, error)
-}
+// inputLoader is an alias for model.ExperimentInputLoader
+type inputLoader = model.ExperimentInputLoader
 
 // newInputLoader creates a new inputLoader.
 func (ed *Experiment) newInputLoader(inputPolicy model.InputPolicy) inputLoader {
+	if ed.newInputLoaderFn != nil {
+		return ed.newInputLoaderFn(inputPolicy)
+	}
 	return &engine.InputLoader{
 		CheckInConfig: &model.OOAPICheckInConfig{
 			RunType:  model.RunTypeManual,

internal/oonirun/experiment_test.go

@@ -2,42 +2,24 @@ package oonirun
 
 import (
 	"context"
-	"os"
+	"errors"
 	"reflect"
 	"sort"
 	"testing"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/engine"
-	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/model"
-	"github.com/ooni/probe-cli/v3/internal/version"
+	"github.com/ooni/probe-cli/v3/internal/model/mocks"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-// TODO(bassosimone): it would be cool to write unit tests. However, to do that
-// we need to ~redesign the engine package for unit-testability.
-
-func newSession(ctx context.Context, t *testing.T) *engine.Session {
-	config := engine.SessionConfig{
-		AvailableProbeServices: []model.OOAPIService{},
-		KVStore:                &kvstore.Memory{},
-		Logger:                 model.DiscardLogger,
-		ProxyURL:               nil,
-		SoftwareName:           "miniooni",
-		SoftwareVersion:        version.Version,
-		TempDir:                os.TempDir(),
-		TorArgs:                []string{},
-		TorBinary:              "",
-		TunnelDir:              "",
-	}
-	sess, err := engine.NewSession(ctx, config)
-	if err != nil {
-		t.Fatal(err)
-	}
-	return sess
-}
-
-func TestExperimentRunWithExample(t *testing.T) {
+func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
+	shuffledInputsPrev := experimentShuffledInputs.Load()
+	var calledSetOptionsAny int
+	var failedToSubmit int
+	var calledKibiBytesReceived int
+	var calledKibiBytesSent int
 	ctx := context.Background()
 	desc := &Experiment{
 		Annotations: map[string]string{
@@ -46,19 +28,89 @@ func TestExperimentRunWithExample(t *testing.T) {
 		ExtraOptions: map[string]any{
 			"SleepTime": int64(10 * time.Millisecond),
 		},
-		Inputs:         []string{},
+		Inputs: []string{
+			"a", "b", "c",
+		},
 		InputFilePaths: []string{},
 		MaxRuntime:     0,
 		Name:           "example",
 		NoCollector:    true,
 		NoJSON:         true,
-		Random:         false,
+		Random:         true, // to test randomness
 		ReportFile:     "",
-		Session:        newSession(ctx, t),
+		Session: &mocks.Session{
+			MockNewExperimentBuilder: func(name string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockInputPolicy: func() model.InputPolicy {
+						return model.InputOptional
+					},
+					MockSetOptionsAny: func(options map[string]any) error {
+						calledSetOptionsAny++
+						return nil
+					},
+					MockNewExperiment: func() model.Experiment {
+						exp := &mocks.Experiment{
+							MockMeasureAsync: func(ctx context.Context, input string) (<-chan *model.Measurement, error) {
+								out := make(chan *model.Measurement)
+								go func() {
+									defer close(out)
+									ff := &testingx.FakeFiller{}
+									var meas model.Measurement
+									ff.Fill(&meas)
+									out <- &meas
+								}()
+								return out, nil
+							},
+							MockKibiBytesReceived: func() float64 {
+								calledKibiBytesReceived++
+								return 1.453
+							},
+							MockKibiBytesSent: func() float64 {
+								calledKibiBytesSent++
+								return 1.648
+							},
+						}
+						return exp
+					},
+				}
+				return eb, nil
+			},
+			MockLogger: func() model.Logger {
+				return model.DiscardLogger
+			},
+		},
+		newExperimentBuilderFn: nil,
+		newInputLoaderFn:       nil,
+		newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+			subm := &mocks.Submitter{
+				MockSubmit: func(ctx context.Context, m *model.Measurement) error {
+					failedToSubmit++
+					return errors.New("mocked error")
+				},
+			}
+			return subm, nil
+		},
+		newSaverFn:          nil,
+		newInputProcessorFn: nil,
 	}
 	if err := desc.Run(ctx); err != nil {
 		t.Fatal(err)
 	}
+	if failedToSubmit < 1 {
+		t.Fatal("expected to see failure to submit")
+	}
+	if experimentShuffledInputs.Load() != shuffledInputsPrev+1 {
+		t.Fatal("did not shuffle inputs")
+	}
+	if calledSetOptionsAny < 1 {
+		t.Fatal("should have called SetOptionsAny")
+	}
+	if calledKibiBytesReceived < 1 {
+		t.Fatal("did not call KibiBytesReceived")
+	}
+	if calledKibiBytesSent < 1 {
+		t.Fatal("did not call KibiBytesSent")
+	}
 }
 
 func Test_experimentOptionsToStringList(t *testing.T) {
@@ -102,3 +154,257 @@ func Test_experimentOptionsToStringList(t *testing.T) {
 		})
 	}
 }
+
+func TestExperimentRun(t *testing.T) {
+	errMocked := errors.New("mocked error")
+	type fields struct {
+		Annotations            map[string]string
+		ExtraOptions           map[string]any
+		Inputs                 []string
+		InputFilePaths         []string
+		MaxRuntime             int64
+		Name                   string
+		NoCollector            bool
+		NoJSON                 bool
+		Random                 bool
+		ReportFile             string
+		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
+	}
+	type args struct {
+		ctx context.Context
+	}
+	tests := []struct {
+		name      string
+		fields    fields
+		args      args
+		expectErr error
+	}{{
+		name: "cannot construct an experiment builder",
+		fields: fields{
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				return nil, errMocked
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
+	}, {
+		name: "cannot load input",
+		fields: fields{
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockInputPolicy: func() model.InputPolicy {
+						return model.InputOptional
+					},
+				}
+				return eb, nil
+			},
+			newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader {
+				return &mocks.ExperimentInputLoader{
+					MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) {
+						return nil, errMocked
+					},
+				}
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
+	}, {
+		name: "cannot set options",
+		fields: fields{
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockInputPolicy: func() model.InputPolicy {
+						return model.InputOptional
+					},
+					MockSetOptionsAny: func(options map[string]any) error {
+						return errMocked
+					},
+				}
+				return eb, nil
+			},
+			newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader {
+				return &mocks.ExperimentInputLoader{
+					MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) {
+						return []model.OOAPIURLInfo{}, nil
+					},
+				}
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
+	}, {
+		name: "cannot create new submitter",
+		fields: fields{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockInputPolicy: func() model.InputPolicy {
+						return model.InputOptional
+					},
+					MockSetOptionsAny: func(options map[string]any) error {
+						return nil
+					},
+					MockNewExperiment: func() model.Experiment {
+						exp := &mocks.Experiment{
+							MockKibiBytesReceived: func() float64 {
+								return 0
+							},
+							MockKibiBytesSent: func() float64 {
+								return 0
+							},
+						}
+						return exp
+					},
+				}
+				return eb, nil
+			},
+			newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader {
+				return &mocks.ExperimentInputLoader{
+					MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) {
+						return []model.OOAPIURLInfo{}, nil
+					},
+				}
+			},
+			newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+				return nil, errMocked
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
+	}, {
+		name: "cannot create new saver",
+		fields: fields{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockInputPolicy: func() model.InputPolicy {
+						return model.InputOptional
+					},
+					MockSetOptionsAny: func(options map[string]any) error {
+						return nil
+					},
+					MockNewExperiment: func() model.Experiment {
+						exp := &mocks.Experiment{
+							MockKibiBytesReceived: func() float64 {
+								return 0
+							},
+							MockKibiBytesSent: func() float64 {
+								return 0
+							},
+						}
+						return exp
+					},
+				}
+				return eb, nil
+			},
+			newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader {
+				return &mocks.ExperimentInputLoader{
+					MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) {
+						return []model.OOAPIURLInfo{}, nil
+					},
+				}
+			},
+			newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+				return &mocks.Submitter{}, nil
+			},
+			newSaverFn: func(experiment model.Experiment) (engine.Saver, error) {
+				return nil, errMocked
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
+	}, {
+		name: "input processor fails",
+		fields: fields{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockInputPolicy: func() model.InputPolicy {
+						return model.InputOptional
+					},
+					MockSetOptionsAny: func(options map[string]any) error {
+						return nil
+					},
+					MockNewExperiment: func() model.Experiment {
+						exp := &mocks.Experiment{
+							MockKibiBytesReceived: func() float64 {
+								return 0
+							},
+							MockKibiBytesSent: func() float64 {
+								return 0
+							},
+						}
+						return exp
+					},
+				}
+				return eb, nil
+			},
+			newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader {
+				return &mocks.ExperimentInputLoader{
+					MockLoad: func(ctx context.Context) ([]model.OOAPIURLInfo, error) {
+						return []model.OOAPIURLInfo{}, nil
+					},
+				}
+			},
+			newSubmitterFn: func(ctx context.Context) (engine.Submitter, error) {
+				return &mocks.Submitter{}, nil
+			},
+			newSaverFn: func(experiment model.Experiment) (engine.Saver, error) {
+				return &mocks.Saver{}, nil
+			},
+			newInputProcessorFn: func(experiment model.Experiment, inputList []model.OOAPIURLInfo,
+				saver engine.Saver, submitter engine.Submitter) inputProcessor {
+				return &mocks.ExperimentInputProcessor{
+					MockRun: func(ctx context.Context) error {
+						return errMocked
+					},
+				}
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ed := &Experiment{
+				Annotations:            tt.fields.Annotations,
+				ExtraOptions:           tt.fields.ExtraOptions,
+				Inputs:                 tt.fields.Inputs,
+				InputFilePaths:         tt.fields.InputFilePaths,
+				MaxRuntime:             tt.fields.MaxRuntime,
+				Name:                   tt.fields.Name,
+				NoCollector:            tt.fields.NoCollector,
+				NoJSON:                 tt.fields.NoJSON,
+				Random:                 tt.fields.Random,
+				ReportFile:             tt.fields.ReportFile,
+				Session:                tt.fields.Session,
+				newExperimentBuilderFn: tt.fields.newExperimentBuilderFn,
+				newInputLoaderFn:       tt.fields.newInputLoaderFn,
+				newSubmitterFn:         tt.fields.newSubmitterFn,
+				newSaverFn:             tt.fields.newSaverFn,
+				newInputProcessorFn:    tt.fields.newInputProcessorFn,
+			}
+			err := ed.Run(tt.args.ctx)
+			if !errors.Is(err, tt.expectErr) {
+				t.Fatalf("Experiment.Run() error = %v, expectErr %v", err, tt.expectErr)
+			}
+		})
+	}
+}

internal/oonirun/v1.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"net/url"
 )
 
@@ -50,7 +51,7 @@ func v1Measure(ctx context.Context, config *LinkConfig, URL string) error {
 		if pu.Host != "nettest" {
 			return ErrInvalidV1URLHost
 		}
-		if pu.Path != "" {
+		if pu.Path != "" && pu.Path != "/" {
 			return ErrInvalidV1URLPath
 		}
 	default:
@@ -58,33 +59,50 @@ func v1Measure(ctx context.Context, config *LinkConfig, URL string) error {
 	}
 	name := pu.Query().Get("tn")
 	if name == "" {
-		return ErrInvalidV1URLQueryArgument
+		return fmt.Errorf("%w: empty test name", ErrInvalidV1URLQueryArgument)
 	}
 	var inputs []string
-	if ra := pu.Query().Get("ta"); ra != "" {
-		pa, err := url.QueryUnescape(ra)
+	if ta := pu.Query().Get("ta"); ta != "" {
+		inputs, err = v1ParseArguments(ta)
 		if err != nil {
 			return err
 		}
-		var arguments v1Arguments
-		if err := json.Unmarshal([]byte(pa), &arguments); err != nil {
-			return err
-		}
-		inputs = arguments.URLs
 	}
-	// TODO(bassosimone): reject mv < 1.2.0
+	if mv := pu.Query().Get("mv"); mv != "1.2.0" {
+		return fmt.Errorf("%w: unknown minimum version", ErrInvalidV1URLQueryArgument)
+	}
 	exp := &Experiment{
-		Annotations:    config.Annotations,
-		ExtraOptions:   nil, // no way to specify with v1 URLs
-		Inputs:         inputs,
-		InputFilePaths: nil,
-		MaxRuntime:     config.MaxRuntime,
-		Name:           name,
-		NoCollector:    config.NoCollector,
-		NoJSON:         config.NoJSON,
-		Random:         config.Random,
-		ReportFile:     config.ReportFile,
-		Session:        config.Session,
+		Annotations:            config.Annotations,
+		ExtraOptions:           nil, // no way to specify with v1 URLs
+		Inputs:                 inputs,
+		InputFilePaths:         nil,
+		MaxRuntime:             config.MaxRuntime,
+		Name:                   name,
+		NoCollector:            config.NoCollector,
+		NoJSON:                 config.NoJSON,
+		Random:                 config.Random,
+		ReportFile:             config.ReportFile,
+		Session:                config.Session,
+		newExperimentBuilderFn: nil,
+		newInputLoaderFn:       nil,
+		newSubmitterFn:         nil,
+		newSaverFn:             nil,
+		newInputProcessorFn:    nil,
 	}
 	return exp.Run(ctx)
 }
+
+// v1ParseArguments parses the `ta` field of the query string.
+func v1ParseArguments(ta string) ([]string, error) {
+	var inputs []string
+	pa, err := url.QueryUnescape(ta)
+	if err != nil {
+		return nil, fmt.Errorf("%w: %s", ErrInvalidV1URLQueryArgument, err.Error())
+	}
+	var arguments v1Arguments
+	if err := json.Unmarshal([]byte(pa), &arguments); err != nil {
+		return nil, fmt.Errorf("%w: %s", ErrInvalidV1URLQueryArgument, err.Error())
+	}
+	inputs = arguments.URLs
+	return inputs, nil
+}

internal/oonirun/v1_test.go

@@ -2,13 +2,60 @@ package oonirun
 
 import (
 	"context"
+	"errors"
+	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
+	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/model/mocks"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-// TODO(bassosimone): it would be cool to write unit tests. However, to do that
-// we need to ~redesign the engine package for unit-testability.
+func newMinimalFakeSession() *mocks.Session {
+	return &mocks.Session{
+		MockLogger: func() model.Logger {
+			return model.DiscardLogger
+		},
+		MockNewExperimentBuilder: func(name string) (model.ExperimentBuilder, error) {
+			eb := &mocks.ExperimentBuilder{
+				MockInputPolicy: func() model.InputPolicy {
+					return model.InputNone
+				},
+				MockSetOptionsAny: func(options map[string]any) error {
+					return nil
+				},
+				MockNewExperiment: func() model.Experiment {
+					exp := &mocks.Experiment{
+						MockMeasureAsync: func(ctx context.Context, input string) (<-chan *model.Measurement, error) {
+							out := make(chan *model.Measurement)
+							go func() {
+								defer close(out)
+								ff := &testingx.FakeFiller{}
+								var meas model.Measurement
+								ff.Fill(&meas)
+								out <- &meas
+							}()
+							return out, nil
+						},
+						MockKibiBytesReceived: func() float64 {
+							return 1.1
+						},
+						MockKibiBytesSent: func() float64 {
+							return 0.1
+						},
+					}
+					return exp
+				},
+			}
+			return eb, nil
+		},
+		MockDefaultHTTPClient: func() model.HTTPClient {
+			return http.DefaultClient
+		},
+	}
+}
 
 func TestOONIRunV1Link(t *testing.T) {
 	ctx := context.Background()
@@ -23,7 +70,7 @@ func TestOONIRunV1Link(t *testing.T) {
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
-		Session:     newSession(ctx, t),
+		Session:     newMinimalFakeSession(),
 	}
 	r := NewLinkRunner(config, "https://run.ooni.io/nettest?tn=example&mv=1.2.0")
 	if err := r.Run(ctx); err != nil {
@@ -34,3 +81,188 @@ func TestOONIRunV1Link(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestV1MeasureInvalidURL(t *testing.T) {
+	t.Run("URL does not parse", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "\t"
+		err := v1Measure(ctx, config, URL)
+		if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with https:// URL and invalid hostname", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "https://run.ooni.nu/nettest"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLHost) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with https:// URL and invalid path", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "https://run.ooni.io/antani"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLPath) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with ooni:// URL and invalid host", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "ooni://antani"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLHost) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with ooni:// URL and path", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "ooni://nettest/x"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLPath) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with invalid URL scheme", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "antani://nettest"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLScheme) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with empty test name", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "ooni://nettest/"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLQueryArgument) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with invalid JSON and explicit / as path", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "ooni://nettest/?tn=web_connectivity&ta=123x"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLQueryArgument) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with invalid JSON and empty path", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "ooni://nettest?tn=web_connectivity&ta=123x"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLQueryArgument) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+
+	t.Run("with missing minimum version", func(t *testing.T) {
+		ctx := context.Background()
+		config := &LinkConfig{
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+		}
+		URL := "ooni://nettest?tn=example"
+		err := v1Measure(ctx, config, URL)
+		if !errors.Is(err, ErrInvalidV1URLQueryArgument) {
+			t.Fatal("unexpected err", err)
+		}
+	})
+}
+
+func TestV1ParseArguments(t *testing.T) {
+	t.Run("with invalid test arguments", func(t *testing.T) {
+		// "[QueryUnescape] returns an error if any % is not followed by two hexadecimal digits."
+		out, err := v1ParseArguments("%KK")
+		if !errors.Is(err, ErrInvalidV1URLQueryArgument) {
+			t.Fatal("unexpected err", err)
+		}
+		if len(out) > 0 {
+			t.Fatal("expected no output")
+		}
+	})
+
+	t.Run("with valid arguments", func(t *testing.T) {
+		out, err := v1ParseArguments("%7B%22urls%22%3A%5B%22https%3A%2F%2Fexample.com%2F%22%5D%7D")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(out) != 1 || out[0] != "https://example.com/" {
+			t.Fatal("unexpected out", out)
+		}
+	})
+}

internal/oonirun/v2.go

@@ -24,6 +24,10 @@ var (
 	// v2CountEmptyNettestNames counts the number of cases in which we have been
 	// given an empty nettest name, which is useful for testing.
 	v2CountEmptyNettestNames = &atomicx.Int64{}
+
+	// v2CountFailedExperiments countes the number of failed experiments
+	// and is useful when testing this package
+	v2CountFailedExperiments = &atomicx.Int64{}
 )
 
 // v2Descriptor describes a single nettest to run.
@@ -172,20 +176,26 @@ func v2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *v2Descri
 			continue
 		}
 		exp := &Experiment{
-			Annotations:    config.Annotations,
-			ExtraOptions:   nettest.Options,
-			Inputs:         nettest.Inputs,
-			InputFilePaths: nil,
-			MaxRuntime:     config.MaxRuntime,
-			Name:           nettest.TestName,
-			NoCollector:    config.NoCollector,
-			NoJSON:         config.NoJSON,
-			Random:         config.Random,
-			ReportFile:     config.ReportFile,
-			Session:        config.Session,
+			Annotations:            config.Annotations,
+			ExtraOptions:           nettest.Options,
+			Inputs:                 nettest.Inputs,
+			InputFilePaths:         nil,
+			MaxRuntime:             config.MaxRuntime,
+			Name:                   nettest.TestName,
+			NoCollector:            config.NoCollector,
+			NoJSON:                 config.NoJSON,
+			Random:                 config.Random,
+			ReportFile:             config.ReportFile,
+			Session:                config.Session,
+			newExperimentBuilderFn: nil,
+			newInputLoaderFn:       nil,
+			newSubmitterFn:         nil,
+			newSaverFn:             nil,
+			newInputProcessorFn:    nil,
 		}
 		if err := exp.Run(ctx); err != nil {
 			logger.Warnf("cannot run experiment: %s", err.Error())
+			v2CountFailedExperiments.Add(1)
 			continue
 		}
 	}

internal/oonirun/v2_test.go

@@ -10,13 +10,11 @@ import (
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
+	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/model/mocks"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
 
-// TODO(bassosimone): it would be cool to write unit tests. However, to do that
-// we need to ~redesign the engine package for unit-testability.
-
 func TestOONIRunV2LinkCommonCase(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		descriptor := &v2Descriptor{
@@ -48,7 +46,7 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) {
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
-		Session:     newSession(ctx, t),
+		Session:     newMinimalFakeSession(),
 	}
 	r := NewLinkRunner(config, server.URL)
 	if err := r.Run(ctx); err != nil {
@@ -95,7 +93,7 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) {
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
-		Session:     newSession(ctx, t),
+		Session:     newMinimalFakeSession(),
 	}
 	r := NewLinkRunner(config, server.URL)
 	err := r.Run(ctx)
@@ -135,7 +133,7 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) {
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
-		Session:     newSession(ctx, t),
+		Session:     newMinimalFakeSession(),
 	}
 	r := NewLinkRunner(config, server.URL)
 	err := r.Run(ctx)
@@ -161,7 +159,7 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) {
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
-		Session:     newSession(ctx, t),
+		Session:     newMinimalFakeSession(),
 	}
 	r := NewLinkRunner(config, server.URL)
 	if err := r.Run(ctx); err != nil {
@@ -170,6 +168,7 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) {
 }
 
 func TestOONIRunV2LinkEmptyTestName(t *testing.T) {
+	emptyTestNamesPrev := v2CountEmptyNettestNames.Load()
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		descriptor := &v2Descriptor{
 			Name:        "",
@@ -200,14 +199,14 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) {
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
-		Session:     newSession(ctx, t),
+		Session:     newMinimalFakeSession(),
 	}
 	r := NewLinkRunner(config, server.URL)
 	if err := r.Run(ctx); err != nil {
 		t.Fatal(err)
 	}
-	if v2CountEmptyNettestNames.Load() != 1 {
-		t.Fatal("expected to see 1 instance of empty nettest names")
+	if v2CountEmptyNettestNames.Load() != emptyTestNamesPrev+1 {
+		t.Fatal("expected to see 1 more instance of empty nettest names")
 	}
 }
 
@@ -220,6 +219,74 @@ func TestV2MeasureDescriptor(t *testing.T) {
 			t.Fatal("unexpected err", err)
 		}
 	})
+
+	t.Run("with failing experiment", func(t *testing.T) {
+		previousFailedExperiments := v2CountFailedExperiments.Load()
+		expected := errors.New("mocked error")
+		ctx := context.Background()
+		sess := newMinimalFakeSession()
+		sess.MockNewSubmitter = func(ctx context.Context) (model.Submitter, error) {
+			subm := &mocks.Submitter{
+				MockSubmit: func(ctx context.Context, m *model.Measurement) error {
+					panic("should not be called")
+				},
+			}
+			return subm, nil
+		}
+		sess.MockNewExperimentBuilder = func(name string) (model.ExperimentBuilder, error) {
+			eb := &mocks.ExperimentBuilder{
+				MockInputPolicy: func() model.InputPolicy {
+					return model.InputNone
+				},
+				MockSetOptionsAny: func(options map[string]any) error {
+					return nil
+				},
+				MockNewExperiment: func() model.Experiment {
+					exp := &mocks.Experiment{
+						MockMeasureAsync: func(ctx context.Context, input string) (<-chan *model.Measurement, error) {
+							return nil, expected
+						},
+						MockKibiBytesReceived: func() float64 {
+							return 1.1
+						},
+						MockKibiBytesSent: func() float64 {
+							return 0.1
+						},
+					}
+					return exp
+				},
+			}
+			return eb, nil
+		}
+		config := &LinkConfig{
+			AcceptChanges: false,
+			Annotations:   map[string]string{},
+			KVStore:       nil,
+			MaxRuntime:    0,
+			NoCollector:   false,
+			NoJSON:        false,
+			Random:        false,
+			ReportFile:    "",
+			Session:       sess,
+		}
+		descr := &v2Descriptor{
+			Name:        "",
+			Description: "",
+			Author:      "",
+			Nettests: []v2Nettest{{
+				Inputs:   []string{},
+				Options:  map[string]any{},
+				TestName: "example",
+			}},
+		}
+		err := v2MeasureDescriptor(ctx, config, descr)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if v2CountFailedExperiments.Load() != previousFailedExperiments+1 {
+			t.Fatal("expected to see a failed experiment")
+		}
+	})
 }
 
 func TestV2MeasureHTTPS(t *testing.T) {
@@ -239,7 +306,7 @@ func TestV2MeasureHTTPS(t *testing.T) {
 			NoJSON:      false,
 			Random:      false,
 			ReportFile:  "",
-			Session:     newSession(ctx, t),
+			Session:     newMinimalFakeSession(),
 		}
 		err := v2MeasureHTTPS(ctx, config, "")
 		if !errors.Is(err, expected) {
@@ -259,7 +326,7 @@ func TestV2MeasureHTTPS(t *testing.T) {
 			NoJSON:        false,
 			Random:        false,
 			ReportFile:    "",
-			Session:       newSession(ctx, t),
+			Session:       newMinimalFakeSession(),
 		}
 		err := v2MeasureHTTPS(ctx, config, "https://example.com") // should not use URL
 		if !errors.Is(err, context.Canceled) {