Stop saving persistence layer across spec tests (#1129)

* Add no-memory-persistence flag to spec tests.

* Restart persistence for spec tests

* [AUTOMATED]: Prettier Code Styling

* No more negative tags

* [AUTOMATED]: Prettier Code Styling

* Put back the no-platform tags

* [AUTOMATED]: Prettier Code Styling

Author: Greg Soltis

packages/firestore/test/unit/specs/describe_spec.ts

@@ -24,13 +24,15 @@ import { SpecStep } from './spec_test_runner';
 // Disables all other tests; useful for debugging. Multiple tests can have
 // this tag and they'll all be run (but all others won't).
 const EXCLUSIVE_TAG = 'exclusive';
-// Multi-client related tests (which imply persistence).
-const MULTI_CLIENT_TAG = 'multi-client';
 // Explicit per-platform disable flags.
 const NO_WEB_TAG = 'no-web';
 const NO_ANDROID_TAG = 'no-android';
 const NO_IOS_TAG = 'no-ios';
-const NO_LRU_TAG = 'no-lru';
+// The remaining tags specify features that must be present to run a given test
+// Multi-client related tests (which imply persistence).
+const MULTI_CLIENT_TAG = 'multi-client';
+const EAGER_GC_TAG = 'eager-gc';
+const DURABLE_PERSISTENCE_TAG = 'durable-persistence';
 const BENCHMARK_TAG = 'benchmark';
 const KNOWN_TAGS = [
   BENCHMARK_TAG,
@@ -39,7 +41,8 @@ const KNOWN_TAGS = [
   NO_WEB_TAG,
   NO_ANDROID_TAG,
   NO_IOS_TAG,
-  NO_LRU_TAG
+  EAGER_GC_TAG,
+  DURABLE_PERSISTENCE_TAG
 ];
 
 // TOOD(mrschmidt): Make this configurable with mocha options.
@@ -78,7 +81,13 @@ export function setSpecJSONHandler(writer: (json: string) => void): void {
 function getTestRunner(tags, persistenceEnabled): Function {
   if (tags.indexOf(NO_WEB_TAG) >= 0) {
     return it.skip;
-  } else if (persistenceEnabled && tags.indexOf(NO_LRU_TAG) !== -1) {
+  } else if (
+    !persistenceEnabled &&
+    tags.indexOf(DURABLE_PERSISTENCE_TAG) !== -1
+  ) {
+    // Test requires actual persistence, but it's not enabled. Skip it.
+    return it.skip;
+  } else if (persistenceEnabled && tags.indexOf(EAGER_GC_TAG) !== -1) {
     // spec should have a comment explaining why it is being skipped.
     return it.skip;
   } else if (!persistenceEnabled && tags.indexOf(MULTI_CLIENT_TAG) !== -1) {

packages/firestore/test/unit/specs/listen_spec.test.ts

@@ -27,7 +27,7 @@ describeSpec('Listens:', [], () => {
   // Obviously this test won't hold with offline persistence enabled.
   specTest(
     'Contents of query are cleared when listen is removed.',
-    ['no-lru'],
+    ['eager-gc'],
     'Explicitly tests eager GC behavior',
     () => {
       const query = Query.atPath(path('collection'));
@@ -228,7 +228,7 @@ describeSpec('Listens:', [], () => {
   // This would only happen when we use a resume token, but omitted for brevity.
   specTest(
     'Will gracefully handle watch stream reverting snapshots (with restart)',
-    [],
+    ['durable-persistence'],
     () => {
       const query = Query.atPath(path('collection'));
       const docAv1 = doc('collection/a', 1000, { v: 'v1000' });
@@ -570,34 +570,38 @@ describeSpec('Listens:', [], () => {
     );
   });
 
-  specTest('Omits global resume tokens for a short while', [], () => {
-    const query = Query.atPath(path('collection'));
-    const docA = doc('collection/a', 1000, { key: 'a' });
+  specTest(
+    'Omits global resume tokens for a short while',
+    ['durable-persistence'],
+    () => {
+      const query = Query.atPath(path('collection'));
+      const docA = doc('collection/a', 1000, { key: 'a' });
 
-    return (
-      spec()
-        .withGCEnabled(false)
-        .userListens(query)
-        .watchAcksFull(query, 1000, docA)
-        .expectEvents(query, { added: [docA] })
+      return (
+        spec()
+          .withGCEnabled(false)
+          .userListens(query)
+          .watchAcksFull(query, 1000, docA)
+          .expectEvents(query, { added: [docA] })
 
-        // One millisecond later, watch sends an updated resume token but the
-        // user doesn't manage to unlisten before restart.
-        .watchSnapshots(2000, [], 'resume-token-2000')
-        .restart()
+          // One millisecond later, watch sends an updated resume token but the
+          // user doesn't manage to unlisten before restart.
+          .watchSnapshots(2000, [], 'resume-token-2000')
+          .restart()
 
-        .userListens(query, 'resume-token-1000')
-        .expectEvents(query, { added: [docA], fromCache: true })
-        .watchAcks(query)
-        .watchCurrents(query, 'resume-token-3000')
-        .watchSnapshots(3000)
-        .expectEvents(query, { fromCache: false })
-    );
-  });
+          .userListens(query, 'resume-token-1000')
+          .expectEvents(query, { added: [docA], fromCache: true })
+          .watchAcks(query)
+          .watchCurrents(query, 'resume-token-3000')
+          .watchSnapshots(3000)
+          .expectEvents(query, { fromCache: false })
+      );
+    }
+  );
 
   specTest(
     'Persists global resume tokens if the snapshot is old enough',
-    [],
+    ['durable-persistence'],
     () => {
       const initialVersion = 1000;
       const minutesLater = 5 * 60 * 1e6 + initialVersion;

packages/firestore/test/unit/specs/offline_spec.test.ts

@@ -61,7 +61,7 @@ describeSpec('Offline:', [], () => {
 
   specTest(
     'Removing all listeners delays "Offline" status on next listen',
-    ['no-lru'],
+    ['eager-gc'],
     'Marked as no-lru because when a listen is re-added, it gets a new target id rather than reusing one',
     () => {
       const query = Query.atPath(path('collection'));

packages/firestore/test/unit/specs/persistence_spec.test.ts

@@ -22,48 +22,56 @@ import { client, spec } from './spec_builder';
 import { TimerId } from '../../../src/util/async_queue';
 
 describeSpec('Persistence:', [], () => {
-  specTest('Local mutations are persisted and re-sent', [], () => {
-    return spec()
-      .userSets('collection/key1', { foo: 'bar' })
-      .userSets('collection/key2', { baz: 'quu' })
-      .restart()
-      .expectNumOutstandingWrites(2)
-      .writeAcks('collection/key1', 1, { expectUserCallback: false })
-      .writeAcks('collection/key2', 2, { expectUserCallback: false })
-      .expectNumOutstandingWrites(0);
-  });
-
-  specTest('Persisted local mutations are visible to listeners', [], () => {
-    const query = Query.atPath(path('collection'));
-    return (
-      spec()
+  specTest(
+    'Local mutations are persisted and re-sent',
+    ['durable-persistence'],
+    () => {
+      return spec()
         .userSets('collection/key1', { foo: 'bar' })
         .userSets('collection/key2', { baz: 'quu' })
         .restart()
-        // It should be visible to listens.
-        .userListens(query)
-        .expectEvents(query, {
-          added: [
-            doc(
-              'collection/key1',
-              0,
-              { foo: 'bar' },
-              { hasLocalMutations: true }
-            ),
-            doc(
-              'collection/key2',
-              0,
-              { baz: 'quu' },
-              { hasLocalMutations: true }
-            )
-          ],
-          fromCache: true,
-          hasPendingWrites: true
-        })
-    );
-  });
+        .expectNumOutstandingWrites(2)
+        .writeAcks('collection/key1', 1, { expectUserCallback: false })
+        .writeAcks('collection/key2', 2, { expectUserCallback: false })
+        .expectNumOutstandingWrites(0);
+    }
+  );
+
+  specTest(
+    'Persisted local mutations are visible to listeners',
+    ['durable-persistence'],
+    () => {
+      const query = Query.atPath(path('collection'));
+      return (
+        spec()
+          .userSets('collection/key1', { foo: 'bar' })
+          .userSets('collection/key2', { baz: 'quu' })
+          .restart()
+          // It should be visible to listens.
+          .userListens(query)
+          .expectEvents(query, {
+            added: [
+              doc(
+                'collection/key1',
+                0,
+                { foo: 'bar' },
+                { hasLocalMutations: true }
+              ),
+              doc(
+                'collection/key2',
+                0,
+                { baz: 'quu' },
+                { hasLocalMutations: true }
+              )
+            ],
+            fromCache: true,
+            hasPendingWrites: true
+          })
+      );
+    }
+  );
 
-  specTest('Remote documents are persisted', [], () => {
+  specTest('Remote documents are persisted', ['durable-persistence'], () => {
     const query = Query.atPath(path('collection'));
     const doc1 = doc('collection/key', 1000, { foo: 'bar' });
     return spec()
@@ -126,7 +134,7 @@ describeSpec('Persistence:', [], () => {
 
   specTest(
     'Mutation Queue is persisted across uid switches (with restarts)',
-    [],
+    ['durable-persistence'],
     () => {
       return spec()
         .userSets('users/anon', { uid: 'anon' })

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -418,10 +418,6 @@ abstract class TestRunner {
 
   async start(): Promise<void> {
     this.persistence = await this.initPersistence(this.serializer);
-    await this.init();
-  }
-
-  private async init(): Promise<void> {
     const garbageCollector = this.getGarbageCollector();
 
     this.sharedClientState = this.getSharedClientState();
@@ -872,13 +868,14 @@ abstract class TestRunner {
   }
 
   private async doRestart(): Promise<void> {
-    // Reinitialize everything, except the persistence.
+    // Reinitialize everything.
     // No local store to shutdown.
     await this.remoteStore.shutdown();
+    await this.persistence.shutdown(/* deleteData= */ false);
 
     // We have to schedule the starts, otherwise we could end up with
     // interleaved events.
-    await this.queue.enqueue(() => this.init());
+    await this.queue.enqueue(() => this.start());
   }
 
   private async doApplyClientState(state: SpecClientState): Promise<void> {

packages/firestore/test/unit/specs/write_spec.test.ts

@@ -455,7 +455,7 @@ describeSpec('Writes:', [], () => {
 
   specTest(
     'Held writes are released when there are no queries left.',
-    ['no-lru'],
+    ['eager-gc'],
     'This test expects a new target id for a new listen, but without eager gc, the same target id is reused',
     () => {
       const query = Query.atPath(path('collection'));