autocomplete: Add "recent senders criterion" to user-mention autocomplete

[sm-sayedi]

Besides recent activity in DMs, activity in the current topic/stream is also considered (as the first criterion) when suggesting users in @-mention autocomplete.

Note: This is the third PR in the series of PRs #608 is divided into, coming after #692. Next PR in the series: #849.

Fixes part of: #228

[gnprice]

Thanks @sm-sayedi for all your work on this!

For the model code, small comments below.

Then in the test code, please do a new self-review of your changes, in light of the comments on #692 and the commits I added to that branch. In particular look to:

• make each test case compact so it's easy to scan through them — in this case there are already helpers that go a long way, but the formatting can be adjusted for more compactness

• think of all the different scenarios you can that should be tested:

  • scenarios where the code's behavior should vary
  • scenarios where, given the way you did write the code, the path taken through the control flow will vary
  • possible bugs that an implementation of this could have if someone made a plausible mistake, or if part of your code were accidentally deleted or disabled, and scenarios that would reproduce those bugs

  and make sure that for each scenario, there's a test that covers it and where the reader can easily confirm that it does so.

[gnprice]

From the name of this, I can't tell whether null comes before all the integers, or after all of them, or something else (equal to zero?). That makes it hard to tell whether or why this function is appropriate to use in any given context.

The name should be something that indicates that, and the summary line in the doc should indicate it too.

It could be something direct about the behavior: like compareNullBeforeInt, or compareNullAsLeast, or something like that.

Or maybe better is to give a name that reflects why it is that two methods on this class both want it: the point is that these aren't just any ints, but specifically are the greatest message IDs in two sets. When the set of message IDs is empty, its value is mathematically $-\infty$, negative infinity; we're representing that as null but it should therefore be treated as more negative than any number.

So the name could be compareRecentMessageIds. The doc can say:

/// 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].

And that's it — the summary line, plus the reference to [Comparable.compareTo] to resolve any ambiguity about what "compare" means, completely specifies the behavior so there's no need to transcribe the implementation into English.

[gnprice]

nit: block body, like at #692 (comment)

[gnprice]

Let's move the assert above about CombinedFeedNarrow to inside here ­— that way we're splitting up the cases for narrow's type in just one place.

To make that comfortable, this will probably want to become a switch statement instead of expression.

[gnprice]

nit: on one line, same way as below

[gnprice]

nit: I think this is a little easier to read if these first two variables are inlined:

Suggested change

	final aMessageId = recentSenders.latestMessageIdOfSenderInTopic(
	streamId: streamId, topic: topic, senderId: userA.userId);
	final bMessageId = recentSenders.latestMessageIdOfSenderInTopic(
	streamId: streamId, topic: topic, senderId: userB.userId);
	final result = -compareNullable(aMessageId, bMessageId);
	final result = -compareNullable(
	recentSenders.latestMessageIdOfSenderInTopic(
	streamId: streamId, topic: topic, senderId: userA.userId),
	recentSenders.latestMessageIdOfSenderInTopic(
	streamId: streamId, topic: topic, senderId: userB.userId));

Not really any denser that way, and there's a little fewer names flying around to have to track.

[sm-sayedi]

Thank you for the review @gnprice. Changes pushed. Please have a look.

[gnprice]

Thanks for the revision!

This didn't quite get the tests to the kind of tests that I was hoping for. I decided the best way this time to communicate that would be by demonstration — so I wound up spending some time today writing tests of the kind I want to see, and revising these tests in that direction. I'll push those changes to the tip of the PR branch.

For each idea that might otherwise have been a separate comment on the PR, I made a separate commit. As a result this ended up being a longer series of separate commits added to the tip of the branch than the number of commits would have been if I were writing the same tests from scratch:

adeb012 autocomplete [nfc]: Format second half of compareByRecency same way as first
a0d0248 autocomplete [nfc]: Set streamId and topic as variables directly
3c7096f autocomplete test [nfc]: Tighten formatting in compareByRecency tests
a5b3cc8 autocomplete test: Check both directions in compareByRecency tests
6e72a5b autocomplete test [nfc]: Tighten names of compareByRecency tests
d91ecca autocomplete test: Check compareByRecency uses per-topic recency
72caacf autocomplete test [nfc]: Deduplicate in test names
4762e4e autocomplete [nfc]: Expose debugCompareUsers, for testing
bfce142 autocomplete test: Unit tests of between-signals logic, for each narrow type
37e643d autocomplete test: Fix test that almost can't fail, of CombinedFeedNarrow
3823ec4 autocomplete test: Cut tests that could flake due to indeterminate sort
a14ea8b autocomplete test: Highlight one end-to-end test, cut the others
1e781dd autocomplete test [nfc]: Do constant-folding in end-to-end test
739fa84 autocomplete test: Fix user IDs of zero
2b8d848 autocomplete test: Separate getting results from checking in end-to-end test
0288ee4 autocomplete test: Test ranking applies in presence of filters, too
b436fb5 autocomplete test [nfc]: Bring details next to details in end-to-end test
363dafa autocomplete test [nfc]: Cut unneeded MessageListView in end-to-end test
3444a68 autocomplete test: Make distinct topic explicitly distinct

Please take a look through each of those commits, including the commit messages, to see the points I had in mind. I also left one GitHub comment below, for an aspect that the commits don't go into full detail on.

With that, this code all looks good; and I also did some quick manual testing and confirms it works end-to-end in the app. So, merging! Thanks again @sm-sayedi for all your work building this feature #228, and @rajveermalviya @hackerkid @chrisbobbe for the previous reviews on this series of PRs.

[gnprice]

It's hard to interpret most of the test cases in this "suggests relevant users" group — for example, why is 3, 0, 1, 2, 4 the correct result to expect here, and why is 0, 3, 1, 2, 4 the correct result to expect in some of the tests above? The details that cause the right answers to be those lists and not something else are separated from here by about 100 lines of code, and they require some careful working-through to verify.

In general it's best for a test to be self-contained — all the key details that tell the story of the test, and determine why one value is expected instead of another, live in the test case's own source code, or failing that very close above it. That way

• a reader can look at the test and confirm it's checking for the right answer instead of a wrong answer;
• a reader can look at the test and understand what it's testing, and look at the ensemble of tests to think about things that aren't yet tested;
• in the future when changing something about the code under test and the test breaks, a reader can understand what the test was trying to say so they can decide whether the failure signals a bug in their change or is an intended effect of their change, and so they can update the test if necessary so it continues to check what it was intended to check.

We use a lot of test helper functions to enable test cases to be concise and focused, because that also aids in all three of the points above. But when doing so, the key is to move out the boring details (the things that can be summarized as the helpers just doing things that ought to be done), and keep in the details that are part of the test's story, or that are necessary in order to make sense of other details in the test. So for example as long as this ranking [3, 0, 1, 2, 4] is here, the details of messages and dmConversations that give rise to it should be here or near here too.

[sm-sayedi]

Thank you @gnprice for your time and patience in improving the PR. I read through all the commits and learned a lot. I believe this will improve my limited ability to write good tests.