Add tests for all our migrations; fix an old broken one #5190

gnprice · 2022-01-11T23:54:37Z

Really we should have done this years ago: the migrations are code that it's important should behave correctly, and they don't naturally get much manual testing from simply using the app.

Well, the second-best time is now. This will also help us out as we migrate the migrations into a new migration system, as part of #5006 (building on #4841).

And in fact, writing these tests turned up a bug in one of the old migrations! This PR fixes that too, though it's too late for the fix to have much practical effect. Fortunately the consequences of the bug weren't especially bad.

chrisbobbe

Great, thanks! See a few small things, below. (Or above, I think, if you're on GitHub's iOS app, as I am sometimes? 🤔)

chrisbobbe · 2022-01-12T03:59:54Z

src/storage/migrations.js

+ * The value of `storeKeys` when the `dropCache` migrations were written.
+ */
+// prettier-ignore
+export const historicalStoreKeys: Array<$Keys<GlobalState>> = [


storage [nfc]: Pull our migrations into their own module Past time for this; they've grown to much longer than the rest of the module, and they have a rather different flavor.

Did historicalStoreKeys (and the related test) mean to go in a different commit? It looks interesting and not related to the move.

Sure, yeah, that's probably cleanest.

Moving the migrations is what prompted me to notice this distinction, as I contemplated importing storeKeys from the new migrations.js and realized that that wouldn't really be right.

chrisbobbe · 2022-01-12T08:10:32Z

src/storage/migrations.js

+/* $FlowFixMe[incompatible-type] This is where we pretend that the storeKeys
+     are all present. */


migration types: Move a FlowFixMe inside migrations list Inside these migration functions, we pretend that the object they're passed is really of type GlobalState, with all the properties and the latest types for those properties. But that's a lie; it wouldn't be much of a migration system if it were true. Make the type exported from this module slightly less of a lie, so that some of the lying is done inside the module. This has the effect of decoupling the lie told inside the module from the one told on the outside. That will let us go on to adjust exactly what lie is told inside this module, without having the new type propagate out to where the migrations are invoked.

The "lie" told outside the module is that migrations: {| [string]: (PartialState) => PartialState |}. Is that right?

I don't think I've understood the design of that "outer" lie. Could you run that by me again, please? I think the "inner" one makes more sense; it has an explanation starting with "The type is a lie, in several ways".

Also, is the $FlowFixMe here accurate about the "outer" lie?

Should it be a $FlowIgnore, or do we want to make some changes to it (what changes) or fix it in the future?

The type assertion with the error we're suppressing doesn't involve special treatment for storeKeys, but the comment mentions storeKeys. Is there a mismatch there?

Should the comment mention cacheKeys too?

Also, is the $FlowFixMe here accurate about the "outer" lie?

Ah, if you mean this one:

/* $FlowFixMe[incompatible-type] This is where we pretend that the storeKeys are all present. */ export const migrations: {| [string]: (PartialState) => PartialState |} = migrationsInner;

then that applies to where we take the inner type and cast it to the outer type -- so it's about the gap between the lie told inside the module and the smaller one told on the outside.

The "outer" lie is that we pretend the object passed to these migrations is a PartialState. That's untrue because if it has, for example, an accounts property, that property may not have the type that GlobalState (and hence PartialState) expect for the accounts property. But no code inside this module is going to be aware that's a lie, because the migrations we export already expects PartialState for the migrations' arguments.

The "inner" lie is that we pretend the object passed to these migrations is a LessPartialState. That's untrue for the same reason as the first one; plus that LessPartialState means that accounts and the other storeKeys are actually all present, while in reality some keys may not be present. So that latter, additional, lie is the one that this $FlowFixMe has to cover.

chrisbobbe · 2022-01-12T08:22:24Z

src/storage/migrations.js

@@ -54,7 +63,7 @@ function dropCache(state: GlobalState): $Shape<GlobalState> {
 }

 // This is the inward-facing type; see later export for jsdoc.
-const migrationsInner: {| [string]: (GlobalState) => GlobalState |} = {
+const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = {


migration types: Be sure to only act on storeKeys Anything else is moot, because we have a dropCache at the end which would drop any such other data anyway. This causes the type-checker to check that we don't have any remaining migrations that try to consume any properties not in storeKeys.

When you say "we have a dropCache at the end", you mean that the current latest migration, 37, involves a dropCache, right?

Is that snapshot all that's relevant, or should it be more like "we do and will continue to handle migrations of cacheKeys data using dropCache"?…

…Ah, reading more in this PR, I think maybe I see: we're going to end this sequence of migrations, and do migrations a different way.

Also in this commit, we've left this bit of comment unchanged:

// The type is a lie, in several ways: // * The actual object contains only the properties we persist: // those in `storeKeys` and `cacheKeys`, but not `discardKeys`. // * The actual input is from an older version of the code, one with // different data structures -- after all, that's the point of the // migration -- which usually have a different type. // * For all but the latest migration, the same is true of the output.

Does the first bullet point need an update? The new type does say that the storeKeys properties are represented.

…Ah, reading more in this PR, I think maybe I see: we're going to end this sequence of migrations, and do migrations a different way.

Yeah. This might not be the absolute last migration we do in this framework (if we land any other PRs with migrations before we land the new AsyncStorage, they'll use this and should just add tests), but it's at least nearly the last.

In the future migration framework all this code will survive, but preserved in amber rolled up into a single migration in the new framework.

When you say "we have a dropCache at the end", you mean that the current latest migration, 37, involves a dropCache, right?

Is that snapshot all that's relevant, or should it be more like "we do and will continue to handle migrations of cacheKeys data using dropCache"?…

Hmm, rereading, for this commit really what's relevant is that latter point, yeah. I must have written the "because we have a dropCache at the end" before I wrote the parent commit which removed the one exception.

Does the first bullet point need an update? The new type does say that the storeKeys properties are represented.

It does, as did the old type: GlobalState. It also claims that the discardKeys properties may be present, and this bullet point is saying that that's false. (We pass redux-persist a list of keys to read, and it's just storeKeys plus cacheKeys.)

Another lie in this type is that it claims the storeKeys properties are definitely present, while in reality any or all keys might be missing. That one's not actually represented in this comment, huh! I'll add it.

it claims the storeKeys properties are definitely present, while in reality any or all keys might be missing.

Small correction to this, reflected in the new revision: any or all keys other than migrations might be missing, because when that's missing we skip trying to run the migrations.

Which means that they might be missing only because of our legacy unsound storage -- in the happy case, we only store a migrations key if we also store all the others, with at least their initial states as supplied by the reducers.

chrisbobbe · 2022-01-13T22:16:09Z

src/storage/__tests__/migrations-test.js

@@ -10,3 +13,204 @@ describe('historicalStoreKeys', () => {
    expect(historicalStoreKeys).toEqual(storeKeys);
  });
 });
+
+describe('migrations', () => {


migration tests: Test all existing migrations

🎉

chrisbobbe · 2022-01-13T22:44:18Z

src/actionTypes.js

 * @prop payload A version of the global Redux state, as persisted by the
 *     app's previous runs.  This will be empty on first startup or if the
- *     persisted state is just missing keys, and will have `null` at each
- *     key where an error was encountered in reading the persisted state.
- *     In any case it will only contain the keys we configure to be persisted.
+ *     persisted state is just missing keys.  In any case it will only
+ *     contain the keys we configure to be persisted.


This will be empty […] if the persisted state is just missing keys.

I guess "missing keys" is ambiguous: does it mean "missing all keys" (keys isn't a thing it has), or "missing some keys"?

I think if the persisted state is just missing some keys, then the payload will also be just missing those keys. It won't necessarily be empty.

Separately, the payload is still affected by errors in reading the persisted state, right: a key where such an error was encountered will be missing. Not null, as before—but "missing" is still interesting, right, and we might want to still mention it here?

I think if the persisted state is just missing some keys, then the payload will also be just missing those keys. It won't necessarily be empty.

Yes, that's right. The existing text could be clearer about this.

Separately, the payload is still affected by errors in reading the persisted state, right: a key where such an error was encountered will be missing. Not null, as before

Yeah. After this change, there's no longer a distinction between what happens if there's an error reading that key's value vs. if it's simply not there -- and talking less about that distinction in the jsdoc is a bonus. I'll reword the text to cover them both but together.

This simplifies reasoning about how this code works.

Good for testing... and really for this logic being cleaner to understand in the first place, too.

The mutation isn't how any of our code normally works, it isn't helpful, and in writing tests it can cause very confusing failures.

Past time for this; they've grown to much longer than the rest of the module, and they have a rather different flavor.

This makes the migrations' behavior self-contained, without depending on what our reducers do for their respective "initial states". That will help us write tests for these migrations. Later it'll also help us disentangle them from the redux-persist architecture, and reuse them in the context of a new migration system.

Inside these migration functions, we pretend that the object they're passed is really of type GlobalState, with all the properties and the latest types for those properties. But that's a lie; it wouldn't be much of a migration system if it were true. Make the type exported from this module slightly less of a lie, so that some of the lying is done inside the module. This has the effect of decoupling the lie told inside the module from the one told on the outside. That will let us go on to adjust exactly what lie is told inside this module, without having the new type propagate out to where the migrations are invoked.

This is subsumed by the dropCache right after it, so it might as well just do a dropCache too.

This causes the type-checker to check that we don't have any remaining migrations that try to consume any properties not in storeKeys. When we have a change to make to the other persisted properties (the ones in cacheKeys), we use dropCache for that.

Really we should have done this years ago: this is code that it's important should behave correctly, and it doesn't naturally get much manual testing from simply using the app. Well, the second-best time is now. In fact, writing these tests turns up a bug in one of the old migrations! We'll fix that bug in a separate commit. Fortunately its consequences aren't too bad -- it caused that migration to drop PM drafts, which is exactly what we'd chosen to do in the case of the migration right before it. But it makes a nice demonstration of why tests are necessary here.

Fixing this will have very little practical effect; the original, buggy, migration was released nearly a year ago, so almost all users that were going to run it will have already run it by now. But now that we've written tests for these migrations and we know it's buggy, it's pretty easy to fix. And doing so lets us clean up the test of the broken behavior, and also record our assessment of the consequences of the bug. (Which aren't especially bad, fortunately.)

gnprice · 2022-01-26T07:22:15Z

Thanks for the review! New revision pushed -- please take a look.

This includes a few more changes in those final "persist" commits, as I saw the jsdoc on RehydrateAction mention reducers and realized that our one reducer that inspects the payload should be updated slightly to match.

I'm not sure if this was ever a possible case in the past; it wouldn't be surprising if it was something eliminated in some of the work we did on redux-persist a couple of months ago. In any case, it seems to clearly not be possible now: looking at `persistStore.js` where we create these actions, the payload is always an object.

We don't use these nulls in any later handling; they go into the REHYDRATE payload, but then in redux-persist-migrate we cast them away to pretend they can't be there. It's not clear how we would usefully do anything with them, either. Instead, on an error here just omit that key, the same as we would if it weren't there in the underlying storage at all. This lets us cut out a fixme step in redux-persist-migrate. The type of the REHYDRATE payload is still a lie, but it's now the same lie on its way into this code as out of it. In the one place our reducers look at this payload, update the type and comment, but nothing substantive changes -- we were already testing for the there-relevant property being falsy, which is just as appropriate for "could be missing" as for "could be missing or null".

chrisbobbe · 2022-01-26T19:18:53Z

Great, thanks! LGTM; merging.

gnprice added the P1 high-priority label Jan 11, 2022

chrisbobbe requested changes Jan 13, 2022

View reviewed changes

gnprice added 14 commits January 25, 2022 23:18

redux-persist-migrate: Drop unused options

ca75cfe

This simplifies reasoning about how this code works.

redux-persist-migrate [nfc]: Collapse now-trivial wrapper layer

e5971c2

redux-persist-migrate [nfc]: Drop a no-op reaffirming of a property

c5dcdf6

redux-persist-migrate [nfc]: Factor out the non-Redux core

347c51c

Good for testing... and really for this logic being cleaner to understand in the first place, too.

redux-persist-migrate: Stop mutating payload

9339495

The mutation isn't how any of our code normally works, it isn't helpful, and in writing tests it can cause very confusing failures.

migration [nfc]: Give dropCache a stable copy of storeKeys

1f7fb9d

migration [nfc]: Pull our migrations into their own module

b066de7

Past time for this; they've grown to much longer than the rest of the module, and they have a rather different flavor.

migration [nfc]: Point out that properties can be missing, too

88f26a7

migration [nfc]: Use dropCache in one remaining migration of cache data

113c8a3

This is subsumed by the dropCache right after it, so it might as well just do a dropCache too.

gnprice force-pushed the pr-migration-tests branch from 8511ce9 to 41ca043 Compare January 26, 2022 07:20

gnprice added 3 commits January 25, 2022 23:22

persist [nfc]: Inline tiny "rehydrate" function

09e4f73

gnprice force-pushed the pr-migration-tests branch from 41ca043 to d807fbc Compare January 26, 2022 07:22

chrisbobbe merged commit d807fbc into zulip:main Jan 26, 2022

gnprice deleted the pr-migration-tests branch January 26, 2022 20:59

gnprice mentioned this pull request Nov 16, 2022

Read our stored Redux data from Kotlin #5117

Open

		/* $FlowFixMe[incompatible-type] This is where we pretend that the storeKeys
		are all present. */

Uh oh!

Add tests for all our migrations; fix an old broken one #5190

Add tests for all our migrations; fix an old broken one #5190

Uh oh!

Conversation

gnprice commented Jan 11, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 13, 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!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jan 26, 2022

Uh oh!

chrisbobbe commented Jan 26, 2022

Uh oh!

Uh oh!

chrisbobbe Jan 13, 2022 •

edited

Loading