Add tests for ZulipAsyncStorage.getItem and .setItem.

android/app/src/main/java/com/zulipmobile/TextCompressionModule.java

@@ -14,6 +14,8 @@
 import java.util.zip.Deflater;
 import java.util.zip.Inflater;
 
+// TODO: Write unit tests; see
+//   https://github.com/zulip/zulip-mobile/blob/master/docs/howto/testing.md#unit-tests-android.
 class TextCompressionModule extends ReactContextBaseJavaModule {
   public TextCompressionModule(ReactApplicationContext reactContext) {
     super(reactContext);

jest/jestSetup.js

@@ -45,6 +45,17 @@ jest.mock('react-native', () => {
       resourceURL:
         'file:///private/var/containers/Bundle/Application/4DCD4D2B-F745-4C70-AD74-8E5F690CF593/ZulipMobile.app/',
     };
+  } else if (ReactNative.Platform.OS === 'android') {
+    const header = 'z|mock|';
+    ReactNative.NativeModules.TextCompressionModule = {
+      header,
+      compress: jest.fn(
+        async (input: string) => `${header}${Buffer.from(input).toString('base64')}`,
+      ),
+      decompress: jest.fn(async (input: string) =>
+        Buffer.from(input.replace(header, ''), 'base64').toString('utf8'),
+      ),
+    };
   }
 
   return ReactNative;

src/boot/ZulipAsyncStorage.js

@@ -4,8 +4,9 @@ import { NativeModules } from 'react-native';
 import * as logging from '../utils/logging';
 
 export default class ZulipAsyncStorage {
-  static async getItem(key: string, callback: (error: ?Error, result: ?string) => void) {
-    let result = await AsyncStorage.getItem(key);
+  static async getItem(key: string) {
+    const item = await AsyncStorage.getItem(key);
+
     // It's possible that getItem() is called on uncompressed state, for
     // example when a user updates their app from a version without
     // compression to a version with compression.  So we need to detect that.
@@ -22,41 +23,34 @@ export default class ZulipAsyncStorage {
     // E.g., `zlib base64` means DATA is a base64 encoding of a zlib
     // encoding of the underlying data.  We call the "z|TRANSFORMS|" part
     // the "header" of the string.
-    if (result !== null && result.startsWith('z')) {
-      const header = result.substring(0, result.indexOf('|', result.indexOf('|') + 1) + 1);
+    if (item !== null && item.startsWith('z')) {
+      // In this block, `item` is compressed state.
+      const header = item.substring(0, item.indexOf('|', item.indexOf('|') + 1) + 1);
       if (
         NativeModules.TextCompressionModule
         && header === NativeModules.TextCompressionModule.header
       ) {
-        result = await NativeModules.TextCompressionModule.decompress(result);
+        return NativeModules.TextCompressionModule.decompress(item);
       } else {
         // Panic! If we are confronted with an unknown format, there is
         // nothing we can do to save the situation. Log an error and ignore
         // the data.  This error should not happen unless a user downgrades
         // their version of the app.
         const err = new Error(`No decompression module found for format ${header}`);
         logging.error(err);
-        if (callback) {
-          callback(err, null);
-        }
         throw err;
       }
     }
-    if (callback) {
-      callback(undefined, result);
-    }
-    return result;
+
+    // Uncompressed state
+    return item;
   }
 
-  static async setItem(key: string, value: string, callback: ?(error: ?Error) => void) {
+  static async setItem(key: string, value: string) {
     if (!NativeModules.TextCompressionModule) {
-      return AsyncStorage.setItem(key, value, callback);
+      return AsyncStorage.setItem(key, value);
     }
-    return AsyncStorage.setItem(
-      key,
-      await NativeModules.TextCompressionModule.compress(value),
-      callback,
-    );
+    return AsyncStorage.setItem(key, await NativeModules.TextCompressionModule.compress(value));
   }
 
   static removeItem = AsyncStorage.removeItem;

src/boot/__tests__/ZulipAsyncStorage-test.js

@@ -0,0 +1,148 @@
+/* @flow strict-local */
+import { Platform, NativeModules } from 'react-native';
+import AsyncStorage from '@react-native-community/async-storage';
+import ZulipAsyncStorage from '../ZulipAsyncStorage';
+import * as logging from '../../utils/logging';
+import * as eg from '../../__tests__/lib/exampleData';
+
+describe('setItem', () => {
+  const key = 'foo!';
+  const value = '123!';
+
+  // For checking that AsyncStorage.setItem is called in ways we expect.
+  const asyncStorageSetItemSpy = jest.spyOn(AsyncStorage, 'setItem');
+  beforeEach(() => asyncStorageSetItemSpy.mockClear());
+
+  const run = async () => ZulipAsyncStorage.setItem(key, value);
+
+  describe('success', () => {
+    // AsyncStorage provides its own mock for `.setItem`, which gives
+    // success every time. So, no need to mock that behavior
+    // ourselves.
+
+    test('resolves correctly', async () => {
+      await expect(run()).resolves.toBe(null);
+    });
+
+    test('AsyncStorage.setItem called correctly', async () => {
+      await run();
+      expect(asyncStorageSetItemSpy).toHaveBeenCalledTimes(1);
+      expect(asyncStorageSetItemSpy).toHaveBeenCalledWith(
+        key,
+        Platform.OS === 'ios' ? value : await NativeModules.TextCompressionModule.compress(value),
+      );
+    });
+  });
+
+  describe('failure', () => {
+    // AsyncStorage provides its own mock for `.setItem`, but it's
+    // not set up to simulate failure. So, mock that behavior
+    // ourselves, and reset to the global mock when we're done.
+    const globalMock = AsyncStorage.setItem;
+    beforeEach(() => {
+      // $FlowFixMe[cannot-write] Make Flow understand about mocking.
+      AsyncStorage.setItem = jest.fn(async (k: string, v: string): Promise<null> => {
+        throw new Error();
+      });
+    });
+    afterAll(() => {
+      // $FlowFixMe[cannot-write] Make Flow understand about mocking.
+      AsyncStorage.setItem = globalMock;
+    });
+
+    test('rejects correctly', async () => {
+      await expect(run()).rejects.toThrow(Error);
+    });
+  });
+});
+
+describe('getItem', () => {
+  const key = 'foo!';
+  const value = '123!';
+
+  // For checking that AsyncStorage.getItem is called in ways we expect.
+  const asyncStorageGetItemSpy = jest.spyOn(AsyncStorage, 'getItem');
+  beforeEach(() => asyncStorageGetItemSpy.mockClear());
+
+  beforeAll(async () => {
+    // `AsyncStorage` mocks storage by writing to a variable instead
+    // of to the disk. Put something there for our
+    // `ZulipAsyncStorage.getItem` to retrieve.
+    await AsyncStorage.setItem(
+      key,
+      Platform.OS === 'ios' ? value : await NativeModules.TextCompressionModule.compress(value),
+    );
+
+    // suppress `logging.error` output
+    // $FlowFixMe - teach Flow about mocks
+    logging.error.mockReturnValue();
+  });
+
+  const run = async () => ZulipAsyncStorage.getItem(key);
+
+  describe('success', () => {
+    // AsyncStorage provides its own mock for `.getItem`, which gives
+    // success every time. So, no need to mock that behavior
+    // ourselves.
+
+    test('calls AsyncStorage.getItem as we expect it to', async () => {
+      await run();
+      expect(asyncStorageGetItemSpy).toHaveBeenCalledTimes(1);
+      expect(asyncStorageGetItemSpy).toHaveBeenCalledWith(key);
+    });
+
+    test('resolves correctly', async () => {
+      await expect(run()).resolves.toBe(value);
+    });
+  });
+
+  describe('failure', () => {
+    // AsyncStorage provides its own mock for `.getItem`, but it's
+    // not set up to simulate failure. So, mock that behavior
+    // ourselves.
+    const globalMock = AsyncStorage.getItem;
+    beforeEach(() => {
+      // $FlowFixMe[cannot-write] Make Flow understand about mocking.
+      AsyncStorage.getItem = jest.fn(async (k: string): Promise<string | null> => {
+        throw new Error();
+      });
+    });
+    afterAll(() => {
+      // $FlowFixMe[cannot-write] Make Flow understand about mocking.
+      AsyncStorage.getItem = globalMock;
+    });
+
+    test('rejects correctly', async () => {
+      await expect(run()).rejects.toThrow(Error);
+    });
+  });
+
+  if (Platform.OS === 'android') {
+    describe('android', () => {
+      test('panics when value is in unknown compression format', async () => {
+        const unknownHeader = 'z|mock-unknown-format|';
+
+        await AsyncStorage.setItem(
+          `${key}-unknown`,
+          `${unknownHeader}${Buffer.from('123!').toString('hex')}`,
+        );
+
+        await expect(ZulipAsyncStorage.getItem(`${key}-unknown`)).rejects.toThrow(
+          `No decompression module found for format ${unknownHeader}`,
+        );
+      });
+    });
+  }
+});
+
+describe('setItem/getItem together', () => {
+  test('round-tripping works', async () => {
+    const key = eg.randString();
+    const value = eg.randString();
+
+    // AsyncStorage provides its own mocks for `.getItem` and
+    // `.setItem`; it writes to a variable instead of storage.
+    await ZulipAsyncStorage.setItem(key, value);
+    expect(await ZulipAsyncStorage.getItem(key)).toEqual(value);
+  });
+});

src/boot/__tests__/storage-test.js

@@ -0,0 +1,89 @@
+/* @flow strict-local */
+import invariant from 'invariant';
+
+import * as eg from '../../__tests__/lib/exampleData';
+import objectEntries from '../../utils/objectEntries';
+import type { GlobalState } from '../../types';
+import ZulipAsyncStorage from '../ZulipAsyncStorage';
+import { stringify, parse } from '../replaceRevive';
+
+const getRoundTrippedStateValue = async <K: $Keys<GlobalState>, V: $Values<GlobalState>>(
+  key: K,
+  value: V,
+): Promise<V> => {
+  // 1: Serialize a "live" object; e.g., a ZulipVersion instance.
+  const stringifiedValue = stringify(value);
+
+  // 2: Write to storage. Parts of this are mocked; so, stress-test
+  // those mocks:
+  //    - Compression via TextCompressionModule on Android
+  //    - AsyncStorage: the library provides a mock implementation
+  //      that writes to a variable instead of to the disk
+  await ZulipAsyncStorage.setItem(key, stringifiedValue);
+
+  // 3: Read from storage. Parts of this are mocked; so, stress-test
+  // those mocks:
+  //    - Decompression via TextCompressionModule on Android
+  //    - AsyncStorage: the library provides a mock implementation
+  //      that reads from a variable instead of from the disk
+  const valueFromStorage = await ZulipAsyncStorage.getItem(key);
+  invariant(valueFromStorage != null, 'valueFromStorage is not null/undefined');
+
+  // 4: "revive", e.g., a ZulipVersion instance.
+  const parsedValueFromStorage = parse(valueFromStorage);
+
+  // $FlowIgnore[incompatible-cast]
+  return (parsedValueFromStorage: V);
+};
+
+const getRoundTrippedState = async (globalState: GlobalState): Promise<GlobalState> => {
+  const entries = await Promise.all(
+    objectEntries(globalState).map(
+      // eslint-disable-next-line flowtype/generic-spacing
+      async <K: $Keys<GlobalState>, V: $Values<GlobalState>>([key: K, value: V]): Promise<
+        [K, V],
+      > => [
+        // $FlowIgnore[incompatible-cast]
+        (key: K),
+        // $FlowIgnore[incompatible-cast]
+        (await getRoundTrippedStateValue(key, value): V),
+      ],
+    ),
+  );
+  // $FlowIgnore[incompatible-indexer]
+  return Object.fromEntries(entries);
+};
+
+/**
+ * Test several pieces of the Redux storage logic at once.
+ *
+ * We're counting on a lot of our Redux state to round-trip through
+ * storage. Jest can't realistically test everything in this process
+ * -- in particular, the storage itself, and any native code involved
+ * -- but, in the places where it can't, at least it can stress-test
+ * the mocks we've written and use elsewhere, to make sure they're
+ * reasonably faithful.
+ *
+ * `eg.plusReduxState` represents standard example data, though not
+ * the entire range of possible data; see its jsdoc. Still, we'll want
+ * to know about any failures to round-trip that arise from changes to
+ * that.
+ */
+test('`eg.plusReduxState` round-trips through storage', async () => {
+  const stateBefore = eg.plusReduxState;
+  // The `GlobalState` annotation seems to guide Flow to better error
+  // messages; without it, Flow seems to get confused by the `await`.
+  const stateAfter: GlobalState = await getRoundTrippedState(stateBefore);
+
+  // `AvatarURL` instances are generally lazy with their `new URL`
+  // computations; they have a stateful piece that might be a string
+  // or a `URL` object. We could figure out how to teach Jest that
+  // that difference doesn't matter, but for now, just normalize both
+  // states by "waking up" their `AvatarURL`s.
+  [stateBefore, stateAfter].forEach(s => {
+    s.users.forEach(u => u.avatar_url.get(100));
+    s.messages.forEach(m => m.avatar_url.get(100));
+  });
+
+  expect(stateAfter).toEqual(stateBefore);
+});

src/boot/store.js

@@ -90,13 +90,11 @@ export const cacheKeys: Array<$Keys<GlobalState>> = [
 function dropCache(state: GlobalState): $Shape<GlobalState> {
   const result: $Shape<GlobalState> = {};
   storeKeys.forEach(key => {
-    // $FlowFixMe[incompatible-indexer]
-    // $FlowFixMe[incompatible-exact]
-    // $FlowFixMe[prop-missing]
-    // $FlowFixMe[incompatible-variance]
-    // $FlowFixMe[incompatible-type-arg]
-    /* $FlowFixMe[incompatible-type]
-       This is well-typed only because it's the same `key` twice. */
+    /* $FlowFixMe[cannot-write]
+       This is well-typed only because it's the same `key` twice. It seems
+       like we should have to suppress an error about not having that
+       guarantee, in addition to the more minor-looking `cannot-write`. Not
+       sure why we don't. */
     result[key] = state[key];
   });
   return result;

src/main/MainTabsScreen.js

@@ -52,7 +52,12 @@ export default function MainTabsScreen(props: Props) {
       <OfflineNotice />
       <Tab.Navigator
         {...bottomTabNavigatorConfig({
-          showLabel: !!Platform.isPad,
+          // TODO: Find a way to tell if we're on an Android tablet,
+          // and use that -- we don't want to assume Android users
+          // aren't on tablets, but `isPad` is iOS only and `Platform`
+          // doesn't have something else for Android (yet):
+          // https://reactnative.dev/docs/platform#ispad-ios
+          showLabel: Platform.OS === 'ios' && Platform.isPad,
           showIcon: true,
         })}
         backBehavior="none"

src/reduxTypes.js

@@ -329,7 +329,7 @@ export type UsersState = User[];
  * See in particular `discardKeys`, `storeKeys`, and `cacheKeys`, which
  * identify which subtrees are persisted and which are not.
  */
-export type GlobalState = {|
+export type GlobalState = $ReadOnly<{|
   accounts: AccountsState,
   alertWords: AlertWordsState,
   caughtUp: CaughtUpState,
@@ -355,7 +355,7 @@ export type GlobalState = {|
   userGroups: UserGroupsState,
   userStatus: UserStatusState,
   users: UsersState,
-|};
+|}>;
 
 /** A selector returning TResult, with extra parameter TParam. */
 // Seems like this should be OutputSelector... but for whatever reason,