Add spec test support for target-scoped resume tokens #969
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt-querysnapshot
branch
from
c4a2f52 to
e71e202
Compare
| 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.
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.
TODO removed.
| @@ -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.
The comment ("snapshot version") is a bit out of date (maybe just change to "snapshot"?)
There was a problem hiding this comment.
Done. Updated the rest of this too.
| @@ -556,7 +559,7 @@ abstract class TestRunner { | |||
|
|
|||
| private doWatchAck( | |||
| ackedTargets: SpecWatchAck, | |||
| watchSnapshot?: SpecSnapshotVersion | |||
| watchSnapshot?: SpecWatchSnapshot | |||
There was a problem hiding this comment.
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.
| this.assertStep('Watch snapshot requires previous watch step'); | ||
| this.currentStep!.watchSnapshot = version; | ||
|
|
||
| if (this.currentStep!.watchSnapshot !== undefined) { |
There was a problem hiding this comment.
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.
Yeah, I considered that (and added it as a suggestion to the PR description. cough cough). Now consider it done.
There was a problem hiding this comment.
Updated PR to make watchSnapshot a separate step everywhere.
This meant that I had to fix a bug in the test "Existence filter limbo resolution is denied". The limbo resolution actually happens at the watchRemove step and not at the snapshot. This was previously masked since we did not drain the queue right away.
| this.assertStep('Watch snapshot requires previous watch step'); | ||
| this.currentStep!.watchSnapshot = version; | ||
|
|
||
| if (this.currentStep!.watchSnapshot !== undefined) { |
There was a problem hiding this comment.
Yeah, I considered that (and added it as a suggestion to the PR description. cough cough). Now consider it done.
| @@ -556,7 +559,7 @@ abstract class TestRunner { | |||
|
|
|||
| private doWatchAck( | |||
| ackedTargets: SpecWatchAck, | |||
| watchSnapshot?: SpecSnapshotVersion | |||
| watchSnapshot?: SpecWatchSnapshot | |||
| 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.
TODO removed.
| @@ -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.
Done. Updated the rest of this too.
…/firebase-js-sdk into mrschmidt-querysnapshot
…/firebase-js-sdk into mrschmidt-querysnapshot
This PR adds some SpecTest functionality that I thought I needed for Multi-Tab but was then able to proceed without. This give us the option to either just ignore that this ever happened, or to see if we want to merge this change to Master nonetheless.
BTW, this might also be a good time to treat
watchSnapshotas a separate operation altogether and not piggy-back on the previous watch operation.