Merge branch 'ri-multi-input' into ri-correctly-set-options

Conflicts:
	internal/oonirun/experiment.go

internal/engine/experimentbuilder.go

@@ -11,6 +11,12 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/registry"
 )
 
+// TODO(bassosimone,DecFox): we should eventually finish merging the code in
+// file with the code inside the ./internal/registry package.
+//
+// If there's time, this could happen at the end of the current (as of 2024-06-27)
+// richer input work, otherwise any time in the future is actually fine.
+
 // experimentBuilder implements [model.ExperimentBuilder].
 //
 // This type is now just a tiny wrapper around registry.Factory.

internal/engine/experimentbuilder_test.go

@@ -2,9 +2,11 @@ package engine
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
@@ -49,3 +51,119 @@ func TestExperimentBuilderEngineWebConnectivity(t *testing.T) {
 		t.Fatal("expected zero length targets")
 	}
 }
+
+func TestExperimentBuilderBasicOperations(t *testing.T) {
+	// create a session for testing that does not use the network at all
+	sess := newSessionForTestingNoLookups(t)
+
+	// create an experiment builder for example
+	builder, err := sess.NewExperimentBuilder("example")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// example should be interruptible
+	t.Run("Interruptible", func(t *testing.T) {
+		if !builder.Interruptible() {
+			t.Fatal("example should be interruptible")
+		}
+	})
+
+	// we expect to see the InputNone input policy
+	t.Run("InputPolicy", func(t *testing.T) {
+		if builder.InputPolicy() != model.InputNone {
+			t.Fatal("unexpectyed input policy")
+		}
+	})
+
+	// get the options and check whether they are what we expect
+	t.Run("Options", func(t *testing.T) {
+		options, err := builder.Options()
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectOptions := map[string]model.ExperimentOptionInfo{
+			"Message":     {Doc: "Message to emit at test completion", Type: "string", Value: "Good day from the example experiment!"},
+			"ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: false},
+			"SleepTime":   {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)},
+		}
+		if diff := cmp.Diff(expectOptions, options); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+
+	// we can set a specific existing option
+	t.Run("SetOptionAny", func(t *testing.T) {
+		if err := builder.SetOptionAny("Message", "foobar"); err != nil {
+			t.Fatal(err)
+		}
+		options, err := builder.Options()
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectOptions := map[string]model.ExperimentOptionInfo{
+			"Message":     {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"},
+			"ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: false},
+			"SleepTime":   {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)},
+		}
+		if diff := cmp.Diff(expectOptions, options); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+
+	// we can set all options at the same time
+	t.Run("SetOptions", func(t *testing.T) {
+		inputs := map[string]any{
+			"Message":     "foobar",
+			"ReturnError": true,
+		}
+		if err := builder.SetOptionsAny(inputs); err != nil {
+			t.Fatal(err)
+		}
+		options, err := builder.Options()
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectOptions := map[string]model.ExperimentOptionInfo{
+			"Message":     {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"},
+			"ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: true},
+			"SleepTime":   {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)},
+		}
+		if diff := cmp.Diff(expectOptions, options); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+
+	// we can set all options using JSON
+	t.Run("SetOptionsJSON", func(t *testing.T) {
+		inputs := json.RawMessage(`{
+			"Message":     "foobar",
+			"ReturnError": true
+		}`)
+		if err := builder.SetOptionsJSON(inputs); err != nil {
+			t.Fatal(err)
+		}
+		options, err := builder.Options()
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectOptions := map[string]model.ExperimentOptionInfo{
+			"Message":     {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"},
+			"ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: true},
+			"SleepTime":   {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)},
+		}
+		if diff := cmp.Diff(expectOptions, options); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+
+	// TODO(bassosimone): we could possibly add more checks here. I am not doing this
+	// right now, because https://github.com/ooni/probe-cli/pull/1629 mostly cares about
+	// providing input and the rest of the codebase did not change.
+	//
+	// Also, it would make sense to eventually merge experimentbuilder.go with the
+	// ./internal/registry package, which also has coverage.
+	//
+	// In conclusion, our main objective for now is to make sure we don't screw the
+	// pooch when setting options using the experiment builder.
+}

internal/mocks/experimentbuilder.go

@@ -50,7 +50,6 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error {
 }
 
 func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error {
-	// TODO(bassosimone): write unit tests for this method
 	return eb.MockSetOptionsJSON(value)
 }
 

internal/mocks/experimentbuilder_test.go

@@ -1,6 +1,7 @@
 package mocks
 
 import (
+	"encoding/json"
 	"errors"
 	"testing"
 
@@ -72,6 +73,19 @@ func TestExperimentBuilder(t *testing.T) {
 		}
 	})
 
+	t.Run("SetOptionsJSON", func(t *testing.T) {
+		expected := errors.New("mocked error")
+		eb := &ExperimentBuilder{
+			MockSetOptionsJSON: func(value json.RawMessage) error {
+				return expected
+			},
+		}
+		err := eb.SetOptionsJSON([]byte(`{}`))
+		if !errors.Is(err, expected) {
+			t.Fatal("unexpected value")
+		}
+	})
+
 	t.Run("SetCallbacks", func(t *testing.T) {
 		var called bool
 		eb := &ExperimentBuilder{

internal/model/experiment.go

@@ -309,6 +309,9 @@ type ExperimentOptionInfo struct {
 
 	// Type contains the type.
 	Type string
+
+	// Value contains the current option value.
+	Value any
 }
 
 // ExperimentTargetLoader loads targets from local or remote sources.

internal/oonirun/experiment.go

@@ -27,13 +27,13 @@ type Experiment struct {
 	Annotations map[string]string
 
 	// ExtraOptions contains OPTIONAL extra options that modify the
-	// default-empty experiment-specific configuration. We apply
+	// default experiment-specific configuration. We apply
 	// the changes described by this field after using the InitialOptions
 	// field to initialize the experiment-specific configuration.
 	ExtraOptions map[string]any
 
 	// InitialOptions contains an OPTIONAL [json.RawMessage] object
-	// used to initialize the default-empty experiment-specific
+	// used to initialize the default experiment-specific
 	// configuration. After we have initialized the configuration
 	// as such, we then apply the changes described by the ExtraOptions.
 	InitialOptions json.RawMessage
@@ -94,16 +94,9 @@ func (ed *Experiment) Run(ctx context.Context) error {
 
 	// 2. configure experiment's options
 	//
-	// We first unmarshal the InitialOptions into the experiment
-	// configuration and afterwards we modify the configuration using
-	// the values contained inside the ExtraOptions field.
-	//
 	// This MUST happen before loading targets because the options will
 	// possibly be used to produce richer input targets.
-	if err := builder.SetOptionsJSON(ed.InitialOptions); err != nil {
-		return err
-	}
-	if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil {
+	if err := ed.setOptions(builder); err != nil {
 		return err
 	}
 
@@ -154,6 +147,16 @@ func (ed *Experiment) Run(ctx context.Context) error {
 	return inputProcessor.Run(ctx)
 }
 
+func (ed *Experiment) setOptions(builder model.ExperimentBuilder) error {
+	// We first unmarshal the InitialOptions into the experiment
+	// configuration and afterwards we modify the configuration using
+	// the values contained inside the ExtraOptions field.
+	if err := builder.SetOptionsJSON(ed.InitialOptions); err != nil {
+		return err
+	}
+	return builder.SetOptionsAny(ed.ExtraOptions)
+}
+
 // inputProcessor is an alias for model.ExperimentInputProcessor
 type inputProcessor = model.ExperimentInputProcessor
 

internal/oonirun/experiment_test.go

@@ -9,6 +9,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/testingx"
@@ -42,14 +44,14 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 					MockInputPolicy: func() model.InputPolicy {
 						return model.InputOptional
 					},
-					MockSetOptionsAny: func(options map[string]any) error {
-						calledSetOptionsAny++
-						return nil
-					},
 					MockSetOptionsJSON: func(value json.RawMessage) error {
 						calledSetOptionsJSON++
 						return nil
 					},
+					MockSetOptionsAny: func(options map[string]any) error {
+						calledSetOptionsAny++
+						return nil
+					},
 					MockNewExperiment: func() model.Experiment {
 						exp := &mocks.Experiment{
 							MockMeasureWithContext: func(
@@ -126,6 +128,67 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 	}
 }
 
+// This test ensures that we honour InitialOptions then ExtraOptions.
+func TestExperimentSetOptions(t *testing.T) {
+
+	// create the Experiment we're using for this test
+	exp := &Experiment{
+		ExtraOptions: map[string]any{
+			"Message": "jarjarbinks",
+		},
+		InitialOptions: []byte(`{"Message": "foobar", "ReturnError": true}`),
+		Name:           "example",
+
+		// TODO(bassosimone): A zero-value session works here. The proper change
+		// however would be to write a engine.NewExperimentBuilder factory that takes
+		// as input an interface for the session. This would help testing.
+		Session: &engine.Session{},
+	}
+
+	// create the experiment builder manually
+	builder, err := exp.newExperimentBuilder(exp.Name)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// invoke the method we're testing
+	if err := exp.setOptions(builder); err != nil {
+		t.Fatal(err)
+	}
+
+	// obtain the options
+	options, err := builder.Options()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// describe what we expect to happen
+	//
+	// we basically want ExtraOptions to override InitialOptions
+	expect := map[string]model.ExperimentOptionInfo{
+		"Message": {
+			Doc:   "Message to emit at test completion",
+			Type:  "string",
+			Value: string("jarjarbinks"), // set by ExtraOptions
+		},
+		"ReturnError": {
+			Doc:   "Toogle to return a mocked error",
+			Type:  "bool",
+			Value: bool(true), // set by InitialOptions
+		},
+		"SleepTime": {
+			Doc:   "Amount of time to sleep for in nanosecond",
+			Type:  "int64",
+			Value: int64(1000000000), // still the default nonzero value
+		},
+	}
+
+	// make sure the result equals expectation
+	if diff := cmp.Diff(expect, options); diff != "" {
+		t.Fatal(diff)
+	}
+}
+
 func Test_experimentOptionsToStringList(t *testing.T) {
 	type args struct {
 		options map[string]any
@@ -206,13 +269,33 @@ func TestExperimentRun(t *testing.T) {
 		},
 		args:      args{},
 		expectErr: errMocked,
+	}, {
+		name: "cannot set InitialOptions",
+		fields: fields{
+			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
+				eb := &mocks.ExperimentBuilder{
+					MockSetOptionsJSON: func(value json.RawMessage) error {
+						return errMocked
+					},
+				}
+				return eb, nil
+			},
+			newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader {
+				return &mocks.ExperimentTargetLoader{
+					MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) {
+						return []model.ExperimentTarget{}, nil
+					},
+				}
+			},
+		},
+		args:      args{},
+		expectErr: errMocked,
 	}, {
 		name: "cannot set ExtraOptions",
 		fields: fields{
 			newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) {
 				eb := &mocks.ExperimentBuilder{
 					MockSetOptionsJSON: func(value json.RawMessage) error {
-						// TODO(bassosimone): need a test case before this one
 						return nil
 					},
 					MockSetOptionsAny: func(options map[string]any) error {

internal/oonirun/v1_test.go

@@ -24,10 +24,10 @@ func newMinimalFakeSession() *mocks.Session {
 				MockInputPolicy: func() model.InputPolicy {
 					return model.InputNone
 				},
-				MockSetOptionsAny: func(options map[string]any) error {
+				MockSetOptionsJSON: func(value json.RawMessage) error {
 					return nil
 				},
-				MockSetOptionsJSON: func(value json.RawMessage) error {
+				MockSetOptionsAny: func(options map[string]any) error {
 					return nil
 				},
 				MockNewExperiment: func() model.Experiment {