msglist-diffing: More tests! 🛠️

[chrisbobbe]

And a bugfix caught by one of the new tests!

I'm going down the list at #5188 (comment), and Greg's comment on CZO:

These do give us a reminder that it'd be good to have HTML snapshots testing those features of the message list, though:

• reactions
• polls
• starred messages
• muted senders

This PR does those HTML snapshots in those four bullet points, and most of this part of #5188 (comment):

And for within a given message:

• Add/remove reactions to a message.

• Update a poll's data.

  • Hmm, I wonder if our existing shortcut I mentioned here: Implement message-list diffing algorithm! #5188 (comment) is correct for this case. These new tests will make it possible to check that!

• Star/unstar a message.

• Change a sender's name or avatar.

• Mute/unmute a sender.

I haven't done "Change a sender's name or avatar" yet because I think we can't until #5208 settles.

[chrisbobbe]

Still to do are these:

• Insert/delete the first message of a block that share a sender name/avatar, i.e. the message-full HTML-element that's followed by some message-brief HTML-elements.
• Insert/delete all the messages of such a block, coming in between messages that would otherwise make up one such block, so that block splits/rejoins. Also between messages that wouldn't do that.
• Similarly with a block of messages that have a common recipient, in between messages with some other common recipient, and also between messages with different recipients.
• Similarly with a block of messages that have a common date. Inserting/deleting the first one (when there are more after it) will change which message the date header is associated with; inserting/deleting all of them will make the date header appear/disappear.

But I figured this much was a good amount to start with.

[gnprice]

Great! I haven't yet read this all in detail, but generally it looks good. Small comments below from what I've read so far.

(Plus a longer comment that's a bit of a digression from this PR itself, more a thought for the next one and about other tests.)

[gnprice]

Hmm, yeah, good catch!

In fact -- does this work correctly in the live code today? It looks to me like it probably doesn't:

  if (
    !isEqual(
      prevProps.messageListElementsForShownMessages,
      nextProps.messageListElementsForShownMessages,
    )
    || !equalFlagsExcludingRead(prevProps.backgroundData.flags, nextProps.backgroundData.flags)
  ) {
    uevents.push(updateContent(prevProps, nextProps));
  }

Not coincidentally, we don't really have tests for that bit of existing logic.

[chrisbobbe]

Hmm, yeah, good catch!

You get the credit for this one: you asked for the test case that caught the bug! 😛

In fact -- does this work correctly in the live code today? It looks to me like it probably doesn't

Ha, right! We'll hope to switch to message-list diffing soon, right.

I could put in a fix in this deprecated live code—basically check for any change at all to the muted-users state—but maybe not, in case I introduce some regression while trying to do so?

[gnprice]

Hmm, yeah, good catch!

You get the credit for this one: you asked for the test case that caught the bug! 😛

:-)

In fact -- does this work correctly in the live code today? It looks to me like it probably doesn't

Ha, right! We'll hope to switch to message-list diffing soon, right.

I could put in a fix in this deprecated live code—basically check for any change at all to the muted-users state—but maybe not, in case I introduce some regression while trying to do so?

Yeah. For this PR I think just adding a TODO comment in the live code would be fine.

[gnprice]

I haven't done "Change a sender's name or avatar" yet because I think we can't until #5208 settles.

Yeah, sounds good.

[gnprice]

It's fine to rely on this kind of comment when necessary, but here I think we can straightforwardly encode that relationship in the code itself -- something like:

Suggested change

	// Should match realmEmojiReaction.
	'80': {
	deactivated: false,
	code: '80',
	name: 'github_parrot',
	source_url: '/user_avatars/2/emoji/images/80.gif',
	[realmEmojiReaction.emoji_code]: {
	deactivated: false,
	code: realmEmojiReaction.emoji_code,
	name: realmEmojiReaction.emoji_name,
	source_url: `/user_avatars/2/emoji/images/${realmEmojiReaction.emoji_code}.gif`,

In the case of the source_url it's not really even necessary for us to mimic the shape of the actual URLs -- that isn't promised in the API, might vary, and we shouldn't rely on it. But I do kind of want it to be something that won't easily collide by accident with some other URL we'd make up (though even that probably shouldn't matter either), and this seems like an easy way to do that.

[gnprice]

Seeing the test cases like this one, here's one thought that occurs to me. This idea isn't really a request for this PR -- I'd actually rather go ahead and merge a version of this PR close to how it currently is and then have this refactor come in the next PR.

I think that rather than have check take an old and a new BackgroundData value, it'd be better to have it take an old and a new PerAccountState, from which it would compute the BackgroundData.

One reason is that that would feel a bit more end-to-end -- I'd worry a bit less about a gap between what these tests are testing, and what we actually do as events come in and update our state.

In particular, we could factor out the selector MessageList uses for computing the BackgroundData, and export it only for tests, and call it from here. (I guess it would also need to take a GlobalSettings and a Debug, as well as a PerAccountState. But check could still take just the PerAccountState, and supply the other two because they're boring here.) But even if check were to basically copy-paste that chunk of MessageList.js, which looks like this:

    const backgroundData: BackgroundData = {
      alertWords: state.alertWords,
      allImageEmojiById: getAllImageEmojiById(state),
      auth: getAuth(state),

then I feel like that'd be an improvement; it'd mostly be pretty hard for a copy of that to become out of sync and wrong without the type-checker telling us so.

Then a further thing that that would enable is that because it'd make this test data closer to the form in which we actually construct and update the data, it'd let us construct it using that same code -- that is, we could construct it using the reducers, rather than with object literals and so on. (That's a direction you've seen me move in for other tests in some recent PRs.)

That would further enhance the end-to-end-ness of these tests. It'd also make it so that they don't have to change so much whenever we rearrange things in the state.

The ideal version might even be that each of these test cases sets up some state, then sets up the new state by hitting that not with one of our Redux actions but with a server event. That's the one really stable API that this all needs to work to, after all -- everything inward of that, we can freely refactor on a whim. But doing it with reducers is basically directly on the path to that, and doing it with state objects (however constructed) is in turn on the path to that, so for the next PR I'd be happy to see just switching from BackgroundData to state objects as the arguments passed to check.

[chrisbobbe]

Cool! I'm excited to go in that direction for the tests in a separate PR.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks for the revision!

All looks good, modulo adding one TODO comment in the existing code: #5225 (comment)

I'll go ahead and add that and merge. And then #5225 (comment) describes a direction for the next PR in this series, alongside adding more tests as you described near the top at #5225 (comment) .