Split the Client API in two

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,32 @@ 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.
+type TestClient interface {
+	Client
+	MustStartSyncing(t ct.TestLike) (stopSyncing func())
+}
+
+// NewTestClient wraps a Client implementation with helper functions which tests can use.
+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 +154,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 +226,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)