Split the Client API in two

We want the Client API to be as small as possible, to make it as easy
as possible to implement clients which can work with complement-crypto.

However, we also want to use that same API when writing tests, where
it is desirable to have helper functions (typically of the form `MustXXX`).

To satisfy both, split the API in two, where the SDK authors implement
the simple `Client` and the test suite then wraps that in another API
called `TestClient` which handles all the nice helper functions.

For now, just convert `MustStartSyncing` to illustrate the design change.
Next PR will move the remaining Must functions.

internal/api/client.go

@@ -33,11 +33,6 @@ type Client interface {
 	// uploaded to the server. Failure to block will result in flakey tests as other users may not
 	// encrypt for this Client due to not detecting keys for the Client.
 	Login(t ct.TestLike, opts ClientCreationOpts) error
-	// MustStartSyncing to begin syncing from sync v2 / sliding sync.
-	// Tests should call stopSyncing() at the end of the test.
-	// MUST BLOCK until the initial sync is complete.
-	// Fails the test if there was a problem syncing.
-	MustStartSyncing(t ct.TestLike) (stopSyncing func())
 	// StartSyncing to begin syncing from sync v2 / sliding sync.
 	// Tests should call stopSyncing() at the end of the test.
 	// MUST BLOCK until the initial sync is complete.
@@ -96,9 +91,31 @@ type Client interface {
 	Opts() ClientCreationOpts
 }
 
-type Notification struct {
-	Event
-	HasMentions *bool
+// TestClient is a Client with extra helper functions added to make writing tests easier.
+// Client implementations are not expected to implement these helper functions, and are
+// instead composed together by the test rig itself. See TestClientImpl.
+type TestClient interface {
+	Client
+	MustStartSyncing(t ct.TestLike) (stopSyncing func())
+}
+
+func NewTestClient(c Client) TestClient {
+	return &testClientImpl{
+		Client: c,
+	}
+}
+
+type testClientImpl struct {
+	Client
+}
+
+func (c *testClientImpl) MustStartSyncing(t ct.TestLike) (stopSyncing func()) {
+	t.Helper()
+	stopSyncing, err := c.StartSyncing(t)
+	if err != nil {
+		ct.Fatalf(t, "MustStartSyncing: %s", err)
+	}
+	return stopSyncing
 }
 
 type LoggedClient struct {
@@ -136,14 +153,6 @@ func (c *LoggedClient) MustGetEvent(t ct.TestLike, roomID, eventID string) Event
 	return c.Client.MustGetEvent(t, roomID, eventID)
 }
 
-func (c *LoggedClient) MustStartSyncing(t ct.TestLike) (stopSyncing func()) {
-	t.Helper()
-	c.Logf(t, "%s MustStartSyncing starting to sync", c.logPrefix())
-	stopSyncing = c.Client.MustStartSyncing(t)
-	c.Logf(t, "%s MustStartSyncing now syncing", c.logPrefix())
-	return
-}
-
 func (c *LoggedClient) StartSyncing(t ct.TestLike) (stopSyncing func(), err error) {
 	t.Helper()
 	c.Logf(t, "%s StartSyncing starting to sync", c.logPrefix())
@@ -216,6 +225,11 @@ func (c *LoggedClient) logPrefix() string {
 	return fmt.Sprintf("[%s](%s)", c.UserID(), c.Type())
 }
 
+type Notification struct {
+	Event
+	HasMentions *bool
+}
+
 // ClientCreationOpts are options to use when creating crypto clients.
 //
 // This contains a mixture of generic options which can be used across any client, and specific

internal/api/js/js.go

@@ -549,13 +549,6 @@ func (c *JSClient) MustGetEvent(t ct.TestLike, roomID, eventID string) api.Event
 	return ev
 }
 
-func (c *JSClient) MustStartSyncing(t ct.TestLike) (stopSyncing func()) {
-	t.Helper()
-	stopSyncing, err := c.StartSyncing(t)
-	must.NotError(t, "StartSyncing", err)
-	return stopSyncing
-}
-
 // StartSyncing to begin syncing from sync v2 / sliding sync.
 // Tests should call stopSyncing() at the end of the test.
 func (c *JSClient) StartSyncing(t ct.TestLike) (stopSyncing func(), err error) {

internal/api/rust/rust.go

@@ -323,13 +323,6 @@ func (c *RustClient) MustGetEvent(t ct.TestLike, roomID, eventID string) api.Eve
 	return *ev
 }
 
-func (c *RustClient) MustStartSyncing(t ct.TestLike) (stopSyncing func()) {
-	t.Helper()
-	stopSyncing, err := c.StartSyncing(t)
-	must.NotError(t, "StartSyncing", err)
-	return stopSyncing
-}
-
 // StartSyncing to begin syncing from sync v2 / sliding sync.
 // Tests should call stopSyncing() at the end of the test.
 func (c *RustClient) StartSyncing(t ct.TestLike) (stopSyncing func(), err error) {

internal/cc/test_context.go

@@ -70,9 +70,9 @@ func (c *TestContext) RegisterNewUser(t *testing.T, clientType api.ClientType, l
 //
 // The callback function is invoked after this, and cleanup functions are called on your behalf when the
 // callback function ends.
-func (c *TestContext) WithClientSyncing(t *testing.T, req *ClientCreationRequest, callback func(cli api.Client)) {
+func (c *TestContext) WithClientSyncing(t *testing.T, req *ClientCreationRequest, callback func(cli api.TestClient)) {
 	t.Helper()
-	c.WithClientsSyncing(t, []*ClientCreationRequest{req}, func(clients []api.Client) {
+	c.WithClientsSyncing(t, []*ClientCreationRequest{req}, func(clients []api.TestClient) {
 		callback(clients[0])
 	})
 }
@@ -87,9 +87,9 @@ func (c *TestContext) WithClientSyncing(t *testing.T, req *ClientCreationRequest
 //
 // The callback function is invoked after this, and cleanup functions are called on your behalf when the
 // callback function ends.
-func (c *TestContext) WithClientsSyncing(t *testing.T, reqs []*ClientCreationRequest, callback func(clients []api.Client)) {
+func (c *TestContext) WithClientsSyncing(t *testing.T, reqs []*ClientCreationRequest, callback func(clients []api.TestClient)) {
 	t.Helper()
-	cryptoClients := make([]api.Client, len(reqs))
+	cryptoClients := make([]api.TestClient, len(reqs))
 	// Login all clients BEFORE starting any of their sync loops.
 	// We do this because Login will send device list updates and cause clients to upload OTKs/device keys.
 	// We want to make sure ALL these keys are on the server before any test client syncs otherwise it
@@ -107,26 +107,26 @@ func (c *TestContext) WithClientsSyncing(t *testing.T, reqs []*ClientCreationReq
 }
 
 // mustCreateMultiprocessClient creates a new RPC process and instructs it to create a client given by the client creation options.
-func (c *TestContext) mustCreateMultiprocessClient(t *testing.T, req *ClientCreationRequest) api.Client {
+func (c *TestContext) mustCreateMultiprocessClient(t *testing.T, req *ClientCreationRequest) api.TestClient {
 	t.Helper()
 	if c.RPCBinaryPath == "" {
 		t.Skipf("RPC binary path not provided, skipping multiprocess test. To run this test, set COMPLEMENT_CRYPTO_RPC_BINARY")
-		return nil
+		return api.NewTestClient(nil)
 	}
 	ctxPrefix := fmt.Sprintf("%d", c.RPCInstance.Add(1))
 	remoteBindings, err := rpc.NewLanguageBindings(c.RPCBinaryPath, req.User.ClientType.Lang, ctxPrefix)
 	if err != nil {
 		t.Fatalf("Failed to create new RPC language bindings: %s", err)
 	}
-	return remoteBindings.MustCreateClient(t, req.Opts)
+	return api.NewTestClient(remoteBindings.MustCreateClient(t, req.Opts))
 }
 
 // WithAliceSyncing is a helper function which creates a rust/js client and automatically logs in Alice and starts
 // a sync loop for her. For more customisation, see WithClientSyncing.
 //
 // The callback function is invoked after this, and cleanup functions are called on your behalf when the
 // callback function ends.
-func (c *TestContext) WithAliceSyncing(t *testing.T, callback func(alice api.Client)) {
+func (c *TestContext) WithAliceSyncing(t *testing.T, callback func(alice api.TestClient)) {
 	t.Helper()
 	must.NotEqual(t, c.Alice, nil, "No Alice defined. Call CreateTestContext() with at least 1 api.ClientType.")
 	c.WithClientSyncing(t, &ClientCreationRequest{
@@ -139,7 +139,7 @@ func (c *TestContext) WithAliceSyncing(t *testing.T, callback func(alice api.Cli
 //
 // The callback function is invoked after this, and cleanup functions are called on your behalf when the
 // callback function ends.
-func (c *TestContext) WithAliceAndBobSyncing(t *testing.T, callback func(alice, bob api.Client)) {
+func (c *TestContext) WithAliceAndBobSyncing(t *testing.T, callback func(alice, bob api.TestClient)) {
 	t.Helper()
 	must.NotEqual(t, c.Bob, nil, "No Bob defined. Call CreateTestContext() with at least 2 api.ClientTypes.")
 	c.WithClientsSyncing(t, []*ClientCreationRequest{
@@ -149,7 +149,7 @@ func (c *TestContext) WithAliceAndBobSyncing(t *testing.T, callback func(alice,
 		{
 			User: c.Bob,
 		},
-	}, func(clients []api.Client) {
+	}, func(clients []api.TestClient) {
 		callback(clients[0], clients[1])
 	})
 }
@@ -159,7 +159,7 @@ func (c *TestContext) WithAliceAndBobSyncing(t *testing.T, callback func(alice,
 //
 // The callback function is invoked after this, and cleanup functions are called on your behalf when the
 // callback function ends.
-func (c *TestContext) WithAliceBobAndCharlieSyncing(t *testing.T, callback func(alice, bob, charlie api.Client)) {
+func (c *TestContext) WithAliceBobAndCharlieSyncing(t *testing.T, callback func(alice, bob, charlie api.TestClient)) {
 	t.Helper()
 	must.NotEqual(t, c.Charlie, nil, "No Charlie defined. Call CreateTestContext() with at least 3 api.ClientTypes.")
 	c.WithClientsSyncing(t, []*ClientCreationRequest{
@@ -172,7 +172,7 @@ func (c *TestContext) WithAliceBobAndCharlieSyncing(t *testing.T, callback func(
 		{
 			User: c.Charlie,
 		},
-	}, func(clients []api.Client) {
+	}, func(clients []api.TestClient) {
 		callback(clients[0], clients[1], clients[2])
 	})
 }
@@ -288,7 +288,7 @@ func (c *TestContext) MustRegisterNewDevice(t *testing.T, user *User, newDeviceI
 }
 
 // MustLoginClient is the same as MustCreateClient but also logs in the client.
-func (c *TestContext) MustLoginClient(t *testing.T, req *ClientCreationRequest) api.Client {
+func (c *TestContext) MustLoginClient(t *testing.T, req *ClientCreationRequest) api.TestClient {
 	t.Helper()
 	client := c.MustCreateClient(t, req)
 	must.NotError(t, "failed to login client", client.Login(t, client.Opts()))
@@ -297,7 +297,7 @@ func (c *TestContext) MustLoginClient(t *testing.T, req *ClientCreationRequest)
 
 // MustCreateClient creates an api.Client from an existing Complement client and the specified client type. Additional options
 // can be set to configure the client beyond that of the Complement client e.g to add persistent storage.
-func (c *TestContext) MustCreateClient(t *testing.T, req *ClientCreationRequest) api.Client {
+func (c *TestContext) MustCreateClient(t *testing.T, req *ClientCreationRequest) api.TestClient {
 	t.Helper()
 	if req.User == nil {
 		ct.Fatalf(t, "MustCreateClient: ClientCreationRequest missing 'user', register one with RegisterNewUser or use an existing one.")
@@ -318,11 +318,11 @@ func (c *TestContext) MustCreateClient(t *testing.T, req *ClientCreationRequest)
 // mustCreateClient creates an api.Client with the specified language/server, else fails the test.
 //
 // Options can be provided to configure clients, such as enabling persistent storage.
-func mustCreateClient(t *testing.T, clientType api.ClientType, cfg api.ClientCreationOpts) api.Client {
+func mustCreateClient(t *testing.T, clientType api.ClientType, cfg api.ClientCreationOpts) api.TestClient {
 	bindings := langs.GetLanguageBindings(clientType.Lang)
 	if bindings == nil {
 		t.Fatalf("unknown language: %s", clientType.Lang)
 	}
 	c := bindings.MustCreateClient(t, cfg)
-	return c
+	return api.NewTestClient(c)
 }

internal/deploy/rpc/client.go

@@ -218,24 +218,6 @@ func (c *RPCClient) Login(t ct.TestLike, opts api.ClientCreationOpts) error {
 	return err
 }
 
-// MustStartSyncing to begin syncing from sync v2 / sliding sync.
-// Tests should call stopSyncing() at the end of the test.
-// MUST BLOCK until the initial sync is complete.
-// Fails the test if there was a problem syncing.
-func (c *RPCClient) MustStartSyncing(t ct.TestLike) (stopSyncing func()) {
-	var void int
-	err := c.client.Call("Server.MustStartSyncing", t.Name(), &void)
-	if err != nil {
-		t.Fatalf("RPCClient.MustStartSyncing: %s", err)
-	}
-	return func() {
-		err := c.client.Call("Server.StopSyncing", t.Name(), &void)
-		if err != nil {
-			t.Fatalf("RPCClient.StopSyncing: %s", err)
-		}
-	}
-}
-
 // StartSyncing to begin syncing from sync v2 / sliding sync.
 // Tests should call stopSyncing() at the end of the test.
 // MUST BLOCK until the initial sync is complete.

internal/deploy/rpc/server.go

@@ -111,12 +111,6 @@ func (s *Server) Login(opts api.ClientCreationOpts, void *int) error {
 	return s.activeClient.Login(&api.MockT{}, opts)
 }
 
-func (s *Server) MustStartSyncing(testName string, void *int) error {
-	defer s.keepAlive()
-	s.stopSyncing = s.activeClient.MustStartSyncing(&api.MockT{TestName: testName})
-	return nil
-}
-
 func (s *Server) StartSyncing(testName string, void *int) error {
 	defer s.keepAlive()
 	stopSyncing, err := s.activeClient.StartSyncing(&api.MockT{TestName: testName})

internal/tests/client_test.go

@@ -29,7 +29,7 @@ var (
 	ssDeployment *deploy.SlidingSyncDeployment
 	// aka functions which make clients, and we don't care about the language.
 	// Tests just loop through this array for each client impl.
-	clientFactories []func(t *testing.T, cfg api.ClientCreationOpts) api.Client
+	clientFactories []func(t *testing.T, cfg api.ClientCreationOpts) api.TestClient
 )
 
 func Deploy(t *testing.T) *deploy.SlidingSyncDeployment {
@@ -47,30 +47,30 @@ func Deploy(t *testing.T) *deploy.SlidingSyncDeployment {
 }
 
 func TestMain(m *testing.M) {
-	rustClientCreator := func(t *testing.T, cfg api.ClientCreationOpts) api.Client {
+	rustClientCreator := func(t *testing.T, cfg api.ClientCreationOpts) api.TestClient {
 		client, err := rust.NewRustClient(t, cfg)
 		if err != nil {
 			t.Fatalf("NewRustClient: %s", err)
 		}
-		return client
+		return api.NewTestClient(client)
 	}
-	jsClientCreator := func(t *testing.T, cfg api.ClientCreationOpts) api.Client {
+	jsClientCreator := func(t *testing.T, cfg api.ClientCreationOpts) api.TestClient {
 		client, err := js.NewJSClient(t, cfg)
 		if err != nil {
 			t.Fatalf("NewJSClient: %s", err)
 		}
-		return client
+		return api.NewTestClient(client)
 	}
 	clientFactories = append(clientFactories, rustClientCreator, jsClientCreator)
 
 	rpcBinary := os.Getenv("COMPLEMENT_CRYPTO_RPC_BINARY")
 	if rpcBinary != "" {
-		clientFactories = append(clientFactories, func(t *testing.T, cfg api.ClientCreationOpts) api.Client {
+		clientFactories = append(clientFactories, func(t *testing.T, cfg api.ClientCreationOpts) api.TestClient {
 			remoteBindings, err := rpc.NewLanguageBindings(rpcBinary, api.ClientTypeRust, "")
 			if err != nil {
 				log.Fatal(err)
 			}
-			return remoteBindings.MustCreateClient(t, cfg)
+			return api.NewTestClient(remoteBindings.MustCreateClient(t, cfg))
 		})
 	}
 	rust.SetupLogs("rust_sdk_logs")
@@ -104,7 +104,7 @@ func TestReceiveTimeline(t *testing.T) {
 	}
 
 	// test that if we start syncing with a room full of events, we see those events.
-	ForEachClient(t, "existing_events", deployment, func(t *testing.T, client api.Client, csapi *client.CSAPI) {
+	ForEachClient(t, "existing_events", deployment, func(t *testing.T, client api.TestClient, csapi *client.CSAPI) {
 		must.NotError(t, "Failed to login", client.Login(t, client.Opts()))
 		roomID, eventIDs := createAndSendEvents(t, csapi)
 		time.Sleep(time.Second) // give time for everything to settle server-side e.g sliding sync proxy
@@ -132,7 +132,7 @@ func TestReceiveTimeline(t *testing.T) {
 	})
 
 	// test that if we are already syncing and then see a room live stream full of events, we see those events.
-	ForEachClient(t, "live_events", deployment, func(t *testing.T, client api.Client, csapi *client.CSAPI) {
+	ForEachClient(t, "live_events", deployment, func(t *testing.T, client api.TestClient, csapi *client.CSAPI) {
 		must.NotError(t, "Failed to login", client.Login(t, client.Opts()))
 		stopSyncing := client.MustStartSyncing(t)
 		defer stopSyncing()
@@ -171,7 +171,7 @@ func TestReceiveTimeline(t *testing.T) {
 
 func TestCanWaitUntilEventInRoomBeforeRoomIsKnown(t *testing.T) {
 	deployment := Deploy(t)
-	ForEachClient(t, "", deployment, func(t *testing.T, client api.Client, csapi *client.CSAPI) {
+	ForEachClient(t, "", deployment, func(t *testing.T, client api.TestClient, csapi *client.CSAPI) {
 		roomID := csapi.MustCreateRoom(t, map[string]interface{}{})
 		eventID := csapi.SendEventSynced(t, roomID, b.Event{
 			Type: "m.room.message",
@@ -196,7 +196,7 @@ func TestCanWaitUntilEventInRoomBeforeRoomIsKnown(t *testing.T) {
 
 func TestSendingEvents(t *testing.T) {
 	deployment := Deploy(t)
-	ForEachClient(t, "", deployment, func(t *testing.T, client api.Client, csapi *client.CSAPI) {
+	ForEachClient(t, "", deployment, func(t *testing.T, client api.TestClient, csapi *client.CSAPI) {
 		must.NotError(t, "Failed to login", client.Login(t, client.Opts()))
 		roomID := csapi.MustCreateRoom(t, map[string]interface{}{})
 		stopSyncing := client.MustStartSyncing(t)
@@ -217,7 +217,7 @@ func TestSendingEvents(t *testing.T) {
 }
 
 // run a subtest for each client factory
-func ForEachClient(t *testing.T, name string, deployment *deploy.SlidingSyncDeployment, fn func(t *testing.T, client api.Client, csapi *client.CSAPI)) {
+func ForEachClient(t *testing.T, name string, deployment *deploy.SlidingSyncDeployment, fn func(t *testing.T, client api.TestClient, csapi *client.CSAPI)) {
 	for _, createClient := range clientFactories {
 		csapiAlice := deployment.Register(t, "hs1", helpers.RegistrationOpts{
 			LocalpartSuffix: "client",

tests/delayed_requests_test.go

@@ -33,7 +33,7 @@ func TestDelayedInviteResponse(t *testing.T) {
 	Instance().ForEachClientType(t, func(t *testing.T, clientType api.ClientType) {
 		tc := Instance().CreateTestContext(t, clientType, clientType)
 		roomID := tc.CreateNewEncryptedRoom(t, tc.Alice)
-		tc.WithAliceAndBobSyncing(t, func(alice, bob api.Client) {
+		tc.WithAliceAndBobSyncing(t, func(alice, bob api.TestClient) {
 			// we send a message first so clients which lazily call /members can do so now.
 			// if we don't do this, the client won't rely on /sync for the member list so won't fail.
 			alice.SendMessage(t, roomID, "dummy message to make /members call")

tests/device_keys_test.go

@@ -41,7 +41,7 @@ func TestFailedDeviceKeyDownloadRetries(t *testing.T) {
 			roomID := tc.CreateNewEncryptedRoom(t, tc.Alice, cc.EncRoomOptions.Invite([]string{tc.Bob.UserID}))
 			tc.Bob.MustJoinRoom(t, roomID, []string{"hs1"})
 
-			tc.WithAliceAndBobSyncing(t, func(alice, bob api.Client) {
+			tc.WithAliceAndBobSyncing(t, func(alice, bob api.TestClient) {
 				// When Alice sends a message
 				alice.SendMessage(t, roomID, "checking whether we can send a message")
 

tests/federation_connectivity_test.go

@@ -32,7 +32,7 @@ func TestNewUserCannotGetKeysForOfflineServer(t *testing.T) {
 		t.Logf("%s joining room %s", tc.Bob.UserID, roomID)
 		tc.Bob.MustJoinRoom(t, roomID, []string{"hs1"})
 
-		tc.WithAliceAndBobSyncing(t, func(alice, bob api.Client) {
+		tc.WithAliceAndBobSyncing(t, func(alice, bob api.TestClient) {
 			// let clients sync device keys
 			time.Sleep(time.Second)
 
@@ -50,7 +50,7 @@ func TestNewUserCannotGetKeysForOfflineServer(t *testing.T) {
 			tc.Alice.MustInviteRoom(t, roomID, tc.Charlie.UserID)
 			tc.WithClientSyncing(t, &cc.ClientCreationRequest{
 				User: tc.Charlie,
-			}, func(charlie api.Client) {
+			}, func(charlie api.TestClient) {
 				tc.Charlie.MustJoinRoom(t, roomID, []string{"hs1"})
 
 				// let charlie sync device keys... and fail to get bob's keys!
@@ -116,7 +116,7 @@ func TestExistingSessionCannotGetKeysForOfflineServer(t *testing.T) {
 		tc.Bob.MustJoinRoom(t, roomIDab, []string{"hs1"})
 		tc.Bob.MustJoinRoom(t, roomIDbc, []string{"hs1"})
 
-		tc.WithAliceBobAndCharlieSyncing(t, func(alice, bob, charlie api.Client) {
+		tc.WithAliceBobAndCharlieSyncing(t, func(alice, bob, charlie api.TestClient) {
 			// let clients sync device keys
 			time.Sleep(time.Second)