Merge branch 'master' into feat/wireguard-2024

internal/engine/experiment_integration_test.go

@@ -3,6 +3,7 @@ package engine
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -11,6 +12,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/probeservices"
 	"github.com/ooni/probe-cli/v3/internal/registry"
 )
 
@@ -384,7 +386,7 @@ func TestOpenReportNewClientFailure(t *testing.T) {
 		Type:    "antani",
 	}
 	err = exp.OpenReportContext(context.Background())
-	if err.Error() != "probe services: unsupported endpoint type" {
+	if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
 		t.Fatal(err)
 	}
 }

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/engine/session.go

@@ -66,7 +66,6 @@ type Session struct {
 	softwareName             string
 	softwareVersion          string
 	tempDir                  string
-	vpnConfig                map[string]model.OOAPIVPNProviderConfig
 
 	// closeOnce allows us to call Close just once.
 	closeOnce sync.Once
@@ -178,7 +177,6 @@ func NewSession(ctx context.Context, config SessionConfig) (*Session, error) {
 		torArgs:                 config.TorArgs,
 		torBinary:               config.TorBinary,
 		tunnelDir:               config.TunnelDir,
-		vpnConfig:               make(map[string]model.OOAPIVPNProviderConfig),
 	}
 	proxyURL := config.ProxyURL
 	if proxyURL != nil {
@@ -381,23 +379,30 @@ func (s *Session) FetchTorTargets(
 // internal cache. We do this to avoid hitting the API for every input.
 func (s *Session) FetchOpenVPNConfig(
 	ctx context.Context, provider, cc string) (*model.OOAPIVPNProviderConfig, error) {
-	if config, ok := s.vpnConfig[provider]; ok {
-		return &config, nil
-	}
 	clnt, err := s.newOrchestraClient(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	// we cannot lock earlier because newOrchestraClient locks the mutex.
+	// ensure that we have fetched the location before fetching openvpn configuration.
+	if err := s.MaybeLookupLocationContext(ctx); err != nil {
+		return nil, err
+	}
+
+	// IMPORTANT!
+	//
+	// We cannot lock earlier because newOrchestraClient and
+	// MaybeLookupLocation both lock the mutex.
+	//
+	// TODO(bassosimone,DecFox): we should consider using the same strategy we used for the
+	// experiments, where we separated mutable state into dedicated types.
 	defer s.mu.Unlock()
 	s.mu.Lock()
 
 	config, err := clnt.FetchOpenVPNConfig(ctx, provider, cc)
 	if err != nil {
 		return nil, err
 	}
-	s.vpnConfig[provider] = config
 	return &config, nil
 }
 
@@ -673,8 +678,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
 	if selected == nil {
 		return ErrAllProbeServicesFailed
 	}
-	s.logger.Infof("session: using probe services: %+v", selected.Endpoint)
-	s.selectedProbeService = &selected.Endpoint
+	s.logger.Infof("session: using probe services: %+v", selected.Service)
+	s.selectedProbeService = &selected.Service
 	s.availableTestHelpers = selected.TestHelpers
 	return nil
 }

internal/engine/session_integration_test.go

@@ -483,7 +483,7 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) {
 		svc.Type = "antani" // should really not be supported for a long time
 	}
 	client, err := sess.newOrchestraClient(context.Background())
-	if !errors.Is(err, probeservices.ErrUnsupportedEndpoint) {
+	if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
 		t.Fatal("not the error we expected")
 	}
 	if client != nil {

internal/experiment/dnscheck/dnscheck_test.go

@@ -60,7 +60,7 @@ func TestDNSCheckFailsWithInvalidInputType(t *testing.T) {
 	}
 	err := measurer.Run(context.Background(), args)
 	if !errors.Is(err, ErrInvalidInputType) {
-		t.Fatal("expected no input error")
+		t.Fatal("expected invalid-input-type error")
 	}
 }