From 130cbd601afdb3bce2fd715eac0bc0a9168c1d08 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 3 Apr 2024 08:14:43 +0200 Subject: [PATCH] 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 https://github.com/ooni/probe/issues/2700 --- internal/engine/inputloader.go | 11 ++++---- internal/engine/inputloader_test.go | 22 +++++++++------ internal/engine/session.go | 15 ++++------- internal/engine/session_integration_test.go | 6 ++--- internal/engine/session_internal_test.go | 30 ++++++++++----------- internal/engine/session_nopsiphon.go | 2 +- internal/mocks/session.go | 4 +-- internal/mocks/session_test.go | 2 +- pkg/oonimkall/session.go | 6 +---- 9 files changed, 47 insertions(+), 51 deletions(-) diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index e9faf039b6..4db0834dc0 100644 --- 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 index e6bbf3dcfa..68e1ab5489 100644 --- 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 index d6610cbb60..da46e18e0e 100644 --- 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 index 4770a6a366..77a1e22fc4 100644 --- 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 index 7eea180ef2..b2971af49b 100644 --- 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 index cc03326d57..472aeb4700 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/mocks/session.go b/internal/mocks/session.go index d5270f8fd0..c0750f7169 100644 --- 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 index eed4e6cf6a..b794941736 100644 --- 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 index a4e6199ebc..db3ce3af4c 100644 --- 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 }