cleanup

Author: dconeybe

packages/firestore/test/integration/api/get_options.test.ts

@@ -496,8 +496,8 @@ apiDescribe('GetOptions', persistence => {
     });
   });
 
-  // We need the deleted doc to stay in cache, so only run this test with
-  // persistence with LRU garbage collection (rather than eager GC).
+  // We need the deleted doc to stay in cache, so only run this test when the
+  // local cache is configured with LRU GC (as opposed to eager GC).
   // eslint-disable-next-line no-restricted-properties,
   (persistence.gc === 'lru' ? it : it.skip)(
     'get deleted doc while offline with source=cache',

packages/firestore/test/integration/api/query.test.ts

@@ -1333,8 +1333,8 @@ apiDescribe('Queries', persistence => {
     });
   });
 
-  // OR Query tests only run when the SDK is configured with persistence that
-  // uses LRU garbage collection (rather than eager garbage collection) because
+  // OR Query tests only run when the SDK's local cache is configured to use
+  // LRU garbage collection (rather than eager garbage collection) because
   // they validate that the result from server and cache match.
   // eslint-disable-next-line no-restricted-properties
   (persistence.gc === 'lru' ? describe : describe.skip)('OR Queries', () => {
@@ -1647,8 +1647,8 @@ apiDescribe('Queries', persistence => {
     });
   });
 
-  // OR Query tests only run when the SDK is configured with persistence that
-  // uses LRU garbage collection (rather than eager garbage collection) because
+  // OR Query tests only run when the SDK's local cache is configured to use
+  // LRU garbage collection (rather than eager garbage collection) because
   // they validate that the result from server and cache match. Additionally,
   // these tests must be skipped if running against production because it
   // results in a 'missing index' error. The Firestore Emulator, however, does
@@ -2034,49 +2034,46 @@ apiDescribe('Queries', persistence => {
   );
 
   // Reproduces https://github.com/firebase/firebase-js-sdk/issues/5873
-  describe(
-    'Caching empty results',
-    () => {
-      it('can raise initial snapshot from cache, even if it is empty', () => {
-        // Use persistence with LRU garbage collection so that the cached resume
-        // token and document data do not get cleared.
-        return withTestCollection(persistence.toLruGc(), {}, async coll => {
-          const snapshot1 = await getDocs(coll); // Populate the cache.
-          expect(snapshot1.metadata.fromCache).to.be.false;
-          expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check.
-
-          // Add a snapshot listener whose first event should be raised from cache.
-          const storeEvent = new EventsAccumulator<QuerySnapshot>();
-          onSnapshot(coll, storeEvent.storeEvent);
-          const snapshot2 = await storeEvent.awaitEvent();
-          expect(snapshot2.metadata.fromCache).to.be.true;
-          expect(toDataArray(snapshot2)).to.deep.equal([]);
-        });
+  describe('Caching empty results', () => {
+    it('can raise initial snapshot from cache, even if it is empty', () => {
+      // Use persistence with LRU garbage collection so the resume token and
+      // document data do not get prematurely deleted from the local cache.
+      return withTestCollection(persistence.toLruGc(), {}, async coll => {
+        const snapshot1 = await getDocs(coll); // Populate the cache.
+        expect(snapshot1.metadata.fromCache).to.be.false;
+        expect(toDataArray(snapshot1)).to.deep.equal([]); // Precondition check.
+
+        // Add a snapshot listener whose first event should be raised from cache.
+        const storeEvent = new EventsAccumulator<QuerySnapshot>();
+        onSnapshot(coll, storeEvent.storeEvent);
+        const snapshot2 = await storeEvent.awaitEvent();
+        expect(snapshot2.metadata.fromCache).to.be.true;
+        expect(toDataArray(snapshot2)).to.deep.equal([]);
       });
+    });
 
-      it('can raise initial snapshot from cache, even if it has become empty', () => {
-        const testDocs = {
-          a: { key: 'a' }
-        };
-        // Use persistence with LRU garbage collection so that the cached resume
-        // token and document data do not get cleared.
-        return withTestCollection(persistence.toLruGc(), testDocs, async coll => {
-          // Populate the cache.
-          const snapshot1 = await getDocs(coll);
-          expect(snapshot1.metadata.fromCache).to.be.false;
-          expect(toDataArray(snapshot1)).to.deep.equal([{ key: 'a' }]);
-          // Empty the collection.
-          void deleteDoc(doc(coll, 'a'));
-
-          const storeEvent = new EventsAccumulator<QuerySnapshot>();
-          onSnapshot(coll, storeEvent.storeEvent);
-          const snapshot2 = await storeEvent.awaitEvent();
-          expect(snapshot2.metadata.fromCache).to.be.true;
-          expect(toDataArray(snapshot2)).to.deep.equal([]);
-        });
+    it('can raise initial snapshot from cache, even if it has become empty', () => {
+      const testDocs = {
+        a: { key: 'a' }
+      };
+      // Use persistence with LRU garbage collection so the resume token and
+      // document data do not get prematurely deleted from the local cache.
+      return withTestCollection(persistence.toLruGc(), testDocs, async coll => {
+        // Populate the cache.
+        const snapshot1 = await getDocs(coll);
+        expect(snapshot1.metadata.fromCache).to.be.false;
+        expect(toDataArray(snapshot1)).to.deep.equal([{ key: 'a' }]);
+        // Empty the collection.
+        void deleteDoc(doc(coll, 'a'));
+
+        const storeEvent = new EventsAccumulator<QuerySnapshot>();
+        onSnapshot(coll, storeEvent.storeEvent);
+        const snapshot2 = await storeEvent.awaitEvent();
+        expect(snapshot2.metadata.fromCache).to.be.true;
+        expect(toDataArray(snapshot2)).to.deep.equal([]);
       });
-    }
-  );
+    });
+  });
 
   it('resuming a query should use bloom filter to avoid full requery', async () => {
     // Prepare the names and contents of the 100 documents to create.
@@ -2142,10 +2139,10 @@ apiDescribe('Queries', persistence => {
         );
       }
 
-      // Skip the verification of the existence filter mismatch when persistence
-      // uses eager garbage collection because with eager GC there is no resume
-      // token specified in the subsequent call to getDocs(), and, therefore,
-      // Watch will _not_ send an existence filter.
+      // Skip the verification of the existence filter mismatch when the local
+      // cache is configured to use eager garbage collection because with eager
+      // GC there is no resume token specified in the subsequent call to
+      // getDocs(), and, therefore, Watch will _not_ send an existence filter.
       // TODO(b/272754156) Re-write this test using a snapshot listener instead
       // of calls to getDocs() and remove this check for disabled persistence.
       if (persistence.gc === 'eager') {

packages/firestore/test/integration/api/validation.test.ts

@@ -56,10 +56,10 @@ import {
 } from '../util/firebase_export';
 import {
   apiDescribe,
+  PersistenceMode,
   withAlternateTestDb,
   withTestCollection,
-  withTestDb,
-  PersistenceMode
+  withTestDb
 } from '../util/helpers';
 import { ALT_PROJECT_ID, DEFAULT_PROJECT_ID } from '../util/settings';
 
@@ -133,20 +133,8 @@ class TestClass {
   constructor(readonly property: string) {}
 }
 
-apiDescribe.only('Validation:', persistence => {
+apiDescribe('Validation:', persistence => {
   describe('FirestoreSettings', () => {
-    validationIt(
-      persistence,
-      'precondition check: verify the Firestore settings are not frozen',
-      async db => {
-        // This should not throw an exception. This test just verifies that the
-        // Firestore instance's initial state does not have its settings frozen,
-        // because if it did then the following tests would not be testing the
-        // behavior they intend to test.
-        connectFirestoreEmulator(db, 'something-else.example.com', 8080)
-      }
-    );
-
     validationIt(
       persistence,
       'disallows changing settings after use',
@@ -175,15 +163,19 @@ apiDescribe.only('Validation:', persistence => {
       });
     });
 
-    validationIt(persistence, 'useEmulator can set host and port', () => {
-      const db = newTestFirestore(newTestApp('test-project'));
-      // Verify that this doesn't throw.
-      connectFirestoreEmulator(db, 'localhost', 9000);
-    });
+    validationIt(
+      persistence,
+      'connectFirestoreEmulator() can set host and port',
+      () => {
+        const db = newTestFirestore(newTestApp('test-project'));
+        // Verify that this doesn't throw.
+        connectFirestoreEmulator(db, 'localhost', 9000);
+      }
+    );
 
     validationIt(
       persistence,
-      'disallows calling useEmulator after use',
+      'disallows calling connectFirestoreEmulator() after use',
       async db => {
         const errorMsg =
           'Firestore has already been started and its settings can no longer be changed.';
@@ -197,7 +189,7 @@ apiDescribe.only('Validation:', persistence => {
 
     validationIt(
       persistence,
-      'useEmulator can set mockUserToken object',
+      'connectFirestoreEmulator() can set mockUserToken object',
       () => {
         const db = newTestFirestore(newTestApp('test-project'));
         // Verify that this doesn't throw.
@@ -209,7 +201,7 @@ apiDescribe.only('Validation:', persistence => {
 
     validationIt(
       persistence,
-      'useEmulator can set mockUserToken string',
+      'connectFirestoreEmulator() can set mockUserToken string',
       () => {
         const db = newTestFirestore(newTestApp('test-project'));
         // Verify that this doesn't throw.
@@ -235,19 +227,11 @@ apiDescribe.only('Validation:', persistence => {
   });
 
   describe('Firestore', () => {
-    validationIt(
-      persistence,
-      'allows calling enableIndexedDbPersistence() when persistence configured in the settings',
-      async db => {
-        await enableIndexedDbPersistence(db);
-      }
-    );
-
     validationIt(
       persistence,
       'disallows calling enableIndexedDbPersistence() after use',
       db => {
-        doc(db, 'foo/bar');
+        //doc(db, 'foo/bar');
         expect(() => enableIndexedDbPersistence(db)).to.throw(
           'SDK cache is already specified.'
         );

packages/firestore/test/integration/api_internal/database.test.ts

@@ -35,7 +35,7 @@ use(chaiAsPromised);
 
 apiDescribe('Database (with internal API)', persistence => {
   // eslint-disable-next-line no-restricted-properties
-  (persistence.gc === 'lru' ? it : it.skip)(
+  (persistence.storage === 'indexeddb' ? it : it.skip)(
     'will reject the promise if clear persistence fails',
     async () => {
       await withTestDoc(persistence, async (docRef, firestore) => {

packages/firestore/test/integration/util/helpers.ts

@@ -71,7 +71,7 @@ export interface PersistenceMode {
    * Creates and returns a new "local cache" object corresponding to this
    * persistence type.
    */
-  toLocalCache(): MemoryLocalCache | PersistentLocalCache;
+  asLocalCacheFirestoreSettings(): MemoryLocalCache | PersistentLocalCache;
 }
 
 export class MemoryEagerPersistenceMode implements PersistenceMode {
@@ -87,7 +87,7 @@ export class MemoryEagerPersistenceMode implements PersistenceMode {
     return new MemoryLruPersistenceMode();
   }
 
-  toLocalCache(): MemoryLocalCache {
+  asLocalCacheFirestoreSettings(): MemoryLocalCache {
     return memoryLocalCache({
       garbageCollector: memoryEagerGarbageCollector()
     });
@@ -107,7 +107,7 @@ export class MemoryLruPersistenceMode implements PersistenceMode {
     return new MemoryLruPersistenceMode();
   }
 
-  toLocalCache(): MemoryLocalCache {
+  asLocalCacheFirestoreSettings(): MemoryLocalCache {
     return memoryLocalCache({ garbageCollector: memoryLruGarbageCollector() });
   }
 }
@@ -125,9 +125,12 @@ export class IndexedDbPersistenceMode implements PersistenceMode {
     return new IndexedDbPersistenceMode();
   }
 
-  toLocalCache(): PersistentLocalCache {
-    if (this.gc != 'lru') {
-      throw new Error(`unsupported gc: ${this.gc}`);
+  asLocalCacheFirestoreSettings(): PersistentLocalCache {
+    if (this.gc !== 'lru') {
+      throw new Error(
+        `PersistentLocalCache does not support the given ` +
+          `garbage collector: ${this.gc}`
+      );
     }
     return persistentLocalCache();
   }
@@ -173,11 +176,11 @@ function apiDescribeInternal(
   }
 
   for (const persistenceMode of persistenceModes) {
-    // Freeze the persistence mode so that tests don't modify the persistence
-    // mode, which is shared between tests.
-    const frozenPersistenceMode = Object.freeze(persistenceMode);
     describeFn(`(Persistence=${persistenceMode.name}) ${message}`, () =>
-      testSuite(frozenPersistenceMode)
+      // Freeze the properties of the `PersistenceMode` object specified to the
+      // test suite so that it cannot (accidentally or intentionally) change
+      // its properties, and affect all subsequent test suites.
+      testSuite(Object.freeze(persistenceMode))
     );
   }
 }
@@ -230,7 +233,7 @@ export function toIds(docSet: QuerySnapshot): string[] {
 }
 
 export function withTestDb(
-  persistence: PersistenceMode | null,
+  persistence: PersistenceMode,
   fn: (db: Firestore) => Promise<void>
 ): Promise<void> {
   return withTestDbs(persistence, 1, ([db]) => {
@@ -255,7 +258,7 @@ export function withAlternateTestDb(
 }
 
 export function withTestDbs(
-  persistence: PersistenceMode | null,
+  persistence: PersistenceMode,
   numDbs: number,
   fn: (db: Firestore[]) => Promise<void>
 ): Promise<void> {
@@ -268,7 +271,7 @@ export function withTestDbs(
   );
 }
 export async function withTestDbsSettings<T>(
-  persistence: PersistenceMode | null,
+  persistence: PersistenceMode,
   projectId: string,
   settings: PrivateSettings,
   numDbs: number,
@@ -281,10 +284,10 @@ export async function withTestDbsSettings<T>(
   const dbs: Firestore[] = [];
 
   for (let i = 0; i < numDbs; i++) {
-    const newSettings = { ...settings };
-    if (persistence !== null) {
-      newSettings.localCache = persistence.toLocalCache();
-    }
+    const newSettings = {
+      ...settings,
+      localCache: persistence.asLocalCacheFirestoreSettings()
+    };
     const db = newTestFirestore(newTestApp(projectId), newSettings);
     dbs.push(db);
   }
@@ -318,7 +321,7 @@ export async function withNamedTestDbsOrSkipUnlessUsingEmulator(
   for (const dbName of dbNames) {
     const newSettings = {
       ...DEFAULT_SETTINGS,
-      localCache: persistence.toLocalCache()
+      localCache: persistence.asLocalCacheFirestoreSettings()
     };
     const db = newTestFirestore(app, newSettings, dbName);
     dbs.push(db);