-
Notifications
You must be signed in to change notification settings - Fork 61
Make SyncEngine more testable #300
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
base: main
Are you sure you want to change the base?
Conversation
| } else if context == .preview { | ||
| print("\($0.expandedDescription)") |
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.
I think it's a bit noisy to get all the SQL queries in tests, so opting out of that for now.
| .dependencies { try $0.bootstrapDatabase() }, | ||
| .dependencies { | ||
| try $0.bootstrapDatabase() | ||
| try await $0.defaultSyncEngine.sendChanges() |
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.
Now users of our library can decide to send all changes to "iCloud" after they bootstrap their database (in reality this sends the records to a completely mocked, in-memory version of iCloud).
| try RemindersList.find(UUID(0)).fetchOne(db) | ||
| } | ||
| ) | ||
| let _ = try await syncEngine.share(record: personalRemindersList, configure: { _ in }) |
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.
There isn't a ton we can test in our demo when it comes to sharing, but we can at least see that the act of sharing this list does indeed populate the share state in the snapshot below. If there was more nuanced logic in this query we would be able to test that.
| else { return } | ||
| try await parentSyncEngine.processPendingRecordZoneChanges(scope: database.databaseScope) | ||
|
|
||
| if !parentSyncEngine.syncEngine(for: database.databaseScope).state.pendingDatabaseChanges |
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.
Currently our mock sync engine was not sending pending database changes (only record changes), and so this fixes that. I'm also realizing that our mocks do not interpret the SendChangesOptions to determine what records and zones we should be processing, but that can be something we incorporate later if we need.
| ) | ||
| } | ||
|
|
||
| func processPendingDatabaseChanges( |
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.
This used to live only in the test target, but now it needs to be apart of the main library.
| containerIdentifier: String? = nil, | ||
| defaultZone: CKRecordZone = CKRecordZone(zoneName: "co.pointfree.SQLiteData.defaultZone"), | ||
| startImmediately: Bool = DependencyValues._current.context == .live, | ||
| startImmediately: Bool = true, |
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.
We previously did not start the sync engine automatically in tests because we weren't sure it was 100% ready for tests. Well now it is a lot more useful in tests so I think we can now start it by default.
| private: privateSyncEngine, | ||
| shared: sharedSyncEngine | ||
| ) | ||
| privateSyncEngine.state.add(pendingDatabaseChanges: [.saveZone(defaultZone)]) |
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.
It is better to preemptively enqueue a saveZone as soon as we start the sync engine rather than waiting around for a signIn iCloud event.
This PR makes a few small adjustments to the
SyncEngineto make it even friendlier to tests. In particular, you can now create shares in tests and see how that affects your queries (e.g. if you have separate lists of private records vs shared records).