From 228c9bdc4b678d1f3ebdb1799d7a35f9053d592f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 26 Feb 2025 16:42:22 -0800 Subject: [PATCH 01/10] store [nfc]: Add account-exists assert in _reloadPerAccount I believe this is effectively already done by the existing assert `_perAccountStores.containsKey`, because of the invariant that _accounts contains all the keys in _perAccountStores. But the explicitness seems helpful. (And loadPerAccount relies on the account existing.) --- lib/model/store.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index bc5c752174..ddc2cce774 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -154,6 +154,7 @@ abstract class GlobalStore extends ChangeNotifier { } Future _reloadPerAccount(int accountId) async { + assert(_accounts.containsKey(accountId)); assert(_perAccountStores.containsKey(accountId)); assert(!_perAccountStoresLoading.containsKey(accountId)); final store = await loadPerAccount(accountId); From 0a2273c37d6dbe0fc014374631e6a478e76da20c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 26 Feb 2025 16:31:40 -0800 Subject: [PATCH 02/10] store [nfc]: Make account-exists assumption explicit in some dartdocs In particular: GlobalStore.loadPerAccount GlobalStore.doLoadPerAccount UpdateMachine.load The first two already have an `assert` that the account exists, and the third has a ! null-check. They're called in a chain from two places, and we have the account in both: the initial account load and the reload (GlobalStore._reloadPerAccount). --- lib/model/store.dart | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index ddc2cce774..83823cb7db 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -170,6 +170,8 @@ abstract class GlobalStore extends ChangeNotifier { /// Load per-account data for the given account, unconditionally. /// + /// The account for `accountId` must exist. + /// /// This method should be called only by the implementation of [perAccount]. /// Other callers interested in per-account data should use [perAccount] /// and/or [perAccountSync]. @@ -214,6 +216,8 @@ abstract class GlobalStore extends ChangeNotifier { /// Load per-account data for the given account, unconditionally. /// + /// The account for `accountId` must exist. + /// /// This method should be called only by [loadPerAccount]. Future doLoadPerAccount(int accountId); @@ -264,6 +268,8 @@ abstract class GlobalStore extends ChangeNotifier { Future doUpdateAccount(int accountId, AccountsCompanion data); /// Remove an account from the store. + /// + /// The account for `accountId` must exist. Future removeAccount(int accountId) async { assert(_accounts.containsKey(accountId)); await doRemoveAccount(accountId); @@ -942,7 +948,10 @@ class UpdateMachine { store.updateMachine = this; } - /// Load the user's data from the server, and start an event queue going. + /// Load data for the given account from the server, + /// and start an event queue going. + /// + /// The account for `accountId` must exist. /// /// In the future this might load an old snapshot from local storage first. static Future load(GlobalStore globalStore, int accountId) async { From 707504584faeb99ceb1ce31b87fe9ed8d579e6f2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 27 Feb 2025 18:43:50 -0800 Subject: [PATCH 03/10] store [nfc]: Use try/finally to clear _perAccountStoresLoading[accountId] In GlobalStore.perAccount, the loadPerAccount future can throw, and in that case the `_perAccountStoresLoading.remove` in these lines of code hasn't been getting called. That's only a latent bug, currently, because loadPerAccount only throws if the account was logged out, and we always do a `_perAccountStoresLoading.remove` on logout (in GlobalStore.removeAccount). But we'd like to add another case where the loadPerAccount future throws (disallow connecting to ancient servers), which doesn't involve logout / removeAccount. So this try/finally will be important for that case. --- lib/model/store.dart | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 83823cb7db..efe9c52a7c 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -147,10 +147,13 @@ abstract class GlobalStore extends ChangeNotifier { // It's up to us. Start loading. future = loadPerAccount(accountId); _perAccountStoresLoading[accountId] = future; - store = await future; - _setPerAccount(accountId, store); - unawaited(_perAccountStoresLoading.remove(accountId)); - return store; + try { + store = await future; + _setPerAccount(accountId, store); + return store; + } finally { + unawaited(_perAccountStoresLoading.remove(accountId)); + } } Future _reloadPerAccount(int accountId) async { From dc1c806d8f593a7f537071a32569b723c29c551c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 5 Mar 2025 14:38:44 -0800 Subject: [PATCH 04/10] store test [nfc]: Remove redundant awaitFakeAsync --- test/model/store_test.dart | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 61b4ece5cf..76b30c116f 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -1018,24 +1018,22 @@ void main() { check(store).isLoading.isTrue(); } - void checkReloadFailure({ + void checkReloadFailure(FakeAsync async, { required FutureOr Function() completeLoading, - }) { - awaitFakeAsync((async) async { - await prepareReload(async); - check(completers()).single.isCompleted.isFalse(); + }) async { + await prepareReload(async); + check(completers()).single.isCompleted.isFalse(); - await completeLoading(); - check(completers()).single.isCompleted.isTrue(); - check(globalStore.takeDoRemoveAccountCalls()).single.equals(eg.selfAccount.id); + await completeLoading(); + check(completers()).single.isCompleted.isTrue(); + check(globalStore.takeDoRemoveAccountCalls()).single.equals(eg.selfAccount.id); - async.elapse(TestGlobalStore.removeAccountDuration); - check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); + async.elapse(TestGlobalStore.removeAccountDuration); + check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); - async.flushTimers(); - // Reload never succeeds and there are no unhandled errors. - check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); - }); + async.flushTimers(); + // Reload never succeeds and there are no unhandled errors. + check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); } Future logOutAndCompleteWithNewStore() async { @@ -1047,7 +1045,7 @@ void main() { } test('user logged out before new store is loaded', () => awaitFakeAsync((async) async { - checkReloadFailure(completeLoading: logOutAndCompleteWithNewStore); + checkReloadFailure(async, completeLoading: logOutAndCompleteWithNewStore); })); void completeWithApiExceptionUnauthorized() { @@ -1055,7 +1053,7 @@ void main() { } test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async { - checkReloadFailure(completeLoading: completeWithApiExceptionUnauthorized); + checkReloadFailure(async, completeLoading: completeWithApiExceptionUnauthorized); })); }); From 44bdbd1654df33e1d56da12df6f55163718fa8b7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 5 Mar 2025 14:42:43 -0800 Subject: [PATCH 05/10] store test [nfc]: Inline checkReloadFailure helper at both callsites For #1354, we'll need an alternative to TestGlobalStore that runs UpdateMachine.load with prepared API responses instead of mocking it. Since FakeApiConnection doesn't yet offer a `Completer`-based way of completing/awaiting requests, these two tests will have to diverge, preparing and awaiting different delays as needed. --- test/model/store_test.dart | 39 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 76b30c116f..3b98ab4461 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -1018,13 +1018,15 @@ void main() { check(store).isLoading.isTrue(); } - void checkReloadFailure(FakeAsync async, { - required FutureOr Function() completeLoading, - }) async { + test('user logged out before new store is loaded', () => awaitFakeAsync((async) async { await prepareReload(async); check(completers()).single.isCompleted.isFalse(); - await completeLoading(); + // [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); @@ -1034,26 +1036,23 @@ void main() { async.flushTimers(); // Reload never succeeds and there are no unhandled errors. check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); - } - - Future logOutAndCompleteWithNewStore() async { - // [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); - } - - test('user logged out before new store is loaded', () => awaitFakeAsync((async) async { - checkReloadFailure(async, completeLoading: logOutAndCompleteWithNewStore); })); - void completeWithApiExceptionUnauthorized() { + test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async { + await prepareReload(async); + check(completers()).single.isCompleted.isFalse(); + completers().single.completeError(eg.apiExceptionUnauthorized()); - } + async.elapse(Duration.zero); + check(completers()).single.isCompleted.isTrue(); + check(globalStore.takeDoRemoveAccountCalls()).single.equals(eg.selfAccount.id); - test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async { - checkReloadFailure(async, completeLoading: completeWithApiExceptionUnauthorized); + async.elapse(TestGlobalStore.removeAccountDuration); + check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); + + async.flushTimers(); + // Reload never succeeds and there are no unhandled errors. + check(globalStore.perAccountSync(eg.selfAccount.id)).isNull(); })); }); From 795b46e8376b3c2c2d397e1a87c406791752049c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 26 Feb 2025 17:50:07 -0800 Subject: [PATCH 06/10] test [nfc]: Factor out some of TestGlobalStore Soon, we'll add an alternative to TestGlobalStore whose doLoadPerAccount makes fake API requests; its doLoadPerAccount will call UpdateMachine.load. These methods will be useful for that new class. (The new class makes sense as a sibling of TestGlobalStore, rather than a subclass, because TestGlobalStore's dartdoc says it makes no network requests.) --- test/model/test_store.dart | 101 +++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 48 deletions(-) diff --git a/test/model/test_store.dart b/test/model/test_store.dart index ebf6e66f1b..76cc82db4e 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -8,31 +8,7 @@ import 'package:zulip/widgets/store.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; -/// A [GlobalStore] containing data provided by callers, -/// and that causes no database queries or network requests. -/// -/// Tests can provide data to the store by calling [add]. -/// -/// The per-account stores will use [FakeApiConnection]. -/// -/// Unlike with [LiveGlobalStore] and the associated [UpdateMachine.load], -/// there is no automatic event-polling loop or other automated requests. -/// For each account loaded, there is a corresponding [UpdateMachine] -/// in [updateMachines], which tests can use for invoking that logic -/// explicitly when desired. -/// -/// See also [TestZulipBinding.globalStore], which provides one of these. -class TestGlobalStore extends GlobalStore { - TestGlobalStore({ - GlobalSettingsData? globalSettings, - required super.accounts, - }) : super(globalSettings: globalSettings ?? eg.globalSettings()); - - @override - Future doUpdateGlobalSettings(GlobalSettingsCompanion data) async { - // Nothing to do. - } - +mixin _ApiConnectionsMixin on GlobalStore { final Map< ({Uri realmUrl, int? zulipFeatureLevel, String? email, String? apiKey}), FakeApiConnection @@ -81,25 +57,12 @@ class TestGlobalStore extends GlobalStore { realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel, email: email, apiKey: apiKey)); } +} - /// A corresponding [UpdateMachine] for each loaded account. - final Map updateMachines = {}; - - final Map _initialSnapshots = {}; - - /// Add an account and corresponding server data to the test data. - /// - /// The given account will be added to the store. - /// The given initial snapshot will be used to initialize a corresponding - /// [PerAccountStore] when [perAccount] is subsequently called for this - /// account, in particular when a [PerAccountStoreWidget] is mounted. - Future add(Account account, InitialSnapshot initialSnapshot) async { - assert(initialSnapshot.zulipVersion == account.zulipVersion); - assert(initialSnapshot.zulipMergeBase == account.zulipMergeBase); - assert(initialSnapshot.zulipFeatureLevel == account.zulipFeatureLevel); - await insertAccount(account.toCompanion(false)); - assert(!_initialSnapshots.containsKey(account.id)); - _initialSnapshots[account.id] = initialSnapshot; +mixin _DatabaseMixin on GlobalStore { + @override + Future doUpdateGlobalSettings(GlobalSettingsCompanion data) async { + // Nothing to do. } int _nextAccountId = 1; @@ -136,10 +99,6 @@ class TestGlobalStore extends GlobalStore { // Nothing to do. } - static const Duration removeAccountDuration = Duration(milliseconds: 1); - Duration? loadPerAccountDuration; - Object? loadPerAccountException; - /// Consume the log of calls made to [doRemoveAccount]. List takeDoRemoveAccountCalls() { final result = _doRemoveAccountCalls; @@ -151,9 +110,55 @@ class TestGlobalStore extends GlobalStore { @override Future doRemoveAccount(int accountId) async { (_doRemoveAccountCalls ??= []).add(accountId); - await Future.delayed(removeAccountDuration); + await Future.delayed(TestGlobalStore.removeAccountDuration); // Nothing else to do. } +} + +/// A [GlobalStore] containing data provided by callers, +/// and that causes no database queries or network requests. +/// +/// Tests can provide data to the store by calling [add]. +/// +/// The per-account stores will use [FakeApiConnection]. +/// +/// Unlike with [LiveGlobalStore] and the associated [UpdateMachine.load], +/// there is no automatic event-polling loop or other automated requests. +/// For each account loaded, there is a corresponding [UpdateMachine] +/// in [updateMachines], which tests can use for invoking that logic +/// explicitly when desired. +/// +/// See also [TestZulipBinding.globalStore], which provides one of these. +class TestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMixin { + TestGlobalStore({ + GlobalSettingsData? globalSettings, + required super.accounts, + }) : super(globalSettings: globalSettings ?? eg.globalSettings()); + + /// A corresponding [UpdateMachine] for each loaded account. + final Map updateMachines = {}; + + final Map _initialSnapshots = {}; + + static const Duration removeAccountDuration = Duration(milliseconds: 1); + + /// Add an account and corresponding server data to the test data. + /// + /// The given account will be added to the store. + /// The given initial snapshot will be used to initialize a corresponding + /// [PerAccountStore] when [perAccount] is subsequently called for this + /// account, in particular when a [PerAccountStoreWidget] is mounted. + Future add(Account account, InitialSnapshot initialSnapshot) async { + assert(initialSnapshot.zulipVersion == account.zulipVersion); + assert(initialSnapshot.zulipMergeBase == account.zulipMergeBase); + assert(initialSnapshot.zulipFeatureLevel == account.zulipFeatureLevel); + await insertAccount(account.toCompanion(false)); + assert(!_initialSnapshots.containsKey(account.id)); + _initialSnapshots[account.id] = initialSnapshot; + } + + Duration? loadPerAccountDuration; + Object? loadPerAccountException; @override Future doLoadPerAccount(int accountId) async { From 874e3bfe4fad422d2ea856961369be4b8e6c438c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 7 Mar 2025 20:13:01 -0800 Subject: [PATCH 07/10] test [nfc]: Simplify out TestGlobalStore.updateMachines Thanks to the introduction of PerAccountStore.updateMachine in be6698e7d, this feature in the test code is no longer needed. --- test/model/store_test.dart | 12 +++++------- test/model/test_store.dart | 8 ++------ 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 3b98ab4461..4107995da5 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -590,14 +590,13 @@ void main() { group('UpdateMachine.poll', () { late TestGlobalStore globalStore; - late UpdateMachine updateMachine; late PerAccountStore store; + late UpdateMachine updateMachine; late FakeApiConnection connection; void updateFromGlobalStore() { - updateMachine = globalStore.updateMachines[eg.selfAccount.id]!; - store = updateMachine.store; - assert(identical(store, globalStore.perAccountSync(eg.selfAccount.id))); + store = globalStore.perAccountSync(eg.selfAccount.id)!; + updateMachine = store.updateMachine!; connection = store.connection as FakeApiConnection; } @@ -1005,9 +1004,8 @@ void main() { completers().single.complete(store); await future; completers().clear(); - final updateMachine = globalStore.updateMachines[eg.selfAccount.id] = - UpdateMachine.fromInitialSnapshot( - store: store, initialSnapshot: eg.initialSnapshot()); + final updateMachine = UpdateMachine.fromInitialSnapshot( + store: store, initialSnapshot: eg.initialSnapshot()); updateMachine.debugPauseLoop(); updateMachine.poll(); diff --git a/test/model/test_store.dart b/test/model/test_store.dart index 76cc82db4e..7045994837 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -124,8 +124,7 @@ mixin _DatabaseMixin on GlobalStore { /// /// Unlike with [LiveGlobalStore] and the associated [UpdateMachine.load], /// there is no automatic event-polling loop or other automated requests. -/// For each account loaded, there is a corresponding [UpdateMachine] -/// in [updateMachines], which tests can use for invoking that logic +/// Tests can use [PerAccountStore.updateMachine] in order to invoke that logic /// explicitly when desired. /// /// See also [TestZulipBinding.globalStore], which provides one of these. @@ -135,9 +134,6 @@ class TestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMi required super.accounts, }) : super(globalSettings: globalSettings ?? eg.globalSettings()); - /// A corresponding [UpdateMachine] for each loaded account. - final Map updateMachines = {}; - final Map _initialSnapshots = {}; static const Duration removeAccountDuration = Duration(milliseconds: 1); @@ -174,7 +170,7 @@ class TestGlobalStore extends GlobalStore with _ApiConnectionsMixin, _DatabaseMi accountId: accountId, initialSnapshot: initialSnapshot, ); - updateMachines[accountId] = UpdateMachine.fromInitialSnapshot( + UpdateMachine.fromInitialSnapshot( store: store, initialSnapshot: initialSnapshot); return Future.value(store); } From bd8554ab5e9f6bdfe1203a558cd243e94e210e51 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 7 Mar 2025 20:20:31 -0800 Subject: [PATCH 08/10] store test [nfc]: Clarify what prepareReload is doing --- test/example_data.dart | 9 +++++++-- test/model/store_test.dart | 12 ++++++++---- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 97527f42db..03cabbda97 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -981,9 +981,14 @@ PerAccountStore store({ } const _store = store; -UpdateMachine updateMachine({Account? account, InitialSnapshot? initialSnapshot}) { +UpdateMachine updateMachine({ + GlobalStore? globalStore, + Account? account, + InitialSnapshot? initialSnapshot, +}) { initialSnapshot ??= _initialSnapshot(); - final store = _store(account: account, initialSnapshot: initialSnapshot); + final store = _store(globalStore: globalStore, + account: account, initialSnapshot: initialSnapshot); return UpdateMachine.fromInitialSnapshot( store: store, initialSnapshot: initialSnapshot); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 4107995da5..780cc00480 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -999,16 +999,20 @@ void main() { Future 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); - final store = eg.store(globalStore: globalStore, account: eg.selfAccount); completers().single.complete(store); await future; completers().clear(); - final updateMachine = UpdateMachine.fromInitialSnapshot( - store: store, initialSnapshot: eg.initialSnapshot()); + updateMachine.debugPauseLoop(); updateMachine.poll(); - (store.connection as FakeApiConnection).prepare( apiException: eg.apiExceptionBadEventQueueId()); updateMachine.debugAdvanceLoop(); From ef8664d7fa9ed0a8b89f6d38bb5897c6d8fc9b06 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 27 Feb 2025 18:35:24 -0800 Subject: [PATCH 09/10] store test: Add and use UpdateMachineTestGlobalStore, for less mocking --- test/api/fake_api.dart | 5 ++ test/model/store_test.dart | 105 +++++++++++++++++++++---------------- test/model/test_store.dart | 66 ++++++++++++++++++++++- 3 files changed, 131 insertions(+), 45 deletions(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 6953515c6b..2b230376ac 100644 --- a/test/api/fake_api.dart +++ b/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 { + Subject get isOpen => has((x) => x.isOpen, 'isOpen'); +} diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 780cc00480..92e1c2d34c 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -158,17 +158,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(); 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 @@ -266,11 +274,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, @@ -284,13 +301,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(); + // 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); }); @@ -992,44 +1010,39 @@ void main() { }); group('UpdateMachine.poll reload failure', () { - late LoadingTestGlobalStore globalStore; - - List> completers() => - globalStore.completers[eg.selfAccount.id]!; - - Future 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 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 = prepareRegisterQueueResponse; + // 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: (connection) { + connection.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); @@ -1038,15 +1051,19 @@ 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: (connection) { + connection.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); diff --git a/test/model/test_store.dart b/test/model/test_store.dart index 7045994837..7e11c62ee1 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -1,8 +1,11 @@ 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/database.dart'; import 'package:zulip/model/store.dart'; +import 'package:zulip/notifications/receive.dart'; import 'package:zulip/widgets/store.dart'; import '../api/fake_api.dart'; @@ -127,7 +130,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({ GlobalSettingsData? globalSettings, @@ -176,6 +182,64 @@ 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({ + GlobalSettingsData? globalSettings, + required super.accounts, + }) : super(globalSettings: globalSettings ?? eg.globalSettings()); + + // [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 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 addUser(User user) async { await handleEvent(RealmUserAddEvent(id: 1, person: user)); From 0922ca5dd52de5b18f1b05e84b8d5e59d7d8575f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 26 Feb 2025 16:59:05 -0800 Subject: [PATCH 10/10] store: Have UpdateMachine.load check account still exists after async gaps Fixes: #1354 --- lib/model/store.dart | 56 +++++++++++++++++--------- test/model/store_test.dart | 80 +++++++++++++++++++++++++++++++------- 2 files changed, 105 insertions(+), 31 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index efe9c52a7c..05a9faabf3 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -175,6 +175,9 @@ abstract class GlobalStore extends ChangeNotifier { /// /// The account for `accountId` must exist. /// + /// Throws [AccountNotFoundException] if after any async gap + /// the account has been removed. + /// /// This method should be called only by the implementation of [perAccount]. /// Other callers interested in per-account data should use [perAccount] /// and/or [perAccountSync]. @@ -189,31 +192,20 @@ abstract class GlobalStore extends ChangeNotifier { // The API key is invalid and the store can never be loaded // unless the user retries manually. final account = getAccount(accountId); - if (account == null) { - // The account was logged out during `await doLoadPerAccount`. - // Here, that seems possible only by the user's own action; - // the logout can't have been done programmatically. - // Even if it were, it would have come with its own UI feedback. - // Anyway, skip showing feedback, to not be confusing or repetitive. - throw AccountNotFoundException(); - } + assert(account != null); // doLoadPerAccount would have thrown AccountNotFoundException final zulipLocalizations = GlobalLocalizations.zulipLocalizations; reportErrorToUserModally( zulipLocalizations.errorCouldNotConnectTitle, message: zulipLocalizations.errorInvalidApiKeyMessage( - account.realmUrl.toString())); + account!.realmUrl.toString())); await logOutAccount(this, accountId); throw AccountNotFoundException(); default: rethrow; } } - if (!_accounts.containsKey(accountId)) { - // TODO(#1354): handle this earlier - // [removeAccount] was called during [doLoadPerAccount]. - store.dispose(); - throw AccountNotFoundException(); - } + // doLoadPerAccount would have thrown AccountNotFoundException + assert(_accounts.containsKey(accountId)); return store; } @@ -221,6 +213,9 @@ abstract class GlobalStore extends ChangeNotifier { /// /// The account for `accountId` must exist. /// + /// Throws [AccountNotFoundException] if after any async gap + /// the account has been removed. + /// /// This method should be called only by [loadPerAccount]. Future doLoadPerAccount(int accountId); @@ -956,13 +951,26 @@ class UpdateMachine { /// /// The account for `accountId` must exist. /// + /// Throws [AccountNotFoundException] if after any async gap + /// the account has been removed. + /// /// In the future this might load an old snapshot from local storage first. static Future load(GlobalStore globalStore, int accountId) async { Account account = globalStore.getAccount(accountId)!; final connection = globalStore.apiConnectionFromAccount(account); + void stopAndThrowIfNoAccount() { + final account = globalStore.getAccount(accountId); + if (account == null) { + assert(debugLog('Account logged out during UpdateMachine.load')); + connection.close(); + throw AccountNotFoundException(); + } + } + final stopwatch = Stopwatch()..start(); - final initialSnapshot = await _registerQueueWithRetry(connection); + final initialSnapshot = await _registerQueueWithRetry(connection, + stopAndThrowIfNoAccount: stopAndThrowIfNoAccount); final t = (stopwatch..stop()).elapsed; assert(debugLog("initial fetch time: ${t.inMilliseconds}ms")); @@ -1004,13 +1012,20 @@ class UpdateMachine { bool _disposed = false; + /// Make the register-queue request, with retries. + /// + /// After each async gap, calls [stopAndThrowIfNoAccount]. static Future _registerQueueWithRetry( - ApiConnection connection) async { + ApiConnection connection, { + required void Function() stopAndThrowIfNoAccount, + }) async { BackoffMachine? backoffMachine; while (true) { + InitialSnapshot? result; try { - return await registerQueue(connection); + result = await registerQueue(connection); } catch (e, s) { + stopAndThrowIfNoAccount(); // TODO(#890): tell user if initial-fetch errors persist, or look non-transient switch (e) { case HttpException(httpStatus: 401): @@ -1025,8 +1040,13 @@ class UpdateMachine { } assert(debugLog('Backing off, then will retry…')); await (backoffMachine ??= BackoffMachine()).wait(); + stopAndThrowIfNoAccount(); assert(debugLog('… Backoff wait complete, retrying initial fetch.')); } + if (result != null) { + stopAndThrowIfNoAccount(); + return result; + } } } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 92e1c2d34c..4bcb272e9e 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -6,6 +6,7 @@ import 'package:fake_async/fake_async.dart'; import 'package:flutter/foundation.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/backoff.dart'; import 'package:zulip/api/core.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; @@ -157,6 +158,42 @@ void main() { await check(future).throws(); })); + test('GlobalStore.perAccount loading succeeds', () => awaitFakeAsync((async) async { + NotificationService.instance.token = ValueNotifier('asdf'); + addTearDown(NotificationService.debugReset); + + final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]); + final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection; + final future = globalStore.perAccount(eg.selfAccount.id); + check(connection.takeRequests()).length.equals(1); // register request + + await future; + // poll, server-emoji-data, register-token requests + check(connection.takeRequests()).length.equals(3); + check(connection).isOpen.isTrue(); + })); + + test('GlobalStore.perAccount account is logged out while loading; then succeeds', () => awaitFakeAsync((async) async { + final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]); + globalStore.prepareRegisterQueueResponse = (connection) => + connection.prepare( + delay: TestGlobalStore.removeAccountDuration + Duration(seconds: 1), + json: eg.initialSnapshot().toJson()); + 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); + + await check(future).throws(); + check(globalStore.takeDoRemoveAccountCalls()).isEmpty(); + // no poll, server-emoji-data, or register-token requests + check(connection.takeRequests()).isEmpty(); + check(connection).isOpen.isFalse(); + })); + test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async { final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]); globalStore.prepareRegisterQueueResponse = (connection) => @@ -175,8 +212,31 @@ void main() { check(globalStore.takeDoRemoveAccountCalls()).isEmpty(); // no poll, server-emoji-data, or register-token requests check(connection.takeRequests()).isEmpty(); - // TODO(#1354) uncomment - // check(connection).isOpen.isFalse(); + check(connection).isOpen.isFalse(); + })); + + test('GlobalStore.perAccount account is logged out during transient-error backoff', () => awaitFakeAsync((async) async { + final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]); + globalStore.prepareRegisterQueueResponse = (connection) => + connection.prepare( + delay: Duration(seconds: 1), + httpException: http.ClientException('Oops')); + final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection; + final future = globalStore.perAccount(eg.selfAccount.id); + BackoffMachine.debugDuration = Duration(seconds: 1); + async.elapse(Duration(milliseconds: 1500)); + check(connection.takeRequests()).length.equals(1); // register request + + assert(TestGlobalStore.removeAccountDuration < Duration(milliseconds: 500)); + await logOutAccount(globalStore, eg.selfAccount.id); + check(globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + await check(future).throws(); + check(globalStore.takeDoRemoveAccountCalls()).isEmpty(); + // no retry-register, poll, server-emoji-data, or register-token requests + check(connection.takeRequests()).isEmpty(); + check(connection).isOpen.isFalse(); })); // TODO test insertAccount @@ -278,10 +338,10 @@ void main() { checkGlobalStore(globalStore, eg.selfAccount.id, expectAccount: true, expectStore: false); - // assert(globalStore.useCachedApiConnections); + 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; + final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection; globalStore.prepareRegisterQueueResponse = (connection) { connection.prepare( @@ -301,14 +361,11 @@ void main() { expectAccount: false, expectStore: false); check(notifyCount).equals(1); - // Actually throws a null-check error; that's the bug #1354. - // TODO(#1354) should specifically throw AccountNotFoundException - await check(loadingFuture).throws(); + 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(connection).isOpen.isFalse(); check(globalStore.debugNumPerAccountStoresLoading).equals(0); }); @@ -1051,10 +1108,7 @@ 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, prepareRegisterQueueResponse: (connection) {