Deliver latest record state in ModifyRecordsCallback test helper#510
Open
lukaskubanek wants to merge 1 commit into
Open
Deliver latest record state in ModifyRecordsCallback test helper#510lukaskubanek wants to merge 1 commit into
ModifyRecordsCallback test helper#510lukaskubanek wants to merge 1 commit into
Conversation
Member
|
Hi @lukaskubanek, thanks for this! It mostly looks good, but I have a few suggestions. I'm not able to push directly to your branch for some reason, but I think we should:
I have pushed these changes to a branch of mine: e5d1563, 8b40128. Feel free to cherry pick them or re-implement them from scratch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The
modifyRecords(…)test helper returns aModifyRecordsCallback, which allows tests to notify the sync engine about server-side changes at a later point. This is useful for testing merge conflict scenarios that depend on ordering, such as fetch-before-retry and retry-before-fetch.However, the callback currently captures the saved records at modification time. If the same record is modified again in the mock database before the callback is invoked, the callback replays stale record versions, so an earlier edit is delivered to the sync engine even though the mock database already holds a newer one. The deletion side has the equivalent problem, as a deferred deletion is still delivered even if the record has been re-created in the meantime. This deviates from CloudKit's behavior, where a fetch always reflects the latest server state. Production code is not involved, but tests relying on deferred callbacks currently exercise scenarios that real CloudKit can never produce.
The issue would become even more relevant with mocked
CKRecord.recordChangeTag(a follow-up to #411 I have lined up), where stale change tags would break the conflict detection required for supporting customizable conflict resolution in the future.This was originally discussed in this comment on #412, where filing it as a separate PR was suggested. I had planned to wait for #507, but the change turned out to be independent of it. It also doesn't reintroduce the shared reference issue addressed there, since
record(for:)already returns copies of the stored records onmain.Implementation
The callback now reads the latest state from the mock database at invocation time instead of using the captured save results:
record(for:), so the sync engine receives the current server state.Both behaviors are covered by regression tests that fail on
mainand pass with this fix.Disclaimer: AI assistance (Claude Code with Fable model) was used to prepare this PR. All changes and verifications were reviewed by me.