Skip to content

Overhaul of merge conflict tests#414

Draft
lukaskubanek wants to merge 20 commits intopointfreeco:mainfrom
structuredpath:merge-conflict-tests
Draft

Overhaul of merge conflict tests#414
lukaskubanek wants to merge 20 commits intopointfreeco:mainfrom
structuredpath:merge-conflict-tests

Conversation

@lukaskubanek
Copy link
Contributor

Warning

This PR depends on #411 and #412 and should only be reviewed once those are merged.

Summary

This PR replaces the existing merge conflict tests in MergeConflictTests.swift with a new test suite that covers a broader set of scenarios across multiple dimensions:

  • different fields / same fields
  • client newer / server newer / equal timestamps
  • conflict on send / conflict on fetch
  • ordering on send: fetch before retry / retry before fetch

The new tests expose two real bugs in the current merge conflict resolution logic. These are temporarily marked with withKnownIssue, as the main goal of this PR is to document the current behavior and form a baseline for future work on configurable conflict resolution.

Improvements

  • A simpler Post table type (standalone, no parent entity required) instead of Reminder, which depends on RemindersList.
  • Snapshots assert on container.privateCloudDatabase directly, reducing boilerplate compared to the full container, as the shared database isn’t interesting for these tests.
  • Conflict scenarios use absolute timestamps (e.g. t=30, t=60) for client modification instead of relative increments. This is now aligned with how CKRecords are being treated.
  • Test steps are now annotated and ordered to match real-world event sequences. This is most notable with the client and server edits, which used to not be ordered chronologically.
  • Local database state is now checked for convergence with the CloudKit state after each scenario resolves.

Bugs Identified

The new tests reveal the following issues in the current implementation:

  1. The client always wins on same-field conflicts. Even when the server record has a newer modification timestamp, the client value is retained. This violates the “last edit wins” conflict resolution strategy as previously reported in #354.
  2. Field removal does not update the server timestamp. When both client and server null out a field, the server-side field timestamp is not updated to the client’s modification time even when the client is newer.

Overview

# Test Name Client Edits Server Edits Expected Result Actual Result Correct? Replaces
1 differentFieldsChange_conflictOnSend_clientNewer isPublished @ t=60 title @ t=30 Both merged Both merged Yes serverRecordUpdatedBeforeClientRecord
2 differentFieldsChange_conflictOnSend_serverNewer isPublished @ t=30 title @ t=60 Both merged Both merged Yes clientRecordUpdatedBeforeServerRecord
3 differentFieldsChange_conflictOnFetch_clientNewer isPublished @ t=60 title @ t=30 Both merged Both merged Yes serverAndClientEditDifferentFields
4 differentFieldsChange_conflictOnFetch_serverNewer isPublished @ t=30 title @ t=60 Both merged Both merged Yes New
5 differentNullableFieldsChange_conflictOnFetch_clientNewer priority @ t=60 dueDate @ t=30 Both merged Both merged Yes mergeWithNullableFields
6 sameFieldChange_conflictOnSend_retryBeforeFetch_clientNewer title @ t=60 title @ t=30 Client Client Yes New
7 sameFieldChange_conflictOnSend_retryBeforeFetch_serverNewer title @ t=30 title @ t=60 Server Client No New
8 sameFieldChange_conflictOnSend_fetchBeforeRetry_clientNewer title @ t=60 title @ t=30 Client Client Yes serverRecordEditedBeforeClientButProcessedAfterClient
9 sameFieldChange_conflictOnSend_fetchBeforeRetry_serverNewer title @ t=30 title @ t=60 Server Client No New (PR #353)
10 sameFieldChange_conflictOnSend_equalTimestamps title @ t=60 title @ t=60 Client Client Yes New
11 sameFieldChange_conflictOnFetch_clientNewer title @ t=60 title @ t=30 Client Client Yes serverRecordEditedAndProcessedBeforeClient
12 sameFieldChange_conflictOnFetch_serverNewer title @ t=30 title @ t=60 Server Client No serverRecordEditedAfterClientButProcessedBeforeClient
13 sameFieldChangeAndRemoval_conflictOnSend_clientNewer Nulls body @ t=60 Changes body @ t=30 Client Client Yes New
14 sameFieldChangeAndRemoval_conflictOnSend_serverNewer Nulls body @ t=30 Changes body @ t=60 Server Client No New
15 sameFieldRemoval_conflictOnSend_clientNewer Nulls body @ t=60 Nulls body @ t=30 Both nil, body @ t=60 Both nil, body @ t=30 No New
16 sameFieldRemoval_conflictOnSend_serverNewer Nulls body @ t=30 Nulls body @ t=60 Both nil, body @ t=60 Both nil, body @ t=60 Yes New

Next Steps

I have follow-up PRs ready with minimal changes to fix the issues described above. However, PRs #410, #411, and #412 need to be addressed first before those can be submitted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant