feat(oonirun): allow true JSON richer input

internal/engine/experimentbuilder.go

@@ -5,11 +5,13 @@ package engine
 //
 
 import (
+	"encoding/json"
+
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/registry"
 )
 
-// experimentBuilder implements ExperimentBuilder.
+// experimentBuilder implements [model.ExperimentBuilder].
 //
 // This type is now just a tiny wrapper around registry.Factory.
 type experimentBuilder struct {
@@ -24,37 +26,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 +74,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/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,11 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error {
 	return eb.MockSetOptionsAny(options)
 }
 
+func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error {
+	// TODO(bassosimone): write unit tests for this method
+	return eb.MockSetOptionsJSON(value)
+}
+
 func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) {
 	eb.MockSetCallbacks(callbacks)
 }

internal/model/experiment.go

@@ -7,6 +7,7 @@ package model
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 )
@@ -235,6 +236,11 @@ 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]
+	// MUST contain a serialization of the experiment config's type.
+	SetOptionsJSON(value json.RawMessage) error
+
 	// SetCallbacks sets the experiment's interactive callbacks.
 	SetCallbacks(callbacks ExperimentCallbacks)
 

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-empty 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
+	// 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,15 +92,22 @@ 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.
 
 	// 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 {
 		return err
 	}

internal/oonirun/experiment_test.go

@@ -2,6 +2,7 @@ package oonirun
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"reflect"
 	"sort"
@@ -16,6 +17,7 @@ import (
 func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 	shuffledInputsPrev := experimentShuffledInputs.Load()
 	var calledSetOptionsAny int
+	var calledSetOptionsJSON int
 	var failedToSubmit int
 	var calledKibiBytesReceived int
 	var calledKibiBytesSent int
@@ -44,6 +46,10 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 						calledSetOptionsAny++
 						return nil
 					},
+					MockSetOptionsJSON: func(value json.RawMessage) error {
+						calledSetOptionsJSON++
+						return nil
+					},
 					MockNewExperiment: func() model.Experiment {
 						exp := &mocks.Experiment{
 							MockMeasureWithContext: func(
@@ -109,6 +115,9 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 	if calledSetOptionsAny < 1 {
 		t.Fatal("should have called SetOptionsAny")
 	}
+	if calledSetOptionsJSON < 1 {
+		t.Fatal("should have called SetOptionsJSON")
+	}
 	if calledKibiBytesReceived < 1 {
 		t.Fatal("did not call KibiBytesReceived")
 	}
@@ -198,10 +207,14 @@ func TestExperimentRun(t *testing.T) {
 		args:      args{},
 		expectErr: errMocked,
 	}, {
-		name: "cannot set options",
+		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 {
 						return errMocked
 					},
@@ -226,6 +239,9 @@ func TestExperimentRun(t *testing.T) {
 					MockInputPolicy: func() model.InputPolicy {
 						return model.InputOptional
 					},
+					MockSetOptionsJSON: func(value json.RawMessage) error {
+						return nil
+					},
 					MockSetOptionsAny: func(options map[string]any) error {
 						return nil
 					},
@@ -255,6 +271,9 @@ func TestExperimentRun(t *testing.T) {
 					MockInputPolicy: func() model.InputPolicy {
 						return model.InputOptional
 					},
+					MockSetOptionsJSON: func(value json.RawMessage) error {
+						return nil
+					},
 					MockSetOptionsAny: func(options map[string]any) error {
 						return nil
 					},
@@ -298,6 +317,9 @@ func TestExperimentRun(t *testing.T) {
 					MockInputPolicy: func() model.InputPolicy {
 						return model.InputOptional
 					},
+					MockSetOptionsJSON: func(value json.RawMessage) error {
+						return nil
+					},
 					MockSetOptionsAny: func(options map[string]any) error {
 						return nil
 					},
@@ -344,6 +366,9 @@ func TestExperimentRun(t *testing.T) {
 					MockInputPolicy: func() model.InputPolicy {
 						return model.InputOptional
 					},
+					MockSetOptionsJSON: func(value json.RawMessage) error {
+						return nil
+					},
 					MockSetOptionsAny: func(options map[string]any) error {
 						return nil
 					},

internal/oonirun/v1_test.go

@@ -2,6 +2,7 @@ package oonirun
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"net/http"
 	"strings"
@@ -26,6 +27,9 @@ func newMinimalFakeSession() *mocks.Session {
 				MockSetOptionsAny: func(options map[string]any) error {
 					return nil
 				},
+				MockSetOptionsJSON: func(value json.RawMessage) error {
+					return nil
+				},
 				MockNewExperiment: func() model.Experiment {
 					exp := &mocks.Experiment{
 						MockMeasureWithContext: func(

internal/oonirun/v2.go

@@ -54,7 +54,7 @@ type V2Nettest struct {
 	// `Safe` will be available for the experiment run, but omitted from
 	// the serialized Measurement that the experiment builder will submit
 	// to the OONI backend.
-	Options map[string]any `json:"options"`
+	Options json.RawMessage `json:"options"`
 
 	// TestName contains the nettest name.
 	TestName string `json:"test_name"`
@@ -183,7 +183,8 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri
 		// construct an experiment from the current nettest
 		exp := &Experiment{
 			Annotations:            config.Annotations,
-			ExtraOptions:           nettest.Options,
+			ExtraOptions:           make(map[string]any),
+			InitialOptions:         nettest.Options,
 			Inputs:                 nettest.Inputs,
 			InputFilePaths:         nil,
 			MaxRuntime:             config.MaxRuntime,

internal/oonirun/v2_test.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/httpclientx"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
@@ -27,9 +26,9 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) {
 			Author:      "",
 			Nettests: []V2Nettest{{
 				Inputs: []string{},
-				Options: map[string]any{
-					"SleepTime": int64(10 * time.Millisecond),
-				},
+				Options: json.RawMessage(`{
+					"SleepTime": 10000000
+				}`),
 				TestName: "example",
 			}},
 		}
@@ -73,9 +72,9 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) {
 			Author:      "",
 			Nettests: []V2Nettest{{
 				Inputs: []string{},
-				Options: map[string]any{
-					"SleepTime": int64(10 * time.Millisecond),
-				},
+				Options: json.RawMessage(`{
+					"SleepTime": 10000000
+				}`),
 				TestName: "example",
 			}},
 		}
@@ -132,9 +131,9 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) {
 			Author:      "",
 			Nettests: []V2Nettest{{
 				Inputs: []string{},
-				Options: map[string]any{
-					"SleepTime": int64(10 * time.Millisecond),
-				},
+				Options: json.RawMessage(`{
+					"SleepTime": 10000000
+				}`),
 				TestName: "example",
 			}},
 		}
@@ -220,9 +219,9 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) {
 			Author:      "",
 			Nettests: []V2Nettest{{
 				Inputs: []string{},
-				Options: map[string]any{
-					"SleepTime": int64(10 * time.Millisecond),
-				},
+				Options: json.RawMessage(`{
+					"SleepTime": 10000000
+				}`),
 				TestName: "", // empty!
 			}},
 		}
@@ -374,6 +373,9 @@ func TestV2MeasureDescriptor(t *testing.T) {
 				MockInputPolicy: func() model.InputPolicy {
 					return model.InputNone
 				},
+				MockSetOptionsJSON: func(value json.RawMessage) error {
+					return nil
+				},
 				MockSetOptionsAny: func(options map[string]any) error {
 					return nil
 				},
@@ -426,7 +428,7 @@ func TestV2MeasureDescriptor(t *testing.T) {
 			Author:      "",
 			Nettests: []V2Nettest{{
 				Inputs:   []string{},
-				Options:  map[string]any{},
+				Options:  json.RawMessage(`{}`),
 				TestName: "example",
 			}},
 		}
@@ -532,5 +534,4 @@ func TestV2DescriptorCacheLoad(t *testing.T) {
 			t.Fatal("expected nil cache")
 		}
 	})
-
 }