From 6a15468f8278b9c9297c265ad864c02a46c61fb5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Feb 2023 00:09:59 -0500 Subject: [PATCH 1/4] Bundle the readTime and resumeToken into a data structure rather than passing them around individually --- .../unit/specs/existence_filter_spec.test.ts | 16 ++--- .../test/unit/specs/limbo_spec.test.ts | 2 +- .../test/unit/specs/limit_spec.test.ts | 2 +- .../firestore/test/unit/specs/spec_builder.ts | 69 ++++++++----------- .../test/unit/specs/spec_test_runner.ts | 12 ++-- .../firestore/test/util/spec_test_helpers.ts | 2 +- 6 files changed, 44 insertions(+), 59 deletions(-) diff --git a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts index 4943d986bd2..5e15dcff93b 100644 --- a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts +++ b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts @@ -31,7 +31,7 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1) .expectEvents(query1, { added: [doc1] }) - .watchFilters([query1], doc1.key) + .watchFilters([query1], [doc1.key]) .watchSnapshots(2000); }); @@ -45,7 +45,7 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) .expectEvents(query1, {}) .watchSends({ affects: [query1] }, doc1) - .watchFilters([query1], doc1.key) + .watchFilters([query1], [doc1.key]) .watchSnapshots(2000) .expectEvents(query1, { added: [doc1] }); }); @@ -59,7 +59,7 @@ describeSpec('Existence Filters:', [], () => { .watchCurrents(query1, 'resume-token-1000') .watchSnapshots(2000) .expectEvents(query1, {}) - .watchFilters([query1], doc1.key) + .watchFilters([query1], [doc1.key]) .watchSnapshots(2000) .expectEvents(query1, { fromCache: true }); }); @@ -96,7 +96,7 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], doc1.key) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key]) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) @@ -130,7 +130,7 @@ describeSpec('Existence Filters:', [], () => { resumeToken: 'existence-filter-resume-token' }) .watchAcks(query1) - .watchFilters([query1], doc1.key) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key]) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) @@ -159,7 +159,7 @@ describeSpec('Existence Filters:', [], () => { // Send a mismatching existence filter with two documents, but don't // send a new global snapshot. We should not see an event until we // receive the snapshot. - .watchFilters([query1], doc1.key, doc2.key) + .watchFilters([query1], [doc1.key, doc2.key]) .watchSends({ affects: [query1] }, doc3) .watchSnapshots(2000) // The query result includes doc3, but is marked as "inconsistent" @@ -193,7 +193,7 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], doc1.key) // in the next sync doc2 was deleted + .watchFilters([query1], [doc1.key]) // in the next sync doc2 was deleted .watchSnapshots(2000) // query is now marked as "inconsistent" because of filter mismatch .expectEvents(query1, { fromCache: true }) @@ -229,7 +229,7 @@ describeSpec('Existence Filters:', [], () => { .userListens(query1) .watchAcksFull(query1, 1000, doc1, doc2) .expectEvents(query1, { added: [doc1, doc2] }) - .watchFilters([query1], doc1.key) // doc2 was deleted + .watchFilters([query1], [doc1.key]) // doc2 was deleted .watchSnapshots(2000) .expectEvents(query1, { fromCache: true }) // The SDK is unable to re-run the query, and does not remove doc2 diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index b7db2049ec9..4451fc5a80a 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -880,7 +880,7 @@ describeSpec('Limbo Documents:', [], () => { // documents that changed since the resume token. This will cause it // to just send the docBs with an existence filter with a count of 3. .watchSends({ affects: [query1] }, docB1, docB2, docB3) - .watchFilters([query1], docB1.key, docB2.key, docB3.key) + .watchFilters([query1], [docB1.key, docB2.key, docB3.key]) .watchSnapshots(1001) .expectEvents(query1, { added: [docB1, docB2, docB3], diff --git a/packages/firestore/test/unit/specs/limit_spec.test.ts b/packages/firestore/test/unit/specs/limit_spec.test.ts index 678f929743d..133fd1b1b22 100644 --- a/packages/firestore/test/unit/specs/limit_spec.test.ts +++ b/packages/firestore/test/unit/specs/limit_spec.test.ts @@ -341,7 +341,7 @@ describeSpec('Limits:', [], () => { // we receive an existence filter, which indicates that our view is // out of sync. .watchSends({ affects: [limitQuery] }, secondDocument) - .watchFilters([limitQuery], secondDocument.key) + .watchFilters([limitQuery], [secondDocument.key]) .watchSnapshots(1004) .expectActiveTargets({ query: limitQuery, resumeToken: '' }) .watchRemoves(limitQuery) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 39e397c0fbf..bf394a07c60 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -81,6 +81,11 @@ export interface ActiveTargetMap { [targetId: string]: ActiveTargetSpec; } +export interface ResumeSpec { + resumeToken?: string; + readTime?: TestSnapshotVersion; +} + /** * Tracks the expected memory state of a client (e.g. the expected active watch * targets based on userListens(), userUnlistens(), and watchRemoves() @@ -255,10 +260,7 @@ export class SpecBuilder { return this; } - userListens( - query: Query, - resume?: { resumeToken?: string; readTime?: TestSnapshotVersion } - ): this { + userListens(query: Query, resume?: ResumeSpec): this { this.nextStep(); const target = queryToTarget(query); @@ -277,12 +279,7 @@ export class SpecBuilder { } this.queryMapping.set(target, targetId); - this.addQueryToActiveTargets( - targetId, - query, - resume?.resumeToken, - resume?.readTime - ); + this.addQueryToActiveTargets(targetId, query, resume); this.currentStep = { userListen: { targetId, query: SpecBuilder.queryToSpec(query) }, expectedState: { activeTargets: { ...this.activeTargets } } @@ -295,14 +292,17 @@ export class SpecBuilder { * Registers a previously active target with the test expectations after a * stream disconnect. */ - restoreListen(query: Query, resumeToken: string): this { + restoreListen( + query: Query, + resumeToken: string + ): this { const targetId = this.queryMapping.get(queryToTarget(query)); if (isNullOrUndefined(targetId)) { throw new Error("Can't restore an unknown query: " + query); } - this.addQueryToActiveTargets(targetId!, query, resumeToken); + this.addQueryToActiveTargets(targetId!, query, { resumeToken }); const currentStep = this.currentStep!; currentStep.expectedState = currentStep.expectedState || {}; @@ -536,12 +536,10 @@ export class SpecBuilder { const currentStep = this.currentStep!; this.clientState.activeTargets = {}; targets.forEach(({ query, resumeToken, readTime }) => { - this.addQueryToActiveTargets( - this.getTargetId(query), - query, + this.addQueryToActiveTargets(this.getTargetId(query), query, { resumeToken, readTime - ); + }); }); currentStep.expectedState = currentStep.expectedState || {}; currentStep.expectedState.activeTargets = { ...this.activeTargets }; @@ -572,7 +570,7 @@ export class SpecBuilder { this.addQueryToActiveTargets( this.limboMapping[path], newQueryForPath(key.path), - '' + { resumeToken: '' } ); }); @@ -769,7 +767,10 @@ export class SpecBuilder { return this; } - watchFilters(queries: Query[], ...docs: DocumentKey[]): this { + watchFilters( + queries: Query[], + docs: DocumentKey[] = [] + ): this { this.nextStep(); const targetIds = queries.map(query => { return this.getTargetId(query); @@ -777,10 +778,7 @@ export class SpecBuilder { const keys = docs.map(key => { return key.path.canonicalString(); }); - const filter: SpecWatchFilter = [targetIds] as SpecWatchFilter; - for (const key of keys) { - filter.push(key); - } + const filter: SpecWatchFilter = { targetIds, keys } as SpecWatchFilter; this.currentStep = { watchFilter: filter }; @@ -910,22 +908,14 @@ export class SpecBuilder { } /** Registers a query that is active in another tab. */ - expectListen( - query: Query, - resume?: { resumeToken?: string; readTime?: TestSnapshotVersion } - ): this { + expectListen(query: Query, resume?: ResumeSpec): this { this.assertStep('Expectations require previous step'); const target = queryToTarget(query); const targetId = this.queryIdGenerator.cachedId(target); this.queryMapping.set(target, targetId); - this.addQueryToActiveTargets( - targetId, - query, - resume?.resumeToken, - resume?.readTime - ); + this.addQueryToActiveTargets(targetId, query, resume); const currentStep = this.currentStep!; currentStep.expectedState = currentStep.expectedState || {}; @@ -1093,8 +1083,7 @@ export class SpecBuilder { private addQueryToActiveTargets( targetId: number, query: Query, - resumeToken?: string, - readTime?: TestSnapshotVersion + resume?: ResumeSpec ): void { if (this.activeTargets[targetId]) { const activeQueries = this.activeTargets[targetId].queries; @@ -1106,21 +1095,21 @@ export class SpecBuilder { // `query` is not added yet. this.activeTargets[targetId] = { queries: [SpecBuilder.queryToSpec(query), ...activeQueries], - resumeToken: resumeToken || '', - readTime + resumeToken: resume?.resumeToken || '', + readTime: resume?.readTime }; } else { this.activeTargets[targetId] = { queries: activeQueries, - resumeToken: resumeToken || '', - readTime + resumeToken: resume?.resumeToken || '', + readTime: resume?.readTime }; } } else { this.activeTargets[targetId] = { queries: [SpecBuilder.queryToSpec(query)], - resumeToken: resumeToken || '', - readTime + resumeToken: resume?.resumeToken || '', + readTime: resume?.readTime }; } } diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index e64dcbfd214..335d17b16c9 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -694,12 +694,11 @@ abstract class TestRunner { } private doWatchFilter(watchFilter: SpecWatchFilter): Promise { - const targetIds: TargetId[] = watchFilter[0]; + const { targetIds, keys } = watchFilter; debugAssert( targetIds.length === 1, 'ExistenceFilters currently support exactly one target only.' ); - const keys = watchFilter.slice(1); const filter = new ExistenceFilter(keys.length); const change = new ExistenceFilterChange(targetIds[0], filter); return this.doWatchEvent(change); @@ -1583,14 +1582,11 @@ export interface SpecClientState { } /** - * [[, ...], , ...] - * Note that the last parameter is really of type ...string (spread operator) * The filter is based of a list of keys to match in the existence filter */ -export interface SpecWatchFilter - extends Array { - '0': TargetId[]; - '1': string | undefined; +export interface SpecWatchFilter { + targetIds: TargetId[]; + keys: string[]; } export type SpecLimitType = 'LimitToFirst' | 'LimitToLast'; diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index f8b8715d4c7..9aeb92c431b 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -44,8 +44,8 @@ export function encodeWatchChange( if (watchChange instanceof ExistenceFilterChange) { return { filter: { + targetId: watchChange.targetId, count: watchChange.existenceFilter.count, - targetId: watchChange.targetId } }; } From 2d1c722217a8012de1c05648d36a4879d5b4fcc5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Feb 2023 00:43:41 -0500 Subject: [PATCH 2/4] yarn prettier --- packages/firestore/test/unit/specs/spec_builder.ts | 10 ++-------- packages/firestore/test/util/spec_test_helpers.ts | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index bf394a07c60..f9cf8671dca 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -292,10 +292,7 @@ export class SpecBuilder { * Registers a previously active target with the test expectations after a * stream disconnect. */ - restoreListen( - query: Query, - resumeToken: string - ): this { + restoreListen(query: Query, resumeToken: string): this { const targetId = this.queryMapping.get(queryToTarget(query)); if (isNullOrUndefined(targetId)) { @@ -767,10 +764,7 @@ export class SpecBuilder { return this; } - watchFilters( - queries: Query[], - docs: DocumentKey[] = [] - ): this { + watchFilters(queries: Query[], docs: DocumentKey[] = []): this { this.nextStep(); const targetIds = queries.map(query => { return this.getTargetId(query); diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index 9aeb92c431b..d0c30789a4c 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -45,7 +45,7 @@ export function encodeWatchChange( return { filter: { targetId: watchChange.targetId, - count: watchChange.existenceFilter.count, + count: watchChange.existenceFilter.count } }; } From 28b5d6003151598a74c4419aa93814fefdf6d109 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Feb 2023 00:51:54 -0500 Subject: [PATCH 3/4] spec_test_helpers.ts: revert the meaningless change --- packages/firestore/test/util/spec_test_helpers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index d0c30789a4c..f8b8715d4c7 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -44,8 +44,8 @@ export function encodeWatchChange( if (watchChange instanceof ExistenceFilterChange) { return { filter: { - targetId: watchChange.targetId, - count: watchChange.existenceFilter.count + count: watchChange.existenceFilter.count, + targetId: watchChange.targetId } }; } From 46ad10602e9262ef0b6544e6ab4380f448a8b53a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Feb 2023 16:06:31 -0500 Subject: [PATCH 4/4] spec_builder.ts: remove unnecessary "as" cast --- packages/firestore/test/unit/specs/spec_builder.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index f9cf8671dca..3dd2816d374 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -772,7 +772,7 @@ export class SpecBuilder { const keys = docs.map(key => { return key.path.canonicalString(); }); - const filter: SpecWatchFilter = { targetIds, keys } as SpecWatchFilter; + const filter: SpecWatchFilter = { targetIds, keys }; this.currentStep = { watchFilter: filter };