Clean up recovery spec tests (#3085)

Author: schmidt-sebastian

packages/firestore/test/unit/local/indexeddb_persistence.test.ts

@@ -1144,7 +1144,7 @@ describe('IndexedDb: allowTabSynchronization', () => {
       'clientA',
       /* multiClient= */ false,
       async db => {
-        db.injectFailures = { updateClientMetadataAndTryBecomePrimary: true };
+        db.injectFailures = ['updateClientMetadataAndTryBecomePrimary'];
         await expect(db.start()).to.eventually.be.rejectedWith(
           'Failed to obtain exclusive access to the persistence layer.'
         );
@@ -1158,7 +1158,7 @@ describe('IndexedDb: allowTabSynchronization', () => {
       'clientA',
       /* multiClient= */ true,
       async db => {
-        db.injectFailures = { updateClientMetadataAndTryBecomePrimary: true };
+        db.injectFailures = ['updateClientMetadataAndTryBecomePrimary'];
         await db.start();
         await db.shutdown();
       }
@@ -1167,10 +1167,10 @@ describe('IndexedDb: allowTabSynchronization', () => {
 
   it('ignores intermittent IndexedDbTransactionError during lease refresh', async () => {
     await withPersistence('clientA', async (db, _, queue) => {
-      db.injectFailures = { updateClientMetadataAndTryBecomePrimary: true };
+      db.injectFailures = ['updateClientMetadataAndTryBecomePrimary'];
       await queue.runDelayedOperationsEarly(TimerId.ClientMetadataRefresh);
       await queue.enqueue(() => {
-        db.injectFailures = undefined;
+        db.injectFailures = [];
         return db.runTransaction('check success', 'readwrite-primary', () =>
           PersistencePromise.resolve()
         );

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

@@ -23,6 +23,10 @@ import { Code } from '../../../src/util/error';
 import { deletedDoc, doc, filter, path } from '../../util/helpers';
 import { RpcError } from './spec_rpc_error';
 
+// The IndexedDB action that the Watch stream uses to detect if IndexedDB access
+// is available again.
+const ASYNC_QUEUE_PROBER = 'Get last remote snapshot version';
+
 describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
   specTest(
     'Write is acknowledged by primary client (with recovery)',
@@ -34,11 +38,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
           .client(1)
           .expectPrimaryState(false)
           .userSets('collection/a', { v: 1 })
-          .failDatabaseTransactions({
-            'Locally write mutations': true,
-            'Synchronize last document change read time': true,
-            'Lookup mutation documents': true
-          })
+          .failDatabaseTransactions('Lookup mutation documents')
           .client(0)
           .writeAcks('collection/a', 1, { expectUserCallback: false })
           .client(1)
@@ -62,24 +62,21 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
     ['multi-client'],
     () => {
       const query = Query.atPath(path('collection'));
+      const doc1 = doc('collection/doc', 1, { foo: 'a' });
 
       return client(0)
         .expectPrimaryState(true)
         .client(1)
         .expectPrimaryState(false)
         .userListens(query)
-        .failDatabaseTransactions({
-          'Allocate target': true,
-          'Lookup mutation documents': true,
-          'Get new document changes': true
-        })
+        .failDatabaseTransactions('Get new document changes')
         .client(0)
         .expectListen(query)
-        .watchAcksFull(query, 1000)
+        .watchAcksFull(query, 1000, doc1)
         .client(1)
         .recoverDatabase()
         .runTimer(TimerId.AsyncQueueRetry)
-        .expectEvents(query, {});
+        .expectEvents(query, { added: [doc1] });
     }
   );
 
@@ -92,10 +89,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       return (
         client(0)
           .expectPrimaryState(true)
-          .failDatabaseTransactions({
-            'Allocate target': true,
-            'Get target data': true
-          })
+          .failDatabaseTransactions('Allocate target', 'Get target data')
           .client(1)
           .userListens(query)
           .client(0)
@@ -105,10 +99,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
           .recoverDatabase()
           .runTimer(TimerId.AsyncQueueRetry)
           .expectListen(query)
-          .failDatabaseTransactions({
-            'Allocate target': true,
-            'Release target': true
-          })
+          .failDatabaseTransactions('Release target')
           .client(1)
           .userUnlistens(query)
           .client(0)
@@ -130,18 +121,12 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
         .expectNumOutstandingWrites(1)
         // We fail the write if we cannot persist the local mutation (via
         // 'Locally write mutations').
-        .failDatabaseTransactions({
-          'Locally write mutations': true
-        })
+        .failDatabaseTransactions('Locally write mutations')
         .userSets('collection/key2', { bar: 'b' })
         .expectUserCallbacks({ rejected: ['collection/key2'] })
         // The write is considered successful if we can persist the local mutation
         // but fail to update view assignments (via 'notifyLocalViewChanges').
-        .failDatabaseTransactions({
-          'Locally write mutations': false,
-          notifyLocalViewChanges: true,
-          'Get next mutation batch': false
-        })
+        .failDatabaseTransactions('notifyLocalViewChanges')
         .userSets('collection/key3', { bar: 'b' })
         .recoverDatabase()
         .expectNumOutstandingWrites(2)
@@ -178,11 +163,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
         fromCache: true,
         hasPendingWrites: true
       })
-      .failDatabaseTransactions({
-        'Locally write mutations': true,
-        notifyLocalViewChanges: true,
-        'Get next mutation batch': true
-      })
+      .failDatabaseTransactions('Locally write mutations')
       .userSets('collection/key2', { foo: 'b' })
       .expectUserCallbacks({ rejected: ['collection/key2'] })
       .recoverDatabase()
@@ -213,12 +194,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       const doc2 = doc('collection/key2', 2, { foo: 'b' });
       return spec()
         .userListens(query)
-        .failDatabaseTransactions({
-          'Locally write mutations': false,
-          notifyLocalViewChanges: true,
-          'Get next mutation batch': false,
-          'Set last stream token': false
-        })
+        .failDatabaseTransactions('notifyLocalViewChanges')
         .userSets('collection/key1', { foo: 'a' })
         .expectEvents(query, {
           added: [doc1Local],
@@ -228,11 +204,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
         .recoverDatabase()
         .runTimer(TimerId.AsyncQueueRetry)
         .writeAcks('collection/key1', 1)
-        .failDatabaseTransactions({
-          'Apply remote event': false,
-          notifyLocalViewChanges: true,
-          'Get last remote snapshot version': false
-        })
+        .failDatabaseTransactions('notifyLocalViewChanges')
         .watchAcksFull(query, 1000, doc1, doc2)
         .expectEvents(query, {
           metadata: [doc1],
@@ -256,11 +228,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
           .expectEvents(query, {
             added: [doc1]
           })
-          .failDatabaseTransactions({
-            'Apply remote event': false,
-            notifyLocalViewChanges: true,
-            'Get last remote snapshot version': false
-          })
+          .failDatabaseTransactions('notifyLocalViewChanges')
           .watchSends({ removed: [query] }, deletedDoc1)
           .watchSnapshots(2000)
           .expectEvents(query, {
@@ -282,7 +250,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       .userListens(query1)
       .watchAcksFull(query1, 1)
       .expectEvents(query1, {})
-      .failDatabaseTransactions({ 'Allocate target': true })
+      .failDatabaseTransactions('Allocate target')
       .userListens(query2)
       .expectEvents(query2, { errorCode: Code.UNAVAILABLE })
       .recoverDatabase()
@@ -298,7 +266,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       .userListens(query1)
       .watchAcksFull(query1, 1)
       .expectEvents(query1, {})
-      .failDatabaseTransactions({ 'Allocate target': true })
+      .failDatabaseTransactions('Allocate target')
       .userListens(query2)
       .expectEvents(query2, { errorCode: Code.UNAVAILABLE })
       .recoverDatabase()
@@ -320,12 +288,9 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
           added: [doc1]
         })
         .watchSends({ affects: [query] }, doc2)
-        .failDatabaseTransactions({
-          'Get last remote snapshot version': true,
-          'Release target': true
-        })
+        .failDatabaseTransactions('Get last remote snapshot version')
         .watchSnapshots(1500)
-        // `failDatabase()` causes us to go offline.
+        // `failDatabaseTransactions()` causes us to go offline.
         .expectActiveTargets()
         .expectEvents(query, { fromCache: true })
         .recoverDatabase()
@@ -357,15 +322,12 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
         .expectEvents(doc2Query, {
           added: [doc2]
         })
-        .failDatabaseTransactions({
-          'Get last remote snapshot version': true,
-          'Release target': true
-        })
+        .failDatabaseTransactions('Release target', ASYNC_QUEUE_PROBER)
         .watchRemoves(
           doc1Query,
           new RpcError(Code.PERMISSION_DENIED, 'Simulated target error')
         )
-        // `failDatabase()` causes us to go offline.
+        // `failDatabaseTransactions()` causes us to go offline.
         .expectActiveTargets()
         .expectEvents(doc1Query, { fromCache: true })
         .expectEvents(doc2Query, { fromCache: true })
@@ -416,7 +378,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
         })
         .watchAcksFull(filteredQuery, 2000)
         .expectLimboDocs(doc1a.key)
-        .failDatabaseTransactions({ 'Get last remote snapshot version': true })
+        .failDatabaseTransactions('Get last remote snapshot version')
         .watchAcksFull(limboQuery, 3000, doc1b)
         .expectActiveTargets()
         .recoverDatabase()
@@ -459,10 +421,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       })
       .watchAcksFull(filteredQuery, 2000)
       .expectLimboDocs(doc1.key)
-      .failDatabaseTransactions({
-        'Apply remote event': true,
-        'Get last remote snapshot version': true
-      })
+      .failDatabaseTransactions('Apply remote event', ASYNC_QUEUE_PROBER)
       .watchRemoves(
         limboQuery,
         new RpcError(Code.PERMISSION_DENIED, 'Test error')

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

@@ -43,9 +43,9 @@ import { RpcError } from './spec_rpc_error';
 import { ObjectMap } from '../../../src/util/obj_map';
 import {
   parseQuery,
+  PersistenceAction,
   runSpec,
   SpecConfig,
-  SpecDatabaseFailures,
   SpecDocument,
   SpecQuery,
   SpecQueryFilter,
@@ -440,11 +440,11 @@ export class SpecBuilder {
    * Fails the specified database transaction until `recoverDatabase()` is
    * called.
    */
-  failDatabaseTransactions(failureMode: SpecDatabaseFailures): this {
+  failDatabaseTransactions(...actions: PersistenceAction[]): this {
     this.nextStep();
     this.injectFailures = true;
     this.currentStep = {
-      failDatabase: failureMode
+      failDatabase: actions
     };
     return this;
   }

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

@@ -36,7 +36,7 @@ import {
   MemoryPersistence
 } from '../../../src/local/memory_persistence';
 import { LruParams } from '../../../src/local/lru_garbage_collector';
-import { PersistenceAction, SpecDatabaseFailures } from './spec_test_runner';
+import { PersistenceAction } from './spec_test_runner';
 import { Connection, Stream } from '../../../src/remote/connection';
 import { StreamBridge } from '../../../src/remote/stream_bridge';
 import * as api from '../../../src/protos/firestore_proto_api';
@@ -57,7 +57,7 @@ import { expect } from 'chai';
  * transaction failures.
  */
 export class MockMemoryPersistence extends MemoryPersistence {
-  injectFailures?: SpecDatabaseFailures;
+  injectFailures: PersistenceAction[] = [];
 
   async runTransaction<T>(
     action: string,
@@ -76,7 +76,7 @@ export class MockMemoryPersistence extends MemoryPersistence {
  * transaction failures.
  */
 export class MockIndexedDbPersistence extends IndexedDbPersistence {
-  injectFailures?: SpecDatabaseFailures;
+  injectFailures: PersistenceAction[] = [];
 
   async runTransaction<T>(
     action: string,
@@ -95,18 +95,15 @@ export class MockIndexedDbPersistence extends IndexedDbPersistence {
  * MockMemoryPersistence that can inject transaction failures.
  */
 function failTransactionIfNeeded(
-  config: SpecDatabaseFailures | undefined,
-  action: string
+  failActions: PersistenceAction[],
+  actionName: string
 ): void {
-  if (config) {
-    const shouldFail = config[action as PersistenceAction];
-    if (shouldFail === undefined) {
-      throw fail('Failure mode not specified for action: ' + action);
-    } else if (shouldFail) {
-      throw new IndexedDbTransactionError(
-        new Error('Simulated retryable error: ' + action)
-      );
-    }
+  const shouldFail =
+    failActions.indexOf(actionName as PersistenceAction) !== -1;
+  if (shouldFail) {
+    throw new IndexedDbTransactionError(
+      new Error('Simulated retryable error: ' + actionName)
+    );
   }
 }
 

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

@@ -371,7 +371,7 @@ abstract class TestRunner {
     await this.queue.enqueue(() => this.eventManager.listen(queryListener));
 
     if (targetFailed) {
-      expect(this.persistence.injectFailures?.['Allocate target']).to.be.true;
+      expect(this.persistence.injectFailures).contains('Allocate target');
     } else {
       // Skip the backoff that may have been triggered by a previous call to
       // `watchStreamCloses()`.
@@ -446,7 +446,9 @@ abstract class TestRunner {
       () => this.rejectedDocs.push(...documentKeys)
     );
 
-    if (this.persistence.injectFailures?.['Locally write mutations'] !== true) {
+    if (
+      this.persistence.injectFailures.indexOf('Locally write mutations') === -1
+    ) {
       this.sharedWrites.push(mutations);
     }
 
@@ -726,13 +728,13 @@ abstract class TestRunner {
   }
 
   private async doFailDatabase(
-    failActions: SpecDatabaseFailures
+    failActions: PersistenceAction[]
   ): Promise<void> {
     this.persistence.injectFailures = failActions;
   }
 
   private async doRecoverDatabase(): Promise<void> {
-    this.persistence.injectFailures = undefined;
+    this.persistence.injectFailures = [];
   }
 
   private validateExpectedSnapshotEvents(
@@ -1225,13 +1227,6 @@ export type PersistenceAction =
   | 'Synchronize last document change read time'
   | 'updateClientMetadataAndTryBecomePrimary';
 
-/** Specifies failure or success for a list of database actions. */
-export type SpecDatabaseFailures = Partial<
-  {
-    readonly [key in PersistenceAction]: boolean;
-  }
->;
-
 /**
  * Union type for each step. The step consists of exactly one `field`
  * set and optionally expected events in the `expect` field.
@@ -1277,7 +1272,7 @@ export interface SpecStep {
   failWrite?: SpecWriteFailure;
 
   /** Fails the listed database actions. */
-  failDatabase?: false | SpecDatabaseFailures;
+  failDatabase?: false | PersistenceAction[];
 
   /**
    * Run a queued timer task (without waiting for the delay to expire). See