From 78999f27aea7b67aca546833a5543edd86ac16bf Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 10 Sep 2024 12:57:44 -0400 Subject: [PATCH 01/14] notif test: Call testBinding.reset consistently testBinding.globalStore gets modified by multiple tests here. Signed-off-by: Zixuan James Li --- test/notifications/display_test.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 0264ff1acd..55d5d0d436 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -856,12 +856,14 @@ void main() { } testWidgets('stream message', (tester) async { + addTearDown(testBinding.reset); testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage()); }); testWidgets('direct message', (tester) async { + addTearDown(testBinding.reset); testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await checkOpenNotification(tester, eg.selfAccount, @@ -875,6 +877,7 @@ void main() { }); testWidgets('mismatching account', (tester) async { + addTearDown(testBinding.reset); testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await openNotification(tester, eg.otherAccount, eg.streamMessage()); @@ -882,6 +885,7 @@ void main() { }); testWidgets('find account among several', (tester) async { + addTearDown(testBinding.reset); final realmUrlA = Uri.parse('https://a-chat.example/'); final realmUrlB = Uri.parse('https://chat-b.example/'); final user1 = eg.user(); @@ -904,6 +908,7 @@ void main() { }); testWidgets('wait for app to become ready', (tester) async { + addTearDown(testBinding.reset); testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester, early: true); final message = eg.streamMessage(); @@ -923,6 +928,7 @@ void main() { }); testWidgets('at app launch', (tester) async { + addTearDown(testBinding.reset); // Set up a value for `getNotificationLaunchDetails` to return. final account = eg.selfAccount; final message = eg.streamMessage(); From f072be1c70a221987ae335d2478ee9c24f9497df Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 12 Sep 2024 00:21:22 -0400 Subject: [PATCH 02/14] dart [nfc]: Return unawaited futures if possible Signed-off-by: Zixuan James Li --- lib/model/autocomplete.dart | 2 +- lib/widgets/message_list.dart | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 1b2b075283..9222a817a5 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -615,7 +615,7 @@ class TopicAutocompleteView extends AutocompleteView e.name); _isFetching = false; - if (_query != null) _startSearch(); + if (_query != null) return _startSearch(); } @override diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b53af058d6..29bd244fdb 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -601,11 +601,11 @@ class ScrollToBottomButton extends StatelessWidget { final ValueNotifier visibleValue; final ScrollController scrollController; - Future _navigateToBottom() async { + Future _navigateToBottom() { final distance = scrollController.position.pixels; final durationMsAtSpeedLimit = (1000 * distance / 8000).ceil(); final durationMs = max(300, durationMsAtSpeedLimit); - scrollController.animateTo( + return scrollController.animateTo( 0, duration: Duration(milliseconds: durationMs), curve: Curves.ease); From 944f637329401d379a8f1393bc4f55b2e23ed183 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 12 Sep 2024 00:24:23 -0400 Subject: [PATCH 03/14] nav [nfc]: Mark unawaited futures from Navigator.push The future Navgiator.push returns completes when the there is a corresponding Navigator.pop call. We don't use that as of now, so we use unawaited to explicitly discard it. Other notes: * _onSubmitted doesn't need to return a Future because we don't intend to call it with await. * This removes an unnecessary return in _navigateForNotification at the end of the method. It was added in commit 681a744443f2381e05491956a22a5edc14ed3551. Signed-off-by: Zixuan James Li --- lib/notifications/display.dart | 6 +++--- lib/widgets/content.dart | 6 ++++-- lib/widgets/login.dart | 12 +++++++----- test/widgets/lightbox_test.dart | 4 ++-- test/widgets/login_test.dart | 4 +++- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index cc1d9b11c3..28fe5be646 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -377,10 +378,9 @@ class NotificationDisplayManager { assert(debugLog(' account: $account, narrow: $narrow')); // TODO(nav): Better interact with existing nav stack on notif open - navigator.push(MaterialAccountWidgetRoute(accountId: account.id, + unawaited(navigator.push(MaterialAccountWidgetRoute(accountId: account.id, // TODO(#82): Open at specific message, not just conversation - page: MessageListPage(initNarrow: narrow))); - return; + page: MessageListPage(initNarrow: narrow)))); } static Future _fetchBitmap(Uri url) async { diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index f7918103fc..3ca200233e 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/cupertino.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; @@ -1213,9 +1215,9 @@ void _launchUrl(BuildContext context, String urlString) async { final internalNarrow = parseInternalLink(url, store); if (internalNarrow != null) { - Navigator.push(context, + unawaited(Navigator.push(context, MessageListPage.buildRoute(context: context, - narrow: internalNarrow)); + narrow: internalNarrow))); return; } diff --git a/lib/widgets/login.dart b/lib/widgets/login.dart index 0a05478be8..21d006a2b5 100644 --- a/lib/widgets/login.dart +++ b/lib/widgets/login.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; @@ -142,7 +144,7 @@ class _AddAccountPageState extends State { super.dispose(); } - Future _onSubmitted(BuildContext context) async { + void _onSubmitted(BuildContext context) async { final zulipLocalizations = ZulipLocalizations.of(context); final url = _parseResult.url; final error = _parseResult.error; @@ -184,8 +186,8 @@ class _AddAccountPageState extends State { return; } - Navigator.push(context, - LoginPage.buildRoute(serverSettings: serverSettings)); + unawaited(Navigator.push(context, + LoginPage.buildRoute(serverSettings: serverSettings))); } finally { setState(() { _inProgress = false; @@ -393,9 +395,9 @@ class _LoginPageState extends State { return; } - Navigator.of(context).pushAndRemoveUntil( + unawaited(Navigator.of(context).pushAndRemoveUntil( HomePage.buildRoute(accountId: accountId), - (route) => (route is! _LoginSequenceRoute), + (route) => (route is! _LoginSequenceRoute)), ); } diff --git a/test/widgets/lightbox_test.dart b/test/widgets/lightbox_test.dart index 003223ce03..4a84f79b27 100644 --- a/test/widgets/lightbox_test.dart +++ b/test/widgets/lightbox_test.dart @@ -213,14 +213,14 @@ void main() { await tester.pumpWidget(const ZulipApp()); await tester.pump(); final navigator = await ZulipApp.navigator; - navigator.push(getImageLightboxRoute( + unawaited(navigator.push(getImageLightboxRoute( accountId: eg.selfAccount.id, message: message ?? eg.streamMessage(), src: src, thumbnailUrl: thumbnailUrl, originalHeight: null, originalWidth: null, - )); + ))); await tester.pump(); // per-account store await tester.pump(const Duration(milliseconds: 301)); // nav transition } diff --git a/test/widgets/login_test.dart b/test/widgets/login_test.dart index e0cbde87cf..70fa959b24 100644 --- a/test/widgets/login_test.dart +++ b/test/widgets/login_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; @@ -90,7 +92,7 @@ void main() { await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); await tester.pump(); final navigator = await ZulipApp.navigator; - navigator.push(LoginPage.buildRoute(serverSettings: serverSettings)); + unawaited(navigator.push(LoginPage.buildRoute(serverSettings: serverSettings))); await tester.pumpAndSettle(); takeStartingRoutes(); check(pushedRoutes).isEmpty(); From b5049333b673b959f67150a02e2147cc1955ab83 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 12 Sep 2024 00:31:01 -0400 Subject: [PATCH 04/14] action_sheet [nfc]: Mark unawaited future from markNarrowAsUnreadFromMessage Signed-off-by: Zixuan James Li --- lib/widgets/action_sheet.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 91ea939419..8442160d40 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; @@ -303,7 +305,7 @@ class MarkAsUnreadButton extends MessageActionSheetMenuItemButton { @override void onPressed(BuildContext context) async { Navigator.of(context).pop(); - markNarrowAsUnreadFromMessage(messageListContext, message, narrow); + unawaited(markNarrowAsUnreadFromMessage(messageListContext, message, narrow)); } } From f5d3898a3fc0ffe409f8a7664f5f627837056df3 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 12 Sep 2024 00:38:55 -0400 Subject: [PATCH 05/14] store [nfc]: Mark unawaited future from unused Map.remove return value Signed-off-by: Zixuan James Li --- lib/model/store.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 8cc25f2759..b099fcab5a 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -121,7 +121,7 @@ abstract class GlobalStore extends ChangeNotifier { _perAccountStoresLoading[accountId] = future; store = await future; _setPerAccount(accountId, store); - _perAccountStoresLoading.remove(accountId); + unawaited(_perAccountStoresLoading.remove(accountId)); return store; } From c3adcf2198ec7a57a9bb29035a04a2890ffb8669 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 27 Sep 2024 19:25:52 -0400 Subject: [PATCH 06/14] notif test [nfc]: Make receiveFcmMessage synchronous The test helper receiveFcmMessage doesn't need to be asychronous given what it currently does. Signed-off-by: Zixuan James Li --- test/notifications/display_test.dart | 68 ++++++++++++++-------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 55d5d0d436..383f8ae108 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -320,7 +320,7 @@ void main() { expectedTagComponent: expectedTagComponent); } - Future receiveFcmMessage(FakeAsync async, FcmMessage data) async { + void receiveFcmMessage(FakeAsync async, FcmMessage data) { testBinding.firebaseMessaging.onMessage.add( RemoteMessage(data: data.toJson())); async.flushMicrotasks(); @@ -372,21 +372,21 @@ void main() { final expectedTitle = '#${stream.name} > $topic'; final expectedTagComponent = 'stream:${stream.streamId}:$topic'; - await receiveFcmMessage(async, data1); + receiveFcmMessage(async, data1); checkNotification(data1, messageStyleMessages: [data1], expectedIsGroupConversation: true, expectedTitle: expectedTitle, expectedTagComponent: expectedTagComponent); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data2); checkNotification(data2, messageStyleMessages: [data1, data2], expectedIsGroupConversation: true, expectedTitle: expectedTitle, expectedTagComponent: expectedTagComponent); - await receiveFcmMessage(async, data3); + receiveFcmMessage(async, data3); checkNotification(data3, messageStyleMessages: [data1, data2, data3], expectedIsGroupConversation: true, @@ -406,21 +406,21 @@ void main() { final message3 = eg.streamMessage(topic: topicA, stream: stream); final data3 = messageFcmMessage(message3, streamName: stream.name); - await receiveFcmMessage(async, data1); + receiveFcmMessage(async, data1); checkNotification(data1, messageStyleMessages: [data1], expectedIsGroupConversation: true, expectedTitle: '#${stream.name} > $topicA', expectedTagComponent: 'stream:${stream.streamId}:$topicA'); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data2); checkNotification(data2, messageStyleMessages: [data2], expectedIsGroupConversation: true, expectedTitle: '#${stream.name} > $topicB', expectedTagComponent: 'stream:${stream.streamId}:$topicB'); - await receiveFcmMessage(async, data3); + receiveFcmMessage(async, data3); checkNotification(data3, messageStyleMessages: [data1, data3], expectedIsGroupConversation: true, @@ -435,7 +435,7 @@ void main() { final message1 = eg.streamMessage(topic: topic, stream: stream); final data1 = messageFcmMessage(message1, streamName: stream.name); - await receiveFcmMessage(async, data1); + receiveFcmMessage(async, data1); checkNotification(data1, messageStyleMessages: [data1], expectedIsGroupConversation: true, @@ -446,7 +446,7 @@ void main() { final message2 = eg.streamMessage(topic: topic, stream: stream); final data2 = messageFcmMessage(message2, streamName: stream.name); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data2); checkNotification(data2, messageStyleMessages: [data1, data2], expectedIsGroupConversation: true, @@ -492,14 +492,14 @@ void main() { final expectedTagComponent = 'dm:${message1.allRecipientIds.join(",")}'; - await receiveFcmMessage(async, data1); + receiveFcmMessage(async, data1); checkNotification(data1, messageStyleMessages: [data1], expectedIsGroupConversation: true, expectedTitle: "${eg.otherUser.fullName} to you and 1 other", expectedTagComponent: expectedTagComponent); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data2); checkNotification(data2, messageStyleMessages: [data1, data2], expectedIsGroupConversation: true, @@ -524,7 +524,7 @@ void main() { final expectedTagComponent = 'dm:${message1.allRecipientIds.join(",")}'; - await receiveFcmMessage(async, data1); + receiveFcmMessage(async, data1); checkNotification(data1, messageStyleMessages: [data1], expectedIsGroupConversation: false, @@ -535,7 +535,7 @@ void main() { final message2 = eg.dmMessage(from: otherUser, to: [eg.selfUser]); final data2 = messageFcmMessage(message2); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data2); checkNotification(data2, messageStyleMessages: [data1, data2], expectedIsGroupConversation: false, @@ -551,7 +551,7 @@ void main() { final expectedTagComponent = 'dm:${message1.allRecipientIds.join(",")}'; - await receiveFcmMessage(async, data1); + receiveFcmMessage(async, data1); checkNotification(data1, messageStyleMessages: [data1], expectedIsGroupConversation: false, @@ -562,7 +562,7 @@ void main() { final message2 = eg.dmMessage(from: otherUser, to: [eg.selfUser]); final data2 = messageFcmMessage(message2); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data2); checkNotification(data2, messageStyleMessages: [data1, data2], expectedIsGroupConversation: false, @@ -575,7 +575,7 @@ void main() { await init(); final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); final data = messageFcmMessage(message); - await receiveFcmMessage(async, data); + receiveFcmMessage(async, data); checkNotification(data, messageStyleMessages: [data], expectedIsGroupConversation: false, @@ -591,7 +591,7 @@ void main() { await init(); final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); final data = messageFcmMessage(message); - await receiveFcmMessage(async, data); + receiveFcmMessage(async, data); checkNotification(data, messageStyleMessages: [data], expectedIsGroupConversation: false, @@ -620,7 +620,7 @@ void main() { check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); // Check on foreground event; onMessage - await receiveFcmMessage(async, data); + receiveFcmMessage(async, data); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data, 'stream:${message.streamId}:${message.topic}'), conditionSummaryActiveNotif(expectedGroupKey), @@ -631,7 +631,7 @@ void main() { check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); // Check on background event; onBackgroundMessage - await receiveFcmMessage(async, data); + receiveFcmMessage(async, data); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data, 'stream:${message.streamId}:${message.topic}'), conditionSummaryActiveNotif(expectedGroupKey), @@ -656,9 +656,9 @@ void main() { check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); - await receiveFcmMessage(async, data1); - await receiveFcmMessage(async, data2); - await receiveFcmMessage(async, data3); + receiveFcmMessage(async, data1); + receiveFcmMessage(async, data2); + receiveFcmMessage(async, data3); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data3, 'stream:${stream.streamId}:$topicA'), conditionSummaryActiveNotif(expectedGroupKey), @@ -690,8 +690,8 @@ void main() { check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); // Two notifications for different conversations; but same account. - await receiveFcmMessage(async, data1); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data1); + receiveFcmMessage(async, data2); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data1, 'stream:${stream.streamId}:$topicA'), conditionSummaryActiveNotif(expectedGroupKey), @@ -699,7 +699,7 @@ void main() { ]); // A RemoveFcmMessage for first conversation; only clears the first conversation notif. - await receiveFcmMessage(async, removeFcmMessage([message1])); + receiveFcmMessage(async, removeFcmMessage([message1])); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionSummaryActiveNotif(expectedGroupKey), conditionActiveNotif(data2, 'stream:${stream.streamId}:$topicB'), @@ -707,7 +707,7 @@ void main() { // Then a RemoveFcmMessage for the only remaining conversation; // clears both the conversation notif and summary notif. - await receiveFcmMessage(async, removeFcmMessage([message2])); + receiveFcmMessage(async, removeFcmMessage([message2])); check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); }))); @@ -737,8 +737,8 @@ void main() { check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); - await receiveFcmMessage(async, data1); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data1); + receiveFcmMessage(async, data2); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data1, 'stream:${stream.streamId}:$topic'), conditionSummaryActiveNotif(groupKey1), @@ -746,13 +746,13 @@ void main() { conditionSummaryActiveNotif(groupKey2), ]); - await receiveFcmMessage(async, removeFcmMessage([message1], account: account1)); + receiveFcmMessage(async, removeFcmMessage([message1], account: account1)); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data2, 'stream:${stream.streamId}:$topic'), conditionSummaryActiveNotif(groupKey2), ]); - await receiveFcmMessage(async, removeFcmMessage([message2], account: account2)); + receiveFcmMessage(async, removeFcmMessage([message2], account: account2)); check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); }))); @@ -776,8 +776,8 @@ void main() { check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); - await receiveFcmMessage(async, data1); - await receiveFcmMessage(async, data2); + receiveFcmMessage(async, data1); + receiveFcmMessage(async, data2); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data1, 'stream:${stream.streamId}:$topic'), conditionSummaryActiveNotif(groupKey1), @@ -785,13 +785,13 @@ void main() { conditionSummaryActiveNotif(groupKey2), ]); - await receiveFcmMessage(async, removeFcmMessage([message1], account: account1)); + receiveFcmMessage(async, removeFcmMessage([message1], account: account1)); check(testBinding.androidNotificationHost.activeNotifications).deepEquals(>[ conditionActiveNotif(data2, 'stream:${stream.streamId}:$topic'), conditionSummaryActiveNotif(groupKey2), ]); - await receiveFcmMessage(async, removeFcmMessage([message2], account: account2)); + receiveFcmMessage(async, removeFcmMessage([message2], account: account2)); check(testBinding.androidNotificationHost.activeNotifications).isEmpty(); }))); }); From c736e955123561aae554802b3097249c9215d66d Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 27 Sep 2024 19:20:26 -0400 Subject: [PATCH 07/14] test: Add await for store.handleEvent calls There may or may not be cases where later awaited calls to async functions effectively wait for store.handleEvent. We add await for store.handleEvent calls anyway for explicitness. Signed-off-by: Zixuan James Li --- test/model/channel_test.dart | 4 ++-- test/widgets/inbox_test.dart | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 444ee31a21..14a9f69ea3 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -264,7 +264,7 @@ void main() { await store.addSubscription( eg.subscription(stream1, isMuted: streamMuted)); } - store.handleEvent(mkEvent(oldPolicy)); + await store.handleEvent(mkEvent(oldPolicy)); final oldVisibleInStream = store.isTopicVisibleInStream(stream1.streamId, 'topic'); final oldVisible = store.isTopicVisible(stream1.streamId, 'topic'); @@ -272,7 +272,7 @@ void main() { final willChangeInStream = store.willChangeIfTopicVisibleInStream(event); final willChange = store.willChangeIfTopicVisible(event); - store.handleEvent(event); + await store.handleEvent(event); final newVisibleInStream = store.isTopicVisibleInStream(stream1.streamId, 'topic'); final newVisible = store.isTopicVisible(stream1.streamId, 'topic'); diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index 0c17fb0b9b..12e2c78800 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -486,7 +486,7 @@ void main() { .isNotNull().isSameColorAs(ChannelColorSwatch.light(initialColor).barBackground); final newColor = Colors.orange.argbInt; - store.handleEvent(SubscriptionUpdateEvent(id: 1, streamId: 1, + await store.handleEvent(SubscriptionUpdateEvent(id: 1, streamId: 1, property: SubscriptionProperty.color, value: newColor)); await tester.pump(); From bb21cb11d223df5f4fa4b8408919a44f8e873b33 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 27 Sep 2024 19:23:58 -0400 Subject: [PATCH 08/14] test: Add await for TestGlobalStore.add calls These tests didn't break previously because the GlobalStore.add calls finished before the per account store was ever needed. A notable example is the test "find account among several" in test/notifications/display_test.dart. The accounts were added in parallel, which actually allowed the GlobalStore.add calls to bypass the invariant check performed by TestGlobalStore. The test passed anyway because it only requires the accounts to be there when some other checks are performed. The invariant check is to ensure that no accounts have the same email address within a realm. To fix this, we can potentially maintain a store of accounts with TestGlobalStore, and check for the constraints there. Signed-off-by: Zixuan James Li --- test/model/test_store.dart | 1 + test/notifications/display_test.dart | 12 ++++++------ test/widgets/login_test.dart | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/model/test_store.dart b/test/model/test_store.dart index e3a7a5cae9..c3a03d50f8 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -80,6 +80,7 @@ class TestGlobalStore extends GlobalStore { // Check for duplication is typically handled by the database but since // we're not using a real database, this needs to be handled here. // See [AppDatabase.createAccount]. + // TODO: Ensure that parallel account insertions do not bypass this check. if (accounts.any((account) => data.realmUrl.value == account.realmUrl && (data.userId.value == account.userId diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 383f8ae108..cc7552877d 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -857,14 +857,14 @@ void main() { testWidgets('stream message', (tester) async { addTearDown(testBinding.reset); - testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage()); }); testWidgets('direct message', (tester) async { addTearDown(testBinding.reset); - testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await checkOpenNotification(tester, eg.selfAccount, eg.dmMessage(from: eg.otherUser, to: [eg.selfUser])); @@ -878,7 +878,7 @@ void main() { testWidgets('mismatching account', (tester) async { addTearDown(testBinding.reset); - testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await openNotification(tester, eg.otherAccount, eg.streamMessage()); check(pushedRoutes).isEmpty(); @@ -897,7 +897,7 @@ void main() { eg.account(id: 1004, realmUrl: realmUrlB, user: user2), ]; for (final account in accounts) { - testBinding.globalStore.add(account, eg.initialSnapshot()); + await testBinding.globalStore.add(account, eg.initialSnapshot()); } await prepare(tester); @@ -909,7 +909,7 @@ void main() { testWidgets('wait for app to become ready', (tester) async { addTearDown(testBinding.reset); - testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester, early: true); final message = eg.streamMessage(); await openNotification(tester, eg.selfAccount, message); @@ -939,7 +939,7 @@ void main() { NotificationAppLaunchDetails(true, notificationResponse: response); // Now start the app. - testBinding.globalStore.add(account, eg.initialSnapshot()); + await testBinding.globalStore.add(account, eg.initialSnapshot()); await prepare(tester, early: true); check(pushedRoutes).isEmpty(); // GlobalStore hasn't loaded yet diff --git a/test/widgets/login_test.dart b/test/widgets/login_test.dart index 70fa959b24..30f21689d4 100644 --- a/test/widgets/login_test.dart +++ b/test/widgets/login_test.dart @@ -161,7 +161,7 @@ void main() { final serverSettings = eg.serverSettings(); await prepare(tester, serverSettings); check(testBinding.globalStore.accounts).isEmpty(); - testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await tester.enterText(findUsernameInput, eg.selfAccount.email); await tester.enterText(findPasswordInput, 'p455w0rd'); From cc65cc695babff0ef992a98d8105e4fc3e4aa478 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 27 Sep 2024 19:24:39 -0400 Subject: [PATCH 09/14] store test: Add await for checking Future.throws These tests actually worked as expected, even though we didn't await for the Future returned by FutureChecks.throws. This is because checks ensure that the asychronous expectations are complete before the test ends. Nonetheless, we should use await so that our intention is explicit (i.e. the future completes with an error as opposed to the future never completes and can't be awaited). See also: https://github.com/dart-lang/test/tree/master/pkgs/checks#checking-asynchronous-expectations Signed-off-by: Zixuan James Li --- test/model/store_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 01ace52e33..d34b9d2dc6 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -155,11 +155,11 @@ void main() { test('reject changing id, realmUrl, or userId', () async { final globalStore = eg.globalStore(accounts: [eg.selfAccount]); - check(globalStore.updateAccount(eg.selfAccount.id, const AccountsCompanion( + await check(globalStore.updateAccount(eg.selfAccount.id, const AccountsCompanion( id: Value(1234)))).throws(); - check(globalStore.updateAccount(eg.selfAccount.id, AccountsCompanion( + await check(globalStore.updateAccount(eg.selfAccount.id, AccountsCompanion( realmUrl: Value(Uri.parse('https://other.example'))))).throws(); - check(globalStore.updateAccount(eg.selfAccount.id, const AccountsCompanion( + await check(globalStore.updateAccount(eg.selfAccount.id, const AccountsCompanion( userId: Value(1234)))).throws(); }); From 75eae91f228e7cc02f59d0bc30ad47c8247bd23c Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 27 Sep 2024 19:25:01 -0400 Subject: [PATCH 10/14] login test [nfc]: Add await for handlePlatformMessage The later `await tester.idle` call helps wait for the earlier mock platform message to be handled. We can remove that call and use await on the handlePlatformMessage call itself. Signed-off-by: Zixuan James Li --- test/widgets/login_test.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/widgets/login_test.dart b/test/widgets/login_test.dart index 30f21689d4..1867bb682f 100644 --- a/test/widgets/login_test.dart +++ b/test/widgets/login_test.dart @@ -221,10 +221,9 @@ void main() { final ByteData message = const JSONMethodCodec().encodeMethodCall( MethodCall('pushRouteInformation', {'location': url.toString()})); - tester.binding.defaultBinaryMessenger.handlePlatformMessage( + await tester.binding.defaultBinaryMessenger.handlePlatformMessage( 'flutter/navigation', message, null); - await tester.idle(); check(testBinding.takeCloseInAppWebViewCallCount()).equals(1); final account = testBinding.globalStore.accounts.single; From c1f16756d1d35d2f2bb779cc0f6ba2568e726a51 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 10 Sep 2024 13:08:16 -0400 Subject: [PATCH 11/14] test [nfc]: Mark unawaited future in tests These are function calls that need to happen asynchronously until we elapse the time with FakeAsync or WidgetTester. Signed-off-by: Zixuan James Li --- test/api/fake_api_test.dart | 10 ++++++---- test/model/store_test.dart | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/test/api/fake_api_test.dart b/test/api/fake_api_test.dart index 243757ace4..25b4bcff2e 100644 --- a/test/api/fake_api_test.dart +++ b/test/api/fake_api_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/exception.dart'; @@ -29,8 +31,8 @@ void main() { json: {'a': 3}); Map? result; - connection.get('aRoute', (json) => json, '/', null) - .then((r) { result = r; }); + unawaited(connection.get('aRoute', (json) => json, '/', null) + .then((r) { result = r; })); async.elapse(const Duration(seconds: 1)); check(result).isNull(); @@ -45,8 +47,8 @@ void main() { exception: Exception("oops")); Object? error; - connection.get('aRoute', (json) => null, '/', null) - .catchError((Object e) { error = e; }); + unawaited(connection.get('aRoute', (json) => null, '/', null) + .catchError((Object e) { error = e; })); async.elapse(const Duration(seconds: 1)); check(error).isNull(); diff --git a/test/model/store_test.dart b/test/model/store_test.dart index d34b9d2dc6..605cae0969 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -295,7 +295,7 @@ void main() { connection.prepare(exception: Exception('failed')); final future = UpdateMachine.load(globalStore, eg.selfAccount.id); bool complete = false; - future.whenComplete(() => complete = true); + unawaited(future.whenComplete(() => complete = true)); async.flushMicrotasks(); checkLastRequest(); check(complete).isFalse(); @@ -363,7 +363,7 @@ void main() { connection.prepare(exception: Exception('failed')); final future = updateMachine.fetchEmojiData(emojiDataUrl); bool complete = false; - future.whenComplete(() => complete = true); + unawaited(future.whenComplete(() => complete = true)); async.flushMicrotasks(); checkLastRequest(); check(complete).isFalse(); From 193a56c9bea2e8bb2b07932a5c9e15d9cc0b18fe Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 10 Oct 2024 18:44:55 -0700 Subject: [PATCH 12/14] api test [nfc]: Use finish helper for async checks This explicitly ensures that these checks complete before the test ends. We can keep these single-line helper calls without adding `await`'s to all of them. Signed-off-by: Zixuan James Li --- test/api/core_test.dart | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/api/core_test.dart b/test/api/core_test.dart index 2cbda4feb3..7da6fcdc54 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -64,15 +64,19 @@ void main() { }); test('send rejects off-realm URL (with default useAuth)', () async { - Future checkAllow(String realmUrl, String requestUrl) async { - check(await makeRequest(realmUrl, requestUrl)) - .isA() - .url.asString.equals(requestUrl); + void checkAllow(String realmUrl, String requestUrl) { + finish(() async { + check(await makeRequest(realmUrl, requestUrl)) + .isA() + .url.asString.equals(requestUrl); + }()); } - Future checkDeny(String realmUrl, String requestUrl) async { - await check(makeRequest(realmUrl, requestUrl)) - .throws(); + void checkDeny(String realmUrl, String requestUrl) async { + finish(() async { + await check(makeRequest(realmUrl, requestUrl)) + .throws(); + }()); } // Baseline: normal requests are allowed. From 3c0ed4979ae724261a123e57bc8e137ba8f4eada Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 10 Sep 2024 13:08:16 -0400 Subject: [PATCH 13/14] lint: Enable unawaited_futures Fixes-partly: #731 Signed-off-by: Zixuan James Li --- analysis_options.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/analysis_options.yaml b/analysis_options.yaml index 36f334d1b1..99cf190fad 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -25,3 +25,4 @@ linter: no_literal_bool_comparisons: true prefer_relative_imports: true unnecessary_statements: true + unawaited_futures: true From 1aef8681447559a173d27f5d5033725b8aed433d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Oct 2024 17:21:51 -0700 Subject: [PATCH 14/14] api test [nfc]: Simplify some unawaited checks The `finish` helper is only needed when the future that needs to finish isn't already one created by `check` (which takes care of the effect of `finish` for itself). --- test/api/core_test.dart | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/test/api/core_test.dart b/test/api/core_test.dart index 7da6fcdc54..f1f0e0d42f 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -65,18 +66,16 @@ void main() { test('send rejects off-realm URL (with default useAuth)', () async { void checkAllow(String realmUrl, String requestUrl) { - finish(() async { - check(await makeRequest(realmUrl, requestUrl)) - .isA() - .url.asString.equals(requestUrl); - }()); + // No need to await directly; `check` ensures the future completes + // before the enclosing test is considered complete. + unawaited(check(makeRequest(realmUrl, requestUrl)) + .completes((it) => it.isA() + .url.asString.equals(requestUrl))); } - void checkDeny(String realmUrl, String requestUrl) async { - finish(() async { - await check(makeRequest(realmUrl, requestUrl)) - .throws(); - }()); + void checkDeny(String realmUrl, String requestUrl) { + unawaited(check(makeRequest(realmUrl, requestUrl)) + .throws()); } // Baseline: normal requests are allowed. @@ -246,7 +245,7 @@ void main() { test('API network errors', () async { void checkRequest( T exception, Condition condition) { - finish(check(tryRequest(exception: exception)) + unawaited(check(tryRequest(exception: exception)) .throws((it) => it ..routeName.equals(kExampleRouteName) ..cause.equals(exception) @@ -300,7 +299,7 @@ void main() { void checkMalformed({ int httpStatus = 400, Map? json, String? body}) { assert((json == null) != (body == null)); - finish(check(tryRequest(httpStatus: httpStatus, json: json, body: body)) + unawaited(check(tryRequest(httpStatus: httpStatus, json: json, body: body)) .throws((it) => it ..routeName.equals(kExampleRouteName) ..httpStatus.equals(httpStatus)