From e7a227ad38871c23055dfa36ed1d342554a491d7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:01:10 -0700 Subject: [PATCH 01/12] msglist test [nfc]: Stop using global store and binding in model test We don't have any need for these here -- the code under test isn't mounting any widgets (let alone GlobalStoreWidget) nor otherwise interacting with the global state. Leave them out, simplifying the tests. --- test/model/message_list_test.dart | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 463860b4e8..8eeb3281de 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -9,17 +9,12 @@ import 'package:zulip/model/narrow.dart'; import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; -import '../model/binding.dart'; import '../model/test_store.dart'; const int userId = 1; Future messageListViewWithMessages(List messages, ZulipStream stream, Narrow narrow) async { - addTearDown(TestZulipBinding.instance.reset); - - await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - - final store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id); + final store = eg.store(); store.addUser(eg.user(userId: userId)); store.addStream(stream); @@ -40,8 +35,6 @@ Future messageListViewWithMessages(List messages, Zuli } void main() async { - TestZulipBinding.ensureInitialized(); - final stream = eg.stream(); final narrow = StreamNarrow(stream.streamId); From 1b6e598ae6ad1754d977ad544aaadc4d8cce50de Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:04:43 -0700 Subject: [PATCH 02/12] msglist test [nfc]: Drop user and stream from store The code under test doesn't actually look at this information -- the only thing it uses the PerAccountStore for is to (a) get an API connection, and (b) unregister itself in `dispose`. So simplify the tests by leaving it out. If later the code under test does start needing this information so that we do need to set it up in the tests, we can revisit then and may find a bit simpler of a way to arrange it. --- test/model/message_list_test.dart | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 8eeb3281de..8ef95c8663 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -9,15 +9,11 @@ import 'package:zulip/model/narrow.dart'; import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; -import '../model/test_store.dart'; const int userId = 1; -Future messageListViewWithMessages(List messages, ZulipStream stream, Narrow narrow) async { +Future messageListViewWithMessages(List messages, Narrow narrow) async { final store = eg.store(); - store.addUser(eg.user(userId: userId)); - store.addStream(stream); - final messageList = MessageListView.init(store: store, narrow: narrow); final connection = store.connection as FakeApiConnection; @@ -42,7 +38,7 @@ void main() async { final m1 = eg.streamMessage(id: 2, stream: stream); final m2 = eg.streamMessage(id: 4, stream: stream); final m3 = eg.streamMessage(id: 6, stream: stream); - final messageList = await messageListViewWithMessages([m1, m2, m3], stream, narrow); + final messageList = await messageListViewWithMessages([m1, m2, m3], narrow); // Exercise the binary search before, at, and after each element of the list. check(messageList.findMessageWithId(1)).equals(-1); @@ -70,7 +66,7 @@ void main() async { renderingOnly: false, ); - final messageList = await messageListViewWithMessages([originalMessage], stream, narrow); + final messageList = await messageListViewWithMessages([originalMessage], narrow); bool listenersNotified = false; messageList.addListener(() { listenersNotified = true; }); @@ -105,7 +101,7 @@ void main() async { renderingOnly: false, ); - final messageList = await messageListViewWithMessages([originalMessage], stream, narrow); + final messageList = await messageListViewWithMessages([originalMessage], narrow); bool listenersNotified = false; messageList.addListener(() { listenersNotified = true; }); @@ -132,7 +128,7 @@ void main() async { userId: null, ); - final messageList = await messageListViewWithMessages([originalMessage], stream, narrow); + final messageList = await messageListViewWithMessages([originalMessage], narrow); bool listenersNotified = false; messageList.addListener(() { listenersNotified = true; }); @@ -171,7 +167,7 @@ void main() async { test('add reaction', () async { final originalMessage = eg.streamMessage(stream: stream, reactions: []); - final messageList = await messageListViewWithMessages([originalMessage], stream, narrow); + final messageList = await messageListViewWithMessages([originalMessage], narrow); final message = messageList.messages.single; @@ -189,7 +185,7 @@ void main() async { test('add reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: []); - final messageList = await messageListViewWithMessages([someMessage], stream, narrow); + final messageList = await messageListViewWithMessages([someMessage], narrow); bool listenersNotified = false; messageList.addListener(() { listenersNotified = true; }); @@ -223,7 +219,7 @@ void main() async { final originalMessage = eg.streamMessage(stream: stream, reactions: [reaction2, reaction3, reaction4]); - final messageList = await messageListViewWithMessages([originalMessage], stream, narrow); + final messageList = await messageListViewWithMessages([originalMessage], narrow); final message = messageList.messages.single; @@ -241,7 +237,7 @@ void main() async { test('remove reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]); - final messageList = await messageListViewWithMessages([someMessage], stream, narrow); + final messageList = await messageListViewWithMessages([someMessage], narrow); bool listenersNotified = false; messageList.addListener(() { listenersNotified = true; }); From f60d4781c46737c7e1db17a69399960fd23de3b8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:24:40 -0700 Subject: [PATCH 03/12] msglist test [nfc]: Cut file-wide userId constant Now that we don't have setup code trying to coordinate on using the same arbitrary user ID as any other code, the two test cases that happen to need to mention a user ID can just mention one inline. That avoids the appearance of this `userId` constant potentially having subtle wider interactions. --- test/model/message_list_test.dart | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 8ef95c8663..344d30e4dd 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -10,8 +10,6 @@ import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; -const int userId = 1; - Future messageListViewWithMessages(List messages, Narrow narrow) async { final store = eg.store(); final messageList = MessageListView.init(store: store, narrow: narrow); @@ -62,7 +60,7 @@ void main() async { renderedContent: "

Hello, edited

", editTimestamp: 99999, isMeMessage: true, - userId: userId, + userId: 1, renderingOnly: false, ); @@ -97,7 +95,7 @@ void main() async { flags: originalMessage.flags, renderedContent: "

Hello, edited

", editTimestamp: 99999, - userId: userId, + userId: 1, renderingOnly: false, ); From 70be31fa8b8abace8f7b5487c30a5808ec7c741e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:17:37 -0700 Subject: [PATCH 04/12] msglist test [nfc]: Rename view-model locals as "model" These values are, as the doc on their class says, view-models for message lists. That's a type of view-model, not a type of message list; or more briefly, it's a type of model. --- test/model/message_list_test.dart | 86 +++++++++++++++---------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 344d30e4dd..5adb0b72ba 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -12,7 +12,7 @@ import '../example_data.dart' as eg; Future messageListViewWithMessages(List messages, Narrow narrow) async { final store = eg.store(); - final messageList = MessageListView.init(store: store, narrow: narrow); + final model = MessageListView.init(store: store, narrow: narrow); final connection = store.connection as FakeApiConnection; connection.prepare(json: GetMessagesResult( @@ -23,9 +23,9 @@ Future messageListViewWithMessages(List messages, Narr historyLimited: false, messages: messages, ).toJson()); - await messageList.fetch(); + await model.fetch(); - return messageList; + return model; } void main() async { @@ -36,16 +36,16 @@ void main() async { final m1 = eg.streamMessage(id: 2, stream: stream); final m2 = eg.streamMessage(id: 4, stream: stream); final m3 = eg.streamMessage(id: 6, stream: stream); - final messageList = await messageListViewWithMessages([m1, m2, m3], narrow); + final model = await messageListViewWithMessages([m1, m2, m3], narrow); // Exercise the binary search before, at, and after each element of the list. - check(messageList.findMessageWithId(1)).equals(-1); - check(messageList.findMessageWithId(2)).equals(0); - check(messageList.findMessageWithId(3)).equals(-1); - check(messageList.findMessageWithId(4)).equals(1); - check(messageList.findMessageWithId(5)).equals(-1); - check(messageList.findMessageWithId(6)).equals(2); - check(messageList.findMessageWithId(7)).equals(-1); + check(model.findMessageWithId(1)).equals(-1); + check(model.findMessageWithId(2)).equals(0); + check(model.findMessageWithId(3)).equals(-1); + check(model.findMessageWithId(4)).equals(1); + check(model.findMessageWithId(5)).equals(-1); + check(model.findMessageWithId(6)).equals(2); + check(model.findMessageWithId(7)).equals(-1); }); group('maybeUpdateMessage', () { @@ -64,20 +64,20 @@ void main() async { renderingOnly: false, ); - final messageList = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage], narrow); bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - final message = messageList.messages.single; + final message = model.messages.single; check(message) ..content.not(it()..equals(updateEvent.renderedContent!)) ..lastEditTimestamp.isNull() ..flags.not(it()..deepEquals(updateEvent.flags)) ..isMeMessage.not(it()..equals(updateEvent.isMeMessage!)); - messageList.maybeUpdateMessage(updateEvent); + model.maybeUpdateMessage(updateEvent); check(listenersNotified).isTrue(); - check(messageList.messages.single) + check(model.messages.single) ..identicalTo(message) ..content.equals(updateEvent.renderedContent!) ..lastEditTimestamp.equals(updateEvent.editTimestamp) @@ -99,13 +99,13 @@ void main() async { renderingOnly: false, ); - final messageList = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage], narrow); bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - messageList.maybeUpdateMessage(updateEvent); + model.maybeUpdateMessage(updateEvent); check(listenersNotified).isFalse(); - check(messageList.messages.single) + check(model.messages.single) ..content.equals(originalMessage.content) ..content.not(it()..equals(updateEvent.renderedContent!)); }); @@ -126,14 +126,14 @@ void main() async { userId: null, ); - final messageList = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage], narrow); bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - final message = messageList.messages.single; - messageList.maybeUpdateMessage(updateEvent); + final message = model.messages.single; + model.maybeUpdateMessage(updateEvent); check(listenersNotified).isTrue(); - check(messageList.messages.single) + check(model.messages.single) ..identicalTo(message) // Content is updated... ..content.equals(updateEvent.renderedContent!) @@ -165,34 +165,34 @@ void main() async { test('add reaction', () async { final originalMessage = eg.streamMessage(stream: stream, reactions: []); - final messageList = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage], narrow); - final message = messageList.messages.single; + final message = model.messages.single; bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - messageList.maybeUpdateMessageReactions( + model.maybeUpdateMessageReactions( mkEvent(eg.unicodeEmojiReaction, ReactionOp.add, originalMessage.id)); check(listenersNotified).isTrue(); - check(messageList.messages.single) + check(model.messages.single) ..identicalTo(message) ..reactions.jsonEquals([eg.unicodeEmojiReaction]); }); test('add reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: []); - final messageList = await messageListViewWithMessages([someMessage], narrow); + final model = await messageListViewWithMessages([someMessage], narrow); bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - messageList.maybeUpdateMessageReactions( + model.maybeUpdateMessageReactions( mkEvent(eg.unicodeEmojiReaction, ReactionOp.add, 1000)); check(listenersNotified).isFalse(); - check(messageList.messages.single).reactions.jsonEquals([]); + check(model.messages.single).reactions.jsonEquals([]); }); test('remove reaction', () async { @@ -217,34 +217,34 @@ void main() async { final originalMessage = eg.streamMessage(stream: stream, reactions: [reaction2, reaction3, reaction4]); - final messageList = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage], narrow); - final message = messageList.messages.single; + final message = model.messages.single; bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - messageList.maybeUpdateMessageReactions( + model.maybeUpdateMessageReactions( mkEvent(eventReaction, ReactionOp.remove, originalMessage.id)); check(listenersNotified).isTrue(); - check(messageList.messages.single) + check(model.messages.single) ..identicalTo(message) ..reactions.jsonEquals([reaction2, reaction3]); }); test('remove reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]); - final messageList = await messageListViewWithMessages([someMessage], narrow); + final model = await messageListViewWithMessages([someMessage], narrow); bool listenersNotified = false; - messageList.addListener(() { listenersNotified = true; }); + model.addListener(() { listenersNotified = true; }); - messageList.maybeUpdateMessageReactions( + model.maybeUpdateMessageReactions( mkEvent(eg.unicodeEmojiReaction, ReactionOp.remove, 1000)); check(listenersNotified).isFalse(); - check(messageList.messages.single).reactions.jsonEquals([eg.unicodeEmojiReaction]); + check(model.messages.single).reactions.jsonEquals([eg.unicodeEmojiReaction]); }); }); }); From 997e07a50023e1d6a4306e7a3b47030184b52493 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:28:21 -0700 Subject: [PATCH 05/12] msglist test: Simplify by using AllMessagesNarrow None of these tests actually care what the narrow is. So use the narrow that means we can have any kind of message there without causing any inconsistency. --- test/model/message_list_test.dart | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 5adb0b72ba..371e7533dd 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -10,7 +10,7 @@ import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; -Future messageListViewWithMessages(List messages, Narrow narrow) async { +Future messageListViewWithMessages(List messages, [Narrow narrow = const AllMessagesNarrow()]) async { final store = eg.store(); final model = MessageListView.init(store: store, narrow: narrow); @@ -30,13 +30,12 @@ Future messageListViewWithMessages(List messages, Narr void main() async { final stream = eg.stream(); - final narrow = StreamNarrow(stream.streamId); test('findMessageWithId', () async { final m1 = eg.streamMessage(id: 2, stream: stream); final m2 = eg.streamMessage(id: 4, stream: stream); final m3 = eg.streamMessage(id: 6, stream: stream); - final model = await messageListViewWithMessages([m1, m2, m3], narrow); + final model = await messageListViewWithMessages([m1, m2, m3]); // Exercise the binary search before, at, and after each element of the list. check(model.findMessageWithId(1)).equals(-1); @@ -64,7 +63,7 @@ void main() async { renderingOnly: false, ); - final model = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage]); bool listenersNotified = false; model.addListener(() { listenersNotified = true; }); @@ -99,7 +98,7 @@ void main() async { renderingOnly: false, ); - final model = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage]); bool listenersNotified = false; model.addListener(() { listenersNotified = true; }); @@ -126,7 +125,7 @@ void main() async { userId: null, ); - final model = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage]); bool listenersNotified = false; model.addListener(() { listenersNotified = true; }); @@ -165,7 +164,7 @@ void main() async { test('add reaction', () async { final originalMessage = eg.streamMessage(stream: stream, reactions: []); - final model = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage]); final message = model.messages.single; @@ -183,7 +182,7 @@ void main() async { test('add reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: []); - final model = await messageListViewWithMessages([someMessage], narrow); + final model = await messageListViewWithMessages([someMessage]); bool listenersNotified = false; model.addListener(() { listenersNotified = true; }); @@ -217,7 +216,7 @@ void main() async { final originalMessage = eg.streamMessage(stream: stream, reactions: [reaction2, reaction3, reaction4]); - final model = await messageListViewWithMessages([originalMessage], narrow); + final model = await messageListViewWithMessages([originalMessage]); final message = model.messages.single; @@ -235,7 +234,7 @@ void main() async { test('remove reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]); - final model = await messageListViewWithMessages([someMessage], narrow); + final model = await messageListViewWithMessages([someMessage]); bool listenersNotified = false; model.addListener(() { listenersNotified = true; }); From 4a4f8c4698060a8ce677e862cfca25b1b6fd49e5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:26:36 -0700 Subject: [PATCH 06/12] msglist test [nfc]: Let eg.streamMessage pick the stream Now that the narrow is AllMessagesNarrow, there's no need for these test messages to be in any particular stream, so we can simplify a bit further. --- test/model/message_list_test.dart | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 371e7533dd..43d08f451e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -29,12 +29,10 @@ Future messageListViewWithMessages(List messages, [Nar } void main() async { - final stream = eg.stream(); - test('findMessageWithId', () async { - final m1 = eg.streamMessage(id: 2, stream: stream); - final m2 = eg.streamMessage(id: 4, stream: stream); - final m3 = eg.streamMessage(id: 6, stream: stream); + final m1 = eg.streamMessage(id: 2); + final m2 = eg.streamMessage(id: 4); + final m3 = eg.streamMessage(id: 6); final model = await messageListViewWithMessages([m1, m2, m3]); // Exercise the binary search before, at, and after each element of the list. @@ -49,7 +47,7 @@ void main() async { group('maybeUpdateMessage', () { test('update a message', () async { - final originalMessage = eg.streamMessage(id: 243, stream: stream, + final originalMessage = eg.streamMessage(id: 243, content: "

Hello, world

"); final updateEvent = UpdateMessageEvent( id: 1, @@ -85,7 +83,7 @@ void main() async { }); test('ignore when message not present', () async { - final originalMessage = eg.streamMessage(id: 243, stream: stream, + final originalMessage = eg.streamMessage(id: 243, content: "

Hello, world

"); final updateEvent = UpdateMessageEvent( id: 1, @@ -111,7 +109,7 @@ void main() async { // TODO(server-5): Cut legacy case for rendering-only message update Future checkRenderingOnly({required bool legacy}) async { - final originalMessage = eg.streamMessage(id: 972, stream: stream, + final originalMessage = eg.streamMessage(id: 972, lastEditTimestamp: 78492, content: "

Hello, world

"); final updateEvent = UpdateMessageEvent( @@ -163,7 +161,7 @@ void main() async { } test('add reaction', () async { - final originalMessage = eg.streamMessage(stream: stream, reactions: []); + final originalMessage = eg.streamMessage(reactions: []); final model = await messageListViewWithMessages([originalMessage]); final message = model.messages.single; @@ -214,7 +212,7 @@ void main() async { final reaction4 = Reaction.fromJson(eventReaction.toJson() ..['emoji_name'] = 'hello'); - final originalMessage = eg.streamMessage(stream: stream, + final originalMessage = eg.streamMessage( reactions: [reaction2, reaction3, reaction4]); final model = await messageListViewWithMessages([originalMessage]); From 2491cb7c208a4a4b798c86947e63f8fa1630d9f9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:30:28 -0700 Subject: [PATCH 07/12] msglist test [nfc]: Simplify findMessageWithId test slightly more --- test/model/message_list_test.dart | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 43d08f451e..27c059843d 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -30,10 +30,11 @@ Future messageListViewWithMessages(List messages, [Nar void main() async { test('findMessageWithId', () async { - final m1 = eg.streamMessage(id: 2); - final m2 = eg.streamMessage(id: 4); - final m3 = eg.streamMessage(id: 6); - final model = await messageListViewWithMessages([m1, m2, m3]); + final model = await messageListViewWithMessages([ + eg.streamMessage(id: 2), + eg.streamMessage(id: 4), + eg.streamMessage(id: 6), + ]); // Exercise the binary search before, at, and after each element of the list. check(model.findMessageWithId(1)).equals(-1); From 9a07d7dd924f309cbb106a71ff27c819a16d9498 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:36:10 -0700 Subject: [PATCH 08/12] msglist test [nfc]: Use package:checks idiom for properties of model --- test/model/message_list_test.dart | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 27c059843d..f4b45637a4 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -3,8 +3,10 @@ import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/route/messages.dart'; +import 'package:zulip/model/content.dart'; import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/store.dart'; import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; @@ -75,7 +77,7 @@ void main() async { model.maybeUpdateMessage(updateEvent); check(listenersNotified).isTrue(); - check(model.messages.single) + check(model).messages.single ..identicalTo(message) ..content.equals(updateEvent.renderedContent!) ..lastEditTimestamp.equals(updateEvent.editTimestamp) @@ -103,7 +105,7 @@ void main() async { model.maybeUpdateMessage(updateEvent); check(listenersNotified).isFalse(); - check(model.messages.single) + check(model).messages.single ..content.equals(originalMessage.content) ..content.not(it()..equals(updateEvent.renderedContent!)); }); @@ -131,7 +133,7 @@ void main() async { final message = model.messages.single; model.maybeUpdateMessage(updateEvent); check(listenersNotified).isTrue(); - check(model.messages.single) + check(model).messages.single ..identicalTo(message) // Content is updated... ..content.equals(updateEvent.renderedContent!) @@ -174,7 +176,7 @@ void main() async { mkEvent(eg.unicodeEmojiReaction, ReactionOp.add, originalMessage.id)); check(listenersNotified).isTrue(); - check(model.messages.single) + check(model).messages.single ..identicalTo(message) ..reactions.jsonEquals([eg.unicodeEmojiReaction]); }); @@ -190,7 +192,7 @@ void main() async { mkEvent(eg.unicodeEmojiReaction, ReactionOp.add, 1000)); check(listenersNotified).isFalse(); - check(model.messages.single).reactions.jsonEquals([]); + check(model).messages.single.reactions.jsonEquals([]); }); test('remove reaction', () async { @@ -226,7 +228,7 @@ void main() async { mkEvent(eventReaction, ReactionOp.remove, originalMessage.id)); check(listenersNotified).isTrue(); - check(model.messages.single) + check(model).messages.single ..identicalTo(message) ..reactions.jsonEquals([reaction2, reaction3]); }); @@ -242,8 +244,16 @@ void main() async { mkEvent(eg.unicodeEmojiReaction, ReactionOp.remove, 1000)); check(listenersNotified).isFalse(); - check(model.messages.single).reactions.jsonEquals([eg.unicodeEmojiReaction]); + check(model).messages.single.reactions.jsonEquals([eg.unicodeEmojiReaction]); }); }); }); } + +extension MessageListViewChecks on Subject { + Subject get store => has((x) => x.store, 'store'); + Subject get narrow => has((x) => x.narrow, 'narrow'); + Subject> get messages => has((x) => x.messages, 'messages'); + Subject> get contents => has((x) => x.contents, 'contents'); + Subject get fetched => has((x) => x.fetched, 'fetched'); +} From dbf86dd29fbe8659ca73437f3adee3a898588142 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 20:35:14 -0700 Subject: [PATCH 09/12] msglist test: Use a realistic GetMessagesResult for fetch --- test/model/message_list_test.dart | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index f4b45637a4..103f43517b 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -17,14 +17,8 @@ Future messageListViewWithMessages(List messages, [Nar final model = MessageListView.init(store: store, narrow: narrow); final connection = store.connection as FakeApiConnection; - connection.prepare(json: GetMessagesResult( - anchor: messages.first.id, - foundNewest: true, - foundOldest: true, - foundAnchor: true, - historyLimited: false, - messages: messages, - ).toJson()); + connection.prepare(json: + newestResult(foundOldest: true, messages: messages).toJson()); await model.fetch(); return model; @@ -257,3 +251,22 @@ extension MessageListViewChecks on Subject { Subject> get contents => has((x) => x.contents, 'contents'); Subject get fetched => has((x) => x.fetched, 'fetched'); } + +/// A GetMessagesResult the server might return on an `anchor=newest` request. +GetMessagesResult newestResult({ + required bool foundOldest, + bool historyLimited = false, + required List messages, +}) { + return GetMessagesResult( + // These anchor, foundAnchor, and foundNewest values are what the server + // appears to always return when the request had `anchor=newest`. + anchor: 10000000000000000, // that's 16 zeros + foundAnchor: false, + foundNewest: true, + + foundOldest: foundOldest, + historyLimited: historyLimited, + messages: messages, + ); +} From fbbbbeb13447e53811caa3e5c8ad5de241866be8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 19:23:39 -0700 Subject: [PATCH 10/12] msglist test: Overhaul setup of tests, with shared helpers All the tests in this file are going to have a common structure to the objects they operate on: chiefly, a MessageListView. So we can hoist those up to the level of `main`, which enables the tests to share helper functions that act on those objects. In principle this means a state leak between tests. We mitigate that by resetting all the shared variables at the same time, in the `prepare` function that each test calls at the outset. We could get a similar effect by putting these shared variables and helpers as fields and methods on a class, and having each test case make its own instance of the class -- its own "tester" object, akin to the WidgetTester object that `testWidgets` provides to its test body. That'd provide more encapsulation, and would be a good strategy if for example this logic were to be shared across several test files. But I think this test file can get away without it. The immediate payoff of the shared helpers is that they let us centralize the logic for checking that the view-model has, or hasn't, notified its listeners. That simplifies each test case, plus it lets us make that logic slightly more sophisticated (at O(1) rather than O(n) cost in the source code): we now detect duplicate calls too. Coming up, we'll add further centralized useful logic to these helpers, and add some tests that benefit from the split between `prepare` and `prepareMessages`. --- test/model/message_list_test.dart | 121 ++++++++++++++++-------------- 1 file changed, 64 insertions(+), 57 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 103f43517b..80e29fc75b 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -12,21 +12,50 @@ import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; -Future messageListViewWithMessages(List messages, [Narrow narrow = const AllMessagesNarrow()]) async { - final store = eg.store(); - final model = MessageListView.init(store: store, narrow: narrow); - - final connection = store.connection as FakeApiConnection; - connection.prepare(json: - newestResult(foundOldest: true, messages: messages).toJson()); - await model.fetch(); - - return model; -} - void main() async { + // These variables are the common state operated on by each test. + // Each test case calls [prepare] to initialize them. + late PerAccountStore store; + late FakeApiConnection connection; + late MessageListView model; + late int notifiedCount; + + void checkNotNotified() { + check(notifiedCount).equals(0); + } + + void checkNotifiedOnce() { + check(notifiedCount).equals(1); + notifiedCount = 0; + } + + /// Initialize [model] and the rest of the test state. + void prepare({Narrow narrow = const AllMessagesNarrow()}) { + store = eg.store(); + connection = store.connection as FakeApiConnection; + notifiedCount = 0; + model = MessageListView.init(store: store, narrow: narrow) + ..addListener(() { notifiedCount++; }); + check(model).fetched.isFalse(); + checkNotNotified(); + } + + /// Perform the initial message fetch for [model]. + /// + /// The test case must have already called [prepare] to initialize the state. + Future prepareMessages({ + required bool foundOldest, + required List messages, + }) async { + connection.prepare(json: + newestResult(foundOldest: foundOldest, messages: messages).toJson()); + await model.fetch(); + checkNotifiedOnce(); + } + test('findMessageWithId', () async { - final model = await messageListViewWithMessages([ + prepare(); + await prepareMessages(foundOldest: true, messages: [ eg.streamMessage(id: 2), eg.streamMessage(id: 4), eg.streamMessage(id: 6), @@ -57,10 +86,8 @@ void main() async { userId: 1, renderingOnly: false, ); - - final model = await messageListViewWithMessages([originalMessage]); - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); + prepare(); + await prepareMessages(foundOldest: true, messages: [originalMessage]); final message = model.messages.single; check(message) @@ -70,7 +97,7 @@ void main() async { ..isMeMessage.not(it()..equals(updateEvent.isMeMessage!)); model.maybeUpdateMessage(updateEvent); - check(listenersNotified).isTrue(); + checkNotifiedOnce(); check(model).messages.single ..identicalTo(message) ..content.equals(updateEvent.renderedContent!) @@ -92,13 +119,11 @@ void main() async { userId: 1, renderingOnly: false, ); - - final model = await messageListViewWithMessages([originalMessage]); - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); + prepare(); + await prepareMessages(foundOldest: true, messages: [originalMessage]); model.maybeUpdateMessage(updateEvent); - check(listenersNotified).isFalse(); + checkNotNotified(); check(model).messages.single ..content.equals(originalMessage.content) ..content.not(it()..equals(updateEvent.renderedContent!)); @@ -119,14 +144,12 @@ void main() async { renderingOnly: legacy ? null : true, userId: null, ); - - final model = await messageListViewWithMessages([originalMessage]); - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); - + prepare(); + await prepareMessages(foundOldest: true, messages: [originalMessage]); final message = model.messages.single; + model.maybeUpdateMessage(updateEvent); - check(listenersNotified).isTrue(); + checkNotifiedOnce(); check(model).messages.single ..identicalTo(message) // Content is updated... @@ -159,17 +182,13 @@ void main() async { test('add reaction', () async { final originalMessage = eg.streamMessage(reactions: []); - final model = await messageListViewWithMessages([originalMessage]); - + prepare(); + await prepareMessages(foundOldest: true, messages: [originalMessage]); final message = model.messages.single; - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); - model.maybeUpdateMessageReactions( mkEvent(eg.unicodeEmojiReaction, ReactionOp.add, originalMessage.id)); - - check(listenersNotified).isTrue(); + checkNotifiedOnce(); check(model).messages.single ..identicalTo(message) ..reactions.jsonEquals([eg.unicodeEmojiReaction]); @@ -177,15 +196,11 @@ void main() async { test('add reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: []); - final model = await messageListViewWithMessages([someMessage]); - - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); - + prepare(); + await prepareMessages(foundOldest: true, messages: [someMessage]); model.maybeUpdateMessageReactions( mkEvent(eg.unicodeEmojiReaction, ReactionOp.add, 1000)); - - check(listenersNotified).isFalse(); + checkNotNotified(); check(model).messages.single.reactions.jsonEquals([]); }); @@ -211,17 +226,13 @@ void main() async { final originalMessage = eg.streamMessage( reactions: [reaction2, reaction3, reaction4]); - final model = await messageListViewWithMessages([originalMessage]); - + prepare(); + await prepareMessages(foundOldest: true, messages: [originalMessage]); final message = model.messages.single; - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); - model.maybeUpdateMessageReactions( mkEvent(eventReaction, ReactionOp.remove, originalMessage.id)); - - check(listenersNotified).isTrue(); + checkNotifiedOnce(); check(model).messages.single ..identicalTo(message) ..reactions.jsonEquals([reaction2, reaction3]); @@ -229,15 +240,11 @@ void main() async { test('remove reaction; message is not in list', () async { final someMessage = eg.streamMessage(id: 1, reactions: [eg.unicodeEmojiReaction]); - final model = await messageListViewWithMessages([someMessage]); - - bool listenersNotified = false; - model.addListener(() { listenersNotified = true; }); - + prepare(); + await prepareMessages(foundOldest: true, messages: [someMessage]); model.maybeUpdateMessageReactions( mkEvent(eg.unicodeEmojiReaction, ReactionOp.remove, 1000)); - - check(listenersNotified).isFalse(); + checkNotNotified(); check(model).messages.single.reactions.jsonEquals([eg.unicodeEmojiReaction]); }); }); From 047c2ba74d3188cd8e6556005c6f88eedd3444d6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Aug 2023 20:18:07 -0700 Subject: [PATCH 11/12] msglist test: Systematically check invariants of MessageListView This takes further advantage of having these centralized helpers. --- test/model/message_list_test.dart | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 80e29fc75b..01a4e83693 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -11,6 +11,7 @@ import 'package:zulip/model/store.dart'; import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; +import 'content_checks.dart'; void main() async { // These variables are the common state operated on by each test. @@ -35,8 +36,12 @@ void main() async { connection = store.connection as FakeApiConnection; notifiedCount = 0; model = MessageListView.init(store: store, narrow: narrow) - ..addListener(() { notifiedCount++; }); + ..addListener(() { + checkInvariants(model); + notifiedCount++; + }); check(model).fetched.isFalse(); + checkInvariants(model); checkNotNotified(); } @@ -251,6 +256,22 @@ void main() async { }); } +void checkInvariants(MessageListView model) { + if (!model.fetched) { + check(model).messages.isEmpty(); + } + + for (int i = 0; i < model.messages.length - 1; i++) { + check(model.messages[i].id).isLessThan(model.messages[i+1].id); + } + + check(model).contents.length.equals(model.messages.length); + for (int i = 0; i < model.contents.length; i++) { + check(model.contents[i]) + .equalsNode(parseContent(model.messages[i].content)); + } +} + extension MessageListViewChecks on Subject { Subject get store => has((x) => x.store, 'store'); Subject get narrow => has((x) => x.narrow, 'narrow'); From c5500b1a66a28195a0e4519213f8d2127aa08bad Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jul 2023 13:11:17 -0700 Subject: [PATCH 12/12] msglist test: Write tests for the rest of MessageListView For the more recently-added functionality, we'd written tests along the way. But we didn't have tests for the logic we built earlier, which is some of the most core functionality. Add tests for all that logic too. --- test/model/message_list_test.dart | 128 ++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 01a4e83693..d0bb1c587f 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1,7 +1,11 @@ +import 'dart:convert'; + import 'package:checks/checks.dart'; +import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/model/narrow.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/content.dart'; import 'package:zulip/model/message_list.dart'; @@ -11,6 +15,7 @@ import 'package:zulip/model/store.dart'; import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; +import '../stdlib_checks.dart'; import 'content_checks.dart'; void main() async { @@ -58,6 +63,106 @@ void main() async { checkNotifiedOnce(); } + void checkLastRequest({ + required ApiNarrow narrow, + required String anchor, + bool? includeAnchor, + required int numBefore, + required int numAfter, + }) { + check(connection.lastRequest).isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode(narrow), + 'anchor': anchor, + if (includeAnchor != null) 'include_anchor': includeAnchor.toString(), + 'num_before': numBefore.toString(), + 'num_after': numAfter.toString(), + }); + } + + test('fetch', () async { + const narrow = AllMessagesNarrow(); + prepare(narrow: narrow); + connection.prepare(json: newestResult( + foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i)), + ).toJson()); + final fetchFuture = model.fetch(); + check(model).fetched.isFalse(); + checkInvariants(model); + + checkNotNotified(); + await fetchFuture; + checkNotifiedOnce(); + check(model).messages.length.equals(100); + checkLastRequest( + narrow: narrow.apiEncode(), + anchor: 'newest', + numBefore: 100, + numAfter: 10, + ); + }); + + test('fetch, short history', () async { + prepare(); + connection.prepare(json: newestResult( + foundOldest: true, + messages: List.generate(30, (i) => eg.streamMessage(id: 1000 + i)), + ).toJson()); + await model.fetch(); + checkNotifiedOnce(); + check(model).messages.length.equals(30); + }); + + test('fetch, no messages found', () async { + prepare(); + connection.prepare(json: newestResult( + foundOldest: true, + messages: [], + ).toJson()); + await model.fetch(); + checkNotifiedOnce(); + check(model) + ..fetched.isTrue() + ..messages.isEmpty(); + }); + + test('maybeAddMessage', () async { + final stream = eg.stream(); + prepare(narrow: StreamNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(id: 1000 + i, stream: stream))); + + check(model).messages.length.equals(30); + model.maybeAddMessage(eg.streamMessage(id: 1100, stream: stream)); + checkNotifiedOnce(); + check(model).messages.length.equals(31); + }); + + test('maybeAddMessage, not in narrow', () async { + final stream = eg.stream(streamId: 123); + prepare(narrow: StreamNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(id: 1000 + i, stream: stream))); + + check(model).messages.length.equals(30); + final otherStream = eg.stream(streamId: 234); + model.maybeAddMessage(eg.streamMessage(id: 1100, stream: otherStream)); + checkNotNotified(); + check(model).messages.length.equals(30); + }); + + test('maybeAddMessage, before fetch', () async { + final stream = eg.stream(); + prepare(narrow: StreamNarrow(stream.streamId)); + model.maybeAddMessage(eg.streamMessage(id: 1100, stream: stream)); + checkNotNotified(); + check(model).fetched.isFalse(); + checkInvariants(model); + }); + test('findMessageWithId', () async { prepare(); await prepareMessages(foundOldest: true, messages: [ @@ -254,6 +359,29 @@ void main() async { }); }); }); + + test('reassemble', () async { + final stream = eg.stream(); + prepare(narrow: StreamNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(id: 1000 + i, stream: stream))); + model.maybeAddMessage(eg.streamMessage(id: 1100, stream: stream)); + checkNotifiedOnce(); + check(model).messages.length.equals(31); + + // Mess with model.contents, to simulate it having come from + // a previous version of the code. + final correctContent = parseContent(model.messages[0].content); + model.contents[0] = const ZulipContent(nodes: [ + ParagraphNode(links: null, nodes: [TextNode('something outdated')]) + ]); + check(model.contents[0]).not(it()..equalsNode(correctContent)); + + model.reassemble(); + checkNotifiedOnce(); + check(model).messages.length.equals(31); + check(model.contents[0]).equalsNode(correctContent); + }); } void checkInvariants(MessageListView model) {