From 79e842467713ba8a40a4d0e3c4d155ddc43b72d6 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 2 Apr 2021 12:03:18 +0200 Subject: [PATCH] refactor: remove model.ExperimentOrchestraClient (#284) * ongoing * while there, make sure we test everything * reorganize previous commit * ensure we have reasonable coverage in session The code in here would be better with unit tests. We have too many integration tests and the tests overall are too slow. But it's also true that I should not write a giant diff as part of this PR. --- internal/engine/experiment/tor/tor.go | 24 ++-- internal/engine/experiment/tor/tor_test.go | 83 ++++++------ internal/engine/internal/mockable/mockable.go | 121 ++++++------------ internal/engine/internal/psiphonx/psiphonx.go | 9 +- .../engine/internal/psiphonx/psiphonx_test.go | 34 +---- .../sessionresolver/childresolver_test.go | 2 +- .../sessionresolver/integration_test.go | 2 +- internal/engine/model/experiment.go | 21 +-- .../engine/netx/archival/archival_test.go | 12 +- internal/engine/session.go | 34 ++++- internal/engine/session_integration_test.go | 29 ++--- internal/engine/session_internal_test.go | 39 ++++++ 12 files changed, 200 insertions(+), 210 deletions(-) diff --git a/internal/engine/experiment/tor/tor.go b/internal/engine/experiment/tor/tor.go index ee409cc..09bc51f 100644 --- a/internal/engine/experiment/tor/tor.go +++ b/internal/engine/experiment/tor/tor.go @@ -55,6 +55,11 @@ type TargetResults struct { TargetSource string `json:"target_source,omitempty"` TCPConnect oonidatamodel.TCPConnectList `json:"tcp_connect"` TLSHandshakes oonidatamodel.TLSHandshakesList `json:"tls_handshakes"` + + // Only for testing. We don't care about this field otherwise. We + // cannot make this private because otherwise the IP address sanitizer + // is going to panic over a private field. + DirPortCount int `json:"-"` } func registerExtensions(m *model.Measurement) { @@ -78,6 +83,7 @@ func (tr *TargetResults) fillSummary() { case "dir_port": // The UI currently doesn't care about this protocol // as long as drawing a table is concerned. + tr.DirPortCount++ case "obfs4": // We currently only perform an OBFS4 handshake, hence // the final Failure is the handshake result @@ -136,20 +142,16 @@ func (tk *TestKeys) fillToplevelKeys() { // Measurer performs the measurement. type Measurer struct { - config Config - fetchTorTargets func(ctx context.Context, clnt model.ExperimentOrchestraClient, cc string) (map[string]model.TorTarget, error) - newOrchestraClient func(ctx context.Context, sess model.ExperimentSession) (model.ExperimentOrchestraClient, error) + config Config + fetchTorTargets func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.TorTarget, error) } // NewMeasurer creates a new Measurer func NewMeasurer(config Config) *Measurer { return &Measurer{ config: config, - fetchTorTargets: func(ctx context.Context, clnt model.ExperimentOrchestraClient, cc string) (map[string]model.TorTarget, error) { - return clnt.FetchTorTargets(ctx, cc) - }, - newOrchestraClient: func(ctx context.Context, sess model.ExperimentSession) (model.ExperimentOrchestraClient, error) { - return sess.NewOrchestraClient(ctx) + fetchTorTargets: func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.TorTarget, error) { + return sess.FetchTorTargets(ctx, cc) }, } } @@ -189,11 +191,7 @@ func (m *Measurer) gimmeTargets( ) (map[string]model.TorTarget, error) { ctx, cancel := context.WithTimeout(ctx, 15*time.Second) defer cancel() - clnt, err := m.newOrchestraClient(ctx, sess) - if err != nil { - return nil, err - } - return m.fetchTorTargets(ctx, clnt, sess.ProbeCC()) + return m.fetchTorTargets(ctx, sess, sess.ProbeCC()) } // keytarget contains a key and the related target diff --git a/internal/engine/experiment/tor/tor_test.go b/internal/engine/experiment/tor/tor_test.go index 9972b59..01eb2da 100644 --- a/internal/engine/experiment/tor/tor_test.go +++ b/internal/engine/experiment/tor/tor_test.go @@ -18,7 +18,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/legacy/oonitemplates" "github.com/ooni/probe-cli/v3/internal/engine/model" "github.com/ooni/probe-cli/v3/internal/engine/netx/errorx" - "github.com/ooni/probe-cli/v3/internal/engine/probeservices" ) func TestNewExperimentMeasurer(t *testing.T) { @@ -31,32 +30,10 @@ func TestNewExperimentMeasurer(t *testing.T) { } } -func TestMeasurerMeasureNewOrchestraClientError(t *testing.T) { - measurer := NewMeasurer(Config{}) - expected := errors.New("mocked error") - measurer.newOrchestraClient = func(ctx context.Context, sess model.ExperimentSession) (model.ExperimentOrchestraClient, error) { - return nil, expected - } - err := measurer.Run( - context.Background(), - &mockable.Session{ - MockableLogger: log.Log, - }, - new(model.Measurement), - model.NewPrinterCallbacks(log.Log), - ) - if !errors.Is(err, expected) { - t.Fatal("not the error we expected") - } -} - func TestMeasurerMeasureFetchTorTargetsError(t *testing.T) { measurer := NewMeasurer(Config{}) expected := errors.New("mocked error") - measurer.newOrchestraClient = func(ctx context.Context, sess model.ExperimentSession) (model.ExperimentOrchestraClient, error) { - return new(probeservices.Client), nil - } - measurer.fetchTorTargets = func(ctx context.Context, clnt model.ExperimentOrchestraClient, cc string) (map[string]model.TorTarget, error) { + measurer.fetchTorTargets = func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.TorTarget, error) { return nil, expected } err := measurer.Run( @@ -74,10 +51,7 @@ func TestMeasurerMeasureFetchTorTargetsError(t *testing.T) { func TestMeasurerMeasureFetchTorTargetsEmptyList(t *testing.T) { measurer := NewMeasurer(Config{}) - measurer.newOrchestraClient = func(ctx context.Context, sess model.ExperimentSession) (model.ExperimentOrchestraClient, error) { - return new(probeservices.Client), nil - } - measurer.fetchTorTargets = func(ctx context.Context, clnt model.ExperimentOrchestraClient, cc string) (map[string]model.TorTarget, error) { + measurer.fetchTorTargets = func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.TorTarget, error) { return nil, nil } measurement := new(model.Measurement) @@ -102,10 +76,7 @@ func TestMeasurerMeasureGoodWithMockedOrchestra(t *testing.T) { // This test mocks orchestra to return a nil list of targets, so the code runs // but we don't perform any actual network actions. measurer := NewMeasurer(Config{}) - measurer.newOrchestraClient = func(ctx context.Context, sess model.ExperimentSession) (model.ExperimentOrchestraClient, error) { - return new(probeservices.Client), nil - } - measurer.fetchTorTargets = func(ctx context.Context, clnt model.ExperimentOrchestraClient, cc string) (map[string]model.TorTarget, error) { + measurer.fetchTorTargets = func(ctx context.Context, sess model.ExperimentSession, cc string) (map[string]model.TorTarget, error) { return nil, nil } err := measurer.Run( @@ -167,10 +138,8 @@ func TestMeasurerMeasureSanitiseOutput(t *testing.T) { measurer := NewMeasurer(Config{}) sess := newsession() key := "xyz-xyz-xyz-theCh2ju-ahG4chei-Ai2eka0a" - sess.MockableOrchestraClient = &mockable.ExperimentOrchestraClient{ - MockableFetchTorTargetsResult: map[string]model.TorTarget{ - key: staticPrivateTestingTarget, - }, + sess.MockableFetchTorTargetsResult = map[string]model.TorTarget{ + key: staticPrivateTestingTarget, } measurement := new(model.Measurement) err := measurer.Run( @@ -929,3 +898,45 @@ func TestSummaryKeysWorksAsIntended(t *testing.T) { }) } } + +func TestTargetResultsFillSummaryDirPort(t *testing.T) { + tr := &TargetResults{ + TargetProtocol: "dir_port", + TCPConnect: oonidatamodel.TCPConnectList{{ + IP: "1.2.3.4", + Port: 443, + Status: oonidatamodel.TCPConnectStatus{ + Failure: nil, + }, + }}, + } + tr.fillSummary() + if tr.DirPortCount != 1 { + t.Fatal("unexpected dirPortCount") + } +} + +func TestTestKeysFillToplevelKeysCoverMissingFields(t *testing.T) { + failureString := "eof_error" + tk := &TestKeys{ + Targets: map[string]TargetResults{ + "foobar": {Failure: &failureString, TargetProtocol: "dir_port"}, + "baz": {TargetProtocol: "dir_port"}, + "jafar": {Failure: &failureString, TargetProtocol: "or_port_dirauth"}, + "jasmine": {TargetProtocol: "or_port_dirauth"}, + }, + } + tk.fillToplevelKeys() + if tk.DirPortTotal != 2 { + t.Fatal("unexpected DirPortTotal") + } + if tk.DirPortAccessible != 1 { + t.Fatal("unexpected DirPortAccessible") + } + if tk.ORPortDirauthTotal != 2 { + t.Fatal("unexpected ORPortDirauthTotal") + } + if tk.ORPortDirauthAccessible != 1 { + t.Fatal("unexpected ORPortDirauthAccessible") + } +} diff --git a/internal/engine/internal/mockable/mockable.go b/internal/engine/internal/mockable/mockable.go index 7c606b2..6e83217 100644 --- a/internal/engine/internal/mockable/mockable.go +++ b/internal/engine/internal/mockable/mockable.go @@ -12,30 +12,32 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/kvstore" "github.com/ooni/probe-cli/v3/internal/engine/model" "github.com/ooni/probe-cli/v3/internal/engine/probeservices" - "github.com/ooni/probe-cli/v3/internal/engine/probeservices/testorchestra" - "github.com/ooni/probe-cli/v3/internal/engine/runtimex" ) // Session allows to mock sessions. type Session struct { - MockableTestHelpers map[string][]model.Service - MockableHTTPClient *http.Client - MockableLogger model.Logger - MockableMaybeResolverIP string - MockableOrchestraClient model.ExperimentOrchestraClient - MockableOrchestraClientError error - MockableProbeASNString string - MockableProbeCC string - MockableProbeIP string - MockableProbeNetworkName string - MockableProxyURL *url.URL - MockableResolverIP string - MockableSoftwareName string - MockableSoftwareVersion string - MockableTempDir string - MockableTorArgs []string - MockableTorBinary string - MockableUserAgent string + MockableTestHelpers map[string][]model.Service + MockableHTTPClient *http.Client + MockableLogger model.Logger + MockableMaybeResolverIP string + MockableProbeASNString string + MockableProbeCC string + MockableProbeIP string + MockableProbeNetworkName string + MockableProxyURL *url.URL + MockableFetchPsiphonConfigResult []byte + MockableFetchPsiphonConfigErr error + MockableFetchTorTargetsResult map[string]model.TorTarget + MockableFetchTorTargetsErr error + MockableFetchURLListResult []model.URLInfo + MockableFetchURLListErr error + MockableResolverIP string + MockableSoftwareName string + MockableSoftwareVersion string + MockableTempDir string + MockableTorArgs []string + MockableTorBinary string + MockableUserAgent string } // GetTestHelpersByName implements ExperimentSession.GetTestHelpersByName @@ -49,6 +51,23 @@ func (sess *Session) DefaultHTTPClient() *http.Client { return sess.MockableHTTPClient } +// FetchPsiphonConfig implements ExperimentSession.FetchPsiphonConfig +func (sess *Session) FetchPsiphonConfig(ctx context.Context) ([]byte, error) { + return sess.MockableFetchPsiphonConfigResult, sess.MockableFetchPsiphonConfigErr +} + +// FetchTorTargets implements ExperimentSession.TorTargets +func (sess *Session) FetchTorTargets( + ctx context.Context, cc string) (map[string]model.TorTarget, error) { + return sess.MockableFetchTorTargetsResult, sess.MockableFetchTorTargetsErr +} + +// FetchURLList implements ExperimentSession.FetchURLList. +func (sess *Session) FetchURLList( + ctx context.Context, config model.URLListConfig) ([]model.URLInfo, error) { + return sess.MockableFetchURLListResult, sess.MockableFetchURLListErr +} + // KeyValueStore returns the configured key-value store. func (sess *Session) KeyValueStore() model.KeyValueStore { return kvstore.NewMemoryKeyValueStore() @@ -64,29 +83,6 @@ func (sess *Session) MaybeResolverIP() string { return sess.MockableMaybeResolverIP } -// NewOrchestraClient implements ExperimentSession.NewOrchestraClient -func (sess *Session) NewOrchestraClient(ctx context.Context) (model.ExperimentOrchestraClient, error) { - if sess.MockableOrchestraClient != nil { - return sess.MockableOrchestraClient, nil - } - if sess.MockableOrchestraClientError != nil { - return nil, sess.MockableOrchestraClientError - } - clnt, err := probeservices.NewClient(sess, model.Service{ - Address: "https://ams-pg-test.ooni.org/", - Type: "https", - }) - runtimex.PanicOnError(err, "orchestra.NewClient should not fail here") - meta := testorchestra.MetadataFixture() - if err := clnt.MaybeRegister(ctx, meta); err != nil { - return nil, err - } - if err := clnt.MaybeLogin(ctx); err != nil { - return nil, err - } - return clnt, nil -} - // ProbeASNString implements ExperimentSession.ProbeASNString func (sess *Session) ProbeASNString() string { return sess.MockableProbeASNString @@ -152,42 +148,3 @@ var _ probeservices.Session = &Session{} var _ psiphonx.Session = &Session{} var _ tunnel.Session = &Session{} var _ torx.Session = &Session{} - -// ExperimentOrchestraClient is the experiment's view of -// a client for querying the OONI orchestra. -type ExperimentOrchestraClient struct { - MockableCheckInInfo *model.CheckInInfo - MockableCheckInErr error - MockableFetchPsiphonConfigResult []byte - MockableFetchPsiphonConfigErr error - MockableFetchTorTargetsResult map[string]model.TorTarget - MockableFetchTorTargetsErr error - MockableFetchURLListResult []model.URLInfo - MockableFetchURLListErr error -} - -// CheckIn implements ExperimentOrchestraClient.CheckIn. -func (c ExperimentOrchestraClient) CheckIn( - ctx context.Context, config model.CheckInConfig) (*model.CheckInInfo, error) { - return c.MockableCheckInInfo, c.MockableCheckInErr -} - -// FetchPsiphonConfig implements ExperimentOrchestraClient.FetchPsiphonConfig -func (c ExperimentOrchestraClient) FetchPsiphonConfig( - ctx context.Context) ([]byte, error) { - return c.MockableFetchPsiphonConfigResult, c.MockableFetchPsiphonConfigErr -} - -// FetchTorTargets implements ExperimentOrchestraClient.TorTargets -func (c ExperimentOrchestraClient) FetchTorTargets( - ctx context.Context, cc string) (map[string]model.TorTarget, error) { - return c.MockableFetchTorTargetsResult, c.MockableFetchTorTargetsErr -} - -// FetchURLList implements ExperimentOrchestraClient.FetchURLList. -func (c ExperimentOrchestraClient) FetchURLList( - ctx context.Context, config model.URLListConfig) ([]model.URLInfo, error) { - return c.MockableFetchURLListResult, c.MockableFetchURLListErr -} - -var _ model.ExperimentOrchestraClient = ExperimentOrchestraClient{} diff --git a/internal/engine/internal/psiphonx/psiphonx.go b/internal/engine/internal/psiphonx/psiphonx.go index c7ba9c1..11cbae0 100644 --- a/internal/engine/internal/psiphonx/psiphonx.go +++ b/internal/engine/internal/psiphonx/psiphonx.go @@ -10,13 +10,12 @@ import ( "path/filepath" "time" - "github.com/ooni/probe-cli/v3/internal/engine/model" "github.com/ooni/psiphon/oopsi/github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib" ) // Session is the way in which this package sees a Session. type Session interface { - NewOrchestraClient(ctx context.Context) (model.ExperimentOrchestraClient, error) + FetchPsiphonConfig(ctx context.Context) ([]byte, error) TempDir() string } @@ -87,11 +86,7 @@ func Start( if config.WorkDir == "" { config.WorkDir = sess.TempDir() } - clnt, err := sess.NewOrchestraClient(ctx) - if err != nil { - return nil, err - } - configJSON, err := clnt.FetchPsiphonConfig(ctx) + configJSON, err := sess.FetchPsiphonConfig(ctx) if err != nil { return nil, err } diff --git a/internal/engine/internal/psiphonx/psiphonx_test.go b/internal/engine/internal/psiphonx/psiphonx_test.go index 427820b..9f69b17 100644 --- a/internal/engine/internal/psiphonx/psiphonx_test.go +++ b/internal/engine/internal/psiphonx/psiphonx_test.go @@ -58,27 +58,10 @@ func TestStartStop(t *testing.T) { tunnel.Stop() } -func TestNewOrchestraClientFailure(t *testing.T) { - expected := errors.New("mocked error") - sess := &mockable.Session{ - MockableOrchestraClientError: expected, - } - tunnel, err := psiphonx.Start(context.Background(), sess, psiphonx.Config{}) - if !errors.Is(err, expected) { - t.Fatal("not the error we expected") - } - if tunnel != nil { - t.Fatal("expected nil tunnel here") - } -} - func TestFetchPsiphonConfigFailure(t *testing.T) { expected := errors.New("mocked error") - clnt := mockable.ExperimentOrchestraClient{ - MockableFetchPsiphonConfigErr: expected, - } sess := &mockable.Session{ - MockableOrchestraClient: clnt, + MockableFetchPsiphonConfigErr: expected, } tunnel, err := psiphonx.Start(context.Background(), sess, psiphonx.Config{}) if !errors.Is(err, expected) { @@ -94,11 +77,8 @@ func TestMakeMkdirAllFailure(t *testing.T) { dependencies := FakeDependencies{ MkdirAllErr: expected, } - clnt := mockable.ExperimentOrchestraClient{ - MockableFetchPsiphonConfigResult: []byte(`{}`), - } sess := &mockable.Session{ - MockableOrchestraClient: clnt, + MockableFetchPsiphonConfigResult: []byte(`{}`), } tunnel, err := psiphonx.Start(context.Background(), sess, psiphonx.Config{ Dependencies: dependencies, @@ -116,11 +96,8 @@ func TestMakeRemoveAllFailure(t *testing.T) { dependencies := FakeDependencies{ RemoveAllErr: expected, } - clnt := mockable.ExperimentOrchestraClient{ - MockableFetchPsiphonConfigResult: []byte(`{}`), - } sess := &mockable.Session{ - MockableOrchestraClient: clnt, + MockableFetchPsiphonConfigResult: []byte(`{}`), } tunnel, err := psiphonx.Start(context.Background(), sess, psiphonx.Config{ Dependencies: dependencies, @@ -138,11 +115,8 @@ func TestMakeStartFailure(t *testing.T) { dependencies := FakeDependencies{ StartErr: expected, } - clnt := mockable.ExperimentOrchestraClient{ - MockableFetchPsiphonConfigResult: []byte(`{}`), - } sess := &mockable.Session{ - MockableOrchestraClient: clnt, + MockableFetchPsiphonConfigResult: []byte(`{}`), } tunnel, err := psiphonx.Start(context.Background(), sess, psiphonx.Config{ Dependencies: dependencies, diff --git a/internal/engine/internal/sessionresolver/childresolver_test.go b/internal/engine/internal/sessionresolver/childresolver_test.go index 8be08ff..888e636 100644 --- a/internal/engine/internal/sessionresolver/childresolver_test.go +++ b/internal/engine/internal/sessionresolver/childresolver_test.go @@ -62,7 +62,7 @@ func TestTimeLimitedLookupFailure(t *testing.T) { func TestTimeLimitedLookupWillTimeout(t *testing.T) { if testing.Short() { - t.Skip("skipping test in short mode") + t.Skip("skip test in short mode") } reso := &Resolver{} re := &FakeResolver{ diff --git a/internal/engine/internal/sessionresolver/integration_test.go b/internal/engine/internal/sessionresolver/integration_test.go index f359aca..52d9fe0 100644 --- a/internal/engine/internal/sessionresolver/integration_test.go +++ b/internal/engine/internal/sessionresolver/integration_test.go @@ -9,7 +9,7 @@ import ( func TestSessionResolverGood(t *testing.T) { if testing.Short() { - t.Skip("skipping test in short mode") + t.Skip("skip test in short mode") } reso := &sessionresolver.Resolver{} defer reso.CloseIdleConnections() diff --git a/internal/engine/model/experiment.go b/internal/engine/model/experiment.go index 25e39e1..a6ed445 100644 --- a/internal/engine/model/experiment.go +++ b/internal/engine/model/experiment.go @@ -5,29 +5,14 @@ import ( "net/http" ) -// ExperimentOrchestraClient is the experiment's view of -// a client for querying the OONI orchestra API. -type ExperimentOrchestraClient interface { - // CheckIn calls the check-in API. - CheckIn(ctx context.Context, config CheckInConfig) (*CheckInInfo, error) - - // FetchPsiphonConfig returns psiphon config from the API. - FetchPsiphonConfig(ctx context.Context) ([]byte, error) - - // FetchTorTargets returns tor targets from the API. - FetchTorTargets(ctx context.Context, cc string) (map[string]TorTarget, error) - - // FetchURLList returns URLs from the API. - // This method is deprecated and will be removed soon. - FetchURLList(ctx context.Context, config URLListConfig) ([]URLInfo, error) -} - // ExperimentSession is the experiment's view of a session. type ExperimentSession interface { GetTestHelpersByName(name string) ([]Service, bool) DefaultHTTPClient() *http.Client + FetchPsiphonConfig(ctx context.Context) ([]byte, error) + FetchTorTargets(ctx context.Context, cc string) (map[string]TorTarget, error) + FetchURLList(ctx context.Context, config URLListConfig) ([]URLInfo, error) Logger() Logger - NewOrchestraClient(ctx context.Context) (ExperimentOrchestraClient, error) ProbeCC() string ResolverIP() string TempDir() string diff --git a/internal/engine/netx/archival/archival_test.go b/internal/engine/netx/archival/archival_test.go index 5172ed6..7fdc471 100644 --- a/internal/engine/netx/archival/archival_test.go +++ b/internal/engine/netx/archival/archival_test.go @@ -129,7 +129,7 @@ func TestNewRequestList(t *testing.T) { }, { Name: "http_response_metadata", HTTPHeaders: http.Header{ - "Server": []string{"orchestra/0.1.0-dev"}, + "Server": []string{"miniooni/0.1.0-dev"}, }, HTTPStatusCode: 200, }, { @@ -194,11 +194,11 @@ func TestNewRequestList(t *testing.T) { HeadersList: []archival.HTTPHeader{{ Key: "Server", Value: archival.MaybeBinaryValue{ - Value: "orchestra/0.1.0-dev", + Value: "miniooni/0.1.0-dev", }, }}, Headers: map[string]archival.MaybeBinaryValue{ - "Server": {Value: "orchestra/0.1.0-dev"}, + "Server": {Value: "miniooni/0.1.0-dev"}, }, Locations: nil, }, @@ -223,7 +223,7 @@ func TestNewRequestList(t *testing.T) { }, { Name: "http_response_metadata", HTTPHeaders: http.Header{ - "Server": []string{"orchestra/0.1.0-dev"}, + "Server": []string{"miniooni/0.1.0-dev"}, "Location": []string{"https://x.example.com", "https://y.example.com"}, }, HTTPStatusCode: 302, @@ -260,11 +260,11 @@ func TestNewRequestList(t *testing.T) { }, { Key: "Server", Value: archival.MaybeBinaryValue{ - Value: "orchestra/0.1.0-dev", + Value: "miniooni/0.1.0-dev", }, }}, Headers: map[string]archival.MaybeBinaryValue{ - "Server": {Value: "orchestra/0.1.0-dev"}, + "Server": {Value: "miniooni/0.1.0-dev"}, "Location": {Value: "https://x.example.com"}, }, Locations: []string{ diff --git a/internal/engine/session.go b/internal/engine/session.go index af165e1..30371aa 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -275,6 +275,35 @@ func (s *Session) DefaultHTTPClient() *http.Client { return &http.Client{Transport: s.httpDefaultTransport} } +// FetchPsiphonConfig fetches psiphon config from the API. +func (s *Session) FetchPsiphonConfig(ctx context.Context) ([]byte, error) { + clnt, err := s.NewOrchestraClient(ctx) + if err != nil { + return nil, err + } + return clnt.FetchPsiphonConfig(ctx) +} + +// FetchTorTargets fetches tor targets from the API. +func (s *Session) FetchTorTargets( + ctx context.Context, cc string) (map[string]model.TorTarget, error) { + clnt, err := s.NewOrchestraClient(ctx) + if err != nil { + return nil, err + } + return clnt.FetchTorTargets(ctx, cc) +} + +// FetchURLList fetches the URL list from the API. +func (s *Session) FetchURLList( + ctx context.Context, config model.URLListConfig) ([]model.URLInfo, error) { + clnt, err := s.NewOrchestraClient(ctx) + if err != nil { + return nil, err + } + return clnt.FetchURLList(ctx, config) +} + // KeyValueStore returns the configured key-value store. func (s *Session) KeyValueStore() model.KeyValueStore { return s.kvStore @@ -387,7 +416,10 @@ func (s *Session) NewSubmitter(ctx context.Context) (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) (model.ExperimentOrchestraClient, error) { +// +// 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) { 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 08aa198..e7954b4 100644 --- a/internal/engine/session_integration_test.go +++ b/internal/engine/session_integration_test.go @@ -166,21 +166,6 @@ func newSessionForTesting(t *testing.T) *Session { return sess } -func TestNewOrchestraClient(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } - sess := newSessionForTestingNoLookups(t) - defer sess.Close() - clnt, err := sess.NewOrchestraClient(context.Background()) - if err != nil { - t.Fatal(err) - } - if clnt == nil { - t.Fatal("expected non nil client here") - } -} - func TestInitOrchestraClientMaybeRegisterError(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") @@ -634,3 +619,17 @@ func TestSessionNewSubmitterReturnsNonNilSubmitter(t *testing.T) { t.Fatal("expected non nil submitter here") } } + +func TestSessionFetchURLList(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + sess := newSessionForTesting(t) + resp, err := sess.FetchURLList(context.Background(), model.URLListConfig{}) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("expected non-nil response here") + } +} diff --git a/internal/engine/session_internal_test.go b/internal/engine/session_internal_test.go index e13e60b..e254e73 100644 --- a/internal/engine/session_internal_test.go +++ b/internal/engine/session_internal_test.go @@ -208,3 +208,42 @@ func TestSessionMaybeLookupLocationContextLookupLocationContextFailure(t *testin t.Fatal("not the error we expected", err) } } + +func TestSessionFetchURLListWithCancelledContext(t *testing.T) { + sess := &Session{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cause failure + resp, err := sess.FetchURLList(ctx, model.URLListConfig{}) + if !errors.Is(err, context.Canceled) { + t.Fatal("not the error we expected", err) + } + if resp != nil { + t.Fatal("expected nil response here") + } +} + +func TestSessionFetchTorTargetsWithCancelledContext(t *testing.T) { + sess := &Session{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cause failure + resp, err := sess.FetchTorTargets(ctx, "IT") + if !errors.Is(err, context.Canceled) { + t.Fatal("not the error we expected", err) + } + if resp != nil { + t.Fatal("expected nil response here") + } +} + +func TestSessionFetchPsiphonConfigWithCancelledContext(t *testing.T) { + sess := &Session{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cause failure + resp, err := sess.FetchPsiphonConfig(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatal("not the error we expected", err) + } + if resp != nil { + t.Fatal("expected nil response here") + } +}