From 427e5414456b3794c0580427eb372ceba0ab0fc6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 17 Jul 2025 18:12:02 -0700 Subject: [PATCH 1/5] action_sheet test: Avoid mutating shared example users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the same sort of state leak that caused #1712; this instance of it just hasn't happened to break any tests for us yet. We'll soon arrange things so that this sort of state-leaking mutation causes an immediate error. This is one of the three total places where it turns out we had such mutations, including the one we just fixed in a04b44ede (#1713). The second of these tests ("no error if recipient was deactivated …") wasn't actually mutating the shared example user `eg.otherUser`, because secretly `setupToMessageActionSheet` makes a new User object with the same user ID and puts that in the store. Still, it *looked* like it was; best to do something that clearly looks correct instead. The first of these tests was indeed mutating `eg.selfUser`, just as it looks like it's doing. --- test/widgets/action_sheet_test.dart | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 177aa43503..bc9b53711f 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -55,6 +55,7 @@ late FakeApiConnection connection; Future setupToMessageActionSheet(WidgetTester tester, { required Message message, required Narrow narrow, + User? selfUser, User? sender, List? mutedUserIds, bool? realmAllowMessageEditing, @@ -67,15 +68,17 @@ Future setupToMessageActionSheet(WidgetTester tester, { // TODO(#1667) will be null in a search narrow; remove `!`. assert(narrow.containsMessage(message)!); + selfUser ??= eg.selfUser; + final selfAccount = eg.account(user: selfUser); await testBinding.globalStore.add( - eg.selfAccount, + selfAccount, eg.initialSnapshot( realmAllowMessageEditing: realmAllowMessageEditing, realmMessageContentEditLimitSeconds: realmMessageContentEditLimitSeconds, )); - store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + store = await testBinding.globalStore.perAccount(selfAccount.id); await store.addUsers([ - eg.selfUser, + selfUser, sender ?? eg.user(userId: message.senderId), if (narrow is DmNarrow) ...narrow.otherRecipientIds.map((id) => eg.user(userId: id)), @@ -97,7 +100,7 @@ Future setupToMessageActionSheet(WidgetTester tester, { connection.prepare(json: eg.newestGetMessagesResult( foundOldest: true, messages: [message]).toJson()); - await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + await tester.pumpWidget(TestZulipApp(accountId: selfAccount.id, child: MessageListPage(initNarrow: narrow))); // global store, per-account store, and message list get loaded @@ -1204,11 +1207,13 @@ void main() { }); testWidgets('no error if user lost posting permission after action sheet opened', (tester) async { + final selfUser = eg.user(role: UserRole.member); final stream = eg.stream(); final message = eg.streamMessage(stream: stream); - await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + await setupToMessageActionSheet(tester, selfUser: selfUser, + message: message, narrow: TopicNarrow.ofMessage(message)); - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: eg.selfUser.userId, + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: selfUser.userId, role: UserRole.guest)); await store.handleEvent(eg.channelUpdateEvent(stream, property: ChannelPropertyName.channelPostPolicy, @@ -1240,7 +1245,8 @@ void main() { }); testWidgets('no error if recipient was deactivated while raw-content request in progress', (tester) async { - final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + final otherUser = eg.user(); + final message = eg.dmMessage(from: eg.selfUser, to: [otherUser]); await setupToMessageActionSheet(tester, message: message, narrow: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); @@ -1253,7 +1259,7 @@ void main() { await tapQuoteAndReplyButton(tester); await tester.pump(const Duration(seconds: 1)); // message not yet fetched - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: eg.otherUser.userId, + await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: otherUser.userId, isActive: false)); await tester.pump(); // no error From d1eaacb7db3ed0e36963ec3bcdf88944486cfd3e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 17 Jul 2025 18:23:25 -0700 Subject: [PATCH 2/5] recent-dms test: Avoid mutating eg.selfUser Like in the parent commit fixing some action-sheet tests, this is a state leak that just hasn't happened to break any tests for us yet. In this case, the fix can also simplify the interface that this prep helper `setupPage` has with its callers: instead of a specific feature for setting `fullName` on the self-user, the function can take an optional User object for the self-user. Then the individual test case can do whatever it likes to set up that User object. --- .../widgets/recent_dm_conversations_test.dart | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/test/widgets/recent_dm_conversations_test.dart b/test/widgets/recent_dm_conversations_test.dart index 3eb49f2ca8..e898218e6e 100644 --- a/test/widgets/recent_dm_conversations_test.dart +++ b/test/widgets/recent_dm_conversations_test.dart @@ -27,16 +27,18 @@ import 'test_app.dart'; Future setupPage(WidgetTester tester, { required List dmMessages, required List users, + User? selfUser, List? mutedUserIds, NavigatorObserver? navigatorObserver, - String? newNameForSelfUser, }) async { addTearDown(testBinding.reset); - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + selfUser ??= eg.selfUser; + final selfAccount = eg.account(user: selfUser); + await testBinding.globalStore.add(selfAccount, eg.initialSnapshot()); + final store = await testBinding.globalStore.perAccount(selfAccount.id); - await store.addUser(eg.selfUser); + await store.addUser(selfUser); for (final user in users) { await store.addUser(user); } @@ -46,13 +48,8 @@ Future setupPage(WidgetTester tester, { await store.addMessages(dmMessages); - if (newNameForSelfUser != null) { - await store.handleEvent(RealmUserUpdateEvent(id: 1, userId: eg.selfUser.userId, - fullName: newNameForSelfUser)); - } - await tester.pumpWidget(TestZulipApp( - accountId: eg.selfAccount.id, + accountId: selfAccount.id, navigatorObservers: navigatorObserver != null ? [navigatorObserver] : [], child: const HomePage())); @@ -187,7 +184,8 @@ void main() { } Future markMessageAsRead(WidgetTester tester, Message message) async { - final store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + final store = await testBinding.globalStore.perAccount( + testBinding.globalStore.accounts.single.id); await store.handleEvent(UpdateMessageFlagsAddEvent( id: 1, flag: MessageFlag.read, all: false, messages: [message.id])); await tester.pump(); @@ -216,18 +214,18 @@ void main() { }); testWidgets('short name takes one line', (tester) async { - final message = eg.dmMessage(from: eg.selfUser, to: []); const name = 'Short name'; - await setupPage(tester, users: [], dmMessages: [message], - newNameForSelfUser: name); + final selfUser = eg.user(fullName: name); + await setupPage(tester, selfUser: selfUser, users: [], + dmMessages: [eg.dmMessage(from: selfUser, to: [])]); checkTitle(tester, name, 1); }); testWidgets('very long name takes two lines (must be ellipsized)', (tester) async { - final message = eg.dmMessage(from: eg.selfUser, to: []); const name = 'Long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name long name'; - await setupPage(tester, users: [], dmMessages: [message], - newNameForSelfUser: name); + final selfUser = eg.user(fullName: name); + await setupPage(tester, selfUser: selfUser, users: [], + dmMessages: [eg.dmMessage(from: selfUser, to: [])]); checkTitle(tester, name, 2); }); From 19439b35439bae72e44c7c0d659067d49cb5e1fe Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 17 Jul 2025 18:00:53 -0700 Subject: [PATCH 3/5] test: Prevent mutation of shared example users This ensures we don't re-introduce the sort of state leak that caused #1712. In three recent commits we fixed every existing example of this sort of state leak; only three test files had them. I also looked through all the other top-level fields in this file `example_data.dart`. Most are immutable atoms like ints and strings. There are a handful of remaining points of mutability which could in principle allow this sort of state leak; so I'll tighten those up too in the next couple of commits. --- test/example_data.dart | 68 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 7 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index fc32781a1e..10b7876c12 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -17,6 +17,7 @@ import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/settings.dart'; import 'package:zulip/model/store.dart'; +import 'model/binding.dart'; import 'model/test_store.dart'; import 'stdlib_checks.dart'; @@ -289,24 +290,77 @@ Account account({ ); } -final User selfUser = user(fullName: 'Self User'); +/// A [User] which throws on attempting to mutate any of its fields. +/// +/// We use this to prevent any tests from leaking state through having a +/// [PerAccountStore] (which will be discarded when [TestZulipBinding.reset] +/// is called at the end of the test case) mutate a [User] in its [UserStore] +/// which happens to a value in this file like [selfUser] (which will not be +/// discarded by [TestZulipBinding.reset]). That was the cause of issue #1712. +class _ImmutableUser extends User { + _ImmutableUser.copyUser(User user) : super( + // When adding a field here, be sure to add the corresponding setter below. + userId: user.userId, + deliveryEmail: user.deliveryEmail, + email: user.email, + fullName: user.fullName, + dateJoined: user.dateJoined, + isActive: user.isActive, + isBillingAdmin: user.isBillingAdmin, + isBot: user.isBot, + botType: user.botType, + botOwnerId: user.botOwnerId, + role: user.role, + timezone: user.timezone, + avatarUrl: user.avatarUrl, + avatarVersion: user.avatarVersion, + profileData: user.profileData == null ? null : Map.unmodifiable(user.profileData!), + isSystemBot: user.isSystemBot, + // When adding a field here, be sure to add the corresponding setter below. + ); + + static final Error _error = UnsupportedError( + 'Cannot mutate immutable User.\n' + 'When a test needs to have the store handle an event which will\n' + 'modify a user, use `eg.user()` to make a fresh User object\n' + 'instead of using a shared User object like `eg.selfUser`.'); + + // userId already immutable + @override set deliveryEmail(_) => throw _error; + @override set email(_) => throw _error; + @override set fullName(_) => throw _error; + // dateJoined already immutable + @override set isActive(_) => throw _error; + @override set isBillingAdmin(_) => throw _error; + // isBot already immutable + // botType already immutable + @override set botOwnerId(_) => throw _error; + @override set role(_) => throw _error; + @override set timezone(_) => throw _error; + @override set avatarUrl(_) => throw _error; + @override set avatarVersion(_) => throw _error; + @override set profileData(_) => throw _error; + // isSystemBot already immutable +} + +final User selfUser = _ImmutableUser.copyUser(user(fullName: 'Self User')); +final User otherUser = _ImmutableUser.copyUser(user(fullName: 'Other User')); +final User thirdUser = _ImmutableUser.copyUser(user(fullName: 'Third User')); +final User fourthUser = _ImmutableUser.copyUser(user(fullName: 'Fourth User')); + +// There's no need for an [Account] analogue of [_ImmutableUser], +// because [Account] (which is generated by Drift) is already immutable. final Account selfAccount = account( id: 1001, user: selfUser, apiKey: 'dQcEJWTq3LczosDkJnRTwf31zniGvMrO', // A Zulip API key is 32 digits of base64. ); - -final User otherUser = user(fullName: 'Other User'); final Account otherAccount = account( id: 1002, user: otherUser, apiKey: '6dxT4b73BYpCTU+i4BB9LAKC5h/CufqY', // A Zulip API key is 32 digits of base64. ); -final User thirdUser = user(fullName: 'Third User'); - -final User fourthUser = user(fullName: 'Fourth User'); - //////////////////////////////////////////////////////////////// // Data attached to the self-account on the realm // From d8d19c83bc7552936cc094d9e72a46290ad30d15 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 17 Jul 2025 20:07:03 -0700 Subject: [PATCH 4/5] test [nfc]: Make all example-data globals final It's unlikely we would make the mistake of accidentally mutating these bindings; but best to rule it out. --- test/example_data.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 10b7876c12..61c363dde7 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -131,7 +131,7 @@ GetServerSettingsResult serverSettings({ ); } -ServerEmojiData serverEmojiDataPopular = ServerEmojiData(codeToNames: { +final ServerEmojiData serverEmojiDataPopular = ServerEmojiData(codeToNames: { '1f44d': ['+1', 'thumbs_up', 'like'], '1f389': ['tada'], '1f642': ['slight_smile'], @@ -158,7 +158,7 @@ ServerEmojiData serverEmojiDataPopularPlus(ServerEmojiData data) { /// /// zulip/zulip@9feba0f16f is a Server 11 commit. // TODO(server-11) can drop this -ServerEmojiData serverEmojiDataPopularLegacy = ServerEmojiData(codeToNames: { +final ServerEmojiData serverEmojiDataPopularLegacy = ServerEmojiData(codeToNames: { '1f44d': ['+1', 'thumbs_up', 'like'], '1f389': ['tada'], '1f642': ['smile'], @@ -502,21 +502,21 @@ UserTopicItem userTopicItem( // Messages, and pieces of messages. // -Reaction unicodeEmojiReaction = Reaction( +final Reaction unicodeEmojiReaction = Reaction( emojiName: 'thumbs_up', emojiCode: '1f44d', reactionType: ReactionType.unicodeEmoji, userId: selfUser.userId, ); -Reaction realmEmojiReaction = Reaction( +final Reaction realmEmojiReaction = Reaction( emojiName: 'twocents', emojiCode: '181', reactionType: ReactionType.realmEmoji, userId: selfUser.userId, ); -Reaction zulipExtraEmojiReaction = Reaction( +final Reaction zulipExtraEmojiReaction = Reaction( emojiName: 'zulip', emojiCode: 'zulip', reactionType: ReactionType.zulipExtraEmoji, From edf1eee417f856b109c8eea23bd6ee1c41c36464 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 17 Jul 2025 19:57:36 -0700 Subject: [PATCH 5/5] test: Prevent mutating shared example ServerEmojiData values In principle these are subject to the same sort of state leak we've run into with the shared example User objects, and fixed in recent commits: these are shared global objects, which don't get discarded or reset by `testBinding.reset`, and until this commit they were mutable. The probability that we'd actually end up with such a state leak was low: ServerEmojiData values never normally get mutated in the app's data structures (unlike User values), plus there'll probably only ever be a small number of tests that would have a reason to use these. But after the preceding couple of commits (notably the one introducing _ImmutableUser), these represent the last remaining mutable data in this file's top-level fields. So let's eliminate that too, and get to 100% in eliminating the possibility of the #1712 class of bug. --- test/example_data.dart | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 61c363dde7..06d6d130c0 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -131,7 +131,14 @@ GetServerSettingsResult serverSettings({ ); } -final ServerEmojiData serverEmojiDataPopular = ServerEmojiData(codeToNames: { +ServerEmojiData _immutableServerEmojiData({ + required Map> codeToNames}) { + return ServerEmojiData( + codeToNames: Map.unmodifiable(codeToNames.map( + (k, v) => MapEntry(k, List.unmodifiable(v))))); +} + +final ServerEmojiData serverEmojiDataPopular = _immutableServerEmojiData(codeToNames: { '1f44d': ['+1', 'thumbs_up', 'like'], '1f389': ['tada'], '1f642': ['slight_smile'], @@ -158,7 +165,7 @@ ServerEmojiData serverEmojiDataPopularPlus(ServerEmojiData data) { /// /// zulip/zulip@9feba0f16f is a Server 11 commit. // TODO(server-11) can drop this -final ServerEmojiData serverEmojiDataPopularLegacy = ServerEmojiData(codeToNames: { +final ServerEmojiData serverEmojiDataPopularLegacy = _immutableServerEmojiData(codeToNames: { '1f44d': ['+1', 'thumbs_up', 'like'], '1f389': ['tada'], '1f642': ['smile'],