Skip to content

Clean up recovery spec tests #3085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 21, 2020
Merged

Clean up recovery spec tests #3085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/firestore/test/unit/local/indexeddb_persistence.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
);
Expand All @@ -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();
}
Expand All @@ -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()
);
Expand Down
91 changes: 25 additions & 66 deletions packages/firestore/test/unit/specs/recovery_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)',
Expand All @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a different test than what was here before. Previously the write of the mutation to IndexedDB would fail (as would the lookup) but now this is failing just the lookup and allowing the write to succeed?

Alternatively, if this is working as intended, better names would help make it clearer just which transaction is failing. For example, if this were named multitab.secondary.mutationLookup it would be much, much clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a dedicated test that test if "Locally write mutations" fails ("Recovers when write cannot be persisted"). The fact that "Locally write mutations" is part of this list seems like an oversight to me - the mutation gets written right before and not after we start injecting failures.

There are certainly improvements to the transaction names that we should make before we port to Android/iOS, but I would like to point out that 'Lookup mutation documents' is also used by the primary tab to load a mutation that has been written by a secondary tab.

.client(0)
.writeAcks('collection/a', 1, { expectUserCallback: false })
.client(1)
Expand All @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern here: previously this failed to allocate a target at all, which should have caused the listen to fail. Now this is only failing much later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar response: The target is already allocated and this seems like a copy/paste issue. Failed target allocations are tested in 'Fails targets that cannot be allocated'.
I am not 100% convinced we need multi-tab specific tests for these initial failures, since these failures happen before we kick off any multi-tab specific failures. If we add a test, it would be a sanity check to ensure that we don't write a Local Storage notification if we failed to persist the target/write.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I see it now. Thanks for talking it through with me.

.client(0)
.expectListen(query)
.watchAcksFull(query, 1000)
.watchAcksFull(query, 1000, doc1)
.client(1)
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
.expectEvents(query, {});
.expectEvents(query, { added: [doc1] });
}
);

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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],
Expand All @@ -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],
Expand All @@ -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, {
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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')
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/test/unit/specs/spec_builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
25 changes: 11 additions & 14 deletions packages/firestore/test/unit/specs/spec_test_components.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -57,7 +57,7 @@ import { expect } from 'chai';
* transaction failures.
*/
export class MockMemoryPersistence extends MemoryPersistence {
injectFailures?: SpecDatabaseFailures;
injectFailures: PersistenceAction[] = [];

async runTransaction<T>(
action: string,
Expand All @@ -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,
Expand All @@ -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)
);
}
}

Expand Down
19 changes: 7 additions & 12 deletions packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()`.
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -718,13 +720,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(
Expand Down Expand Up @@ -1217,13 +1219,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.
Expand Down Expand Up @@ -1269,7 +1264,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
Expand Down