Clean up and tighten UpdateMessageEvent tests

test/example_data.dart

@@ -94,7 +94,6 @@ ZulipStream stream({
 
 final _messagePropertiesBase = {
   'is_me_message': false,
-  'last_edit_timestamp': null,
   'recipient_id': 32, // obsolescent in API, and ignored
 };
 
@@ -132,6 +131,7 @@ StreamMessage streamMessage({
   String? topic,
   String? content,
   String? contentMarkdown,
+  int? lastEditTimestamp,
   List<String>? flags,
 }) {
   final effectiveStream = stream ?? _stream();
@@ -148,6 +148,7 @@ StreamMessage streamMessage({
     'stream_id': effectiveStream.streamId,
     'flags': flags ?? [],
     'id': id ?? 1234567, // TODO generate example IDs
+    'last_edit_timestamp': lastEditTimestamp,
     'subject': topic ?? 'example topic',
     'timestamp': 1678139636,
     'type': 'stream',
@@ -164,6 +165,7 @@ DmMessage dmMessage({
   required List<User> to,
   String? content,
   String? contentMarkdown,
+  int? lastEditTimestamp,
   List<String>? flags,
 }) {
   assert(!to.any((user) => user.userId == from.userId));
@@ -177,6 +179,7 @@ DmMessage dmMessage({
 
     'flags': flags ?? [],
     'id': id ?? 1234567, // TODO generate example IDs
+    'last_edit_timestamp': lastEditTimestamp,
     'subject': '',
     'timestamp': 1678139636,
     'type': 'private',

test/model/message_list_test.dart

@@ -5,17 +5,16 @@ import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/messages.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';
+import '../example_data.dart' as eg;
 import '../model/binding.dart';
 import '../model/test_store.dart';
-import '../example_data.dart' as eg;
 
 const int userId = 1;
 
-Future<PerAccountStore> setupStore(ZulipStream stream) async {
+Future<MessageListView> messageListViewWithMessages(List<Message> messages, ZulipStream stream, Narrow narrow) async {
   addTearDown(TestZulipBinding.instance.reset);
 
   await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot());
@@ -24,14 +23,9 @@ Future<PerAccountStore> setupStore(ZulipStream stream) async {
   store.addUser(eg.user(userId: userId));
   store.addStream(stream);
 
-  return store;
-}
-
-Future<MessageListView> messageListViewWithMessages(List<Message> messages, PerAccountStore store, Narrow narrow) async {
   final messageList = MessageListView.init(store: store, narrow: narrow);
 
   final connection = store.connection as FakeApiConnection;
-
   connection.prepare(json: GetMessagesResult(
     anchor: messages.first.id,
     foundNewest: true,
@@ -40,11 +34,8 @@ Future<MessageListView> messageListViewWithMessages(List<Message> messages, PerA
     historyLimited: false,
     messages: messages,
   ).toJson());
-
   await messageList.fetch();
 
-  check(messageList.messages.length).equals(messages.length);
-
   return messageList;
 }
 
@@ -54,172 +45,122 @@ void main() async {
   final stream = eg.stream();
   final narrow = StreamNarrow(stream.streamId);
 
-  group('update message tests', () {
-
-    test('find message in message list returns index of message', () async {
-      final store = await setupStore(stream);
-
-      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], store, narrow);
-      // The implementation of this uses a binary search, so let's test it
-      // a bit more exhaustively.
-
-      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);
-
-      // Invalid IDs
-      check(messageList.findMessageWithId(-8409)).equals(-1);
-      check(messageList.findMessageWithId(0)).equals(-1);
-    });
-
-    test('update events are correctly applied to message when it is in the stream', () async {
-      final store = await setupStore(stream);
-
-      const oldContent = "<p>Hello, world</p>";
-      const newContent = "<p>Hello, edited</p>";
-      const newTimestamp = 99999;
-
-      List<String> oldFlags = [];
-      List<String> newFlags = ["starred"];
-
-      final originalMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent, flags: oldFlags);
-      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
+  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 messageList = await messageListViewWithMessages([m1, m2, m3], stream, 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);
+  });
 
+  group('maybeUpdateMessage', () {
+    test('update a message', () async {
+      final originalMessage = eg.streamMessage(id: 243, stream: stream,
+        content: "<p>Hello, world</p>");
       final updateEvent = UpdateMessageEvent(
         id: 1,
         messageId: originalMessage.id,
         messageIds: [originalMessage.id],
-        flags: newFlags,
-        renderedContent: newContent,
-        editTimestamp: newTimestamp,
+        flags: ["starred"],
+        renderedContent: "<p>Hello, edited</p>",
+        editTimestamp: 99999,
         isMeMessage: true,
         userId: userId,
         renderingOnly: false,
       );
 
+      final messageList = await messageListViewWithMessages([originalMessage], stream, narrow);
+      bool listenersNotified = false;
+      messageList.addListener(() { listenersNotified = true; });
+
       final message = messageList.messages.single;
       check(message)
-        ..content.equals(oldContent)
-        ..flags.deepEquals(oldFlags)
-        ..isMeMessage.isFalse();
-
-      var listenersNotified = false;
+        ..content.not(it()..equals(updateEvent.renderedContent!))
+        ..lastEditTimestamp.isNull()
+        ..flags.not(it()..deepEquals(updateEvent.flags))
+        ..isMeMessage.not(it()..equals(updateEvent.isMeMessage!));
 
-      messageList.addListener(() { listenersNotified = true; });
       messageList.maybeUpdateMessage(updateEvent);
-
       check(listenersNotified).isTrue();
-
-      check(message)
-        ..identicalTo(messageList.messages.single)
-        ..content.equals(newContent)
-        ..lastEditTimestamp.equals(newTimestamp)
-        ..flags.equals(newFlags)
-        ..isMeMessage.isTrue();
+      check(messageList.messages.single)
+        ..identicalTo(message)
+        ..content.equals(updateEvent.renderedContent!)
+        ..lastEditTimestamp.equals(updateEvent.editTimestamp)
+        ..flags.equals(updateEvent.flags)
+        ..isMeMessage.equals(updateEvent.isMeMessage!);
     });
 
-    test('update event is ignored when message is not in the message list', () async {
-      final store = await setupStore(stream);
-
-      const oldContent = "<p>Hello, world</p>";
-      const newContent = "<p>Hello, edited</p>";
-      const newTimestamp = 99999;
-
-      final originalMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent);
-      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
-
+    test('ignore when message not present', () async {
+      final originalMessage = eg.streamMessage(id: 243, stream: stream,
+        content: "<p>Hello, world</p>");
       final updateEvent = UpdateMessageEvent(
         id: 1,
         messageId: originalMessage.id + 1,
         messageIds: [originalMessage.id + 1],
         flags: originalMessage.flags,
-        renderedContent: newContent,
-        editTimestamp: newTimestamp,
+        renderedContent: "<p>Hello, edited</p>",
+        editTimestamp: 99999,
         userId: userId,
         renderingOnly: false,
       );
 
-      final message = messageList.messages.single;
-      check(message).content.equals(oldContent);
-
-      var listenersNotified = false;
-
+      final messageList = await messageListViewWithMessages([originalMessage], stream, narrow);
+      bool listenersNotified = false;
       messageList.addListener(() { listenersNotified = true; });
-      messageList.maybeUpdateMessage(updateEvent);
 
+      messageList.maybeUpdateMessage(updateEvent);
       check(listenersNotified).isFalse();
-      check(message).content.equals(oldContent);
+      check(messageList.messages.single)
+        ..content.equals(originalMessage.content)
+        ..content.not(it()..equals(updateEvent.renderedContent!));
     });
 
-    test('rendering-only update does not change timestamp', () async {
-      final store = await setupStore(stream);
-
-      const oldContent = "<p>Hello, world</p>";
-      const oldTimestamp = 78492;
-      const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
-      const newTimestamp = 99999;
-
-      final originalMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
-      originalMessage.lastEditTimestamp = oldTimestamp;
-
-      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
-
+    // TODO(server-5): Cut legacy case for rendering-only message update
+    Future<void> checkRenderingOnly({required bool legacy}) async {
+      final originalMessage = eg.streamMessage(id: 972, stream: stream,
+        lastEditTimestamp: 78492,
+        content: "<p>Hello, world</p>");
       final updateEvent = UpdateMessageEvent(
         id: 1,
         messageId: originalMessage.id,
         messageIds: [originalMessage.id],
         flags: originalMessage.flags,
-        renderedContent: newContent,
-        editTimestamp: newTimestamp,
-        renderingOnly: true,
+        renderedContent: "<p>Hello, world</p> <div>Some link preview</div>",
+        editTimestamp: 99999,
+        renderingOnly: legacy ? null : true,
         userId: null,
       );
 
-      final message = messageList.messages[0];
+      final messageList = await messageListViewWithMessages([originalMessage], stream, narrow);
+      bool listenersNotified = false;
+      messageList.addListener(() { listenersNotified = true; });
+
+      final message = messageList.messages.single;
       messageList.maybeUpdateMessage(updateEvent);
-      check(message)
-        ..content.equals(newContent)
-        ..lastEditTimestamp.equals(oldTimestamp);
+      check(listenersNotified).isTrue();
+      check(messageList.messages.single)
+        ..identicalTo(message)
+        // Content is updated...
+        ..content.equals(updateEvent.renderedContent!)
+        // ... edit timestamp is not.
+        ..lastEditTimestamp.equals(originalMessage.lastEditTimestamp)
+        ..lastEditTimestamp.not(it()..equals(updateEvent.editTimestamp));
+    }
+
+    test('rendering-only update does not change timestamp', () async {
+      await checkRenderingOnly(legacy: false);
     });
 
-    // TODO(server-5): Cut this test; rely on renderingOnly from FL 114
     test('rendering-only update does not change timestamp (for old server versions)', () async {
-      final store = await setupStore(stream);
-
-      const oldContent = "<p>Hello, world</p>";
-      const oldTimestamp = 78492;
-      const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
-      const newTimestamp = 99999;
-
-      final originalMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
-      originalMessage.lastEditTimestamp = oldTimestamp;
-
-      final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
-
-      final updateEvent = UpdateMessageEvent(
-        id: 1,
-        messageId: originalMessage.id,
-        messageIds: [originalMessage.id],
-        flags: originalMessage.flags,
-        renderedContent: newContent,
-        editTimestamp: newTimestamp,
-        renderingOnly: null,
-        userId: null,
-      );
-
-      final message = messageList.messages.single;
-      messageList.maybeUpdateMessage(updateEvent);
-      check(message)
-        ..content.equals(newContent)
-        ..lastEditTimestamp.equals(oldTimestamp);
+      await checkRenderingOnly(legacy: true);
     });
   });
 }