From 2154d5f59ef45e29e39328bce4922be739823550 Mon Sep 17 00:00:00 2001 From: decfox Date: Mon, 10 Feb 2025 16:53:20 +0530 Subject: [PATCH] refactor: use neworchestraclient to mock --- internal/engine/session.go | 19 ++++--- internal/engine/session_integration_test.go | 8 +-- internal/engine/session_nopsiphon.go | 2 +- internal/probeservices/benchselect.go | 9 ++- internal/probeservices/login.go | 7 +-- internal/probeservices/login_test.go | 60 +++++--------------- internal/probeservices/probeservices.go | 2 + internal/probeservices/probeservices_test.go | 2 +- internal/probeservices/psiphon_test.go | 4 +- internal/probeservices/register.go | 8 +-- internal/probeservices/register_test.go | 51 ++++------------- internal/probeservices/tor_test.go | 4 +- 12 files changed, 61 insertions(+), 115 deletions(-) diff --git a/internal/engine/session.go b/internal/engine/session.go index 4b13631cc..8f4062e3d 100644 --- a/internal/engine/session.go +++ b/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 diff --git a/internal/engine/session_integration_test.go b/internal/engine/session_integration_test.go index 15cd55f08..629a70a25 100644 --- a/internal/engine/session_integration_test.go +++ b/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") } diff --git a/internal/engine/session_nopsiphon.go b/internal/engine/session_nopsiphon.go index 472aeb470..fe35a4caa 100644 --- a/internal/engine/session_nopsiphon.go +++ b/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 } diff --git a/internal/probeservices/benchselect.go b/internal/probeservices/benchselect.go index 433dfdcd9..cb8ff4fdf 100644 --- a/internal/probeservices/benchselect.go +++ b/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", diff --git a/internal/probeservices/login.go b/internal/probeservices/login.go index 46d1025e6..95a923e58 100644 --- a/internal/probeservices/login.go +++ b/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 } diff --git a/internal/probeservices/login_test.go b/internal/probeservices/login_test.go index be9c98dd9..444fa8d83 100644 --- a/internal/probeservices/login_test.go +++ b/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` { diff --git a/internal/probeservices/probeservices.go b/internal/probeservices/probeservices.go index c7313610c..db0dce268 100644 --- a/internal/probeservices/probeservices.go +++ b/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 } diff --git a/internal/probeservices/probeservices_test.go b/internal/probeservices/probeservices_test.go index 7190d14c4..87271ee45 100644 --- a/internal/probeservices/probeservices_test.go +++ b/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() diff --git a/internal/probeservices/psiphon_test.go b/internal/probeservices/psiphon_test.go index 78ba8bda5..00acbcbd9 100644 --- a/internal/probeservices/psiphon_test.go +++ b/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) } diff --git a/internal/probeservices/register.go b/internal/probeservices/register.go index c7fe283c7..8b4bd335d 100644 --- a/internal/probeservices/register.go +++ b/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 } diff --git a/internal/probeservices/register_test.go b/internal/probeservices/register_test.go index fb4f45f69..c858b2bc4 100644 --- a/internal/probeservices/register_test.go +++ b/internal/probeservices/register_test.go @@ -25,12 +25,12 @@ func TestMaybeRegister(t *testing.T) { clnt := newclient() // attempt to register once - if err := clnt.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } // try again (we want to make sure it's idempotent once we've registered) - if err := clnt.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } @@ -40,33 +40,6 @@ func TestMaybeRegister(t *testing.T) { } }) - t.Run("is working as intended with the passed baseURL", func(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - - // create client - clnt := newEmptyClient() - - // attempt to register once - if err := clnt.MaybeRegister(context.Background(), "https://api.dev.ooni.io", MetadataFixture()); err != nil { - t.Fatal(err) - } - - // try again (we want to make sure it's idempotent once we've registered) - if err := clnt.MaybeRegister(context.Background(), "https://api.dev.ooni.io", MetadataFixture()); err != nil { - t.Fatal(err) - } - - // make sure we indeed only called it once - if clnt.RegisterCalls.Load() != 1 { - t.Fatal("called register API too many times") - } - }) - - // Now let's construct a test server that returns a valid response and try - // to communicate with such a test server successfully and with errors - t.Run("is working as intended with a local test server", func(t *testing.T) { // create state for emulating the OONI backend state := &testingx.OONIBackendWithLoginFlow{} @@ -92,12 +65,12 @@ func TestMaybeRegister(t *testing.T) { } // attempt to register once - if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } // try again (we want to make sure it's idempotent once we've registered) - if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } @@ -139,12 +112,12 @@ func TestMaybeRegister(t *testing.T) { } // attempt to register once - if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } // try again (we want to make sure it's idempotent once we've registered) - if err := client.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := client.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } @@ -176,7 +149,7 @@ func TestMaybeRegister(t *testing.T) { } // attempt to register - err := client.MaybeRegister(context.Background(), "", MetadataFixture()) + err := client.MaybeRegister(context.Background(), MetadataFixture()) // we do expect an error if !errors.Is(err, netxlite.ECONNRESET) { @@ -213,7 +186,7 @@ func TestMaybeRegister(t *testing.T) { } // attempt to register - err := client.MaybeRegister(context.Background(), "", MetadataFixture()) + err := client.MaybeRegister(context.Background(), MetadataFixture()) // we do expect an error if err == nil || err.Error() != "unexpected end of JSON input" { @@ -229,7 +202,7 @@ func TestMaybeRegister(t *testing.T) { t.Run("when metadata is not valid", func(t *testing.T) { // we expect ErrInvalidMetadata when metadata is empty clnt := newclient() - err := clnt.MaybeRegister(context.Background(), "", model.OOAPIProbeMetadata{}) + err := clnt.MaybeRegister(context.Background(), model.OOAPIProbeMetadata{}) if !errors.Is(err, ErrInvalidMetadata) { t.Fatal("expected an error here") } @@ -250,7 +223,7 @@ func TestMaybeRegister(t *testing.T) { } // attempt to register, which should immediately succeed - if err := clnt.MaybeRegister(context.Background(), "", MetadataFixture()); err != nil { + if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) } @@ -265,7 +238,7 @@ func TestMaybeRegister(t *testing.T) { clnt.BaseURL = "\t\t\t" // makes it fail ctx := context.Background() metadata := MetadataFixture() - err := clnt.MaybeRegister(ctx, "", metadata) + err := clnt.MaybeRegister(ctx, metadata) if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { t.Fatal("expected an error here") } @@ -279,7 +252,7 @@ func TestMaybeRegister(t *testing.T) { client.BaseURL = "\t\t\t" // attempt to register - err := client.MaybeRegister(context.Background(), "", MetadataFixture()) + err := client.MaybeRegister(context.Background(), MetadataFixture()) // we do expect an error if err == nil || err.Error() != `parse "\t\t\t": net/url: invalid control character in URL` { diff --git a/internal/probeservices/tor_test.go b/internal/probeservices/tor_test.go index 33eeffe9a..79dd2adc2 100644 --- a/internal/probeservices/tor_test.go +++ b/internal/probeservices/tor_test.go @@ -24,10 +24,10 @@ func TestFetchTorTargets(t *testing.T) { // torflow is the flow with which we invoke the tor API torflow := func(t *testing.T, client *Client) (map[string]model.OOAPITorTarget, 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) }