-
Notifications
You must be signed in to change notification settings - Fork 929
Add spec test support for target-scoped resume tokens #969
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.
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
Changes from 2 commits
e71e202
6d58872
cd8e93a
2067357
c03219b
0ab2d3f
9173791
614c373
a61cfea
47a1987
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -482,6 +482,9 @@ abstract class TestRunner { | |
return this.doRestart(); | ||
} else if ('changeUser' in step) { | ||
return this.doChangeUser(step.changeUser!); | ||
} else if ('watchSnapshot' in step) { | ||
// Handle a single `watchSnapshot` event without any other watch change | ||
return this.doWatchSnapshot(step.watchSnapshot!); | ||
} else { | ||
return fail('Unknown step: ' + JSON.stringify(step)); | ||
} | ||
|
@@ -556,7 +559,7 @@ abstract class TestRunner { | |
|
||
private doWatchAck( | ||
ackedTargets: SpecWatchAck, | ||
watchSnapshot?: SpecSnapshotVersion | ||
watchSnapshot?: SpecWatchSnapshot | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW it seems like we could probably remove "watchSnapshot?" from all of these and just have the test do the watchSnapshot step explicitly as a separate step. I don't care strongly though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
): Promise<void> { | ||
const change = new WatchTargetChange( | ||
WatchTargetChangeState.Added, | ||
|
@@ -567,7 +570,7 @@ abstract class TestRunner { | |
|
||
private doWatchCurrent( | ||
currentTargets: SpecWatchCurrent, | ||
watchSnapshot?: SpecSnapshotVersion | ||
watchSnapshot?: SpecWatchSnapshot | ||
): Promise<void> { | ||
const targets = currentTargets[0]; | ||
const resumeToken = currentTargets[1] as ProtoByteString; | ||
|
@@ -581,7 +584,7 @@ abstract class TestRunner { | |
|
||
private doWatchReset( | ||
targetIds: SpecWatchReset, | ||
watchSnapshot?: SpecSnapshotVersion | ||
watchSnapshot?: SpecWatchSnapshot | ||
): Promise<void> { | ||
const change = new WatchTargetChange( | ||
WatchTargetChangeState.Reset, | ||
|
@@ -592,7 +595,7 @@ abstract class TestRunner { | |
|
||
private doWatchRemove( | ||
removed: SpecWatchRemove, | ||
watchSnapshot?: SpecSnapshotVersion | ||
watchSnapshot?: SpecWatchSnapshot | ||
): Promise<void> { | ||
const cause = | ||
removed.cause && | ||
|
@@ -624,7 +627,7 @@ abstract class TestRunner { | |
|
||
private doWatchEntity( | ||
watchEntity: SpecWatchEntity, | ||
watchSnapshot?: SpecSnapshotVersion | ||
watchSnapshot?: SpecWatchSnapshot | ||
): Promise<void> { | ||
if (watchEntity.docs) { | ||
assert( | ||
|
@@ -672,7 +675,7 @@ abstract class TestRunner { | |
|
||
private doWatchFilter( | ||
watchFilter: SpecWatchFilter, | ||
watchSnapshot?: SpecSnapshotVersion | ||
watchSnapshot?: SpecWatchSnapshot | ||
): Promise<void> { | ||
const targetIds: TargetId[] = watchFilter[0]; | ||
assert( | ||
|
@@ -685,27 +688,39 @@ abstract class TestRunner { | |
return this.doWatchEvent(change, watchSnapshot); | ||
} | ||
|
||
private doWatchEvent( | ||
watchChange: WatchChange, | ||
watchSnapshot?: SpecSnapshotVersion | ||
): Promise<void> { | ||
let protoJSON = this.serializer.toTestWatchChange(watchChange); | ||
this.connection.watchStream!.callOnMessage(protoJSON); | ||
|
||
private doWatchSnapshot(watchSnapshot: SpecWatchSnapshot): Promise<void> { | ||
// The client will only respond to watchSnapshots if they are on a target | ||
// change with an empty set of target IDs. So we should be sure to send a | ||
// separate event. TODO(klimt): We should make the spec tests behave more | ||
// like the backend. Alternatively, we could hook in to the system at a | ||
// level higher than the stream. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know what this TODO is referring to exactly? This PR definitely moves us closer to behaving like the backend. Maybe the TODO could be removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO removed. |
||
const snapshot = | ||
watchSnapshot !== undefined ? version(watchSnapshot) : undefined; | ||
if (snapshot) { | ||
protoJSON = { | ||
targetChange: { | ||
readTime: this.serializer.toVersion(snapshot) | ||
} | ||
}; | ||
this.connection.watchStream!.callOnMessage(protoJSON); | ||
const protoJSON: api.ListenResponse = { | ||
targetChange: { | ||
readTime: this.serializer.toVersion(version(watchSnapshot.version)) | ||
} | ||
}; | ||
if (watchSnapshot.resumeToken) { | ||
protoJSON.targetChange.resumeToken = watchSnapshot.resumeToken; | ||
} | ||
if (watchSnapshot.targetIds) { | ||
protoJSON.targetChange.targetIds = watchSnapshot.targetIds; | ||
} | ||
this.connection.watchStream!.callOnMessage(protoJSON); | ||
|
||
// Put a no-op in the queue so that we know when any outstanding RemoteStore | ||
// writes on the network are complete. | ||
return this.queue.enqueue(async () => {}); | ||
} | ||
|
||
private async doWatchEvent( | ||
watchChange: WatchChange, | ||
watchSnapshot?: SpecWatchSnapshot | ||
): Promise<void> { | ||
const protoJSON = this.serializer.toTestWatchChange(watchChange); | ||
this.connection.watchStream!.callOnMessage(protoJSON); | ||
|
||
if (watchSnapshot) { | ||
await this.doWatchSnapshot(watchSnapshot); | ||
} | ||
// Put a no-op in the queue so that we know when any outstanding RemoteStore | ||
// writes on the network are complete. | ||
|
@@ -1109,7 +1124,7 @@ export interface SpecStep { | |
* Optional snapshot version that can be additionally specified on any other | ||
* watch event | ||
*/ | ||
watchSnapshot?: SpecSnapshotVersion; | ||
watchSnapshot?: SpecWatchSnapshot; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment ("snapshot version") is a bit out of date (maybe just change to "snapshot"?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Updated the rest of this too. |
||
/** A step that the watch stream restarts. */ | ||
watchStreamClose?: SpecWatchStreamClose; | ||
|
||
|
@@ -1183,7 +1198,11 @@ export type SpecWatchRemove = { | |
cause?: SpecError; | ||
}; | ||
|
||
export type SpecSnapshotVersion = TestSnapshotVersion; | ||
export type SpecWatchSnapshot = { | ||
version: TestSnapshotVersion; | ||
targetIds: TargetId[]; | ||
resumeToken?: string; | ||
}; | ||
|
||
export type SpecWatchStreamClose = { | ||
error: SpecError; | ||
|
@@ -1192,7 +1211,7 @@ export type SpecWatchStreamClose = { | |
|
||
export type SpecWriteAck = { | ||
/** The version the backend uses to ack the write. */ | ||
version: SpecSnapshotVersion; | ||
version: TestSnapshotVersion; | ||
/** Whether the ack is expected to generate a user callback. */ | ||
expectUserCallback: boolean; | ||
}; | ||
|
@@ -1257,7 +1276,7 @@ export interface SpecQuery { | |
*/ | ||
export type SpecDocument = [ | ||
string, | ||
SpecSnapshotVersion, | ||
TestSnapshotVersion, | ||
JsonObject<AnyJs> | null | ||
]; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just always make the watchSnapshot a separate step (i.e. remove this check? and perhaps the assert above?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I considered that (and added it as a suggestion to the PR description. cough cough). Now consider it done.