Rewrite logOutAccount actions test to use realistic routes

[PIG208]

This rewrite ended being a bit more substantial than just switching both
accounts initial routes to the result of MessageListPage.buildRoute().

We do not use popUtil because the navigator stack normally should not
become empty, so the HomePage route for account1 stays. Additionally,
because we are pushing a different page route, we no longer need to set
up different messages as the discriminator, further simplifying the
test.

This is rebased on top of #1235, and taken from #1183.

See also: #1183 (comment)

[PIG208]

It would be possible to use two MessageListPage, but there were some issues that
need to be resolved.

A partially done implementation might look like this:

diff

diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart
index e75bc84df..dd30b062c 100644
--- a/test/widgets/actions_test.dart
+++ b/test/widgets/actions_test.dart
@@ -19,6 +19,7 @@ import 'package:zulip/notifications/receive.dart';
 import 'package:zulip/widgets/actions.dart';
 import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/inbox.dart';
+import 'package:zulip/widgets/message_list.dart';
 import 'package:zulip/widgets/page.dart';
 import 'package:zulip/widgets/store.dart';
 
@@ -157,14 +158,15 @@ void main() {
     });
 
     testWidgets("logged-out account's routes removed from nav; other accounts' remain", (tester) async {
-      Future<void> makeUnreadTopicInInbox(int accountId, String topic) async {
+      Future<StreamMessage> prepareMessage(int accountId, String topic, String content) async {
         final stream = eg.stream();
-        final message = eg.streamMessage(stream: stream, topic: topic);
+        final message = eg.streamMessage(stream: stream, topic: topic, content: content);
         final store = await testBinding.globalStore.perAccount(accountId);
         await store.addStream(stream);
         await store.addSubscription(eg.subscription(stream));
         await store.addMessage(message);
         await tester.pump();
+        return message;
       }
 
       addTearDown(testBinding.reset);
@@ -181,25 +183,29 @@ void main() {
       navigator.popUntil((_) => false); // clear starting routes
       await tester.pumpAndSettle();
 
+      final account1Message = await prepareMessage(account1.id, 'topic in account1', 'content in account1');
+      final findAccount1PageContent = find.text('content in account1', skipOffstage: false);
+
+      final account2Message = await prepareMessage(account2.id, 'topic in account2', 'content in account2');
+      final findAccount2PageContent = find.text('content in account2', skipOffstage: false);
+
       final pushedRoutes = <Route<dynamic>>[];
       testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route);
       // TODO: switch to a realistic setup:
       //   https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363
-      final account1Route = MaterialAccountWidgetRoute(
-        accountId: account1.id, page: const InboxPageBody());
-      final account2Route = MaterialAccountWidgetRoute(
-        accountId: account2.id, page: const InboxPageBody());
+      final account1Route = MessageListPage.buildRoute(accountId: account1.id, narrow: const CombinedFeedNarrow());
+      final account2Route = MessageListPage.buildRoute(accountId: account2.id, narrow: const CombinedFeedNarrow());
+      (testBinding.globalStore.perAccountSync(account1.id)!.connection as FakeApiConnection)
+        .prepare(json: eg.newestGetMessagesResult(
+          foundOldest: true, messages: [account1Message]).toJson());
       unawaited(navigator.push(account1Route));
+      (testBinding.globalStore.perAccountSync(account2.id)!.connection as FakeApiConnection)
+        .prepare(json: eg.newestGetMessagesResult(
+          foundOldest: true, messages: [account2Message]).toJson());
       unawaited(navigator.push(account2Route));
       await tester.pumpAndSettle();
       check(pushedRoutes).deepEquals([account1Route, account2Route]);
 
-      await makeUnreadTopicInInbox(account1.id, 'topic in account1');
-      final findAccount1PageContent = find.text('topic in account1', skipOffstage: false);
-
-      await makeUnreadTopicInInbox(account2.id, 'topic in account2');
-      final findAccount2PageContent = find.text('topic in account2', skipOffstage: false);
-
       final findLoadingPage = find.byType(LoadingPlaceholderPage, skipOffstage: false);
 
       check(findAccount1PageContent).findsOne();

But there are some remaining issues, most notably, it's failing an assertion by the navigator:

'_history.isNotEmpty': is not true.

══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following assertion was thrown building
Navigator-[LabeledGlobalKey<NavigatorState>#8845c](dependencies: [HeroControllerScope,
InheritedCupertinoTheme, UnmanagedRestorationScope, _InheritedTheme,
_LocalizationsScope-[GlobalKey#199bf]], state: NavigatorState#e4b98(tickers: tracking 0 tickers)):
'package:flutter/src/widgets/navigator.dart': Failed assertion: line 5860 pos 12:
'_history.isNotEmpty': is not true.

Either the assertion indicates an error in the framework itself, or we should provide substantially
more information in this error message to help you determine and fix the underlying cause.
In either case, please report this assertion by filing a bug on GitHub:
  https://github.com/flutter/flutter/issues/new?template=2_bug.yml

The relevant error-causing widget was:
  MaterialApp MaterialApp:file:///home/zixuan/zulip-stuff/zulip-flutter/lib/widgets/app.dart:183:16

When the exception was thrown, this was the stack:
#2      NavigatorState.build (package:flutter/src/widgets/navigator.dart:5860:12)
#3      StatefulElement.build (package:flutter/src/widgets/framework.dart:5841:27)
#4      ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:5733:15)
#5      StatefulElement.performRebuild (package:flutter/src/widgets/framework.dart:5892:11)
#6      Element.rebuild (package:flutter/src/widgets/framework.dart:5445:7)
#7      StatefulElement.update (package:flutter/src/widgets/framework.dart:5917:5)
[...]
#282    TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:74:41)
#283    WidgetTester.pump (package:flutter_test/src/widget_tester.dart:653:27)
#284    main.<anonymous closure>.<anonymous closure>.prepareMessage (file:///home/zixuan/zulip-stuff/zulip-flutter/test/widgets/actions_test.dart:168:22)
<asynchronous suspension>
#285    main.<anonymous closure>.<anonymous closure> (file:///home/zixuan/zulip-stuff/zulip-flutter/test/widgets/actions_test.dart:189:31)
<asynchronous suspension>
#286    testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:193:15)
<asynchronous suspension>
#287    TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1064:5)
<asynchronous suspension>
<asynchronous suspension>
(elided 7 frames from class _AssertionError, dart:async, and package:stack_trace)

This motivated me to avoid clearing the navigator stack in the first place, and write the test to set up a HomePage for account1 and a MessageListPage for account2.

[chrisbobbe]

LGTM, thanks! Marking for Greg's review.

[gnprice]

Thanks. Some initial comments below.

This feels surprisingly complex. Let's hold off investing further engineering into it until we've completed #1183, which seems to have overlapping work and is a priority.

[gnprice]

login [nfc]: Pass GlobalStore to logOutAccount

This allows us to call it from model code when GlobalStore is available.

Hmm. If we were going to call this from model code (i.e. lib/model/), it should move out of lib/widgets/ — in general widgets should be importing models but not the other way around.

It doesn't look like later commits actually do that, though — the only call site that stops using a BuildContext for the call is in this function's own tests. So this change can just be dropped.

[gnprice]

To get a context if there isn't a more direct way at hand:

      final future = logOutAccount(ZulipApp.navigatorKey.currentContext!, account1.id);

Oh hey or in this case:

Suggested change

	final future = logOutAccount(testBinding.globalStore, account1.id);
	final future = logOutAccount(navigator.context, account1.id);

since this code has already accessed ZulipApp.navigator.

[PIG208]

Ended up leaving this unchanged and access the context the old way, because we no longer need to use the navigator.

[gnprice]

This still doesn't seem realistic — how would you get to a CombinedFeedNarrow for account 2 atop a HomePage for account 1?

[PIG208]

How about using openNotification to get a MessageListPage route with a TopicMessage narrow?

[PIG208]

Updated the PR to make openNotification public, dropped the logOutAccount refactor, and use notification to set up the navigator stack. No need to prioritize reviewing this, though, before we complete #1183.