Clean up recovery spec tests #3085
Conversation
Binary Size ReportAffected SDKsNo changes between base commit (d656e03) and head commit (6433a50). Test Logs
|
| 'Synchronize last document change read time': true, | ||
| 'Lookup mutation documents': true | ||
| }) | ||
| .failDatabaseTransactions('Lookup mutation documents') |
There was a problem hiding this comment.
This seems like a different test than what was here before. Previously the write of the mutation to IndexedDB would fail (as would the lookup) but now this is failing just the lookup and allowing the write to succeed?
Alternatively, if this is working as intended, better names would help make it clearer just which transaction is failing. For example, if this were named multitab.secondary.mutationLookup it would be much, much clearer.
There was a problem hiding this comment.
There is already a dedicated test that test if "Locally write mutations" fails ("Recovers when write cannot be persisted"). The fact that "Locally write mutations" is part of this list seems like an oversight to me - the mutation gets written right before and not after we start injecting failures.
There are certainly improvements to the transaction names that we should make before we port to Android/iOS, but I would like to point out that 'Lookup mutation documents' is also used by the primary tab to load a mutation that has been written by a secondary tab.
| 'Lookup mutation documents': true, | ||
| 'Get new document changes': true | ||
| }) | ||
| .failDatabaseTransactions('Get new document changes') |
There was a problem hiding this comment.
Same concern here: previously this failed to allocate a target at all, which should have caused the listen to fail. Now this is only failing much later.
There was a problem hiding this comment.
Similar response: The target is already allocated and this seems like a copy/paste issue. Failed target allocations are tested in 'Fails targets that cannot be allocated'.
I am not 100% convinced we need multi-tab specific tests for these initial failures, since these failures happen before we kick off any multi-tab specific failures. If we add a test, it would be a sanity check to ensure that we don't write a Local Storage notification if we failed to persist the target/write.
There was a problem hiding this comment.
OK. I see it now. Thanks for talking it through with me.
| 'Lookup mutation documents': true, | ||
| 'Get new document changes': true | ||
| }) | ||
| .failDatabaseTransactions('Get new document changes') |
There was a problem hiding this comment.
OK. I see it now. Thanks for talking it through with me.
Almost all recovery spec tests only fail a single Transaction type. This PR cleans up the spec tests to make the calling code a lot prettier.