Skip to content

Commit b59dbb5

Browse files
dconeybedconeybe
committed
withRetry helper added, and a test was modified to use it: 'resuming a query should use bloom filter to avoid full requery'
1 parent d41ee2f commit b59dbb5

File tree

2 files changed

+128
-122
lines changed

2 files changed

+128
-122
lines changed

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

Lines changed: 108 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,12 @@ import {
6060
} from '../util/firebase_export';
6161
import {
6262
apiDescribe,
63+
RetryError,
6364
toChangesArray,
6465
toDataArray,
6566
toIds,
6667
withEmptyTestCollection,
68+
withRetry,
6769
withTestCollection,
6870
withTestDb
6971
} from '../util/helpers';
@@ -2082,134 +2084,118 @@ apiDescribe('Queries', persistence => {
20822084
testDocs['doc' + (1000 + i)] = { key: 42 };
20832085
}
20842086

2085-
// The function that runs a single iteration of the test.
2086-
// Below this definition, there is a "while" loop that calls this function
2087-
// potentially multiple times.
2088-
const runTestIteration = async (
2089-
coll: CollectionReference,
2090-
db: Firestore
2091-
): Promise<'retry' | 'passed'> => {
2092-
// Run a query to populate the local cache with the 100 documents and a
2093-
// resume token.
2094-
const snapshot1 = await getDocs(coll);
2095-
expect(snapshot1.size, 'snapshot1.size').to.equal(100);
2096-
const createdDocuments = snapshot1.docs.map(snapshot => snapshot.ref);
2097-
2098-
// Delete 50 of the 100 documents. Do this in a transaction, rather than
2099-
// deleteDoc(), to avoid affecting the local cache.
2100-
const deletedDocumentIds = new Set<string>();
2101-
await runTransaction(db, async txn => {
2102-
for (let i = 0; i < createdDocuments.length; i += 2) {
2103-
const documentToDelete = createdDocuments[i];
2104-
txn.delete(documentToDelete);
2105-
deletedDocumentIds.add(documentToDelete.id);
2106-
}
2107-
});
2087+
// Ensure that the local cache is configured to use LRU garbage
2088+
// collection (rather than eager garbage collection) so that the resume
2089+
// token and document data does not get prematurely evicted.
2090+
const lruPersistence = persistence.toLruGc();
21082091

2109-
// Wait for 10 seconds, during which Watch will stop tracking the query
2110-
// and will send an existence filter rather than "delete" events when the
2111-
// query is resumed.
2112-
await new Promise(resolve => setTimeout(resolve, 10000));
2113-
2114-
// Resume the query and save the resulting snapshot for verification.
2115-
// Use some internal testing hooks to "capture" the existence filter
2116-
// mismatches to verify that Watch sent a bloom filter, and it was used to
2117-
// avert a full requery.
2118-
const [existenceFilterMismatches, snapshot2] =
2119-
await captureExistenceFilterMismatches(() => getDocs(coll));
2120-
2121-
// Verify that the snapshot from the resumed query contains the expected
2122-
// documents; that is, that it contains the 50 documents that were _not_
2123-
// deleted.
2124-
// TODO(b/270731363): Remove the "if" condition below once the
2125-
// Firestore Emulator is fixed to send an existence filter. At the time of
2126-
// writing, the Firestore emulator fails to send an existence filter,
2127-
// resulting in the client including the deleted documents in the snapshot
2128-
// of the resumed query.
2129-
if (!(USE_EMULATOR && snapshot2.size === 100)) {
2130-
const actualDocumentIds = snapshot2.docs
2131-
.map(documentSnapshot => documentSnapshot.ref.id)
2132-
.sort();
2133-
const expectedDocumentIds = createdDocuments
2134-
.filter(documentRef => !deletedDocumentIds.has(documentRef.id))
2135-
.map(documentRef => documentRef.id)
2136-
.sort();
2137-
expect(actualDocumentIds, 'snapshot2.docs').to.deep.equal(
2138-
expectedDocumentIds
2139-
);
2140-
}
2092+
return withRetry(async attemptNumber => {
2093+
return withTestCollection(lruPersistence, testDocs, async (coll, db) => {
2094+
// Run a query to populate the local cache with the 100 documents and a
2095+
// resume token.
2096+
const snapshot1 = await getDocs(coll);
2097+
expect(snapshot1.size, 'snapshot1.size').to.equal(100);
2098+
const createdDocuments = snapshot1.docs.map(snapshot => snapshot.ref);
2099+
2100+
// Delete 50 of the 100 documents. Do this in a transaction, rather than
2101+
// deleteDoc(), to avoid affecting the local cache.
2102+
const deletedDocumentIds = new Set<string>();
2103+
await runTransaction(db, async txn => {
2104+
for (let i = 0; i < createdDocuments.length; i += 2) {
2105+
const documentToDelete = createdDocuments[i];
2106+
txn.delete(documentToDelete);
2107+
deletedDocumentIds.add(documentToDelete.id);
2108+
}
2109+
});
21412110

2142-
// Skip the verification of the existence filter mismatch when testing
2143-
// against the Firestore emulator because the Firestore emulator fails to
2144-
// to send an existence filter at all.
2145-
// TODO(b/270731363): Enable the verification of the existence filter
2146-
// mismatch once the Firestore emulator is fixed to send an existence
2147-
// filter.
2148-
if (USE_EMULATOR) {
2149-
return 'passed';
2150-
}
2111+
// Wait for 10 seconds, during which Watch will stop tracking the query
2112+
// and will send an existence filter rather than "delete" events when
2113+
// the query is resumed.
2114+
await new Promise(resolve => setTimeout(resolve, 10000));
2115+
2116+
// Resume the query and save the resulting snapshot for verification.
2117+
// Use some internal testing hooks to "capture" the existence filter
2118+
// mismatches to verify that Watch sent a bloom filter, and it was used
2119+
// to avert a full requery.
2120+
const [existenceFilterMismatches, snapshot2] =
2121+
await captureExistenceFilterMismatches(() => getDocs(coll));
2122+
2123+
// Verify that the snapshot from the resumed query contains the expected
2124+
// documents; that is, that it contains the 50 documents that were _not_
2125+
// deleted.
2126+
// TODO(b/270731363): Remove the "if" condition below once the
2127+
// Firestore Emulator is fixed to send an existence filter. At the time
2128+
// of writing, the Firestore emulator fails to send an existence filter,
2129+
// resulting in the client including the deleted documents in the
2130+
// snapshot of the resumed query.
2131+
if (!(USE_EMULATOR && snapshot2.size === 100)) {
2132+
const actualDocumentIds = snapshot2.docs
2133+
.map(documentSnapshot => documentSnapshot.ref.id)
2134+
.sort();
2135+
const expectedDocumentIds = createdDocuments
2136+
.filter(documentRef => !deletedDocumentIds.has(documentRef.id))
2137+
.map(documentRef => documentRef.id)
2138+
.sort();
2139+
expect(actualDocumentIds, 'snapshot2.docs').to.deep.equal(
2140+
expectedDocumentIds
2141+
);
2142+
}
21512143

2152-
// Verify that Watch sent an existence filter with the correct counts when
2153-
// the query was resumed.
2154-
expect(
2155-
existenceFilterMismatches,
2156-
'existenceFilterMismatches'
2157-
).to.have.length(1);
2158-
const { localCacheCount, existenceFilterCount, bloomFilter } =
2159-
existenceFilterMismatches[0];
2160-
expect(localCacheCount, 'localCacheCount').to.equal(100);
2161-
expect(existenceFilterCount, 'existenceFilterCount').to.equal(50);
2162-
2163-
// Verify that Watch sent a valid bloom filter.
2164-
if (!bloomFilter) {
2165-
expect.fail(
2166-
'The existence filter should have specified a bloom filter in its ' +
2167-
'`unchanged_names` field.'
2168-
);
2169-
throw new Error('should never get here');
2170-
}
2144+
// Skip the verification of the existence filter mismatch when testing
2145+
// against the Firestore emulator because the Firestore emulator fails
2146+
// to to send an existence filter at all.
2147+
// TODO(b/270731363): Enable the verification of the existence filter
2148+
// mismatch once the Firestore emulator is fixed to send an existence
2149+
// filter.
2150+
if (USE_EMULATOR) {
2151+
return;
2152+
}
21712153

2172-
expect(bloomFilter.hashCount, 'bloomFilter.hashCount').to.be.above(0);
2173-
expect(bloomFilter.bitmapLength, 'bloomFilter.bitmapLength').to.be.above(
2174-
0
2175-
);
2176-
expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0);
2177-
expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8);
2178-
2179-
// Verify that the bloom filter was successfully used to avert a full
2180-
// requery. If a false positive occurred then retry the entire test.
2181-
// Although statistically rare, false positives are expected to happen
2182-
// occasionally. When a false positive _does_ happen, just retry the test
2183-
// with a different set of documents. If that retry _also_ experiences a
2184-
// false positive, then fail the test because that is so improbable that
2185-
// something must have gone wrong.
2186-
if (attemptNumber === 1 && !bloomFilter.applied) {
2187-
return 'retry';
2188-
}
2189-
expect(
2190-
bloomFilter.applied,
2191-
`bloomFilter.applied with attemptNumber=${attemptNumber}`
2192-
).to.be.true;
2154+
// Verify that Watch sent an existence filter with the correct counts
2155+
// when the query was resumed.
2156+
expect(
2157+
existenceFilterMismatches,
2158+
'existenceFilterMismatches'
2159+
).to.have.length(1);
2160+
const { localCacheCount, existenceFilterCount, bloomFilter } =
2161+
existenceFilterMismatches[0];
2162+
expect(localCacheCount, 'localCacheCount').to.equal(100);
2163+
expect(existenceFilterCount, 'existenceFilterCount').to.equal(50);
2164+
2165+
// Verify that Watch sent a valid bloom filter.
2166+
if (!bloomFilter) {
2167+
expect.fail(
2168+
'The existence filter should have specified a bloom filter in its ' +
2169+
'`unchanged_names` field.'
2170+
);
2171+
throw new Error('should never get here');
2172+
}
21932173

2194-
return 'passed';
2195-
};
2174+
expect(bloomFilter.hashCount, 'bloomFilter.hashCount').to.be.above(0);
2175+
expect(
2176+
bloomFilter.bitmapLength,
2177+
'bloomFilter.bitmapLength'
2178+
).to.be.above(0);
2179+
expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0);
2180+
expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8);
2181+
2182+
// Verify that the bloom filter was successfully used to avert a full
2183+
// requery. If a false positive occurred then retry the entire test.
2184+
// Although statistically rare, false positives are expected to happen
2185+
// occasionally. When a false positive _does_ happen, just retry the
2186+
// test with a different set of documents. If that retry _also_
2187+
// experiences a false positive, then fail the test because that is so
2188+
// improbable that something must have gone wrong.
2189+
if (attemptNumber === 1 && !bloomFilter.applied) {
2190+
throw new RetryError();
2191+
}
21962192

2197-
// Run the test
2198-
let attemptNumber = 0;
2199-
while (true) {
2200-
attemptNumber++;
2201-
const iterationResult = await withTestCollection(
2202-
// Ensure that the local cache is configured to use LRU garbage
2203-
// collection (rather than eager garbage collection) so that the resume
2204-
// token and document data does not get prematurely evicted.
2205-
persistence.toLruGc(),
2206-
testDocs,
2207-
runTestIteration
2208-
);
2209-
if (iterationResult === 'passed') {
2210-
break;
2211-
}
2212-
}
2193+
expect(
2194+
bloomFilter.applied,
2195+
`bloomFilter.applied with attemptNumber=${attemptNumber}`
2196+
).to.be.true;
2197+
});
2198+
});
22132199
}).timeout('90s');
22142200
});
22152201

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,26 @@ export function withTestDocAndInitialData(
384384
});
385385
}
386386

387+
export class RetryError extends Error {
388+
readonly name = 'FirestoreIntegrationTestRetryError';
389+
}
390+
391+
export async function withRetry<T>(
392+
fn: (attemptNumber: number) => Promise<T>
393+
): Promise<T> {
394+
let attemptNumber = 0;
395+
while (true) {
396+
attemptNumber++;
397+
try {
398+
return await fn(attemptNumber);
399+
} catch (error) {
400+
if (!(error instanceof RetryError)) {
401+
throw error;
402+
}
403+
}
404+
}
405+
}
406+
387407
export function withTestCollection<T>(
388408
persistence: PersistenceMode,
389409
docs: { [key: string]: DocumentData },

0 commit comments

Comments
 (0)