x

internal/oonirun/experiment.go

@@ -7,9 +7,7 @@ package oonirun
 import (
 	"context"
 	"encoding/json"
-	"fmt"
 	"math/rand"
-	"strings"
 	"sync/atomic"
 	"time"
 
@@ -235,22 +233,6 @@ func (ed *Experiment) newTargetLoader(builder model.ExperimentBuilder) targetLoa
 	})
 }
 
-// experimentOptionsToStringList convers the options to []string, which is
-// the format with which we include them into a OONI Measurement. The resulting
-// []string will skip any option that is named with a `Safe` prefix (case
-// sensitive).
-func experimentOptionsToStringList(options map[string]any) (out []string) {
-	// the prefix to skip inclusion in the string list
-	safeOptionPrefix := "Safe"
-	for key, value := range options {
-		if strings.HasPrefix(key, safeOptionPrefix) {
-			continue
-		}
-		out = append(out, fmt.Sprintf("%s=%v", key, value))
-	}
-	return
-}
-
 // experimentWrapper wraps an experiment and logs progress
 type experimentWrapper struct {
 	// child is the child experiment wrapper

internal/oonirun/experiment_test.go

@@ -4,8 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"reflect"
-	"sort"
 	"testing"
 	"time"
 
@@ -189,48 +187,6 @@ func TestExperimentSetOptions(t *testing.T) {
 	}
 }
 
-func Test_experimentOptionsToStringList(t *testing.T) {
-	type args struct {
-		options map[string]any
-	}
-	tests := []struct {
-		name    string
-		args    args
-		wantOut []string
-	}{
-		{
-			name: "happy path: a map with three entries returns three items",
-			args: args{
-				map[string]any{
-					"foo":  1,
-					"bar":  2,
-					"baaz": 3,
-				},
-			},
-			wantOut: []string{"baaz=3", "bar=2", "foo=1"},
-		},
-		{
-			name: "an option beginning with `Safe` is skipped from the output",
-			args: args{
-				map[string]any{
-					"foo":     1,
-					"Safefoo": 42,
-				},
-			},
-			wantOut: []string{"foo=1"},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			gotOut := experimentOptionsToStringList(tt.args.options)
-			sort.Strings(gotOut)
-			if !reflect.DeepEqual(gotOut, tt.wantOut) {
-				t.Errorf("experimentOptionsToStringList() = %v, want %v", gotOut, tt.wantOut)
-			}
-		})
-	}
-}
-
 func TestExperimentRun(t *testing.T) {
 	errMocked := errors.New("mocked error")
 	type fields struct {

internal/oonirun/inputprocessor.go

@@ -143,8 +143,11 @@ func (ip *InputProcessor) run(ctx context.Context) (int, error) {
 		meas.AddAnnotations(ip.Annotations)
 		err = ip.Submitter.Submit(ctx, idx, meas)
 		if err != nil {
-			// TODO(bassosimone): the default submitter used by
-			// miniooni ignores errors. Should we make it the default?
+			// TODO(bassosimone): when re-reading this code, I find it confusing that
+			// we return on error because I am always like "wait, this is not the right
+			// thing to do here". Then, I remember that the experimentSubmitterWrapper
+			// ignores this error and so it's like it does not exist. Maybe we should
+			// rewrite the code to do the right thing here.
 			return 0, err
 		}
 		// Note: must be after submission because submission modifies