From 68c90d782f8101d88bafb97293679764d3f65223 Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Wed, 29 May 2024 12:18:19 +0430 Subject: [PATCH 01/21] autocomplete [nfc]: Factor out `compareRecentMessageIds` --- lib/model/autocomplete.dart | 19 +++++++++++++++---- test/model/autocomplete_test.dart | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 5250dba6d4..797a7ec636 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -204,10 +204,21 @@ class MentionAutocompleteView extends ChangeNotifier { final aLatestMessageId = recentDms.latestMessagesByRecipient[userA.userId]; final bLatestMessageId = recentDms.latestMessagesByRecipient[userB.userId]; - return switch((aLatestMessageId, bLatestMessageId)) { - (int a, int b) => -a.compareTo(b), - (int(), _) => -1, - (_, int()) => 1, + return -compareRecentMessageIds(aLatestMessageId, bLatestMessageId); + } + + /// Compares [a] to [b], with null less than all integers. + /// + /// The values should represent the most recent message ID in each of two + /// sets of messages, with null meaning the set is empty. + /// + /// Return values are as with [Comparable.compareTo]. + @visibleForTesting + static int compareRecentMessageIds(int? a, int? b) { + return switch ((a, b)) { + (int a, int b) => a.compareTo(b), + (int(), _) => 1, + (_, int()) => -1, _ => 0, }; } diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 5493598200..40576d1ff5 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -365,6 +365,23 @@ void main() { await store.addUsers(users); } + group('MentionAutocompleteView.compareRecentMessageIds', () { + test('both a and b are non-null', () async { + check(MentionAutocompleteView.compareRecentMessageIds(2, 5)).isLessThan(0); + check(MentionAutocompleteView.compareRecentMessageIds(5, 2)).isGreaterThan(0); + check(MentionAutocompleteView.compareRecentMessageIds(5, 5)).equals(0); + }); + + test('one of a and b is null', () async { + check(MentionAutocompleteView.compareRecentMessageIds(null, 5)).isLessThan(0); + check(MentionAutocompleteView.compareRecentMessageIds(5, null)).isGreaterThan(0); + }); + + test('both of a and b are null', () async { + check(MentionAutocompleteView.compareRecentMessageIds(null, null)).equals(0); + }); + }); + group('MentionAutocompleteView.compareByDms', () { const idA = 1; const idB = 2; From 5cd4eee399e208e6b3db3cbc267edc0240f8605f Mon Sep 17 00:00:00 2001 From: Sayed Mahmood Sayedi Date: Mon, 1 Jul 2024 16:25:42 +0430 Subject: [PATCH 02/21] autocomplete: Add "recent activity in current topic/stream" criterion In @-mention autocomplete, users are suggested based on: 1. Recent activity in the current topic/stream. 2. Recent DM conversations. Note: By "recent activity" we mean recent messages sent to a topic/stream. Fixes part of: #228 --- lib/model/autocomplete.dart | 69 ++++++++- test/model/autocomplete_test.dart | 223 ++++++++++++++++++++++++++---- 2 files changed, 264 insertions(+), 28 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 797a7ec636..558e101ac9 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -187,9 +187,74 @@ class MentionAutocompleteView extends ChangeNotifier { required PerAccountStore store, required Narrow narrow, }) { - assert(narrow is! CombinedFeedNarrow); + final (int?, String?) streamAndTopic; + switch (narrow) { + case StreamNarrow(): + streamAndTopic = (narrow.streamId, null); + case TopicNarrow(): + streamAndTopic = (narrow.streamId, narrow.topic); + case DmNarrow(): + streamAndTopic = (null, null); + case CombinedFeedNarrow(): + assert(false, 'No compose box, thus no autocomplete is available in ${narrow.runtimeType}.'); + streamAndTopic = (null, null); + } + + final (streamId, topic) = streamAndTopic; return store.users.values.toList() - ..sort((userA, userB) => compareByDms(userA, userB, store: store)); + ..sort((userA, userB) => _compareByRelevance(userA, userB, + streamId: streamId, + topic: topic, + store: store)); + } + + static int _compareByRelevance(User userA, User userB, { + required int? streamId, + required String? topic, + required PerAccountStore store, + }) { + if (streamId != null) { + final result = compareByRecency(userA, userB, + streamId: streamId, + topic: topic, + store: store); + if (result != 0) return result; + } + return compareByDms(userA, userB, store: store); + } + + /// Determines which of the two users has more recent activity (messages sent + /// recently) in the topic/stream. + /// + /// First checks for the activity in [topic] if provided. + /// + /// If no [topic] is provided, or there is no activity in the topic at all, + /// then checks for the activity in the stream with [streamId]. + /// + /// Returns a negative number if [userA] has more recent activity than [userB], + /// returns a positive number if [userB] has more recent activity than [userA], + /// and returns `0` if both [userA] and [userB] have no activity at all. + @visibleForTesting + static int compareByRecency(User userA, User userB, { + required int streamId, + required String? topic, + required PerAccountStore store, + }) { + final recentSenders = store.recentSenders; + if (topic != null) { + final result = -compareRecentMessageIds( + recentSenders.latestMessageIdOfSenderInTopic( + streamId: streamId, topic: topic, senderId: userA.userId), + recentSenders.latestMessageIdOfSenderInTopic( + streamId: streamId, topic: topic, senderId: userB.userId)); + if (result != 0) return result; + } + + final aMessageId = recentSenders.latestMessageIdOfSenderInStream( + streamId: streamId, senderId: userA.userId); + final bMessageId = recentSenders.latestMessageIdOfSenderInStream( + streamId: streamId, senderId: userB.userId); + return -compareRecentMessageIds(aMessageId, bMessageId); } /// Determines which of the two users is more recent in DM conversations. diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 40576d1ff5..7b138bdba7 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -8,11 +8,14 @@ import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/autocomplete.dart'; +import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/compose_box.dart'; +import '../api/fake_api.dart'; import '../example_data.dart' as eg; +import 'message_list_test.dart'; import 'test_store.dart'; import 'autocomplete_checks.dart'; @@ -359,10 +362,12 @@ void main() { Future prepare({ List users = const [], List dmConversations = const [], + List messages = const [], }) async { store = eg.store(initialSnapshot: eg.initialSnapshot( recentPrivateConversations: dmConversations)); await store.addUsers(users); + await store.addMessages(messages); } group('MentionAutocompleteView.compareRecentMessageIds', () { @@ -382,6 +387,61 @@ void main() { }); }); + group('MentionAutocompleteView.compareByRecency', () { + final userA = eg.otherUser; + final userB = eg.thirdUser; + final stream = eg.stream(); + const topic1 = 'topic1'; + const topic2 = 'topic2'; + + Message message(User sender, String topic) { + return eg.streamMessage( + sender: sender, + stream: stream, + topic: topic, + ); + } + + int compareAB({required String? topic}) { + return MentionAutocompleteView.compareByRecency(userA, userB, + streamId: stream.streamId, + topic: topic, + store: store, + ); + } + + test('prioritizes the user with more recent activity in the topic', () async { + await prepare(messages: [ + message(userA, topic1), + message(userB, topic1), + ]); + check(compareAB(topic: topic1)).isGreaterThan(0); + }); + + test('no activity in topic -> prioritizes the user with more recent ' + 'activity in the stream', () async { + await prepare(messages: [ + message(userA, topic1), + message(userB, topic1), + ]); + check(compareAB(topic: topic2)).isGreaterThan(0); + }); + + test('no topic provided -> prioritizes the user with more recent ' + 'activity in the stream', () async { + await prepare(messages: [ + message(userA, topic1), + message(userB, topic2), + ]); + check(compareAB(topic: null)).isGreaterThan(0); + }); + + test('no activity in topic/stream -> prioritizes none', () async { + await prepare(messages: []); + check(compareAB(topic: null)).equals(0); + }); + }); + group('MentionAutocompleteView.compareByDms', () { const idA = 1; const idB = 2; @@ -437,24 +497,37 @@ void main() { group('autocomplete suggests relevant users in the intended order', () { // The order should be: - // 1. Users most recent in the DM conversations + // 1. Users most recent in the current topic/stream. + // 2. Users most recent in the DM conversations. + + final stream = eg.stream(); + const topic = 'topic'; + final streamNarrow = StreamNarrow(stream.streamId); + final topicNarrow = TopicNarrow(stream.streamId, topic); + final dmNarrow = DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId); + + final users = List.generate(5, (i) => eg.user(userId: i)); + + final dmConversations = [ + RecentDmConversation(userIds: [3], maxMessageId: 300), + RecentDmConversation(userIds: [0], maxMessageId: 200), + RecentDmConversation(userIds: [0, 1], maxMessageId: 100), + ]; + + StreamMessage streamMessage({required int id, required int senderId, String? topic}) => + eg.streamMessage(id: id, sender: users[senderId], topic: topic, stream: stream); + + final messages = [ + streamMessage(id: 50, senderId: 0, topic: topic), + streamMessage(id: 60, senderId: 4), + ]; + + Future prepareStore({bool includeMessageHistory = false}) async { + await prepare(users: users, dmConversations: dmConversations, + messages: includeMessageHistory ? messages : []); + } Future checkResultsIn(Narrow narrow, {required List expected}) async { - final users = [ - eg.user(userId: 0), - eg.user(userId: 1), - eg.user(userId: 2), - eg.user(userId: 3), - eg.user(userId: 4), - ]; - - final dmConversations = [ - RecentDmConversation(userIds: [3], maxMessageId: 300), - RecentDmConversation(userIds: [0], maxMessageId: 200), - RecentDmConversation(userIds: [0, 1], maxMessageId: 100), - ]; - - await prepare(users: users, dmConversations: dmConversations); final view = MentionAutocompleteView.init(store: store, narrow: narrow); bool done = false; @@ -467,22 +540,120 @@ void main() { check(results).deepEquals(expected); } - test('StreamNarrow', () async { - await checkResultsIn(const StreamNarrow(1), expected: [3, 0, 1, 2, 4]); - }); + group('StreamNarrow & TopicNarrow', () { + late FakeApiConnection connection; + late MessageListView messageList; + + Future fetchInitialMessagesIn(Narrow narrow) async { + connection = store.connection as FakeApiConnection; + connection.prepare(json: newestResult( + foundOldest: false, + messages: narrow is StreamNarrow + ? messages + : messages.where((m) => m.topic == topic).toList(), + ).toJson()); + messageList = MessageListView.init(store: store, narrow: narrow); + await messageList.fetchInitial(); + } - test('TopicNarrow', () async { - await checkResultsIn(const TopicNarrow(1, 'topic'), expected: [3, 0, 1, 2, 4]); + Future checkInitialResultsIn(Narrow narrow, + {required List expected, bool includeStream = false}) async { + assert(narrow is! StreamNarrow || !includeStream); + await prepareStore(includeMessageHistory: includeStream); + await fetchInitialMessagesIn(narrow); + await checkResultsIn(narrow, expected: expected); + } + + test('StreamNarrow', () async { + await checkInitialResultsIn(streamNarrow, expected: [4, 0, 3, 1, 2]); + }); + + test('StreamNarrow, new message arrives', () async { + await checkInitialResultsIn(streamNarrow, expected: [4, 0, 3, 1, 2]); + + // Until now, latest message id in [stream] is 60. + await store.addMessage(streamMessage(id: 70, senderId: 2)); + + await checkResultsIn(streamNarrow, expected: [2, 4, 0, 3, 1]); + }); + + test('StreamNarrow, a batch of older messages arrives', () async { + await checkInitialResultsIn(streamNarrow, expected: [4, 0, 3, 1, 2]); + + // Until now, oldest message id in [stream] is 50. + final oldMessages = [ + streamMessage(id: 30, senderId: 1), + streamMessage(id: 40, senderId: 2), + ]; + connection.prepare(json: olderResult( + anchor: 50, foundOldest: false, + messages: oldMessages, + ).toJson()); + await messageList.fetchOlder(); + + await checkResultsIn(streamNarrow, expected: [4, 0, 2, 1, 3]); + }); + + test('TopicNarrow, no other messages are in stream', () async { + await checkInitialResultsIn(topicNarrow, expected: [0, 3, 1, 2, 4]); + }); + + test('TopicNarrow, other messages are in stream', () async { + await checkInitialResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2], + includeStream: true); + }); + + test('TopicNarrow, new message arrives', () async { + await checkInitialResultsIn(topicNarrow, expected: [0, 3, 1, 2, 4]); + + // Until now, latest message id in [topic] is 50. + await store.addMessage(streamMessage(id: 60, senderId: 2, topic: topic)); + + await checkResultsIn(topicNarrow, expected: [2, 0, 3, 1, 4]); + }); + + test('TopicNarrow, a batch of older messages arrives', () async { + await checkInitialResultsIn(topicNarrow, expected: [0, 3, 1, 2, 4]); + + // Until now, oldest message id in [topic] is 50. + final oldMessages = [ + streamMessage(id: 30, senderId: 2, topic: topic), + streamMessage(id: 40, senderId: 4, topic: topic), + ]; + connection.prepare(json: olderResult( + anchor: 50, foundOldest: false, + messages: oldMessages, + ).toJson()); + + await messageList.fetchOlder(); + await checkResultsIn(topicNarrow, expected: [0, 4, 2, 3, 1]); + }); }); - test('DmNarrow', () async { - await checkResultsIn( - DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId), - expected: [3, 0, 1, 2, 4], - ); + group('DmNarrow', () { + test('DmNarrow, with no topic/stream message history', () async { + await prepareStore(); + await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]); + }); + + test('DmNarrow, with topic/stream message history', () async { + await prepareStore(includeMessageHistory: true); + await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]); + }); + + test('DmNarrow, new message arrives', () async { + await prepareStore(); + await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]); + + // Until now, latest message id in recent DMs is 300. + await store.addMessage(eg.dmMessage(id: 400, from: users[1], to: [eg.selfUser])); + + await checkResultsIn(dmNarrow, expected: [1, 3, 0, 2, 4]); + }); }); test('CombinedFeedNarrow', () async { + await prepareStore(); // As we do not expect a compose box in [CombinedFeedNarrow], it should // not proceed to show any results. await check(checkResultsIn( From adeb01200b0a0b136bd27a0e3b12b2e5da2ae48a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:12:43 -0700 Subject: [PATCH 03/21] autocomplete [nfc]: Format second half of compareByRecency same way as first --- lib/model/autocomplete.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 558e101ac9..745cc00811 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -250,11 +250,11 @@ class MentionAutocompleteView extends ChangeNotifier { if (result != 0) return result; } - final aMessageId = recentSenders.latestMessageIdOfSenderInStream( - streamId: streamId, senderId: userA.userId); - final bMessageId = recentSenders.latestMessageIdOfSenderInStream( - streamId: streamId, senderId: userB.userId); - return -compareRecentMessageIds(aMessageId, bMessageId); + return -compareRecentMessageIds( + recentSenders.latestMessageIdOfSenderInStream( + streamId: streamId, senderId: userA.userId), + recentSenders.latestMessageIdOfSenderInStream( + streamId: streamId, senderId: userB.userId)); } /// Determines which of the two users is more recent in DM conversations. From a0d0248021a83b64cd8e296c22c9fab8aec4538e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:24:27 -0700 Subject: [PATCH 04/21] autocomplete [nfc]: Set streamId and topic as variables directly I think this is a bit more idiomatic than setting a record which we promptly destructure; and it doesn't come out any longer. --- lib/model/autocomplete.dart | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 745cc00811..765336fa20 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -187,20 +187,20 @@ class MentionAutocompleteView extends ChangeNotifier { required PerAccountStore store, required Narrow narrow, }) { - final (int?, String?) streamAndTopic; + int? streamId; + String? topic; switch (narrow) { case StreamNarrow(): - streamAndTopic = (narrow.streamId, null); + streamId = narrow.streamId; case TopicNarrow(): - streamAndTopic = (narrow.streamId, narrow.topic); + streamId = narrow.streamId; + topic = narrow.topic; case DmNarrow(): - streamAndTopic = (null, null); + break; case CombinedFeedNarrow(): assert(false, 'No compose box, thus no autocomplete is available in ${narrow.runtimeType}.'); - streamAndTopic = (null, null); } - final (streamId, topic) = streamAndTopic; return store.users.values.toList() ..sort((userA, userB) => _compareByRelevance(userA, userB, streamId: streamId, From 3c7096f0b8afb21a058b9cc50d27ea5374491529 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:00:30 -0700 Subject: [PATCH 05/21] autocomplete test [nfc]: Tighten formatting in compareByRecency tests --- test/model/autocomplete_test.dart | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 7b138bdba7..203bef8339 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -395,44 +395,28 @@ void main() { const topic2 = 'topic2'; Message message(User sender, String topic) { - return eg.streamMessage( - sender: sender, - stream: stream, - topic: topic, - ); + return eg.streamMessage(sender: sender, stream: stream, topic: topic); } int compareAB({required String? topic}) { return MentionAutocompleteView.compareByRecency(userA, userB, - streamId: stream.streamId, - topic: topic, - store: store, - ); + streamId: stream.streamId, topic: topic, store: store); } test('prioritizes the user with more recent activity in the topic', () async { - await prepare(messages: [ - message(userA, topic1), - message(userB, topic1), - ]); + await prepare(messages: [message(userA, topic1), message(userB, topic1)]); check(compareAB(topic: topic1)).isGreaterThan(0); }); test('no activity in topic -> prioritizes the user with more recent ' 'activity in the stream', () async { - await prepare(messages: [ - message(userA, topic1), - message(userB, topic1), - ]); + await prepare(messages: [message(userA, topic1), message(userB, topic1)]); check(compareAB(topic: topic2)).isGreaterThan(0); }); test('no topic provided -> prioritizes the user with more recent ' 'activity in the stream', () async { - await prepare(messages: [ - message(userA, topic1), - message(userB, topic2), - ]); + await prepare(messages: [message(userA, topic1), message(userB, topic2)]); check(compareAB(topic: null)).isGreaterThan(0); }); From a5b3cc89e7cf9ba59b3825cad2577ccea21ac26f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:04:21 -0700 Subject: [PATCH 06/21] autocomplete test: Check both directions in compareByRecency tests --- test/model/autocomplete_test.dart | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 203bef8339..c8d7ec64e4 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -399,8 +399,16 @@ void main() { } int compareAB({required String? topic}) { - return MentionAutocompleteView.compareByRecency(userA, userB, + final resultAB = MentionAutocompleteView.compareByRecency(userA, userB, streamId: stream.streamId, topic: topic, store: store); + final resultBA = MentionAutocompleteView.compareByRecency(userB, userA, + streamId: stream.streamId, topic: topic, store: store); + switch (resultAB) { + case <0: check(resultBA).isGreaterThan(0); + case >0: check(resultBA).isLessThan(0); + default: check(resultBA).equals(0); + } + return resultAB; } test('prioritizes the user with more recent activity in the topic', () async { From 6e72a5bc0874521f1089af1d92a617ca8356f3e3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:09:18 -0700 Subject: [PATCH 07/21] autocomplete test [nfc]: Tighten names of compareByRecency tests This way they fit on a line. --- test/model/autocomplete_test.dart | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index c8d7ec64e4..d5e918271c 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -411,24 +411,22 @@ void main() { return resultAB; } - test('prioritizes the user with more recent activity in the topic', () async { + test('favor user most recent in topic', () async { await prepare(messages: [message(userA, topic1), message(userB, topic1)]); check(compareAB(topic: topic1)).isGreaterThan(0); }); - test('no activity in topic -> prioritizes the user with more recent ' - 'activity in the stream', () async { + test('no activity in topic -> favor user most recent in stream', () async { await prepare(messages: [message(userA, topic1), message(userB, topic1)]); check(compareAB(topic: topic2)).isGreaterThan(0); }); - test('no topic provided -> prioritizes the user with more recent ' - 'activity in the stream', () async { + test('no topic provided -> favor user most recent in stream', () async { await prepare(messages: [message(userA, topic1), message(userB, topic2)]); check(compareAB(topic: null)).isGreaterThan(0); }); - test('no activity in topic/stream -> prioritizes none', () async { + test('no activity in topic/stream -> favor none', () async { await prepare(messages: []); check(compareAB(topic: null)).equals(0); }); From d91eccaf7c792f4d68fa3b3b2cb5918eec7f1314 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:09:57 -0700 Subject: [PATCH 08/21] autocomplete test: Check compareByRecency uses per-topic recency Without this, the method's other unit tests (the other tests in this group) pass even if the method were to ignore its `topic` parameter and only use the per-stream data. --- test/model/autocomplete_test.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index d5e918271c..3c3ea7d047 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -416,6 +416,12 @@ void main() { check(compareAB(topic: topic1)).isGreaterThan(0); }); + test('favor most recent in topic ahead of most recent in stream', () async { + await prepare(messages: [ + message(userA, topic1), message(userB, topic1), message(userA, topic2)]); + check(compareAB(topic: topic1)).isGreaterThan(0); + }); + test('no activity in topic -> favor user most recent in stream', () async { await prepare(messages: [message(userA, topic1), message(userB, topic1)]); check(compareAB(topic: topic2)).isGreaterThan(0); From 72caacfb509738d1cac78d9b05d99f4f0eeb721b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 14:19:46 -0700 Subject: [PATCH 09/21] autocomplete test [nfc]: Deduplicate in test names The full names of these tests were like this: MentionAutocompleteView sorting users results MentionAutocompleteView.compareRecentMessageIds both a and b are non-null MentionAutocompleteView sorting users results MentionAutocompleteView.compareRecentMessageIds one of a and b is null MentionAutocompleteView sorting users results MentionAutocompleteView.compareRecentMessageIds both of a and b are null MentionAutocompleteView sorting users results MentionAutocompleteView.compareByRecency favor user most recent in topic To see the names, one can run a command like: $ flutter test test/model/autocomplete_test.dart -r expanded We can cut the second mention of MentionAutocompleteView. --- test/model/autocomplete_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 3c3ea7d047..93988670cd 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -370,7 +370,7 @@ void main() { await store.addMessages(messages); } - group('MentionAutocompleteView.compareRecentMessageIds', () { + group('compareRecentMessageIds', () { test('both a and b are non-null', () async { check(MentionAutocompleteView.compareRecentMessageIds(2, 5)).isLessThan(0); check(MentionAutocompleteView.compareRecentMessageIds(5, 2)).isGreaterThan(0); @@ -387,7 +387,7 @@ void main() { }); }); - group('MentionAutocompleteView.compareByRecency', () { + group('compareByRecency', () { final userA = eg.otherUser; final userB = eg.thirdUser; final stream = eg.stream(); @@ -438,7 +438,7 @@ void main() { }); }); - group('MentionAutocompleteView.compareByDms', () { + group('compareByDms', () { const idA = 1; const idB = 2; From 4762e4ea9297b912eadb4d32048f888854523c33 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 15:06:48 -0700 Subject: [PATCH 10/21] autocomplete [nfc]: Expose debugCompareUsers, for testing --- lib/model/autocomplete.dart | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 765336fa20..9a9cea0333 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -186,6 +186,30 @@ class MentionAutocompleteView extends ChangeNotifier { static List _usersByRelevance({ required PerAccountStore store, required Narrow narrow, + }) { + return store.users.values.toList() + ..sort(_comparator(store: store, narrow: narrow)); + } + + /// Compare the users the same way they would be sorted as + /// autocomplete candidates. + /// + /// This behaves the same as the comparator used for sorting in + /// [_usersByRelevance], but calling this for each comparison would be a bit + /// less efficient because some of the logic is independent of the users and + /// can be precomputed. + /// + /// This is useful for tests in order to distinguish "A comes before B" + /// from "A ranks equal to B, and the sort happened to put A before B", + /// particularly because [List.sort] makes no guarantees about the order + /// of items that compare equal. + int debugCompareUsers(User userA, User userB) { + return _comparator(store: store, narrow: narrow)(userA, userB); + } + + static int Function(User, User) _comparator({ + required PerAccountStore store, + required Narrow narrow, }) { int? streamId; String? topic; @@ -200,12 +224,9 @@ class MentionAutocompleteView extends ChangeNotifier { case CombinedFeedNarrow(): assert(false, 'No compose box, thus no autocomplete is available in ${narrow.runtimeType}.'); } - - return store.users.values.toList() - ..sort((userA, userB) => _compareByRelevance(userA, userB, - streamId: streamId, - topic: topic, - store: store)); + return (userA, userB) => _compareByRelevance(userA, userB, + streamId: streamId, topic: topic, + store: store); } static int _compareByRelevance(User userA, User userB, { From bfce142a2133c8ce841cd1ad59a34c22d0d5a633 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 15:48:58 -0700 Subject: [PATCH 11/21] autocomplete test: Unit tests of between-signals logic, for each narrow type --- test/model/autocomplete_test.dart | 76 +++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 93988670cd..19e9f812f1 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -491,6 +491,82 @@ void main() { }); }); + group('ranking across signals', () { + void checkPrecedes(Narrow narrow, User userA, Iterable usersB) { + final view = MentionAutocompleteView.init(store: store, narrow: narrow); + for (final userB in usersB) { + check(view.debugCompareUsers(userA, userB)).isLessThan(0); + check(view.debugCompareUsers(userB, userA)).isGreaterThan(0); + } + } + + void checkRankEqual(Narrow narrow, List users) { + final view = MentionAutocompleteView.init(store: store, narrow: narrow); + for (int i = 0; i < users.length; i++) { + for (int j = i + 1; j < users.length; j++) { + check(view.debugCompareUsers(users[i], users[j])).equals(0); + check(view.debugCompareUsers(users[j], users[i])).equals(0); + } + } + } + + test('TopicNarrow: topic recency > stream recency > DM recency', () async { + final users = List.generate(5, (i) => eg.user()); + final stream = eg.stream(); + final narrow = TopicNarrow(stream.streamId, 'this'); + await prepare(users: users, messages: [ + eg.streamMessage(sender: users[1], stream: stream, topic: 'this'), + eg.streamMessage(sender: users[0], stream: stream, topic: 'this'), + eg.streamMessage(sender: users[2], stream: stream, topic: 'other'), + eg.streamMessage(sender: users[1], stream: stream, topic: 'other'), + eg.dmMessage(from: users[3], to: [users[4], eg.selfUser]), + eg.dmMessage(from: users[2], to: [eg.selfUser]), + ]); + checkPrecedes(narrow, users[0], users.skip(1)); + checkPrecedes(narrow, users[1], users.skip(2)); + checkPrecedes(narrow, users[2], users.skip(3)); + checkRankEqual(narrow, [users[3], users[4]]); + }); + + test('StreamNarrow: stream recency > DM recency', () async { + final users = List.generate(4, (i) => eg.user()); + final stream = eg.stream(); + final narrow = StreamNarrow(stream.streamId); + await prepare(users: users, messages: [ + eg.streamMessage(sender: users[1], stream: stream), + eg.streamMessage(sender: users[0], stream: stream), + eg.dmMessage(from: users[2], to: [users[3], eg.selfUser]), + eg.dmMessage(from: users[1], to: [eg.selfUser]), + ]); + checkPrecedes(narrow, users[0], users.skip(1)); + checkPrecedes(narrow, users[1], users.skip(2)); + checkRankEqual(narrow, [users[2], users[3]]); + }); + + test('DmNarrow: DM recency > this-conversation recency or stream recency', () async { + final users = List.generate(4, (i) => eg.user()); + await prepare(users: users, messages: [ + eg.dmMessage(from: users[3], to: [eg.selfUser]), + eg.dmMessage(from: users[1], to: [users[2], eg.selfUser]), + eg.dmMessage(from: users[0], to: [eg.selfUser]), + eg.streamMessage(sender: users[1]), + eg.streamMessage(sender: users[2]), + eg.streamMessage(sender: users[3]), + ]); + for (final narrow in [ + DmNarrow.withUser(users[3].userId, selfUserId: eg.selfUser.userId), + DmNarrow.withOtherUsers([users[1].userId, users[2].userId], + selfUserId: eg.selfUser.userId), + DmNarrow.withUser(users[1].userId, selfUserId: eg.selfUser.userId), + ]) { + checkPrecedes(narrow, users[0], users.skip(1)); + checkRankEqual(narrow, [users[1], users[2]]); + checkPrecedes(narrow, users[1], users.skip(3)); + checkPrecedes(narrow, users[2], users.skip(3)); + } + }); + }); + group('autocomplete suggests relevant users in the intended order', () { // The order should be: // 1. Users most recent in the current topic/stream. From 37e643d159c801ade622b18139d9ea14772dea9a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 17:12:54 -0700 Subject: [PATCH 12/21] autocomplete test: Fix test that almost can't fail, of CombinedFeedNarrow If for example one substitutes some other narrow like `topicNarrow` in this `checkResultsIn` call, the test still passes. That's because now the autocomplete does return results... but they don't match the arbitrary `expected` list that was passed in, so the inner `check` call fails, and the outer one succeeds because of that exception. Instead, check more specifically that the MentionAutocompleteView.init call throws. --- test/model/autocomplete_test.dart | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 19e9f812f1..559b9b2dd7 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -565,6 +565,13 @@ void main() { checkPrecedes(narrow, users[2], users.skip(3)); } }); + + test('CombinedFeedNarrow gives error', () async { + await prepare(users: [eg.user(), eg.user()], messages: []); + const narrow = CombinedFeedNarrow(); + check(() => MentionAutocompleteView.init(store: store, narrow: narrow)) + .throws(); + }); }); group('autocomplete suggests relevant users in the intended order', () { @@ -723,16 +730,6 @@ void main() { await checkResultsIn(dmNarrow, expected: [1, 3, 0, 2, 4]); }); }); - - test('CombinedFeedNarrow', () async { - await prepareStore(); - // As we do not expect a compose box in [CombinedFeedNarrow], it should - // not proceed to show any results. - await check(checkResultsIn( - const CombinedFeedNarrow(), - expected: [0, 1, 2, 3, 4]) - ).throws(); - }); }); }); } From 3823ec494225ed22eda33de6ecc3306dd92c1300 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 17:03:43 -0700 Subject: [PATCH 13/21] autocomplete test: Cut tests that could flake due to indeterminate sort The [List.sort] method is documented as being not necessarily stable, meaning that elements that compare equal could end up in an arbitrary order in the result. Here, that means that users which aren't distinguished by recency in the topic or stream or in DM conversations can appear in any order in the autocomplete results. Several of these test cases were therefore vulnerable to flaking because users 2 and 4 have no DMs and (in these tests) no messages in the stream or topic. I think what they cover is now also covered by the "ranking across signals" tests above, together with the tests for RecentSenders and RecentDmConversations that check those data structures' handling of events. So just cut the tests. --- test/model/autocomplete_test.dart | 53 ------------------------------- 1 file changed, 53 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 559b9b2dd7..d821f96e9a 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -583,7 +583,6 @@ void main() { const topic = 'topic'; final streamNarrow = StreamNarrow(stream.streamId); final topicNarrow = TopicNarrow(stream.streamId, topic); - final dmNarrow = DmNarrow.withUser(eg.selfUser.userId, selfUserId: eg.selfUser.userId); final users = List.generate(5, (i) => eg.user(userId: i)); @@ -673,62 +672,10 @@ void main() { await checkResultsIn(streamNarrow, expected: [4, 0, 2, 1, 3]); }); - test('TopicNarrow, no other messages are in stream', () async { - await checkInitialResultsIn(topicNarrow, expected: [0, 3, 1, 2, 4]); - }); - test('TopicNarrow, other messages are in stream', () async { await checkInitialResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2], includeStream: true); }); - - test('TopicNarrow, new message arrives', () async { - await checkInitialResultsIn(topicNarrow, expected: [0, 3, 1, 2, 4]); - - // Until now, latest message id in [topic] is 50. - await store.addMessage(streamMessage(id: 60, senderId: 2, topic: topic)); - - await checkResultsIn(topicNarrow, expected: [2, 0, 3, 1, 4]); - }); - - test('TopicNarrow, a batch of older messages arrives', () async { - await checkInitialResultsIn(topicNarrow, expected: [0, 3, 1, 2, 4]); - - // Until now, oldest message id in [topic] is 50. - final oldMessages = [ - streamMessage(id: 30, senderId: 2, topic: topic), - streamMessage(id: 40, senderId: 4, topic: topic), - ]; - connection.prepare(json: olderResult( - anchor: 50, foundOldest: false, - messages: oldMessages, - ).toJson()); - - await messageList.fetchOlder(); - await checkResultsIn(topicNarrow, expected: [0, 4, 2, 3, 1]); - }); - }); - - group('DmNarrow', () { - test('DmNarrow, with no topic/stream message history', () async { - await prepareStore(); - await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]); - }); - - test('DmNarrow, with topic/stream message history', () async { - await prepareStore(includeMessageHistory: true); - await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]); - }); - - test('DmNarrow, new message arrives', () async { - await prepareStore(); - await checkResultsIn(dmNarrow, expected: [3, 0, 1, 2, 4]); - - // Until now, latest message id in recent DMs is 300. - await store.addMessage(eg.dmMessage(id: 400, from: users[1], to: [eg.selfUser])); - - await checkResultsIn(dmNarrow, expected: [1, 3, 0, 2, 4]); - }); }); }); }); From a14ea8b20989c7371d149f09d3d7e8967a2eb2ee Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 17:25:06 -0700 Subject: [PATCH 14/21] autocomplete test: Highlight one end-to-end test, cut the others It's good to have an end-to-end test here that the actual autocomplete results reflect the sorting that's tested by all the unit tests in the rest of this file: a test that makes a MentionAutocompleteView, then gives it a query, and inspects the results. The several such test cases that were here, though, don't exercise any distinct scenarios when it comes to that end-to-end logic; they differ from each other only in ways that are exercised by the unit tests above, plus the tests for RecentSenders and RecentDmConversations that check those data structures' handling of events. So pick just one of these test cases and cut the rest. --- test/model/autocomplete_test.dart | 83 +++++++++---------------------- 1 file changed, 24 insertions(+), 59 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index d821f96e9a..0f92373c00 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -574,14 +574,13 @@ void main() { }); }); - group('autocomplete suggests relevant users in the intended order', () { + test('final results end-to-end', () async { // The order should be: // 1. Users most recent in the current topic/stream. // 2. Users most recent in the DM conversations. final stream = eg.stream(); const topic = 'topic'; - final streamNarrow = StreamNarrow(stream.streamId); final topicNarrow = TopicNarrow(stream.streamId, topic); final users = List.generate(5, (i) => eg.user(userId: i)); @@ -618,65 +617,31 @@ void main() { check(results).deepEquals(expected); } - group('StreamNarrow & TopicNarrow', () { - late FakeApiConnection connection; - late MessageListView messageList; - - Future fetchInitialMessagesIn(Narrow narrow) async { - connection = store.connection as FakeApiConnection; - connection.prepare(json: newestResult( - foundOldest: false, - messages: narrow is StreamNarrow - ? messages - : messages.where((m) => m.topic == topic).toList(), - ).toJson()); - messageList = MessageListView.init(store: store, narrow: narrow); - await messageList.fetchInitial(); - } - - Future checkInitialResultsIn(Narrow narrow, - {required List expected, bool includeStream = false}) async { - assert(narrow is! StreamNarrow || !includeStream); - await prepareStore(includeMessageHistory: includeStream); - await fetchInitialMessagesIn(narrow); - await checkResultsIn(narrow, expected: expected); - } - - test('StreamNarrow', () async { - await checkInitialResultsIn(streamNarrow, expected: [4, 0, 3, 1, 2]); - }); - - test('StreamNarrow, new message arrives', () async { - await checkInitialResultsIn(streamNarrow, expected: [4, 0, 3, 1, 2]); - - // Until now, latest message id in [stream] is 60. - await store.addMessage(streamMessage(id: 70, senderId: 2)); - - await checkResultsIn(streamNarrow, expected: [2, 4, 0, 3, 1]); - }); - - test('StreamNarrow, a batch of older messages arrives', () async { - await checkInitialResultsIn(streamNarrow, expected: [4, 0, 3, 1, 2]); - - // Until now, oldest message id in [stream] is 50. - final oldMessages = [ - streamMessage(id: 30, senderId: 1), - streamMessage(id: 40, senderId: 2), - ]; - connection.prepare(json: olderResult( - anchor: 50, foundOldest: false, - messages: oldMessages, - ).toJson()); - await messageList.fetchOlder(); + late FakeApiConnection connection; + late MessageListView messageList; + + Future fetchInitialMessagesIn(Narrow narrow) async { + connection = store.connection as FakeApiConnection; + connection.prepare(json: newestResult( + foundOldest: false, + messages: narrow is StreamNarrow + ? messages + : messages.where((m) => m.topic == topic).toList(), + ).toJson()); + messageList = MessageListView.init(store: store, narrow: narrow); + await messageList.fetchInitial(); + } - await checkResultsIn(streamNarrow, expected: [4, 0, 2, 1, 3]); - }); + Future checkInitialResultsIn(Narrow narrow, + {required List expected, bool includeStream = false}) async { + assert(narrow is! StreamNarrow || !includeStream); + await prepareStore(includeMessageHistory: includeStream); + await fetchInitialMessagesIn(narrow); + await checkResultsIn(narrow, expected: expected); + } - test('TopicNarrow, other messages are in stream', () async { - await checkInitialResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2], - includeStream: true); - }); - }); + await checkInitialResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2], + includeStream: true); }); }); } From 1e781dd6a62cf610b358f6dd2093b1008fce3f83 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 17:33:15 -0700 Subject: [PATCH 15/21] autocomplete test [nfc]: Do constant-folding in end-to-end test Now that many of these helpers have just one call site, they can be simplified. --- test/model/autocomplete_test.dart | 42 +++++++++++-------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 0f92373c00..7979ec433a 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -599,9 +599,19 @@ void main() { streamMessage(id: 60, senderId: 4), ]; - Future prepareStore({bool includeMessageHistory = false}) async { + Future prepareStore() async { await prepare(users: users, dmConversations: dmConversations, - messages: includeMessageHistory ? messages : []); + messages: messages); + } + + Future fetchInitialMessagesIn(Narrow narrow) async { + final connection = store.connection as FakeApiConnection; + connection.prepare(json: newestResult( + foundOldest: false, + messages: messages.where((m) => m.topic == topic).toList(), + ).toJson()); + final messageList = MessageListView.init(store: store, narrow: narrow); + await messageList.fetchInitial(); } Future checkResultsIn(Narrow narrow, {required List expected}) async { @@ -617,31 +627,9 @@ void main() { check(results).deepEquals(expected); } - late FakeApiConnection connection; - late MessageListView messageList; - - Future fetchInitialMessagesIn(Narrow narrow) async { - connection = store.connection as FakeApiConnection; - connection.prepare(json: newestResult( - foundOldest: false, - messages: narrow is StreamNarrow - ? messages - : messages.where((m) => m.topic == topic).toList(), - ).toJson()); - messageList = MessageListView.init(store: store, narrow: narrow); - await messageList.fetchInitial(); - } - - Future checkInitialResultsIn(Narrow narrow, - {required List expected, bool includeStream = false}) async { - assert(narrow is! StreamNarrow || !includeStream); - await prepareStore(includeMessageHistory: includeStream); - await fetchInitialMessagesIn(narrow); - await checkResultsIn(narrow, expected: expected); - } - - await checkInitialResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2], - includeStream: true); + await prepareStore(); + await fetchInitialMessagesIn(topicNarrow); + await checkResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2]); }); }); } From 739fa8491b113b1f3701f5b2faa75fa0cb9bfd8b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 20:53:26 -0700 Subject: [PATCH 16/21] autocomplete test: Fix user IDs of zero Zulip user IDs (like channel/stream IDs, message IDs, and so on) are positive integers. So avoiding zero helps keep the test data realistic. --- test/model/autocomplete_test.dart | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 7979ec433a..de1800395f 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -583,20 +583,20 @@ void main() { const topic = 'topic'; final topicNarrow = TopicNarrow(stream.streamId, topic); - final users = List.generate(5, (i) => eg.user(userId: i)); + final users = List.generate(5, (i) => eg.user(userId: 1 + i)); final dmConversations = [ - RecentDmConversation(userIds: [3], maxMessageId: 300), - RecentDmConversation(userIds: [0], maxMessageId: 200), - RecentDmConversation(userIds: [0, 1], maxMessageId: 100), + RecentDmConversation(userIds: [4], maxMessageId: 300), + RecentDmConversation(userIds: [1], maxMessageId: 200), + RecentDmConversation(userIds: [1, 2], maxMessageId: 100), ]; StreamMessage streamMessage({required int id, required int senderId, String? topic}) => - eg.streamMessage(id: id, sender: users[senderId], topic: topic, stream: stream); + eg.streamMessage(id: id, sender: users[senderId-1], topic: topic, stream: stream); final messages = [ - streamMessage(id: 50, senderId: 0, topic: topic), - streamMessage(id: 60, senderId: 4), + streamMessage(id: 50, senderId: 1, topic: topic), + streamMessage(id: 60, senderId: 5), ]; Future prepareStore() async { @@ -629,7 +629,7 @@ void main() { await prepareStore(); await fetchInitialMessagesIn(topicNarrow); - await checkResultsIn(topicNarrow, expected: [0, 4, 3, 1, 2]); + await checkResultsIn(topicNarrow, expected: [1, 5, 4, 2, 3]); }); }); } From 2b8d8480fbbd30afc5c2ecf0f6f92e41f7fde2da Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 20:47:40 -0700 Subject: [PATCH 17/21] autocomplete test: Separate getting results from checking in end-to-end test This is nearly NFC, but also adds a `dispose` call to tidy things up after getting the results. --- test/model/autocomplete_test.dart | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index de1800395f..a71da2ddc7 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -614,22 +614,24 @@ void main() { await messageList.fetchInitial(); } - Future checkResultsIn(Narrow narrow, {required List expected}) async { - final view = MentionAutocompleteView.init(store: store, narrow: narrow); - + Future> getResults( + Narrow narrow, MentionAutocompleteQuery query) async { bool done = false; + final view = MentionAutocompleteView.init(store: store, narrow: narrow); view.addListener(() { done = true; }); - view.query = MentionAutocompleteQuery(''); + view.query = query; await Future(() {}); check(done).isTrue(); final results = view.results .map((e) => (e as UserMentionAutocompleteResult).userId); - check(results).deepEquals(expected); + view.dispose(); + return results; } await prepareStore(); await fetchInitialMessagesIn(topicNarrow); - await checkResultsIn(topicNarrow, expected: [1, 5, 4, 2, 3]); + check(await getResults(topicNarrow, MentionAutocompleteQuery(''))) + .deepEquals([1, 5, 4, 2, 3]); }); }); } From 0288ee439737bd122c6b12546bbe2a5471e9c46c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 20:55:47 -0700 Subject: [PATCH 18/21] autocomplete test: Test ranking applies in presence of filters, too --- test/model/autocomplete_test.dart | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index a71da2ddc7..bc4349b005 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -583,7 +583,13 @@ void main() { const topic = 'topic'; final topicNarrow = TopicNarrow(stream.streamId, topic); - final users = List.generate(5, (i) => eg.user(userId: 1 + i)); + final users = [ + eg.user(userId: 1, fullName: 'User One'), + eg.user(userId: 2, fullName: 'User Two'), + eg.user(userId: 3, fullName: 'User Three'), + eg.user(userId: 4, fullName: 'User Four'), + eg.user(userId: 5, fullName: 'User Five'), + ]; final dmConversations = [ RecentDmConversation(userIds: [4], maxMessageId: 300), @@ -630,8 +636,14 @@ void main() { await prepareStore(); await fetchInitialMessagesIn(topicNarrow); + // Check the ranking of the full list of users. check(await getResults(topicNarrow, MentionAutocompleteQuery(''))) .deepEquals([1, 5, 4, 2, 3]); + // Check the ranking applies also to results filtered by a query. + check(await getResults(topicNarrow, MentionAutocompleteQuery('t'))) + .deepEquals([2, 3]); + check(await getResults(topicNarrow, MentionAutocompleteQuery('f'))) + .deepEquals([5, 4]); }); }); } From b436fb54389949b58fe0889b895320b4f110fd10 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 21:26:43 -0700 Subject: [PATCH 19/21] autocomplete test [nfc]: Bring details next to details in end-to-end test This way it's possible to look at the expected results at the end of this test and compare them with the test data that leads to those results, without scanning past a lot of other code that isn't related to why these particular results are the right answers. --- test/model/autocomplete_test.dart | 70 ++++++++++++++----------------- 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index bc4349b005..7bd7e808d4 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -575,46 +575,11 @@ void main() { }); test('final results end-to-end', () async { - // The order should be: - // 1. Users most recent in the current topic/stream. - // 2. Users most recent in the DM conversations. - - final stream = eg.stream(); - const topic = 'topic'; - final topicNarrow = TopicNarrow(stream.streamId, topic); - - final users = [ - eg.user(userId: 1, fullName: 'User One'), - eg.user(userId: 2, fullName: 'User Two'), - eg.user(userId: 3, fullName: 'User Three'), - eg.user(userId: 4, fullName: 'User Four'), - eg.user(userId: 5, fullName: 'User Five'), - ]; - - final dmConversations = [ - RecentDmConversation(userIds: [4], maxMessageId: 300), - RecentDmConversation(userIds: [1], maxMessageId: 200), - RecentDmConversation(userIds: [1, 2], maxMessageId: 100), - ]; - - StreamMessage streamMessage({required int id, required int senderId, String? topic}) => - eg.streamMessage(id: id, sender: users[senderId-1], topic: topic, stream: stream); - - final messages = [ - streamMessage(id: 50, senderId: 1, topic: topic), - streamMessage(id: 60, senderId: 5), - ]; - - Future prepareStore() async { - await prepare(users: users, dmConversations: dmConversations, - messages: messages); - } - - Future fetchInitialMessagesIn(Narrow narrow) async { + Future fetchInitialMessagesIn(Narrow narrow, List messages) async { final connection = store.connection as FakeApiConnection; connection.prepare(json: newestResult( foundOldest: false, - messages: messages.where((m) => m.topic == topic).toList(), + messages: messages, ).toJson()); final messageList = MessageListView.init(store: store, narrow: narrow); await messageList.fetchInitial(); @@ -634,11 +599,38 @@ void main() { return results; } - await prepareStore(); - await fetchInitialMessagesIn(topicNarrow); + final stream = eg.stream(); + const topic = 'topic'; + final topicNarrow = TopicNarrow(stream.streamId, topic); + + final users = [ + eg.user(userId: 1, fullName: 'User One'), + eg.user(userId: 2, fullName: 'User Two'), + eg.user(userId: 3, fullName: 'User Three'), + eg.user(userId: 4, fullName: 'User Four'), + eg.user(userId: 5, fullName: 'User Five'), + ]; + + final messages = [ + eg.streamMessage(id: 50, sender: users[1-1], stream: stream, topic: topic), + eg.streamMessage(id: 60, sender: users[5-1], stream: stream), + ]; + + await prepare(users: users, messages: messages, dmConversations: [ + RecentDmConversation(userIds: [4], maxMessageId: 300), + RecentDmConversation(userIds: [1], maxMessageId: 200), + RecentDmConversation(userIds: [1, 2], maxMessageId: 100), + ]); + await fetchInitialMessagesIn(topicNarrow, + messages.where((m) => m.topic == topic).toList()); + // Check the ranking of the full list of users. + // The order should be: + // 1. Users most recent in the current topic/stream. + // 2. Users most recent in the DM conversations. check(await getResults(topicNarrow, MentionAutocompleteQuery(''))) .deepEquals([1, 5, 4, 2, 3]); + // Check the ranking applies also to results filtered by a query. check(await getResults(topicNarrow, MentionAutocompleteQuery('t'))) .deepEquals([2, 3]); From 363dafa2236c28073edda15496738e9a2fb4777f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 21:29:54 -0700 Subject: [PATCH 20/21] autocomplete test [nfc]: Cut unneeded MessageListView in end-to-end test --- test/model/autocomplete_test.dart | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 7bd7e808d4..2d3b7a08b4 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -8,14 +8,11 @@ import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/autocomplete.dart'; -import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/compose_box.dart'; -import '../api/fake_api.dart'; import '../example_data.dart' as eg; -import 'message_list_test.dart'; import 'test_store.dart'; import 'autocomplete_checks.dart'; @@ -575,16 +572,6 @@ void main() { }); test('final results end-to-end', () async { - Future fetchInitialMessagesIn(Narrow narrow, List messages) async { - final connection = store.connection as FakeApiConnection; - connection.prepare(json: newestResult( - foundOldest: false, - messages: messages, - ).toJson()); - final messageList = MessageListView.init(store: store, narrow: narrow); - await messageList.fetchInitial(); - } - Future> getResults( Narrow narrow, MentionAutocompleteQuery query) async { bool done = false; @@ -611,18 +598,14 @@ void main() { eg.user(userId: 5, fullName: 'User Five'), ]; - final messages = [ + await prepare(users: users, messages: [ eg.streamMessage(id: 50, sender: users[1-1], stream: stream, topic: topic), eg.streamMessage(id: 60, sender: users[5-1], stream: stream), - ]; - - await prepare(users: users, messages: messages, dmConversations: [ + ], dmConversations: [ RecentDmConversation(userIds: [4], maxMessageId: 300), RecentDmConversation(userIds: [1], maxMessageId: 200), RecentDmConversation(userIds: [1, 2], maxMessageId: 100), ]); - await fetchInitialMessagesIn(topicNarrow, - messages.where((m) => m.topic == topic).toList()); // Check the ranking of the full list of users. // The order should be: From 3444a687ee5491406017037c24e7432220f32406 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 24 Jul 2024 21:31:49 -0700 Subject: [PATCH 21/21] autocomplete test: Make distinct topic explicitly distinct This test was relying on its `topic` variable (whose value is "topic") being different from the topic chosen by default by eg.streamMessage. It does happen to be different, but it could just as well not have been. Because the difference matters -- the test would fail if they happened to be the same -- the test case itself should be explicit about that. --- test/model/autocomplete_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 2d3b7a08b4..391d939ba7 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -600,7 +600,7 @@ void main() { await prepare(users: users, messages: [ eg.streamMessage(id: 50, sender: users[1-1], stream: stream, topic: topic), - eg.streamMessage(id: 60, sender: users[5-1], stream: stream), + eg.streamMessage(id: 60, sender: users[5-1], stream: stream, topic: 'other $topic'), ], dmConversations: [ RecentDmConversation(userIds: [4], maxMessageId: 300), RecentDmConversation(userIds: [1], maxMessageId: 200),