cleanup(oonimkall): use (*Session).CheckIn directly (#1538)

We don't need an indirect call through (*Session).NewProbeServicesClient as long as we modify the value returned by CheckIn to be the full check-in response rather than being just the response's .Test field. Part of ooni/probe#2700
ooni · Apr 3, 2024 · 130cbd6 · 130cbd6
1 parent f9cb93e
commit 130cbd6
Show file tree

Hide file tree

Showing 9 changed files with 47 additions and 51 deletions.
diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go
@@ -27,8 +27,7 @@ var (
 // InputLoaderSession is the session according to an InputLoader. We
 // introduce this abstraction because it helps us with testing.
 type InputLoaderSession interface {
-	CheckIn(ctx context.Context,
-		config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error)
+	CheckIn(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error)
 }
 
 // InputLoaderLogger is the logger according to an InputLoader.
@@ -328,12 +327,12 @@ func (il *InputLoader) checkIn(
 		return nil, err
 	}
 	// Note: safe to assume that reply is not nil if err is nil
-	if reply.WebConnectivity != nil && len(reply.WebConnectivity.URLs) > 0 {
-		reply.WebConnectivity.URLs = il.preventMistakes(
-			reply.WebConnectivity.URLs, config.WebConnectivity.CategoryCodes,
+	if reply.Tests.WebConnectivity != nil && len(reply.Tests.WebConnectivity.URLs) > 0 {
+		reply.Tests.WebConnectivity.URLs = il.preventMistakes(
+			reply.Tests.WebConnectivity.URLs, config.WebConnectivity.CategoryCodes,
 		)
 	}
-	return reply, nil
+	return &reply.Tests, nil
 }
 
 // preventMistakes makes the code more robust with respect to any possible

diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go
@@ -446,7 +446,7 @@ func TestInputLoaderReadfileScannerFailure(t *testing.T) {
 type InputLoaderMockableSession struct {
 	// Output contains the output of CheckIn. It should
 	// be nil when Error is not-nil.
-	Output *model.OOAPICheckInResultNettests
+	Output *model.OOAPICheckInResult
 
 	// Error is the error to be returned by CheckIn. It
 	// should be nil when Output is not-nil.
@@ -455,7 +455,7 @@ type InputLoaderMockableSession struct {
 
 // CheckIn implements InputLoaderSession.CheckIn.
 func (sess *InputLoaderMockableSession) CheckIn(
-	ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
+	ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) {
 	if sess.Output == nil && sess.Error == nil {
 		return nil, errors.New("both Output and Error are nil")
 	}
@@ -480,7 +480,9 @@ func TestInputLoaderCheckInFailure(t *testing.T) {
 func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) {
 	il := &InputLoader{
 		Session: &InputLoaderMockableSession{
-			Output: &model.OOAPICheckInResultNettests{},
+			Output: &model.OOAPICheckInResult{
+				Tests: model.OOAPICheckInResultNettests{},
+			},
 		},
 	}
 	out, err := il.loadRemote(context.Background())
@@ -495,8 +497,10 @@ func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) {
 func TestInputLoaderCheckInSuccessWithNoURLs(t *testing.T) {
 	il := &InputLoader{
 		Session: &InputLoaderMockableSession{
-			Output: &model.OOAPICheckInResultNettests{
-				WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{},
+			Output: &model.OOAPICheckInResult{
+				Tests: model.OOAPICheckInResultNettests{
+					WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{},
+				},
 			},
 		},
 	}
@@ -521,9 +525,11 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) {
 	}}
 	il := &InputLoader{
 		Session: &InputLoaderMockableSession{
-			Output: &model.OOAPICheckInResultNettests{
-				WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{
-					URLs: expect,
+			Output: &model.OOAPICheckInResult{
+				Tests: model.OOAPICheckInResultNettests{
+					WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{
+						URLs: expect,
+					},
 				},
 			},
 		},

diff --git a/internal/engine/session.go b/internal/engine/session.go
@@ -264,9 +264,7 @@ func (s *Session) KibiBytesSent() float64 {
 //
 // The return value is either the check-in response or an error.
 func (s *Session) CheckIn(
-	ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
-	// TODO(bassosimone): consider refactoring this function to return
-	// the whole check-in response to the caller.
+	ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) {
 	if err := s.maybeLookupLocationContext(ctx); err != nil {
 		return nil, err
 	}
@@ -299,7 +297,7 @@ func (s *Session) CheckIn(
 	if err != nil {
 		return nil, err
 	}
-	return &resp.Tests, nil
+	return resp, nil
 }
 
 // maybeLookupLocationContext is a wrapper for MaybeLookupLocationContext that calls
@@ -366,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
 	}
@@ -430,12 +428,9 @@ func (s *Session) NewSubmitter(ctx context.Context) (Submitter, error) {
 	return probeservices.NewSubmitter(psc, s.Logger()), nil
 }
 
-// NewOrchestraClient creates a new orchestra client. This client is registered
+// newOrchestraClient creates a new orchestra client. This client is registered
 // and logged in with the OONI orchestra. An error is returned on failure.
-//
-// This function is DEPRECATED. New code SHOULD NOT use it. It will eventually
-// be made private or entirely removed from the codebase.
-func (s *Session) NewOrchestraClient(ctx context.Context) (*probeservices.Client, error) {
+func (s *Session) newOrchestraClient(ctx context.Context) (*probeservices.Client, error) {
 	clnt, err := s.NewProbeServicesClient(ctx)
 	if err != nil {
 		return nil, err

diff --git a/internal/engine/session_integration_test.go b/internal/engine/session_integration_test.go
@@ -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())
 	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())
 	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())
 	if !errors.Is(err, probeservices.ErrUnsupportedEndpoint) {
 		t.Fatal("not the error we expected")
 	}

diff --git a/internal/engine/session_internal_test.go b/internal/engine/session_internal_test.go
@@ -65,24 +65,24 @@ func (c *mockableProbeServicesClientForCheckIn) CheckIn(
 }
 
 func TestSessionCheckInSuccessful(t *testing.T) {
-	results := &model.OOAPICheckInResultNettests{
-		WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{
-			ReportID: "xxx-x-xx",
-			URLs: []model.OOAPIURLInfo{{
-				CategoryCode: "NEWS",
-				CountryCode:  "IT",
-				URL:          "https://www.repubblica.it/",
-			}, {
-				CategoryCode: "NEWS",
-				CountryCode:  "IT",
-				URL:          "https://www.unita.it/",
-			}},
+	results := &model.OOAPICheckInResult{
+		Tests: model.OOAPICheckInResultNettests{
+			WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{
+				ReportID: "xxx-x-xx",
+				URLs: []model.OOAPIURLInfo{{
+					CategoryCode: "NEWS",
+					CountryCode:  "IT",
+					URL:          "https://www.repubblica.it/",
+				}, {
+					CategoryCode: "NEWS",
+					CountryCode:  "IT",
+					URL:          "https://www.unita.it/",
+				}},
+			},
 		},
 	}
 	mockedClnt := &mockableProbeServicesClientForCheckIn{
-		Results: &model.OOAPICheckInResult{
-			Tests: *results,
-		},
+		Results: results,
 	}
 	s := &Session{
 		location: &enginelocate.Results{

diff --git 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/mocks/session.go b/internal/mocks/session.go
@@ -55,7 +55,7 @@ type Session struct {
 	MockNewSubmitter func(ctx context.Context) (model.Submitter, error)
 
 	MockCheckIn func(ctx context.Context,
-		config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error)
+		config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error)
 }
 
 func (sess *Session) GetTestHelpersByName(name string) ([]model.OOAPIService, bool) {
@@ -148,6 +148,6 @@ func (sess *Session) NewSubmitter(ctx context.Context) (model.Submitter, error)
 }
 
 func (sess *Session) CheckIn(ctx context.Context,
-	config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
+	config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) {
 	return sess.MockCheckIn(ctx, config)
 }
diff --git a/internal/mocks/session_test.go b/internal/mocks/session_test.go
@@ -326,7 +326,7 @@ func TestSession(t *testing.T) {
 	t.Run("CheckIn", func(t *testing.T) {
 		expected := errors.New("mocked err")
 		s := &Session{
-			MockCheckIn: func(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
+			MockCheckIn: func(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) {
 				return nil, expected
 			},
 		}

diff --git a/pkg/oonimkall/session.go b/pkg/oonimkall/session.go
@@ -451,10 +451,6 @@ func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo,
 	if sess.TestingCheckInBeforeNewProbeServicesClient != nil {
 		sess.TestingCheckInBeforeNewProbeServicesClient(ctx) // for testing
 	}
-	psc, err := sess.sessp.NewProbeServicesClient(ctx.ctx)
-	if err != nil {
-		return nil, err
-	}
 	if sess.TestingCheckInBeforeCheckIn != nil {
 		sess.TestingCheckInBeforeCheckIn(ctx) // for testing
 	}
@@ -469,7 +465,7 @@ func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo,
 		SoftwareVersion: config.SoftwareVersion,
 		WebConnectivity: config.WebConnectivity.toModel(),
 	}
-	result, err := psc.CheckIn(ctx.ctx, cfg)
+	result, err := sess.sessp.CheckIn(ctx.ctx, &cfg)
 	if err != nil {
 		return nil, err
 	}