Add more Recovery spec tests #3084
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
schmidt-sebastian
force-pushed
the
mrschmidt/synchronizereadtime
branch
from
60bf67c to
7f14814
Compare
…tation documents' recovery
schmidt-sebastian
force-pushed
the
mrschmidt/synchronizereadtime
branch
from
7f14814 to
c8cc6f8
Compare
| 'Clients fail to lookup mutations (with recovery)', | ||
| ['multi-client'], | ||
| () => { | ||
| return client(0) |
There was a problem hiding this comment.
A comment giving some context for what this test is trying to do would be helpful. It took me some time to realize that this was simulating a non-master tab writing, failing, and then recovering.
There was a problem hiding this comment.
Added top-level comment and comments inline to better explain this test.
| .failDatabaseTransactions('updateClientMetadataAndTryBecomePrimary') | ||
| .runTimer(TimerId.ClientMetadataRefresh) | ||
| .client(1) | ||
| .failDatabaseTransactions('updateClientMetadataAndTryBecomePrimary') |
There was a problem hiding this comment.
I'm sort of surprised we need to specify this on a per-client basis. My expectation would be that if the database is failing it would fail for all clients, no?
Do you find that you're writing tests where different clients see different failures? I wonder if we could make that the exception, rather than the rule?
There was a problem hiding this comment.
There are already a couple multi-tab tests in this file that use different failure modes per client, which also matches my expected behavior. A foreground tab is able to access IndexedDB, while a background tab lost filesystem access.
FWIW, I am not planning on adding further multi-tab tests for now.
| .expectPrimaryState(true) | ||
| .client(1) | ||
| .recoverDatabase() | ||
| .runTimer(TimerId.ClientMetadataRefresh) |
There was a problem hiding this comment.
What happens if client 1 happens to run their metadata refresh timer first?
There was a problem hiding this comment.
I added comment and a second section that verifies the reverse. The primary state doesn't transition during an IndexedDB failure and the order here should not affect which tab is primary.
Note that this is because of: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/local/indexeddb_persistence.ts#L433
schmidt-sebastian
requested review from
alikn,
egilmorez,
Feiyang1,
hiranya911,
hsubox76 and
zijianjoy
as code owners
Addresses #2755