feat(oonirun): allow true JSON richer input (#1629)

Closes ooni/probe#2759

internal/engine/experimentbuilder.go

@@ -5,11 +5,19 @@ package engine
 //
 
 import (
+	"encoding/json"
+
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/registry"
 )
 
-// experimentBuilder implements ExperimentBuilder.
+// 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.
 type experimentBuilder struct {
@@ -24,37 +32,42 @@ type experimentBuilder struct {
 
 var _ model.ExperimentBuilder = &experimentBuilder{}
 
-// Interruptible implements ExperimentBuilder.Interruptible.
+// Interruptible implements [model.ExperimentBuilder].
 func (b *experimentBuilder) Interruptible() bool {
 	return b.factory.Interruptible()
 }
 
-// InputPolicy implements ExperimentBuilder.InputPolicy.
+// InputPolicy implements [model.ExperimentBuilder].
 func (b *experimentBuilder) InputPolicy() model.InputPolicy {
 	return b.factory.InputPolicy()
 }
 
-// Options implements ExperimentBuilder.Options.
+// Options implements [model.ExperimentBuilder].
 func (b *experimentBuilder) Options() (map[string]model.ExperimentOptionInfo, error) {
 	return b.factory.Options()
 }
 
-// SetOptionAny implements ExperimentBuilder.SetOptionAny.
+// SetOptionAny implements [model.ExperimentBuilder].
 func (b *experimentBuilder) SetOptionAny(key string, value any) error {
 	return b.factory.SetOptionAny(key, value)
 }
 
-// SetOptionsAny implements ExperimentBuilder.SetOptionsAny.
+// SetOptionsAny implements [model.ExperimentBuilder].
 func (b *experimentBuilder) SetOptionsAny(options map[string]any) error {
 	return b.factory.SetOptionsAny(options)
 }
 
-// SetCallbacks implements ExperimentBuilder.SetCallbacks.
+// SetOptionsJSON implements [model.ExperimentBuilder].
+func (b *experimentBuilder) SetOptionsJSON(value json.RawMessage) error {
+	return b.factory.SetOptionsJSON(value)
+}
+
+// SetCallbacks implements [model.ExperimentBuilder].
 func (b *experimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) {
 	b.callbacks = callbacks
 }
 
-// NewExperiment creates the experiment
+// NewExperiment creates a new [model.Experiment] instance.
 func (b *experimentBuilder) NewExperiment() model.Experiment {
 	measurer := b.factory.NewExperimentMeasurer()
 	experiment := newExperiment(b.session, measurer)
@@ -67,7 +80,7 @@ func (b *experimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoader
 	return b.factory.NewTargetLoader(config)
 }
 
-// newExperimentBuilder creates a new experimentBuilder instance.
+// newExperimentBuilder creates a new [*experimentBuilder] instance.
 func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) {
 	factory, err := registry.NewFactory(name, session.kvStore, session.logger)
 	if err != nil {

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

@@ -1,6 +1,10 @@
 package mocks
 
-import "github.com/ooni/probe-cli/v3/internal/model"
+import (
+	"encoding/json"
+
+	"github.com/ooni/probe-cli/v3/internal/model"
+)
 
 // ExperimentBuilder mocks model.ExperimentBuilder.
 type ExperimentBuilder struct {
@@ -14,6 +18,8 @@ type ExperimentBuilder struct {
 
 	MockSetOptionsAny func(options map[string]any) error
 
+	MockSetOptionsJSON func(value json.RawMessage) error
+
 	MockSetCallbacks func(callbacks model.ExperimentCallbacks)
 
 	MockNewExperiment func() model.Experiment
@@ -43,6 +49,10 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error {
 	return eb.MockSetOptionsAny(options)
 }
 
+func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error {
+	return eb.MockSetOptionsJSON(value)
+}
+
 func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) {
 	eb.MockSetCallbacks(callbacks)
 }

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

@@ -7,6 +7,7 @@ package model
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 )
@@ -235,6 +236,12 @@ type ExperimentBuilder interface {
 	// the SetOptionAny method for more information.
 	SetOptionsAny(options map[string]any) error
 
+	// SetOptionsJSON uses the given [json.RawMessage] to initialize fields
+	// of the configuration for running the experiment. The [json.RawMessage], if
+	// not empty, MUST contain a serialization of the experiment config's
+	// type. An empty [json.RawMessage] will silently be ignored.
+	SetOptionsJSON(value json.RawMessage) error
+
 	// SetCallbacks sets the experiment's interactive callbacks.
 	SetCallbacks(callbacks ExperimentCallbacks)
 
@@ -290,6 +297,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

@@ -6,6 +6,7 @@ package oonirun
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"math/rand"
 	"strings"
@@ -25,9 +26,18 @@ type Experiment struct {
 	// Annotations contains OPTIONAL Annotations for the experiment.
 	Annotations map[string]string
 
-	// ExtraOptions contains OPTIONAL extra options for the experiment.
+	// ExtraOptions contains OPTIONAL extra options that modify the
+	// 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 experiment-specific
+	// configuration. After we have initialized the configuration
+	// as such, we then apply the changes described by the ExtraOptions.
+	InitialOptions json.RawMessage
+
 	// Inputs contains the OPTIONAL experiment Inputs
 	Inputs []string
 
@@ -82,16 +92,18 @@ func (ed *Experiment) Run(ctx context.Context) error {
 		return err
 	}
 
-	// TODO(bassosimone,DecFox): when we're executed by OONI Run v2, it probably makes
-	// slightly more sense to set options from a json.RawMessage because the current
-	// command line limitation is that it's hard to set non scalar parameters and instead
-	// with using OONI Run v2 we can completely bypass such a limitation.
+	// TODO(bassosimone): we need another patch after the current one
+	// to correctly serialize the options as configured using InitialOptions
+	// and ExtraOptions otherwise the Measurement.Options field turns out
+	// to always be empty and this is highly suboptimal for us.
+	//
+	// The next patch is https://github.com/ooni/probe-cli/pull/1630.
 
 	// 2. configure experiment's options
 	//
 	// This MUST happen before loading targets because the options will
 	// possibly be used to produce richer input targets.
-	if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil {
+	if err := ed.setOptions(builder); err != nil {
 		return err
 	}
 
@@ -142,6 +154,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