refactor: use neworchestraclient to mock

internal/engine/session.go

@@ -364,7 +364,7 @@ func (s *Session) DefaultHTTPClient() model.HTTPClient {
 // FetchTorTargets fetches tor targets from the API.
 func (s *Session) FetchTorTargets(
 	ctx context.Context, cc string) (map[string]model.OOAPITorTarget, error) {
-	clnt, err := s.newOrchestraClient(ctx)
+	clnt, err := s.newOrchestraClient(ctx, "")
 	if err != nil {
 		return nil, err
 	}
@@ -379,7 +379,7 @@ 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) {
-	clnt, err := s.newOrchestraClient(ctx)
+	clnt, err := s.newOrchestraClient(ctx, "")
 	if err != nil {
 		return nil, err
 	}
@@ -465,8 +465,12 @@ func (s *Session) NewSubmitter(ctx context.Context) (model.Submitter, error) {
 
 // newOrchestraClient creates a new orchestra client. This client is registered
 // and logged in with the OONI orchestra. An error is returned on failure.
-func (s *Session) newOrchestraClient(ctx context.Context) (*probeservices.Client, error) {
-	clnt, err := s.newProbeServicesClient(ctx)
+func (s *Session) newOrchestraClient(ctx context.Context, baseURL string) (*probeservices.Client, error) {
+	if ctx.Err() != nil {
+		return nil, ctx.Err() // helps with testing
+	}
+	orchestrateService := probeservices.DefaultOrchestrator(baseURL)
+	clnt, err := probeservices.NewClient(s, orchestrateService)
 	if err != nil {
 		return nil, err
 	}
@@ -627,7 +631,7 @@ func (s *Session) getAvailableProbeServicesUnlocked() []model.OOAPIService {
 
 func (s *Session) initOrchestraClient(
 	ctx context.Context, clnt *probeservices.Client,
-	maybeLogin func(ctx context.Context, baseURL string) error,
+	maybeLogin func(ctx context.Context) error,
 ) (*probeservices.Client, error) {
 	// The original implementation has as its only use case that we
 	// were registering and logging in for sending an update regarding
@@ -645,11 +649,10 @@ func (s *Session) initOrchestraClient(
 		SupportedTests:  []string{"web_connectivity"},
 	}
 
-	orchestrator := probeservices.DefaultOrchestrator()
-	if err := clnt.MaybeRegister(ctx, orchestrator.Address, meta); err != nil {
+	if err := clnt.MaybeRegister(ctx, meta); err != nil {
 		return nil, err
 	}
-	if err := maybeLogin(ctx, orchestrator.Address); err != nil {
+	if err := maybeLogin(ctx); err != nil {
 		return nil, err
 	}
 	return clnt, nil

internal/engine/session_integration_test.go

@@ -212,7 +212,7 @@ func TestInitOrchestraClientMaybeLoginError(t *testing.T) {
 	}
 	expected := errors.New("mocked error")
 	outclnt, err := sess.initOrchestraClient(
-		ctx, clnt, func(context.Context, string) error {
+		ctx, clnt, func(context.Context) error {
 			return expected
 		},
 	)
@@ -447,7 +447,7 @@ func TestNewOrchestraClientMaybeLookupBackendsFailure(t *testing.T) {
 	sess.testMaybeLookupBackendsContext = func(ctx context.Context) error {
 		return errMocked
 	}
-	client, err := sess.newOrchestraClient(context.Background())
+	client, err := sess.newOrchestraClient(context.Background(), "https://api.dev.ooni.io")
 	if !errors.Is(err, errMocked) {
 		t.Fatal("not the error we expected", err)
 	}
@@ -465,7 +465,7 @@ func TestNewOrchestraClientMaybeLookupLocationFailure(t *testing.T) {
 	sess.testMaybeLookupLocationContext = func(ctx context.Context) error {
 		return errMocked
 	}
-	client, err := sess.newOrchestraClient(context.Background())
+	client, err := sess.newOrchestraClient(context.Background(), "https://api.dev.ooni.io")
 	if !errors.Is(err, errMocked) {
 		t.Fatalf("not the error we expected: %+v", err)
 	}
@@ -482,7 +482,7 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) {
 	sess.selectedProbeServiceHook = func(svc *model.OOAPIService) {
 		svc.Type = "antani" // should really not be supported for a long time
 	}
-	client, err := sess.newOrchestraClient(context.Background())
+	client, err := sess.newOrchestraClient(context.Background(), "https://api.dev.ooni.io")
 	if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
 		t.Fatal("not the error we expected")
 	}

internal/engine/session_nopsiphon.go

@@ -9,7 +9,7 @@ import (
 
 // FetchPsiphonConfig fetches psiphon config from the API.
 func (s *Session) FetchPsiphonConfig(ctx context.Context) ([]byte, error) {
-	clnt, err := s.newOrchestraClient(ctx)
+	clnt, err := s.newOrchestraClient(ctx, "")
 	if err != nil {
 		return nil, err
 	}

internal/probeservices/benchselect.go

@@ -20,7 +20,14 @@ func Default() []model.OOAPIService {
 }
 
 // DefaultOrchestrator returns the defaul orchestrate probe service
-func DefaultOrchestrator() model.OOAPIService {
+func DefaultOrchestrator(baseURL string) model.OOAPIService {
+	// allows testing of orchestrate services
+	if baseURL != "" {
+		return model.OOAPIService{
+			Address: baseURL,
+			Type:    "orchestrate",
+		}
+	}
 	return model.OOAPIService{
 		Address: "https://api.ooni.org",
 		Type:    "orchestrate",

internal/probeservices/login.go

@@ -9,7 +9,7 @@ import (
 )
 
 // MaybeLogin performs login if necessary
-func (c Client) MaybeLogin(ctx context.Context, baseURL string) error {
+func (c Client) MaybeLogin(ctx context.Context) error {
 	state := c.StateFile.Get()
 	if state.Auth() != nil {
 		return nil // we're already good
@@ -21,10 +21,7 @@ func (c Client) MaybeLogin(ctx context.Context, baseURL string) error {
 	c.LoginCalls.Add(1)
 
 	// construct the URL to use
-	if baseURL == "" {
-		baseURL = c.BaseURL // fallback to the client BaseURL if the passed url is empty
-	}
-	URL, err := urlx.ResolveReference(baseURL, "/api/v1/login", "")
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/login", "")
 	if err != nil {
 		return err
 	}

internal/probeservices/login_test.go

@@ -25,50 +25,18 @@ func TestMaybeLogin(t *testing.T) {
 		clnt := newclient()
 
 		// we need to register first because we don't have state yet
-		if err := clnt.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil {
+		if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil {
 			t.Fatal(err)
 		}
 
 		// now we try to login and get a token
-		if err := clnt.MaybeLogin(context.Background(), ""); err != nil {
+		if err := clnt.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
 		// do this again, and later on we'll verify that we
 		// did actually issue just a single login call
-		if err := clnt.MaybeLogin(context.Background(), ""); err != nil {
-			t.Fatal(err)
-		}
-
-		// make sure we did call login just once: the second call
-		// should not invoke login because we have good state
-		if clnt.LoginCalls.Load() != 1 {
-			t.Fatal("called login API too many times")
-		}
-	})
-
-	// Let's also test that the passes baseURL is given precedence over the client baseURL
-	t.Run("is working with passed baseURL", func(t *testing.T) {
-		if testing.Short() {
-			t.Skip("skip test in short mode")
-		}
-
-		// create client
-		clnt := newEmptyClient()
-
-		// we need to register first because we don't have state yet
-		if err := clnt.MaybeRegister(context.Background(), "https://api.dev.ooni.io", MetadataFixture()); err != nil {
-			t.Fatal(err)
-		}
-
-		// now we try to login and get a token
-		if err := clnt.MaybeLogin(context.Background(), "https://api.dev.ooni.io"); err != nil {
-			t.Fatal(err)
-		}
-
-		// do this again, and later on we'll verify that we
-		// did actually issue just a single login call
-		if err := clnt.MaybeLogin(context.Background(), "https://api.dev.ooni.io"); err != nil {
+		if err := clnt.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
@@ -107,18 +75,18 @@ func TestMaybeLogin(t *testing.T) {
 		}
 
 		// we need to register first because we don't have state yet
-		if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil {
+		if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil {
 			t.Fatal(err)
 		}
 
 		// now we try to login and get a token
-		if err := client.MaybeLogin(context.Background(), ""); err != nil {
+		if err := client.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
 		// do this again, and later on we'll verify that we
 		// did actually issue just a single login call
-		if err := client.MaybeLogin(context.Background(), ""); err != nil {
+		if err := client.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
@@ -161,18 +129,18 @@ func TestMaybeLogin(t *testing.T) {
 		}
 
 		// we need to register first because we don't have state yet
-		if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil {
+		if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil {
 			t.Fatal(err)
 		}
 
 		// now we try to login and get a token
-		if err := client.MaybeLogin(context.Background(), ""); err != nil {
+		if err := client.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
 		// do this again, and later on we'll verify that we
 		// did actually issue just a single login call
-		if err := client.MaybeLogin(context.Background(), ""); err != nil {
+		if err := client.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
@@ -214,7 +182,7 @@ func TestMaybeLogin(t *testing.T) {
 		}))
 
 		// now we try to login and get a token
-		err := client.MaybeLogin(context.Background(), "")
+		err := client.MaybeLogin(context.Background())
 
 		// we do expect an error
 		if !errors.Is(err, netxlite.ECONNRESET) {
@@ -260,7 +228,7 @@ func TestMaybeLogin(t *testing.T) {
 		}))
 
 		// now we try to login and get a token
-		err := client.MaybeLogin(context.Background(), "")
+		err := client.MaybeLogin(context.Background())
 
 		// we do expect an error
 		if err == nil || err.Error() != "unexpected end of JSON input" {
@@ -289,7 +257,7 @@ func TestMaybeLogin(t *testing.T) {
 
 		// now call login and we expect no error because we should
 		// already have what we need to perform a login
-		if err := clnt.MaybeLogin(context.Background(), ""); err != nil {
+		if err := clnt.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 
@@ -311,7 +279,7 @@ func TestMaybeLogin(t *testing.T) {
 		}
 
 		// now try to login and expect to see we've not registered yet
-		if err := clnt.MaybeLogin(context.Background(), ""); !errors.Is(err, ErrNotRegistered) {
+		if err := clnt.MaybeLogin(context.Background()); !errors.Is(err, ErrNotRegistered) {
 			t.Fatal("unexpected error", err)
 		}
 
@@ -338,7 +306,7 @@ func TestMaybeLogin(t *testing.T) {
 		}))
 
 		// now we try to login and get a token
-		err := client.MaybeLogin(context.Background(), "")
+		err := client.MaybeLogin(context.Background())
 
 		// we do expect an error
 		if err == nil || err.Error() != `parse "\t\t\t": net/url: invalid control character in URL` {

internal/probeservices/probeservices.go

@@ -125,6 +125,8 @@ func NewClient(sess Session, service model.OOAPIService) (*Client, error) {
 			return nil, err
 		}
 		return client, nil
+	case "orchestrate":
+		return client, nil
 	default:
 		return nil, ErrUnsupportedServiceType
 	}

internal/probeservices/probeservices_test.go

@@ -596,7 +596,7 @@ func TestGetCredsAndAuthNotLoggedIn(t *testing.T) {
 	}
 
 	clnt := newclient()
-	if err := clnt.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil {
+	if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil {
 		t.Fatal(err)
 	}
 	creds, auth, err := clnt.GetCredsAndAuth()

internal/probeservices/psiphon_test.go

@@ -21,10 +21,10 @@ func TestFetchPsiphonConfig(t *testing.T) {
 	// psiphonflow is the flow with which we invoke the psiphon API
 	psiphonflow := func(t *testing.T, client *Client) ([]byte, error) {
 		// we need to make sure we're registered and logged in
-		if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil {
+		if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil {
 			t.Fatal(err)
 		}
-		if err := client.MaybeLogin(context.Background(), ""); err != nil {
+		if err := client.MaybeLogin(context.Background()); err != nil {
 			t.Fatal(err)
 		}
 

internal/probeservices/register.go

@@ -14,7 +14,7 @@ import (
 )
 
 // MaybeRegister registers this client if not already registered
-func (c Client) MaybeRegister(ctx context.Context, baseURL string, metadata model.OOAPIProbeMetadata) error {
+func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMetadata) error {
 	if !metadata.Valid() {
 		return ErrInvalidMetadata
 	}
@@ -31,11 +31,7 @@ func (c Client) MaybeRegister(ctx context.Context, baseURL string, metadata mode
 		Password:           pwd,
 	}
 
-	// construct the URL to use
-	if baseURL == "" {
-		baseURL = c.BaseURL // fallback to the client BaseURL if the passed url is empty
-	}
-	URL, err := urlx.ResolveReference(baseURL, "/api/v1/register", "")
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/register", "")
 	if err != nil {
 		return err
 	}