Skip to content

Commit 711bc33

Browse files
milaGGLchuanr
milaGGL
authored and
chuanr
committed
Validate target purpose in spec tests (#7113)
1 parent 19259b8 commit 711bc33

File tree

7 files changed

+110
-34
lines changed

7 files changed

+110
-34
lines changed

‎packages/firestore/test/unit/specs/existence_filter_spec.test.ts

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
*/
1717

1818
import { newQueryForPath } from '../../../src/core/query';
19+
import { TargetPurpose } from '../../../src/local/target_data';
1920
import { Code } from '../../../src/util/error';
2021
import { deletedDoc, doc, query } from '../../util/helpers';
2122

@@ -61,7 +62,11 @@ describeSpec('Existence Filters:', [], () => {
6162
.expectEvents(query1, {})
6263
.watchFilters([query1], [doc1.key])
6364
.watchSnapshots(2000)
64-
.expectEvents(query1, { fromCache: true });
65+
.expectEvents(query1, { fromCache: true })
66+
.expectActiveTargets({
67+
query: query1,
68+
targetPurpose: TargetPurpose.ExistenceFilterMismatch
69+
});
6570
});
6671

6772
specTest('Existence filter ignored with pending target', [], () => {
@@ -100,7 +105,11 @@ describeSpec('Existence Filters:', [], () => {
100105
.watchSnapshots(2000)
101106
// query is now marked as "inconsistent" because of filter mismatch
102107
.expectEvents(query1, { fromCache: true })
103-
.expectActiveTargets({ query: query1, resumeToken: '' })
108+
.expectActiveTargets({
109+
query: query1,
110+
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
111+
resumeToken: ''
112+
})
104113
.watchRemoves(query1) // Acks removal of query
105114
.watchAcksFull(query1, 2000, doc1)
106115
.expectLimboDocs(doc2.key) // doc2 is now in limbo
@@ -134,7 +143,11 @@ describeSpec('Existence Filters:', [], () => {
134143
.watchSnapshots(2000)
135144
// query is now marked as "inconsistent" because of filter mismatch
136145
.expectEvents(query1, { fromCache: true })
137-
.expectActiveTargets({ query: query1, resumeToken: '' })
146+
.expectActiveTargets({
147+
query: query1,
148+
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
149+
resumeToken: ''
150+
})
138151
.watchRemoves(query1) // Acks removal of query
139152
.watchAcksFull(query1, 2000, doc1)
140153
.expectLimboDocs(doc2.key) // doc2 is now in limbo
@@ -165,7 +178,11 @@ describeSpec('Existence Filters:', [], () => {
165178
// The query result includes doc3, but is marked as "inconsistent"
166179
// due to the filter mismatch
167180
.expectEvents(query1, { added: [doc3], fromCache: true })
168-
.expectActiveTargets({ query: query1, resumeToken: '' })
181+
.expectActiveTargets({
182+
query: query1,
183+
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
184+
resumeToken: ''
185+
})
169186
.watchRemoves(query1) // Acks removal of query
170187
.watchAcksFull(query1, 3000, doc1, doc2, doc3)
171188
.expectEvents(query1, { added: [doc2] })
@@ -197,7 +214,11 @@ describeSpec('Existence Filters:', [], () => {
197214
.watchSnapshots(2000)
198215
// query is now marked as "inconsistent" because of filter mismatch
199216
.expectEvents(query1, { fromCache: true })
200-
.expectActiveTargets({ query: query1, resumeToken: '' })
217+
.expectActiveTargets({
218+
query: query1,
219+
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
220+
resumeToken: ''
221+
})
201222
.watchRemoves(query1) // Acks removal of query
202223
.watchAcksFull(query1, 2000, doc1)
203224
.expectLimboDocs(doc2.key) // doc2 is now in limbo
@@ -232,6 +253,10 @@ describeSpec('Existence Filters:', [], () => {
232253
.watchFilters([query1], [doc1.key]) // doc2 was deleted
233254
.watchSnapshots(2000)
234255
.expectEvents(query1, { fromCache: true })
256+
.expectActiveTargets({
257+
query: query1,
258+
targetPurpose: TargetPurpose.ExistenceFilterMismatch
259+
})
235260
// The SDK is unable to re-run the query, and does not remove doc2
236261
.restart()
237262
.userListens(query1)

‎packages/firestore/test/unit/specs/limbo_spec.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
newQueryForPath,
2121
queryWithLimit
2222
} from '../../../src/core/query';
23+
import { TargetPurpose } from '../../../src/local/target_data';
2324
import { TimerId } from '../../../src/util/async_queue';
2425
import { Code } from '../../../src/util/error';
2526
import { deletedDoc, doc, filter, orderBy, query } from '../../util/helpers';
@@ -889,7 +890,11 @@ describeSpec('Limbo Documents:', [], () => {
889890
// The view now contains the docAs and the docBs (6 documents), but
890891
// the existence filter indicated only 3 should match. This causes
891892
// the client to re-listen without a resume token.
892-
.expectActiveTargets({ query: query1, resumeToken: '' })
893+
.expectActiveTargets({
894+
query: query1,
895+
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
896+
resumeToken: ''
897+
})
893898
// When the existence filter mismatch was detected, the client removed
894899
// then re-added the target. Watch needs to acknowledge the removal.
895900
.watchRemoves(query1)

‎packages/firestore/test/unit/specs/limit_spec.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
*/
1717

1818
import { LimitType, queryWithLimit } from '../../../src/core/query';
19+
import { TargetPurpose } from '../../../src/local/target_data';
1920
import { deletedDoc, doc, filter, orderBy, query } from '../../util/helpers';
2021

2122
import { describeSpec, specTest } from './describe_spec';
@@ -343,7 +344,11 @@ describeSpec('Limits:', [], () => {
343344
.watchSends({ affects: [limitQuery] }, secondDocument)
344345
.watchFilters([limitQuery], [secondDocument.key])
345346
.watchSnapshots(1004)
346-
.expectActiveTargets({ query: limitQuery, resumeToken: '' })
347+
.expectActiveTargets({
348+
query: limitQuery,
349+
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
350+
resumeToken: ''
351+
})
347352
.watchRemoves(limitQuery)
348353
.watchAcksFull(limitQuery, 1005, secondDocument)
349354
// The snapshot after the existence filter mismatch triggers limbo

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
*/
1717

1818
import { newQueryForPath } from '../../../src/core/query';
19+
import { TargetPurpose } from '../../../src/local/target_data';
1920
import { TimerId } from '../../../src/util/async_queue';
2021
import { Code } from '../../../src/util/error';
2122
import { deletedDoc, doc, filter, query } from '../../util/helpers';
@@ -137,7 +138,9 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
137138
// The primary client 0 receives a notification that the query can
138139
// be released, but it can only process the change after we recover
139140
// the database.
140-
.expectActiveTargets({ query: query1 })
141+
.expectActiveTargets({
142+
query: query1
143+
})
141144
.recoverDatabase()
142145
.runTimer(TimerId.AsyncQueueRetry)
143146
.expectActiveTargets()
@@ -641,7 +644,10 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
641644
query: filteredQuery,
642645
resumeToken: 'resume-token-2000'
643646
},
644-
{ query: limboQuery }
647+
{
648+
query: limboQuery,
649+
targetPurpose: TargetPurpose.LimboResolution
650+
}
645651
)
646652
.watchAcksFull(filteredQuery, 4000)
647653
.watchAcksFull(limboQuery, 4000, doc1b)
@@ -682,7 +688,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
682688
.runTimer(TimerId.AsyncQueueRetry)
683689
.expectActiveTargets(
684690
{ query: filteredQuery, resumeToken: 'resume-token-2000' },
685-
{ query: limboQuery }
691+
{ query: limboQuery, targetPurpose: TargetPurpose.LimboResolution }
686692
)
687693
.watchAcksFull(filteredQuery, 3000)
688694
.watchRemoves(
@@ -719,15 +725,19 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
719725
.expectActiveTargets()
720726
.recoverDatabase()
721727
.runTimer(TimerId.AsyncQueueRetry)
722-
.expectActiveTargets({ query: query1 })
728+
.expectActiveTargets({
729+
query: query1
730+
})
723731
.expectEvents(query1, { removed: [doc1], fromCache: true })
724732
.failDatabaseTransactions('Handle user change')
725733
.changeUser('user1')
726734
// The network is offline due to the failed user change
727735
.expectActiveTargets()
728736
.recoverDatabase()
729737
.runTimer(TimerId.AsyncQueueRetry)
730-
.expectActiveTargets({ query: query1 })
738+
.expectActiveTargets({
739+
query: query1
740+
})
731741
.expectEvents(query1, {
732742
added: [doc1],
733743
fromCache: true,
@@ -763,7 +773,9 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
763773
.changeUser('user1')
764774
.recoverDatabase()
765775
.runTimer(TimerId.AsyncQueueRetry)
766-
.expectActiveTargets({ query: query1 })
776+
.expectActiveTargets({
777+
query: query1
778+
})
767779
// We are now user 2
768780
.expectEvents(query1, { removed: [doc1], fromCache: true })
769781
.runTimer(TimerId.AsyncQueueRetry)

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

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
import { canonifyTarget, Target, targetEquals } from '../../../src/core/target';
2929
import { TargetIdGenerator } from '../../../src/core/target_id_generator';
3030
import { TargetId } from '../../../src/core/types';
31+
import { TargetPurpose } from '../../../src/local/target_data';
3132
import { Document } from '../../../src/model/document';
3233
import { DocumentKey } from '../../../src/model/document_key';
3334
import { FieldIndex } from '../../../src/model/field_index';
@@ -73,6 +74,7 @@ export interface LimboMap {
7374

7475
export interface ActiveTargetSpec {
7576
queries: SpecQuery[];
77+
targetPurpose?: TargetPurpose;
7678
resumeToken?: string;
7779
readTime?: TestSnapshotVersion;
7880
}
@@ -299,7 +301,9 @@ export class SpecBuilder {
299301
throw new Error("Can't restore an unknown query: " + query);
300302
}
301303

302-
this.addQueryToActiveTargets(targetId!, query, { resumeToken });
304+
this.addQueryToActiveTargets(targetId!, query, {
305+
resumeToken
306+
});
303307

304308
const currentStep = this.currentStep!;
305309
currentStep.expectedState = currentStep.expectedState || {};
@@ -525,18 +529,24 @@ export class SpecBuilder {
525529
expectActiveTargets(
526530
...targets: Array<{
527531
query: Query;
532+
targetPurpose?: TargetPurpose;
528533
resumeToken?: string;
529534
readTime?: TestSnapshotVersion;
530535
}>
531536
): this {
532537
this.assertStep('Active target expectation requires previous step');
533538
const currentStep = this.currentStep!;
534539
this.clientState.activeTargets = {};
535-
targets.forEach(({ query, resumeToken, readTime }) => {
536-
this.addQueryToActiveTargets(this.getTargetId(query), query, {
537-
resumeToken,
538-
readTime
539-
});
540+
targets.forEach(({ query, targetPurpose, resumeToken, readTime }) => {
541+
this.addQueryToActiveTargets(
542+
this.getTargetId(query),
543+
query,
544+
{
545+
resumeToken,
546+
readTime
547+
},
548+
targetPurpose
549+
);
540550
});
541551
currentStep.expectedState = currentStep.expectedState || {};
542552
currentStep.expectedState.activeTargets = { ...this.activeTargets };
@@ -567,7 +577,8 @@ export class SpecBuilder {
567577
this.addQueryToActiveTargets(
568578
this.limboMapping[path],
569579
newQueryForPath(key.path),
570-
{ resumeToken: '' }
580+
{ resumeToken: '' },
581+
TargetPurpose.LimboResolution
571582
);
572583
});
573584

@@ -1077,7 +1088,8 @@ export class SpecBuilder {
10771088
private addQueryToActiveTargets(
10781089
targetId: number,
10791090
query: Query,
1080-
resume?: ResumeSpec
1091+
resume: ResumeSpec = {},
1092+
targetPurpose?: TargetPurpose
10811093
): void {
10821094
if (this.activeTargets[targetId]) {
10831095
const activeQueries = this.activeTargets[targetId].queries;
@@ -1089,21 +1101,24 @@ export class SpecBuilder {
10891101
// `query` is not added yet.
10901102
this.activeTargets[targetId] = {
10911103
queries: [SpecBuilder.queryToSpec(query), ...activeQueries],
1092-
resumeToken: resume?.resumeToken || '',
1093-
readTime: resume?.readTime
1104+
targetPurpose,
1105+
resumeToken: resume.resumeToken || '',
1106+
readTime: resume.readTime
10941107
};
10951108
} else {
10961109
this.activeTargets[targetId] = {
10971110
queries: activeQueries,
1098-
resumeToken: resume?.resumeToken || '',
1099-
readTime: resume?.readTime
1111+
targetPurpose,
1112+
resumeToken: resume.resumeToken || '',
1113+
readTime: resume.readTime
11001114
};
11011115
}
11021116
} else {
11031117
this.activeTargets[targetId] = {
11041118
queries: [SpecBuilder.queryToSpec(query)],
1105-
resumeToken: resume?.resumeToken || '',
1106-
readTime: resume?.readTime
1119+
targetPurpose,
1120+
resumeToken: resume.resumeToken || '',
1121+
readTime: resume.readTime
11071122
};
11081123
}
11091124
}
@@ -1115,6 +1130,7 @@ export class SpecBuilder {
11151130
if (queriesAfterRemoval.length > 0) {
11161131
this.activeTargets[targetId] = {
11171132
queries: queriesAfterRemoval,
1133+
targetPurpose: this.activeTargets[targetId].targetPurpose,
11181134
resumeToken: this.activeTargets[targetId].resumeToken
11191135
};
11201136
} else {

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ import { Mutation } from '../../../src/model/mutation';
5353
import { encodeBase64 } from '../../../src/platform/base64';
5454
import { newSerializer } from '../../../src/platform/serializer';
5555
import * as api from '../../../src/protos/firestore_proto_api';
56+
import { ApiClientObjectMap } from '../../../src/protos/firestore_proto_api';
5657
import { Connection, Stream } from '../../../src/remote/connection';
5758
import { Datastore, newDatastore } from '../../../src/remote/datastore';
5859
import { WriteRequest } from '../../../src/remote/persistent_stream';
@@ -259,7 +260,12 @@ export class MockConnection implements Connection {
259260
* Tracks the currently active watch targets as detected by the mock watch
260261
* stream, as a mapping from target ID to query Target.
261262
*/
262-
activeTargets: { [targetId: number]: api.Target } = {};
263+
activeTargets: {
264+
[targetId: number]: {
265+
target: api.Target;
266+
labels?: ApiClientObjectMap<string>;
267+
};
268+
} = {};
263269

264270
/** A Deferred that is resolved once watch opens. */
265271
watchOpen = new Deferred<void>();
@@ -398,7 +404,10 @@ export class MockConnection implements Connection {
398404
++this.watchStreamRequestCount;
399405
if (request.addTarget) {
400406
const targetId = request.addTarget.targetId!;
401-
this.activeTargets[targetId] = request.addTarget;
407+
this.activeTargets[targetId] = {
408+
target: request.addTarget,
409+
labels: request.labels
410+
};
402411
} else if (request.removeTarget) {
403412
delete this.activeTargets[request.removeTarget];
404413
} else {

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ import {
105105
import { mapCodeFromRpcCode } from '../../../src/remote/rpc_error';
106106
import {
107107
JsonProtoSerializer,
108+
toListenRequestLabels,
108109
toMutation,
109110
toTarget,
110111
toVersion
@@ -1095,15 +1096,13 @@ abstract class TestRunner {
10951096
undefined,
10961097
'Expected active target not found: ' + JSON.stringify(expected)
10971098
);
1098-
const actualTarget = actualTargets[targetId];
1099+
const { target: actualTarget, labels: actualLabels } =
1100+
actualTargets[targetId];
10991101

1100-
// TODO(mcg): populate the purpose of the target once it's possible to
1101-
// encode that in the spec tests. For now, hard-code that it's a listen
1102-
// despite the fact that it's not always the right value.
11031102
let targetData = new TargetData(
11041103
queryToTarget(parseQuery(expected.queries[0])),
11051104
targetId,
1106-
TargetPurpose.Listen,
1105+
expected.targetPurpose ?? TargetPurpose.Listen,
11071106
ARBITRARY_SEQUENCE_NUMBER
11081107
);
11091108
if (expected.resumeToken && expected.resumeToken !== '') {
@@ -1117,6 +1116,11 @@ abstract class TestRunner {
11171116
version(expected.readTime!)
11181117
);
11191118
}
1119+
1120+
const expectedLabels =
1121+
toListenRequestLabels(this.serializer, targetData) ?? undefined;
1122+
expect(actualLabels).to.deep.equal(expectedLabels);
1123+
11201124
const expectedTarget = toTarget(this.serializer, targetData);
11211125
expect(actualTarget.query).to.deep.equal(expectedTarget.query);
11221126
expect(actualTarget.targetId).to.equal(expectedTarget.targetId);

0 commit comments

Comments
 (0)