(More) consistently clear old data on all ways of leaving an account

[chrisbobbe]

Oops, it seems I should have thought a bit harder for #5606. 🙂 In particular, this PR picks up some threads I dropped there:

• Give test coverage for all the reducers that use the new RESET_ACCOUNT_DATA action.
• Clear per-account session state on RESET_ACCOUNT_DATA
• Clear per-account settings state on RESET_ACCOUNT_DATA

Those interesting changes come after a string of code-style tightenings (and one "start using Flow" commit) for several of the test files they touch.

Fixes: #4446

[gnprice]

Thanks for fixing these up! Small comments below about the tests.

[gnprice]

BTW one style/formatting tweak I often find handy on expressions like this is to pull out a long sub-expression as a local variable, so that the small bits surrounding it can get put together on one line instead of spread out on separate lines each. Like this:

like this:

-      expect(
-        settingsReducer(
-          prevState,
-          eg.mkActionRegisterComplete({
-            enable_offline_push_notifications: true,
-            enable_online_push_notifications: true,
-            enable_stream_push_notifications: true,
-            user_settings: undefined,
-          }),
-        ),
-      ).toMatchObject({
+      const action = eg.mkActionRegisterComplete({
+        enable_offline_push_notifications: true,
+        enable_online_push_notifications: true,
+        enable_stream_push_notifications: true,
+        user_settings: undefined,
+      });
+      expect(settingsReducer(prevState, action)).toMatchObject({

where effectively the expect(, settingsReducer(, prevState, ),, and ) get moved from 5 separate lines to a single line, because the mere name action is short enough to fit on a line with all of them at once.

(I'm not requesting that you go back and reformat all this code in that style — doesn't matter nearly enough for that. But it's a suggestion for when making other changes in the future.)

[chrisbobbe]

pull out a long sub-expression as a local variable

Makes sense. I think there are one or two small tradeoffs with this approach, though:

• I think you'll often end up doing some by-hand "weight" comparison of each sub-expression, seeing which one(s) could save you the most lines by being pulled out like this: is it the previous state, the action, the actual or expected state, etc.
• The right choice won't consistently be the same kind of sub-expression (previous state, action, actual or expected state), right? So in a large group of very similar tests, one ends up reading a slightly different pattern in the code.

I'm happy to try it going forward, though, and see how big these tradeoffs are in practice.

[gnprice]

Yep, agreed.

I think in a lot of these reducer tests, pulling the action out has this effect, because the action is usually long and the previous state is usually already a local variable. So one approach on those trade-offs would be to consistently pull the action out.

[gnprice]

Another example of the same idea:

-      expect(
-        settingsReducer(
-          baseState,
-          deepFreeze({ type: SET_GLOBAL_SETTINGS, update: { theme: 'night' } }),
-        ),
-      ).toEqual({ ...baseState, theme: 'night' });
+      const action = deepFreeze({ type: SET_GLOBAL_SETTINGS, update: { theme: 'night' } });
+      expect(settingsReducer(baseState, action)).toEqual({ ...baseState, theme: 'night' });

[gnprice]

This one seems different from the rest of what's happening in these "Tighten code style" commits — it's changing the actual test data. The old code started from a state with something at [HOME_NARROW_STR], while the new code starts from the base state (which is empty for caughtUp.)

Maybe that change in test data is fine and good, but it'd be best to keep it separate from these changes whose effects are otherwise just a smidge more complex than a pure reformatting.

[gnprice]

similar comment as on caughtUp test above

[gnprice]

The test should mention HOME_NARROW in the action here (even though it happens to be that already in eg.action.message_fetch_complete), because it's essential to the test that it's the same narrow as in the state above.

[gnprice]

Hmm. I like the idea of using actions and the reducer to construct the starting state. But I find this specific way of writing the test a bit unsatisfying because

• for the test to be effective (that is, for it to not just vacuously pass even if we were to break the thing it's testing), it's essential that the first action in this list produces some state that isn't equal to the base/initial state;
• but it's not immediately obvious on reading the test that that's true — it depends on some details of what's in that action, and one has to read it closely and think through the meaning of caughtUp and how it should behave here to confirm that it's true;
• relatedly, if we were to make some future change to the reducer's semantics such that this first action didn't produce anything different from the initial state — not super likely in this example, but again to reach that conclusion one has to think about the semantics of caughtUp and decide that this is fundamental to it — then the test would become vacuous, and nothing would cause us to closely reread this code and notice that.

Here's an alternative version, which preserves the property that we construct the state through the reducer rather than as an object literal:

  test('RESET_ACCOUNT_DATA', () => {
    const initialState = eg.baseReduxState.caughtUp;
    const prevState = caughtUpReducer(initialState, {
      ...eg.action.message_fetch_complete,
      foundNewest: true,
      foundOldest: true,
    });
    expect(prevState).not.toEqual(initialState);

    expect(caughtUpReducer(prevState, eg.action.reset_account_data)).toEqual(initialState);
  });

That way, the setup explicitly checks that the state before reset isn't already equal to the initial state.

(Similarly for several other reducers' tests added in this commit.)

[chrisbobbe]

Ah, yeah, makes sense. Thanks!

[chrisbobbe]

Thanks for the review! Revision pushed. This one includes #5622.

I'd like to get at least the changes to sessionReducer in before I continue on disallow-ancient-servers, #5102.

[gnprice]

Thanks! Looks good — merging.