msglist diffing tests: Fix a flake with stream-subscribed status changing #5415

chrisbobbe · 2022-06-14T00:14:33Z

In 371e9b4, we said we preferred spreading
eg.plusReduxState.subscriptions...but I guess at that point we
didn't predict this flake.

See 932c28d for a solution to another flake that involved streams
unstably being subscribed / unsubscribed.

Fixes: #5414

gnprice

Thanks! Comments below.

gnprice · 2022-06-14T00:27:32Z

src/webview/__tests__/generateInboundEventEditSequence-test.js

            // no subscription for stream1
+            //
+            // Could spread eg.plusReduxState.subscriptions, for
+            // completeness…but then we'd invite flakes where random stream
+            // IDs in there sometimes collide with our fixed ones, changing
+            // whether a stream is subscribed


More specifically: it would defeat the "no subscription for stream1", because it might happen to contain a subscription for stream1.

gnprice · 2022-06-14T00:34:35Z

src/webview/__tests__/generateInboundEventEditSequence-test.js

 ];

 const baseState = eg.reduxStatePlus({
  streams: [...eg.plusReduxState.streams, stream1, stream2],


This could also collide. I'm not sure what the consequences would be -- it might be that the later entry wins, in which case it's a purely latent bug (but a bug because our streams array isn't supposed to have duplicate stream IDs.)

gnprice · 2022-06-14T00:35:52Z

src/webview/__tests__/generateInboundEventEditSequence-test.js

+    // Could spread eg.plusReduxState.subscriptions, for completeness…but
+    // then we'd invite flakes where random stream IDs in there sometimes
+    // collide with our fixed ones, changing whether a stream is subscribed
    eg.makeSubscription({ stream: stream1 }),
    eg.makeSubscription({ stream: stream2 }),


Do we have any fixed streamIDs here other than those of stream1 and stream2?

I think we don't. That was also the idea of 932c28d , which was to make the streams always subscribed (except in the one test where we specifically want them unsubscribed.)

gnprice · 2022-06-14T00:36:40Z

src/webview/__tests__/generateInboundEventEditSequence-test.js

        state: eg.reduxStatePlus({
          streams: [...eg.plusReduxState.streams, stream1, stream2],


This could be simplified, I think, by starting from baseState rather than use eg.reduxStatePlus directly.

Hmm, now I'm wondering if the other uses of eg.reduxStatePlus in these tests should be switched to start from baseState too. … I guess many of them avoid an issue here by using baseSingleMessage, which has its stream ID fixed at -1. Which is cleverly outside the range that makeStream will ever choose.

The one awkward thing about that situation is that -1 isn't a valid stream ID at all. Possibly a good thing to do here would be for makeStream to never generate a stream ID in some range of small numbers, like never less than 10; and then tests can safely make up constant stream IDs within that range, and never flakily collide. We could do the same for user IDs and message IDs, too.

This could be simplified, I think, by starting from baseState rather than use eg.reduxStatePlus directly.

PerAccountState is an opaque type, so we can't write down a literal one in this file, which we'd want to do if making a state that starts from baseState. Is that right?

Hmm, now I'm wondering if the other uses of eg.reduxStatePlus in these tests should be switched to start from baseState too.

To do this, we'd need to decide that baseState can serve its current consumers and these new consumers equally well, as both groups of tests evolve. So far, I've felt that the groups are different enough that it's probably not great to tie them together in this way. What do you think?

Possibly a good thing to do here would be for makeStream to never generate a stream ID in some range of small numbers, like never less than 10; and then tests can safely make up constant stream IDs within that range, and never flakily collide. We could do the same for user IDs and message IDs, too.

This sounds good; I'll include this in my next revision.

PerAccountState is an opaque type, so we can't write down a literal one in this file, which we'd want to do if making a state that starts from baseState.

Hmm yeah, I see.

gnprice · 2022-06-14T05:36:47Z

Oh, and here's a comment I apparently didn't submit:

In 371e9b4, we said we preferred spreading
eg.plusReduxState.subscriptions...but I guess at that point we
didn't predict this flake.

Hmm, yeah. That said:

msglist tests: Remove some accidental clobbering of `plusReduxState`

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.

But I think what this flake shows is that there is a good reason to leave out such data: we're making up some constant stream IDs, and the data there may accidentally refer to those.

gnprice · 2022-06-14T05:37:08Z

CI failed. Looks like another iteration of #5404.

…tests In generateInboundEventEditSequence-test.js, when we build state objects from eg.plusReduxState, the data we add includes some constant message, stream, and user IDs. Constant because they end up appearing in test snapshots, and we want those snapshots to be stable. This is a problem when the data we're building from already claims those IDs, for example, by saying that a stream with ID 1 exists and that we have a subscription for it. Since eg.plusReduxState uses random IDs, this has been happening some small fraction of the time, resulting in flakes where the snapshot says a stream is *not* subscribed, but the input data says it *is*. That's zulip#5415. So, to fix, reserve a small section of stream IDs, including 1 and 2, which we use in the mentioned snapshot tests, so that eg.plusReduxState never chooses them. And similarly reserve a range of message and user IDs, too. As suggested by Greg: zulip#5415 (comment) In another approach to fix those flakes, we might have instead built on eg.plusReduxState by erasing some of it first, removing spreads like the one in streams: [...eg.plusReduxState.streams, stream1, stream2], But as we mentioned in 371e9b4, that'd risk corrupting the state we get from eg.plusReduxState. That's the only example state that claims to offer a realistic uncorrupted state with server data, and it's good not to interfere with that guarantee if we can avoid it. (For example, if eg.plusReduxState grows a reference to a stream in `muteModel`, it'd be corrupt if that stream is absent in the `streams` state.) It's too bad that the easy, natural thing to do is to omit the spreads. Fixes: zulip#5415

After the previous commit, 1 is now in the range `makeStream` won't pick stream IDs from. We originally chose -1 to accomplish that, but it has the downside of not being a realistic, valid stream ID. As suggested by Greg; see zulip#5415 (comment)

chrisbobbe · 2022-06-15T18:14:02Z

Thanks for the review! Revision pushed, and replies above.

We'll use this to reserve a few low stream IDs to use as constants in snapshot tests.

…tests In generateInboundEventEditSequence-test.js, when we build state objects from eg.plusReduxState, the data we add includes some constant message, stream, and user IDs. Constant because they end up appearing in test snapshots, and we want those snapshots to be stable. This is a problem when the data we're building from already claims those IDs, for example, by saying that a stream with ID 1 exists and that we have a subscription for it. Since eg.plusReduxState uses random IDs, this has been happening some small fraction of the time, resulting in flakes where the snapshot says a stream is *not* subscribed, but the input data says it *is*. That's zulip#5415. So, to fix, reserve a small section of stream IDs, including 1 and 2, which we use in the mentioned snapshot tests, so that eg.plusReduxState never chooses them. And similarly reserve a range of message and user IDs, too. As suggested by Greg: zulip#5415 (comment) In another approach to fix those flakes, we might have instead built on eg.plusReduxState by erasing some of it first, removing spreads like the one in streams: [...eg.plusReduxState.streams, stream1, stream2], But as we mentioned in 371e9b4, that'd risk corrupting the state we get from eg.plusReduxState. That's the only example state that claims to offer a realistic uncorrupted state with server data, and it's good not to interfere with that guarantee if we can avoid it. (For example, if eg.plusReduxState grows a reference to a stream in `muteModel`, it'd be corrupt if that stream is absent in the `streams` state.) It's too bad that the easy, natural thing to do is to omit the spreads. Fixes: zulip#5415

After the previous commit, 1 is now in the range `makeStream` won't pick stream IDs from. We originally chose -1 to accomplish that, but it has the downside of not being a realistic, valid stream ID. As suggested by Greg; see zulip#5415 (comment)

gnprice · 2022-06-17T05:03:12Z

Thanks for the revision! Looks good; merging.

chrisbobbe · 2022-06-17T11:43:28Z

Thanks!

gnprice reviewed Jun 14, 2022

View reviewed changes

chrisbobbe force-pushed the pr-flake-stream-subscribed branch from 14846db to 72419c3 Compare June 15, 2022 18:13

chrisbobbe added 3 commits June 16, 2022 22:02

exampleData: Give makeUniqueRandInt an optional start param

89de470

We'll use this to reserve a few low stream IDs to use as constants in snapshot tests.

gnprice force-pushed the pr-flake-stream-subscribed branch from 72419c3 to ddaf486 Compare June 17, 2022 05:03

gnprice closed this in e7cf714 Jun 17, 2022

gnprice merged commit ddaf486 into zulip:main Jun 17, 2022

		state: eg.reduxStatePlus({
		streams: [...eg.plusReduxState.streams, stream1, stream2],

Uh oh!

msglist diffing tests: Fix a flake with stream-subscribed status changing #5415

msglist diffing tests: Fix a flake with stream-subscribed status changing #5415

Uh oh!

Conversation

chrisbobbe commented Jun 14, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 17, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jun 14, 2022

Uh oh!

gnprice commented Jun 14, 2022

Uh oh!

chrisbobbe commented Jun 15, 2022

Uh oh!

gnprice commented Jun 17, 2022

Uh oh!

chrisbobbe commented Jun 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe Jun 15, 2022 •

edited

Loading