msglist tests: Use Redux state in inputs, not background data #5290

chrisbobbe · 2022-03-10T20:29:35Z

Thanks to Greg's #5272, which pulled out a getBackgroundData that this uses.

chrisbobbe · 2022-03-11T19:25:52Z

Looks like a flake in the Jest tests; investigating.

One of the `check` functions hasn't been using our `baseState` that we put together with stream1 and stream2, to give background for the messages in those streams (streamMessages1, streamMessages2, etc.) that we pass to `check`. So, use that `baseState`. Also, the question of whether those streams are subscribed has effectively been left up to chance: if `eg.plusReduxState` *happens* to have a `subscriptions` entry for stream1 or stream2 (i.e., one where the stream_id matches), then we render a stream header as "subscribed"; otherwise not [1]. `eg.plusReduxState` currently has two `subscriptions` entries, both with random stream_ids. Most of the time they differ from stream1 and stream2 (effectively making those streams "unsubscribed"), but we saw a flake in CI for zulip#5290 where they must've collided. Settle the question by saying the streams are actually subscribed, which is the common case. [1] If "subscribed", the background color is the stream's color (#123456 in current example data); otherwise, the fallback hsl(0, 0%, 80%). And the text color is `foregroundColorFromBackground` of that.

chrisbobbe · 2022-03-11T20:38:02Z

OK, I think I've fixed the flake with this revision.

gnprice

Thanks! This generally looks good -- two comments below.

(And the first commit is shared with #5283, I think -- is that right?)

gnprice · 2022-03-11T21:59:33Z

src/webview/__tests__/generateInboundEventEditSequence-test.js

@@ -486,14 +536,14 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', ()
      });
    });

-    Object.keys(eg.baseReduxState.flags).forEach(flag => {
+    Object.keys(eg.plusReduxState.flags).forEach(flag => {
      test(`message with flag: ${flag}`, () => {
        const flags: ReadWrite<FlagsState> = { ...eg.baseBackgroundData.flags };


This gets changed to "plus" when the rest of the eg.baseBackgroundData does, but then changed back to "base" when other things are getting changed to plus:

- Object.keys(eg.baseReduxState.flags).forEach(flag => { + Object.keys(eg.plusReduxState.flags).forEach(flag => { test(`message with flag: ${flag}`, () => { - const flags: ReadWrite<FlagsState> = { ...eg.plusBackgroundData.flags }; + const flags: ReadWrite<FlagsState> = { ...eg.baseBackgroundData.flags }; flags[flag] = { [baseSingleMessage.id]: true };

Is that intentional? It feels like it's going in the opposite direction from the rest of that commit.

Thanks for catching this! No, it wasn't intentional.

gnprice · 2022-03-11T22:18:23Z

src/webview/__tests__/generateInboundEventEditSequence-test.js

-  const check = ({ backgroundData = baseBackgroundData, narrow, messages }) => {
+  const check = ({
+    state = baseState,
+    globalState = assumeSecretlyGlobalState(state),


I'd rather not proliferate calls to assumeSecretlyGlobalState like this -- each one of them is effectively a TODO that will need to be resolved in order to complete #5006.

In this case (and that of the other check function below), I think what's really happening semantically is that it wants a copy of the account-independent part of the state. That is, all the parts of the global state that aren't any account's state.

We don't yet have a type for expressing that directly. Perhaps I should add one. But I think two good ways to express what these test functions want in the meantime would be:

Use a global state from example data:

globalState = eg.plusReduxState,

It's fine that that may not agree with the per-account state it gets, because we're really only looking at the account-independent parts of this -- for everything account-specific, we rely on that per-account state.

Or pass the two pieces we actually use, separately:

globalSettings = getGlobalSettings(eg.plusReduxState), debug = getDebug(eg.plusReduxState),

No existing call site gets any more verbose, because they don't actually pass these anyway. Future call sites mostly won't either, because they'll be passing only the pieces that those respective tests care about.

Yeah, seems sensible. My next revision uses the "pass the two pieces we actually use" approach.

One of the `check` functions hasn't been using our `baseState` that we put together with stream1 and stream2, to give background for the messages in those streams (streamMessages1, streamMessages2, etc.) that we pass to `check`. So, use that `baseState`. Also, the question of whether those streams are subscribed has effectively been left up to chance: if `eg.plusReduxState` *happens* to have a `subscriptions` entry for stream1 or stream2 (i.e., one where the stream_id matches), then we render a stream header as "subscribed"; otherwise not [1]. `eg.plusReduxState` currently has two `subscriptions` entries, both with random stream_ids. Most of the time they differ from stream1 and stream2 (effectively making those streams "unsubscribed"), but we saw a flake in CI for zulip#5290 where they must've collided. Settle the question by saying the streams are actually subscribed, which is the common case. [1] If "subscribed", the background color is the stream's color (#123456 in current example data); otherwise, the fallback hsl(0, 0%, 80%). And the text color is `foregroundColorFromBackground` of that.

chrisbobbe · 2022-03-11T22:55:27Z

Thanks for the review! Revision pushed.

gnprice · 2022-03-14T19:23:54Z

Thanks for the revision! Looks good, but there are some errors after rebase -- #5283 added more tests to this file, which need to be adjusted for this refactor.

Please go ahead and merge after resolving those.

See zulip#5225 (comment)

One of the `check` functions hasn't been using our `baseState` that we put together with stream1 and stream2, to give background for the messages in those streams (streamMessages1, streamMessages2, etc.) that we pass to `check`. So, use that `baseState`. Also, the question of whether those streams are subscribed has effectively been left up to chance: if `eg.plusReduxState` *happens* to have a `subscriptions` entry for stream1 or stream2 (i.e., one where the stream_id matches), then we render a stream header as "subscribed"; otherwise not [1]. `eg.plusReduxState` currently has two `subscriptions` entries, both with random stream_ids. Most of the time they differ from stream1 and stream2 (effectively making those streams "unsubscribed"), but we saw a flake in CI for zulip#5290 where they must've collided. Settle the question by saying the streams are actually subscribed, which is the common case. [1] If "subscribed", the background color is the stream's color (#123456 in current example data); otherwise, the fallback hsl(0, 0%, 80%). And the text color is `foregroundColorFromBackground` of that.

On principle; not because there's anything in these particular fields that we really want. The principle is that we strictly want to add to the "standard example data" that `plusReduxState` exists to offer; we have no reason to erase or corrupt it.

The previous stream object was in an awkward in-between position: its ID happened to match the `stream1` and `stream1`'s subscription in the state (`baseState`) that the `check` function used as a default. But the stream's name differed in an arbitrary and unimportant way. (Both stream objects are "nonrandom" enough for current purposes.) So, just use `stream1`.

chrisbobbe · 2022-03-15T05:22:51Z

Thanks for the review!

Please go ahead and merge after resolving those.

Done.

chrisbobbe requested a review from gnprice March 10, 2022 20:29

chrisbobbe force-pushed the pr-msglist-test-state branch from c96e10f to 303d268 Compare March 10, 2022 20:30

chrisbobbe changed the title ~~msglist tests [nfc]: Express inputs as Redux state, not background data~~ msglist tests [nfc]: Use Redux state in inputs, not background data Mar 10, 2022

chrisbobbe force-pushed the pr-msglist-test-state branch from 303d268 to ad8d134 Compare March 10, 2022 20:31

gnprice reviewed Mar 11, 2022

View reviewed changes

chrisbobbe force-pushed the pr-msglist-test-state branch from 813eccc to db2dc4a Compare March 11, 2022 22:55

chrisbobbe added 5 commits March 14, 2022 22:13

msglist tests [nfc]: Use Redux state in inputs, not background data

ca4c8cf

See zulip#5225 (comment)

msglist tests: Add snapshot with unsubscribed-stream header

51b582f

chrisbobbe changed the title ~~msglist tests [nfc]: Use Redux state in inputs, not background data~~ msglist tests: Use Redux state in inputs, not background data Mar 15, 2022

chrisbobbe force-pushed the pr-msglist-test-state branch from db2dc4a to 51b582f Compare March 15, 2022 05:22

chrisbobbe merged commit 51b582f into zulip:main Mar 15, 2022

chrisbobbe deleted the pr-msglist-test-state branch March 15, 2022 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist tests: Use Redux state in inputs, not background data #5290

msglist tests: Use Redux state in inputs, not background data #5290

chrisbobbe commented Mar 10, 2022

chrisbobbe commented Mar 11, 2022 •

edited

Loading

chrisbobbe commented Mar 11, 2022

gnprice left a comment

gnprice Mar 11, 2022

chrisbobbe Mar 11, 2022

gnprice Mar 11, 2022

chrisbobbe Mar 11, 2022

chrisbobbe commented Mar 11, 2022

gnprice commented Mar 14, 2022

chrisbobbe commented Mar 15, 2022

msglist tests: Use Redux state in inputs, not background data #5290

msglist tests: Use Redux state in inputs, not background data #5290

Conversation

chrisbobbe commented Mar 10, 2022

chrisbobbe commented Mar 11, 2022 • edited Loading

chrisbobbe commented Mar 11, 2022

gnprice left a comment

Choose a reason for hiding this comment

gnprice Mar 11, 2022

Choose a reason for hiding this comment

chrisbobbe Mar 11, 2022

Choose a reason for hiding this comment

gnprice Mar 11, 2022

Choose a reason for hiding this comment

chrisbobbe Mar 11, 2022

Choose a reason for hiding this comment

chrisbobbe commented Mar 11, 2022

gnprice commented Mar 14, 2022

chrisbobbe commented Mar 15, 2022

chrisbobbe commented Mar 11, 2022 •

edited

Loading