test(sync): make the two-peer tests pin what they claim — 2.6.54 - #164
Merged
Conversation
A review of the previous release found no defect in the code but showed that two of its four tests proved nothing. The JVM two-peer test passed on the parent commit: its store mirror had already been retiring conflict rows on every path before the fix that introduced that behaviour, which is also why this suite never caught the fleet-wide deletion it was written to guard. Worse, its resolution always chose the live version, so the tap that actually loses data — keeping a winner that is a deletion — could not be expressed at all. The mirror now has the store's four arrival paths, its supersession rule and a resolution that can choose either side, and the three-peer test performs the losing tap and asserts the record survives on all peers. With the mirror reverted to the previous retire paths it fails on the phantom row; with the previous supersession rule as well it fails on the task being gone, which is the loss itself. The instrumented test claimed the record equalled the row's alternative and it did not — the loser JSON was hand-written and matched neither side, so the previous rule dropped the row too. It now uses the note's real serialised payload, and with the previous predicate restored the note is deleted, confirmed on the device rather than argued. Recovery after a crash between applying settings and recording their bookkeeping never recorded the base, leaving it naming a version that was never applied. It is recorded on the recovery path, skipping journals written by a conflict resolution and only while the record has not moved since. 343 unit tests, 93 instrumentation tests, 0 failures; lint 0 errors.
Test coverage (unit + instrumentation)
Files
|
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
1 participant
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.
Why
A review of 2.6.53 found no defect in the production logic — the first clean round in eight — but showed that two of the four tests that release added proved nothing.
The JVM two-peer test passed on the parent commit. Its store mirror had already been retiring conflict rows on every arrival path before the fix that introduced that behaviour, which is also why this suite never caught the fleet-wide deletion it was written to guard against. Worse, its resolution always chose the live version, so the tap that actually loses data — keeping a winner that is a deletion — could not be expressed in it at all.
The instrumented test claimed the record equalled the row's alternative. It did not: the loser JSON was hand-written and matched neither side, so the previous supersession rule would have dropped the row too. The narrowing whose predecessor caused the fleet-wide deletion had no test that failed if it were reverted.
What changed
The mirror now has the store's four arrival paths, its supersession rule, and a resolution that can choose either side. The three-peer test performs the losing tap and asserts the record survives on all peers. Both failure modes were reproduced rather than argued: with the mirror reverted to the previous retire paths the test fails on the phantom row, and with the previous supersession rule as well it fails on the task being gone — the loss itself.
The instrumented test now uses the note's real serialised payload as the row's alternative. With the previous predicate temporarily restored it was run on the Pixel and the note is deleted, confirming the test catches a revert.
Recovery after a crash between applying settings and recording their bookkeeping never recorded the base, leaving it naming a version that was never applied. It is recorded on the recovery path, skipping journals written by a conflict resolution and only while the record has not moved since. This is the only production change in the PR.
Verification
343 unit tests, 93 instrumentation tests, 0 failures; lint 0 errors; R8 clean.
Verified separately on a Pixel 7a across this release series, as peers against one real Drive: all four record types converge in one resolution — notes (local edit publishes with no dialog, the 2.6.50 upgrade path gives exactly one conflict then converges), tasks (delete versus edit, the record returns to the deleting peer carrying the other's edit), tags (divergent renames, both peers settle on the kept name), categories (a deletion is accepted with no conflict). The three-peer phantom-row scenario was reproduced by hand: a fresh peer receives a replicated conflict row for a record it never held, another peer revives the record, and the newcomer's next sync retires the phantom and ends up with the record alive.