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/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{}

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
 	}

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,

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{

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()