store test: Add and use UpdateMachineTestGlobalStore, for less mocking

Author: chrisbobbe

test/api/fake_api.dart

@@ -1,6 +1,7 @@
 import 'dart:collection';
 import 'dart:convert';
 
+import 'package:checks/checks.dart';
 import 'package:flutter/foundation.dart';
 import 'package:http/http.dart' as http;
 import 'package:zulip/api/core.dart';
@@ -278,3 +279,7 @@ class FakeApiConnection extends ApiConnection {
     );
   }
 }
+
+extension FakeApiConnectionChecks on Subject<FakeApiConnection> {
+  Subject<bool> get isOpen => has((x) => x.isOpen, 'isOpen');
+}

test/model/store_test.dart

@@ -131,17 +131,25 @@ void main() {
   }));
 
   test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async {
-    final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
+    final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
+    globalStore.prepareRegisterQueueResponse = (connection) =>
+      connection.prepare(
+        delay: TestGlobalStore.removeAccountDuration + Duration(seconds: 1),
+        apiException: eg.apiExceptionUnauthorized());
+    final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
     final future = globalStore.perAccount(eg.selfAccount.id);
+    check(connection.takeRequests()).length.equals(1); // register request
 
     await logOutAccount(globalStore, eg.selfAccount.id);
     check(globalStore.takeDoRemoveAccountCalls())
       .single.equals(eg.selfAccount.id);
 
-    globalStore.completers[eg.selfAccount.id]!
-      .single.completeError(eg.apiExceptionUnauthorized());
     await check(future).throws<AccountNotFoundException>();
     check(globalStore.takeDoRemoveAccountCalls()).isEmpty();
+    // no poll, server-emoji-data, or register-token requests
+    check(connection.takeRequests()).isEmpty();
+    // TODO(#1354) uncomment
+    // check(connection).isOpen.isFalse();
   }));
 
   // TODO test insertAccount
@@ -239,11 +247,20 @@ void main() {
     });
 
     test('when store loading', () async {
-      final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
+      final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
       checkGlobalStore(globalStore, eg.selfAccount.id,
         expectAccount: true, expectStore: false);
 
-      // don't await; we'll complete/await it manually after removeAccount
+      // assert(globalStore.useCachedApiConnections);
+      // Cache a connection and get this reference to it,
+      // so we can check later that it gets closed.
+      // final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
+
+      globalStore.prepareRegisterQueueResponse = (connection) {
+        connection.prepare(
+          delay: TestGlobalStore.removeAccountDuration + Duration(seconds: 1),
+          json: eg.initialSnapshot().toJson());
+      };
       final loadingFuture = globalStore.perAccount(eg.selfAccount.id);
 
       checkGlobalStore(globalStore, eg.selfAccount.id,
@@ -257,13 +274,14 @@ void main() {
         expectAccount: false, expectStore: false);
       check(notifyCount).equals(1);
 
-      globalStore.completers[eg.selfAccount.id]!.single
-        .complete(eg.store(account: eg.selfAccount, initialSnapshot: eg.initialSnapshot()));
-      // TODO test that the never-used store got disposed and its connection closed
-      await check(loadingFuture).throws<AccountNotFoundException>();
+      // Actually throws a null-check error; that's the bug #1354.
+      // TODO(#1354) should specifically throw AccountNotFoundException
+      await check(loadingFuture).throws();
       checkGlobalStore(globalStore, eg.selfAccount.id,
         expectAccount: false, expectStore: false);
       check(notifyCount).equals(1); // no extra notify
+      // TODO(#1354) uncomment
+      // check(connection).isOpen.isFalse();
 
       check(globalStore.debugNumPerAccountStoresLoading).equals(0);
     });
@@ -965,44 +983,42 @@ void main() {
   });
 
   group('UpdateMachine.poll reload failure', () {
-    late LoadingTestGlobalStore globalStore;
-
-    List<Completer<PerAccountStore>> completers() =>
-      globalStore.completers[eg.selfAccount.id]!;
-
-    Future<void> prepareReload(FakeAsync async) async {
-      globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
-
-      // Simulate the setup that [TestGlobalStore.doLoadPerAccount] would do.
-      // (These tests use [LoadingTestGlobalStore] for greater control in
-      // later steps; that requires this setup step to be finer-grained too.)
-      final updateMachine = eg.updateMachine(
-        globalStore: globalStore, account: eg.selfAccount);
-      final store = updateMachine.store;
-      final future = globalStore.perAccount(eg.selfAccount.id);
-      completers().single.complete(store);
-      await future;
-      completers().clear();
+    late UpdateMachineTestGlobalStore globalStore;
 
-      updateMachine.debugPauseLoop();
-      updateMachine.poll();
-      (store.connection as FakeApiConnection).prepare(
+    Future<void> prepareReload(FakeAsync async, {
+      required void Function(FakeApiConnection) prepareRegisterQueueResponse,
+    }) async {
+      globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
+
+      final store = await globalStore.perAccount(eg.selfAccount.id);
+      final updateMachine = store.updateMachine!;
+
+      final connection = store.connection as FakeApiConnection;
+      connection.prepare(
         apiException: eg.apiExceptionBadEventQueueId());
+      globalStore.prepareRegisterQueueResponse = (newConnection) {
+        prepareRegisterQueueResponse(newConnection);
+      };
+      // When we reload, we should get a new connection,
+      // just like when the app runs live. This is more realistic,
+      // and we don't want a glitch where we try to double-close a connection
+      // just because of the test infrastructure. (One of the tests
+      // logs out the account, and the connection shouldn't be used after that.)
+      globalStore.clearCachedApiConnections();
       updateMachine.debugAdvanceLoop();
-      async.elapse(Duration.zero);
+      async.elapse(Duration.zero); // the bad-event-queue error arrives
       check(store).isLoading.isTrue();
     }
 
     test('user logged out before new store is loaded', () => awaitFakeAsync((async) async {
-      await prepareReload(async);
-      check(completers()).single.isCompleted.isFalse();
+      await prepareReload(async,
+        prepareRegisterQueueResponse: (newConnection) {
+          newConnection.prepare(
+            delay: TestGlobalStore.removeAccountDuration + Duration(seconds: 1),
+            json: eg.initialSnapshot().toJson());
+        });
 
-      // [PerAccountStore.fromInitialSnapshot] requires the account
-      // to be in the global store when called; do so before logging out.
-      final newStore = eg.store(globalStore: globalStore, account: eg.selfAccount);
       await logOutAccount(globalStore, eg.selfAccount.id);
-      completers().single.complete(newStore);
-      check(completers()).single.isCompleted.isTrue();
       check(globalStore.takeDoRemoveAccountCalls()).single.equals(eg.selfAccount.id);
 
       async.elapse(TestGlobalStore.removeAccountDuration);
@@ -1011,15 +1027,20 @@ void main() {
       async.flushTimers();
       // Reload never succeeds and there are no unhandled errors.
       check(globalStore.perAccountSync(eg.selfAccount.id)).isNull();
-    }));
+    }),
+      // An unhandled error is actually the bug #1354, so skip for now
+      // TODO(#1354) unskip
+      skip: true);
 
     test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async {
-      await prepareReload(async);
-      check(completers()).single.isCompleted.isFalse();
+      await prepareReload(async,
+        prepareRegisterQueueResponse: (newConnection) {
+          newConnection.prepare(
+            delay: Duration(seconds: 1),
+            apiException: eg.apiExceptionUnauthorized());
+        });
 
-      completers().single.completeError(eg.apiExceptionUnauthorized());
-      async.elapse(Duration.zero);
-      check(completers()).single.isCompleted.isTrue();
+      async.elapse(const Duration(seconds: 1));
       check(globalStore.takeDoRemoveAccountCalls()).single.equals(eg.selfAccount.id);
 
       async.elapse(TestGlobalStore.removeAccountDuration);

test/model/test_store.dart

@@ -1,7 +1,10 @@
 import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/initial_snapshot.dart';
 import 'package:zulip/api/model/model.dart';
+import 'package:zulip/api/route/events.dart';
+import 'package:zulip/api/route/realm.dart';
 import 'package:zulip/model/store.dart';
+import 'package:zulip/notifications/receive.dart';
 import 'package:zulip/widgets/store.dart';
 
 import '../api/fake_api.dart';
@@ -121,7 +124,10 @@ mixin _DatabaseMixin on GlobalStore {
 /// Tests can use [PerAccountStore.updateMachine] in order to invoke that logic
 /// explicitly when desired.
 ///
-/// See also [TestZulipBinding.globalStore], which provides one of these.
+/// See also:
+///   * [TestZulipBinding.globalStore], which provides one of these.
+///   * [UpdateMachineTestGlobalStore], which prepares per-account data
+///     using [UpdateMachine.load] (like [LiveGlobalStore] does).
 class TestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMixin {
   TestGlobalStore({required super.accounts});
 
@@ -167,6 +173,61 @@ class TestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMi
   }
 }
 
+/// A [GlobalStore] that causes no database queries,
+/// and loads per-account data from API responses prepared by callers.
+///
+/// The per-account stores will use [FakeApiConnection].
+///
+/// Like [LiveGlobalStore] and unlike [TestGlobalStore],
+/// account data is loaded via [UpdateMachine.load].
+/// Callers can set [prepareRegisterQueueResponse]
+/// to prepare a register-queue payload or an exception.
+/// The implementation pauses the event-polling loop
+/// to avoid being a nuisance and does a boring
+/// [FakeApiConnection.prepare] for the register-token request.
+///
+/// See also:
+///   * [TestGlobalStore], which prepares per-account data
+///     without using [UpdateMachine.load].
+class UpdateMachineTestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMixin {
+  UpdateMachineTestGlobalStore({required super.accounts});
+
+  // [doLoadPerAccount] depends on the cache to prepare the API responses.
+  // Calling [clearCachedApiConnections] is permitted, though.
+  @override bool get useCachedApiConnections => true;
+  @override set useCachedApiConnections(bool value) =>
+    throw UnsupportedError(
+      'Setting UpdateMachineTestGlobalStore.useCachedApiConnections '
+      'is not supported.');
+
+  void Function(FakeApiConnection)? prepareRegisterQueueResponse;
+
+  void _prepareRegisterQueueSuccess(FakeApiConnection connection) {
+    connection.prepare(json: eg.initialSnapshot().toJson());
+  }
+
+  @override
+  Future<PerAccountStore> doLoadPerAccount(int accountId) async {
+    final account = getAccount(accountId);
+
+    // UpdateMachine.load should pick up the connection
+    // with the network-request responses that we've prepared.
+    assert(useCachedApiConnections);
+
+    final connection = apiConnectionFromAccount(account!) as FakeApiConnection;
+    (prepareRegisterQueueResponse ?? _prepareRegisterQueueSuccess)(connection);
+    connection
+      ..prepare(json: GetEventsResult(events: [HeartbeatEvent(id: 2)], queueId: null).toJson())
+      ..prepare(json: ServerEmojiData(codeToNames: {}).toJson());
+    if (NotificationService.instance.token.value != null) {
+      connection.prepare(json: {}); // register-token
+    }
+    final updateMachine = await UpdateMachine.load(this, accountId);
+    updateMachine.debugPauseLoop();
+    return updateMachine.store;
+  }
+}
+
 extension PerAccountStoreTestExtension on PerAccountStore {
   Future<void> addUser(User user) async {
     await handleEvent(RealmUserAddEvent(id: 1, person: user));