Adding onSnapshotsInSync (not public) #2100
Conversation
thebrianchen
force-pushed
the
bc/global-snapshots
branch
from
4349b31 to
beb7267
Compare
There was a problem hiding this comment.
The core of this looks good to me, but I have a few small suggestions, and I think Gil is right that we can implement tests for this as spec tests. I expect this will be a bit of a learning adventure for you and take some time, but I left you some pointers to hopefully help. But feel free to holler as you dig in and have questions.
thebrianchen
force-pushed
the
bc/global-snapshots
branch
from
859bc3a to
5c93eb2
Compare
There was a problem hiding this comment.
Nice! Generally this looks quite good. I think there's some question as to the best way to represent these expectations in the spec tests. I think we have a bit of debt with regard to how we differentiate (or not) state expectations vs event-based expectations. Not sure we need to clean that up in this PR though.
| }); | ||
| } | ||
|
|
||
| it('fires after local listeners fire', () => { |
There was a problem hiding this comment.
Not sure if this is testing anything not covered by the spec tests?
There was a problem hiding this comment.
The spec tests can't check that the onSnapshotsInSync listener fires after the other listeners are added. Not sure if it's vital that we keep this one though.
There was a problem hiding this comment.
I don't think it's vital. And I think this test may actually be racey on other platforms.
Because JS is single-threaded, you're guaranteed to add the snapshots-in-sync listener before the normal snapshot listeners fire (because they have to wait for data from the server). But on Android / iOS, it's conceivable that we could add one of the snapshot listeners, get data back from the server, and raise the snapshot event before the snapshots-in-sync listener is added. So you could actually get a different event ordering...
So I think we shouldn't keep this test... but I do think one integration test that goes through the public API would be a good idea. So I'd probably try to create a single very simple smoke test.
| }); | ||
| }); | ||
|
|
||
| it('fires for duplicate query listeners', () => { |
There was a problem hiding this comment.
I guess you have this test because spec tests don't let you listen to the same query twice? That's fine I guess, but I think I would try to pare it down as much as possible. Offhand it looks more complicated than I'd expect.
There was a problem hiding this comment.
Tried cutting out some unused variables.
| expectation.numSnapshotsInSyncEvents | ||
| ); | ||
| // Reset count after checking so tests don't have to track state. | ||
| this.snapshotsInSyncEvents = 0; |
There was a problem hiding this comment.
I think of the stateExpectations as global client state, which accumulate over time and represents the current state of the client at all times. There is already some state in stateExpectations that breaks this assumption (like userCallbacks), so it is probably fine to continue down the path and reset the count here. If you can think of a clever way to merge this with stepExpectations, it might be worth exploring that as an option (or we could have a separate "expected events" section for queries and callbacks).
Totally optional.
I also want to point out that I like that this is not a global counter that tracks every event since the spec test started, so I suggest to keep it that way.
|
@mikelehen Is this sufficient integration testing? I'm worried that I might have cut out too much -- specifically, an end-to-end integration test that checks a snapshotsInSync event is raised after a document is set. Or is this properly covered by spec tests already? |
| }); | ||
| } | ||
|
|
||
| it('fires after local listeners fire', () => { |
There was a problem hiding this comment.
I don't think it's vital. And I think this test may actually be racey on other platforms.
Because JS is single-threaded, you're guaranteed to add the snapshots-in-sync listener before the normal snapshot listeners fire (because they have to wait for data from the server). But on Android / iOS, it's conceivable that we could add one of the snapshot listeners, get data back from the server, and raise the snapshot event before the snapshots-in-sync listener is added. So you could actually get a different event ordering...
So I think we shouldn't keep this test... but I do think one integration test that goes through the public API would be a good idea. So I'd probably try to create a single very simple smoke test.
| expectation.numSnapshotsInSyncEvents | ||
| ); | ||
| // Reset count after checking so tests don't have to track state. | ||
| this.snapshotsInSyncEvents = 0; |
There was a problem hiding this comment.
Hrm... This expect validation is a little looser than I would like. The way expectEvents() works, you must call expectEvents() directly after a step that generates events, and if a step generates an event that you didn't explicitly expect, then the test fails.
With expectSnapshotsInSyncEvent(), you have a looser contract. The test runner will basically aggregate all of the events raised since last time you called expectSnapshotsInSyncEvent() and then validate the count if/when you call expectSnapshotsInSyncEvent() again. This is less explicit and makes it unclear exactly which step(s) are generating the expected events. So I think you should change this to something in the spirit of:
expect(this.snapshotsInSyncEvents).to.eq(
expectation.numSnapshotsInSyncEvents || 0
);
this.snapshotsInSyncEvents = 0;
I think this will result in you having to explicitly declare expected events immediately after the step that generated them, and if you don't, then the test fails... you will probably need to tweak some tests as a result.
First pass at plumbing through
onSnapshotsInSync.onViewSnapshotsandapplyOnlineStateChangesmainly because I'm not sure how they do. I'd be happy to add comments to the individual methods in EventManager since it's pretty hard to follow along with just the code.applyOnlineStateChangesandonViewSnapshots. Otherwise, the inSync callback would fire for each query listener.