Skip to content

Validate target purpose in spec tests #7113

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 5 commits into from
Mar 11, 2023
Merged

Validate target purpose in spec tests #7113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e4d6b6
Validate target purpose in spec tests
milaGGL Mar 10, 2023
78e126c
move the TargetPurpose default value to the actual validation step
milaGGL Mar 10, 2023
eb67903
give resume parameter default value
milaGGL Mar 10, 2023
2fe63c2
handle specs with persistence
milaGGL Mar 10, 2023
e995f3a
resolve comments
milaGGL Mar 10, 2023
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
31 changes: 26 additions & 5 deletions packages/firestore/test/unit/specs/existence_filter_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { newQueryForPath } from '../../../src/core/query';
import { TargetPurpose } from '../../../src/local/target_data';
import { Code } from '../../../src/util/error';
import { deletedDoc, doc, query } from '../../util/helpers';

Expand Down Expand Up @@ -61,7 +62,11 @@ describeSpec('Existence Filters:', [], () => {
.expectEvents(query1, {})
.watchFilters([query1], [doc1.key])
.watchSnapshots(2000)
.expectEvents(query1, { fromCache: true });
.expectEvents(query1, { fromCache: true })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch
});
});

specTest('Existence filter ignored with pending target', [], () => {
Expand Down Expand Up @@ -100,7 +105,11 @@ describeSpec('Existence Filters:', [], () => {
.watchSnapshots(2000)
// query is now marked as "inconsistent" because of filter mismatch
.expectEvents(query1, { fromCache: true })
.expectActiveTargets({ query: query1, resumeToken: '' })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
resumeToken: ''
})
.watchRemoves(query1) // Acks removal of query
.watchAcksFull(query1, 2000, doc1)
.expectLimboDocs(doc2.key) // doc2 is now in limbo
Expand Down Expand Up @@ -134,7 +143,11 @@ describeSpec('Existence Filters:', [], () => {
.watchSnapshots(2000)
// query is now marked as "inconsistent" because of filter mismatch
.expectEvents(query1, { fromCache: true })
.expectActiveTargets({ query: query1, resumeToken: '' })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
resumeToken: ''
})
.watchRemoves(query1) // Acks removal of query
.watchAcksFull(query1, 2000, doc1)
.expectLimboDocs(doc2.key) // doc2 is now in limbo
Expand Down Expand Up @@ -165,7 +178,11 @@ describeSpec('Existence Filters:', [], () => {
// The query result includes doc3, but is marked as "inconsistent"
// due to the filter mismatch
.expectEvents(query1, { added: [doc3], fromCache: true })
.expectActiveTargets({ query: query1, resumeToken: '' })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
resumeToken: ''
})
.watchRemoves(query1) // Acks removal of query
.watchAcksFull(query1, 3000, doc1, doc2, doc3)
.expectEvents(query1, { added: [doc2] })
Expand Down Expand Up @@ -197,7 +214,11 @@ describeSpec('Existence Filters:', [], () => {
.watchSnapshots(2000)
// query is now marked as "inconsistent" because of filter mismatch
.expectEvents(query1, { fromCache: true })
.expectActiveTargets({ query: query1, resumeToken: '' })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
resumeToken: ''
})
.watchRemoves(query1) // Acks removal of query
.watchAcksFull(query1, 2000, doc1)
.expectLimboDocs(doc2.key) // doc2 is now in limbo
Expand Down
7 changes: 6 additions & 1 deletion packages/firestore/test/unit/specs/limbo_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
newQueryForPath,
queryWithLimit
} from '../../../src/core/query';
import { TargetPurpose } from '../../../src/local/target_data';
import { TimerId } from '../../../src/util/async_queue';
import { Code } from '../../../src/util/error';
import { deletedDoc, doc, filter, orderBy, query } from '../../util/helpers';
Expand Down Expand Up @@ -889,7 +890,11 @@ describeSpec('Limbo Documents:', [], () => {
// The view now contains the docAs and the docBs (6 documents), but
// the existence filter indicated only 3 should match. This causes
// the client to re-listen without a resume token.
.expectActiveTargets({ query: query1, resumeToken: '' })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
resumeToken: ''
})
// When the existence filter mismatch was detected, the client removed
// then re-added the target. Watch needs to acknowledge the removal.
.watchRemoves(query1)
Expand Down
7 changes: 6 additions & 1 deletion packages/firestore/test/unit/specs/limit_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

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

import { describeSpec, specTest } from './describe_spec';
Expand Down Expand Up @@ -343,7 +344,11 @@ describeSpec('Limits:', [], () => {
.watchSends({ affects: [limitQuery] }, secondDocument)
.watchFilters([limitQuery], [secondDocument.key])
.watchSnapshots(1004)
.expectActiveTargets({ query: limitQuery, resumeToken: '' })
.expectActiveTargets({
query: limitQuery,
targetPurpose: TargetPurpose.ExistenceFilterMismatch,
resumeToken: ''
})
.watchRemoves(limitQuery)
.watchAcksFull(limitQuery, 1005, secondDocument)
// The snapshot after the existence filter mismatch triggers limbo
Expand Down
28 changes: 22 additions & 6 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 @@ -16,6 +16,7 @@
*/

import { newQueryForPath } from '../../../src/core/query';
import { TargetPurpose } from '../../../src/local/target_data';
import { TimerId } from '../../../src/util/async_queue';
import { Code } from '../../../src/util/error';
import { deletedDoc, doc, filter, query } from '../../util/helpers';
Expand Down Expand Up @@ -137,7 +138,10 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
// The primary client 0 receives a notification that the query can
// be released, but it can only process the change after we recover
// the database.
.expectActiveTargets({ query: query1 })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch
})
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
.expectActiveTargets()
Expand Down Expand Up @@ -641,7 +645,10 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
query: filteredQuery,
resumeToken: 'resume-token-2000'
},
{ query: limboQuery }
{
query: limboQuery,
targetPurpose: TargetPurpose.LimboResolution
}
)
.watchAcksFull(filteredQuery, 4000)
.watchAcksFull(limboQuery, 4000, doc1b)
Expand Down Expand Up @@ -682,7 +689,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
.runTimer(TimerId.AsyncQueueRetry)
.expectActiveTargets(
{ query: filteredQuery, resumeToken: 'resume-token-2000' },
{ query: limboQuery }
{ query: limboQuery, targetPurpose: TargetPurpose.LimboResolution }
)
.watchAcksFull(filteredQuery, 3000)
.watchRemoves(
Expand Down Expand Up @@ -719,15 +726,21 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
.expectActiveTargets()
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
.expectActiveTargets({ query: query1 })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch
})
.expectEvents(query1, { removed: [doc1], fromCache: true })
.failDatabaseTransactions('Handle user change')
.changeUser('user1')
// The network is offline due to the failed user change
.expectActiveTargets()
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
.expectActiveTargets({ query: query1 })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch
})
.expectEvents(query1, {
added: [doc1],
fromCache: true,
Expand Down Expand Up @@ -763,7 +776,10 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
.changeUser('user1')
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
.expectActiveTargets({ query: query1 })
.expectActiveTargets({
query: query1,
targetPurpose: TargetPurpose.ExistenceFilterMismatch
})
// We are now user 2
.expectEvents(query1, { removed: [doc1], fromCache: true })
.runTimer(TimerId.AsyncQueueRetry)
Expand Down
42 changes: 35 additions & 7 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 @@ -28,6 +28,7 @@ import {
import { canonifyTarget, Target, targetEquals } from '../../../src/core/target';
import { TargetIdGenerator } from '../../../src/core/target_id_generator';
import { TargetId } from '../../../src/core/types';
import { TargetPurpose } from '../../../src/local/target_data';
import { Document } from '../../../src/model/document';
import { DocumentKey } from '../../../src/model/document_key';
import { FieldIndex } from '../../../src/model/field_index';
Expand Down Expand Up @@ -73,6 +74,7 @@ export interface LimboMap {

export interface ActiveTargetSpec {
queries: SpecQuery[];
targetPurpose: TargetPurpose;
resumeToken?: string;
readTime?: TestSnapshotVersion;
}
Expand Down Expand Up @@ -279,7 +281,12 @@ export class SpecBuilder {
}

this.queryMapping.set(target, targetId);
this.addQueryToActiveTargets(targetId, query, resume);
this.addQueryToActiveTargets(
targetId,
query,
TargetPurpose.Listen,
resume
);
this.currentStep = {
userListen: { targetId, query: SpecBuilder.queryToSpec(query) },
expectedState: { activeTargets: { ...this.activeTargets } }
Expand All @@ -299,7 +306,9 @@ export class SpecBuilder {
throw new Error("Can't restore an unknown query: " + query);
}

this.addQueryToActiveTargets(targetId!, query, { resumeToken });
this.addQueryToActiveTargets(targetId!, query, TargetPurpose.Listen, {
resumeToken
});

const currentStep = this.currentStep!;
currentStep.expectedState = currentStep.expectedState || {};
Expand Down Expand Up @@ -525,19 +534,32 @@ export class SpecBuilder {
expectActiveTargets(
...targets: Array<{
query: Query;
targetPurpose?: TargetPurpose;
resumeToken?: string;
readTime?: TestSnapshotVersion;
}>
): this {
this.assertStep('Active target expectation requires previous step');
const currentStep = this.currentStep!;
this.clientState.activeTargets = {};
targets.forEach(({ query, resumeToken, readTime }) => {
this.addQueryToActiveTargets(this.getTargetId(query), query, {
targets.forEach(
({
query,
targetPurpose = TargetPurpose.Listen,
resumeToken,
readTime
});
});
}) => {
this.addQueryToActiveTargets(
this.getTargetId(query),
query,
targetPurpose,
{
resumeToken,
readTime
}
);
}
);
currentStep.expectedState = currentStep.expectedState || {};
currentStep.expectedState.activeTargets = { ...this.activeTargets };
return this;
Expand Down Expand Up @@ -567,6 +589,7 @@ export class SpecBuilder {
this.addQueryToActiveTargets(
this.limboMapping[path],
newQueryForPath(key.path),
TargetPurpose.LimboResolution,
{ resumeToken: '' }
);
});
Expand Down Expand Up @@ -909,7 +932,7 @@ export class SpecBuilder {
const targetId = this.queryIdGenerator.cachedId(target);
this.queryMapping.set(target, targetId);

this.addQueryToActiveTargets(targetId, query, resume);
this.addQueryToActiveTargets(targetId, query, TargetPurpose.Listen, resume);

const currentStep = this.currentStep!;
currentStep.expectedState = currentStep.expectedState || {};
Expand Down Expand Up @@ -1077,6 +1100,7 @@ export class SpecBuilder {
private addQueryToActiveTargets(
targetId: number,
query: Query,
targetPurpose: TargetPurpose,
resume?: ResumeSpec
): void {
if (this.activeTargets[targetId]) {
Expand All @@ -1089,19 +1113,22 @@ export class SpecBuilder {
// `query` is not added yet.
this.activeTargets[targetId] = {
queries: [SpecBuilder.queryToSpec(query), ...activeQueries],
targetPurpose,
resumeToken: resume?.resumeToken || '',
readTime: resume?.readTime
};
} else {
this.activeTargets[targetId] = {
queries: activeQueries,
targetPurpose,
resumeToken: resume?.resumeToken || '',
readTime: resume?.readTime
};
}
} else {
this.activeTargets[targetId] = {
queries: [SpecBuilder.queryToSpec(query)],
targetPurpose,
resumeToken: resume?.resumeToken || '',
readTime: resume?.readTime
};
Expand All @@ -1115,6 +1142,7 @@ export class SpecBuilder {
if (queriesAfterRemoval.length > 0) {
this.activeTargets[targetId] = {
queries: queriesAfterRemoval,
targetPurpose: this.activeTargets[targetId].targetPurpose,
resumeToken: this.activeTargets[targetId].resumeToken
};
} else {
Expand Down
13 changes: 11 additions & 2 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 @@ -53,6 +53,7 @@ import { Mutation } from '../../../src/model/mutation';
import { encodeBase64 } from '../../../src/platform/base64';
import { newSerializer } from '../../../src/platform/serializer';
import * as api from '../../../src/protos/firestore_proto_api';
import { ApiClientObjectMap } from '../../../src/protos/firestore_proto_api';
import { Connection, Stream } from '../../../src/remote/connection';
import { Datastore, newDatastore } from '../../../src/remote/datastore';
import { WriteRequest } from '../../../src/remote/persistent_stream';
Expand Down Expand Up @@ -259,7 +260,12 @@ export class MockConnection implements Connection {
* Tracks the currently active watch targets as detected by the mock watch
* stream, as a mapping from target ID to query Target.
*/
activeTargets: { [targetId: number]: api.Target } = {};
activeTargets: {
[targetId: number]: {
target: api.Target;
labels?: ApiClientObjectMap<string>;
};
} = {};

/** A Deferred that is resolved once watch opens. */
watchOpen = new Deferred<void>();
Expand Down Expand Up @@ -398,7 +404,10 @@ export class MockConnection implements Connection {
++this.watchStreamRequestCount;
if (request.addTarget) {
const targetId = request.addTarget.targetId!;
this.activeTargets[targetId] = request.addTarget;
this.activeTargets[targetId] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Record the actual listen request label to validate targetPurpose in active targets.

target: request.addTarget,
labels: request.labels
};
} else if (request.removeTarget) {
delete this.activeTargets[request.removeTarget];
} else {
Expand Down
Loading