Skip to content

docs: auto-update for PR #2972 — test(storage): un-ignore nested CRDT merge suite#2984

Closed
meroreviewer[bot] wants to merge 2 commits into
masterfrom
docs/auto-pr2972-07f2fca
Closed

docs: auto-update for PR #2972 — test(storage): un-ignore nested CRDT merge suite#2984
meroreviewer[bot] wants to merge 2 commits into
masterfrom
docs/auto-pr2972-07f2fca

Conversation

@meroreviewer

@meroreviewer meroreviewer Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Automatic Documentation Update

Opened automatically after PR #2972 merged.

Each block shows the documentation change as a diff (added lines in green, removed in red); expand "Why this changed" for the source rationale.

Documentation changes

architecture/storage-schema.html — Enable nested_crdt_merge test module; Replace Root wrappers with direct UnorderedMap usage; Replace String leaf values with LwwRegister; Replace clone-to-fork pattern with independent node construction; Use distinct executor IDs per replica in counter merge tests; Replace sleep-based LWW timestamp ordering with explicit deterministic timestamps; Add ts() helper for constructing deterministic HybridTimestamps; Rename test_map_merge_with_different_keys to test_map_union_merge_with_disjoint_keys; Add #[serial] attribute to all nested CRDT merge tests; Remove all #[ignore] attributes from nested CRDT merge tests

 seq (8 BE) Big-endian sequence number for ordered scans
 var Variable-length suffix (application keys)
+CRDT Nested Merge Test Module
+The nested_crdt_merge test module was previously disabled with a TODO comment in
+lib.rs due to a Clone dependency on storage-backed collections. All five tests
+are now unconditionally compiled, active, and included in the default test run.
+Key Changes
+Module enabled: The #[cfg(...)] / comment guard around nested_crdt_merge in
+lib.rs has been removed. The module compiles unconditionally.
+No more #[ignore]: All five tests previously carried #[ignore]. These attributes
+have been removed; the tests now run by default with cargo test.
+#[serial] on every test: Because storage-backed CRDT instances share process-
+level state, all tests in this module are annotated with #[serial] (from the
+serial_test crate) to prevent interference from parallel execution.
+Root<T> → Direct UnorderedMap Usage
+Tests previously wrapped collections in Root<T> and called Mergeable::merge on
+the wrapper. Root<T> drives merge through an app-registered callback, not
+through Mergeable, so the tests never exercised merge logic. All tests now call
+Mergeable::merge directly on UnorderedMap instances.
+String Leaves → LwwRegister<String>
+String intentionally does not implement Mergeable. All leaf CRDT values in
+nested map tests are now LwwRegister<String>, which implements Last-Write-Wins
+merge semantics and satisfies the Mergeable bound.
+Clone-to-Fork → Independent Node Construction
+Storage-backed collections cannot be cloned. Each test now constructs two
+completely independent collection instances from scratch. Both are populated
+with the same shared initial state, then each receives node-specific divergent
+writes before the merge assertion.
+Deterministic Timestamps in LWW Tests
+The test_map_of_lww_registers_merge test previously used std::thread::sleep to
+force wall-clock ordering between writes. This made timestamp ordering depend on
+scheduler behavior. All LwwRegister insertions now use
+LwwRegister::new_with_metadata with explicit HybridTimestamp values:
+// module-private helper
+fn ts(time: u64, node: NonZeroU128) -> HybridTimestamp { ... }
+// usage in tests
+let reg = LwwRegister::new_with_metadata(value, ts(10, node1));
+NonZeroU128 is used for the node parameter so that accidental zero values
+produce a compile error rather than a silent correctness bug.
+Distinct Executor IDs in Counter Merge Tests
Why this changed (source: PR #2972)

The nested_crdt_merge module was commented out in lib.rs with a TODO about Clone. It is now unconditionally compiled and included in the test suite.

All five tests previously wrapped collections in Root<T> and called Mergeable::merge on the root. Root<T> uses a special app-registered callback, not Mergeable, so the tests never actually tested merge logic. Tests now call Mergeable::merge directly on UnorderedMap instances.

Nested maps used String as the leaf CRDT value. String intentionally does not implement Mergeable. All leaf values are now LwwRegister<String>, which does implement Mergeable via Last-Write-Wins semantics.

Tests previously attempted to clone one collection to simulate a second replica. Storage-backed collections cannot be cloned. Each test now constructs two completely independent collection instances from scratch, each populated with the same initial data plus node-specific divergent writes.

In test_map_of_counters_merge, node 1 uses executor ID [100;32] and node 2 uses [200;32] before their respective increment calls. GCounter tracks increments per executor key; using the same ID on both nodes would cause max-semantics to yield 3 instead of the correct sum of 6 after merge.

test_map_of_lww_registers_merge previously used std::thread::sleep to create real-time gaps between writes, making timestamp ordering depend on wall-clock behavior. All LwwRegister insertions now use LwwRegister::new_with_metadata with explicit HybridTimestamp values constructed via a ts(time, node) helper function.

A module-private ts(time: u64, node: NonZeroU128) -> HybridTimestamp function was added. It constructs a HybridTimestamp from an explicit logical time and node ID. NonZeroU128 is used for the node parameter to make zero-value mistakes visible at the call site.

The test was renamed to accurately describe its behavior: it validates that merging two maps with completely disjoint keys produces a union (both keys present in result), not that it tests counter merge semantics.

All five tests are now annotated with #[serial] from the serial_test crate, ensuring they run sequentially rather than in parallel.

Five tests that were marked #[ignore] are now active and included in the default test run.

architecture/migrations.html — Enable nested_crdt_merge test module; Replace Root wrappers with direct UnorderedMap usage; Replace String leaf values with LwwRegister; Replace clone-to-fork pattern with independent node construction; Use distinct executor IDs per replica in counter merge tests; Replace sleep-based LWW timestamp ordering with explicit deterministic timestamps; Add ts() helper for constructing deterministic HybridTimestamps; Rename test_map_merge_with_different_keys to test_map_union_merge_with_disjoint_keys; Add #[serial] attribute to all nested CRDT merge tests; Remove all #[ignore] attributes from nested CRDT merge tests

 panic! on bad input (non-destructive abort)Expect a Result from the migrate fn
 Keep old version blobs obtainable so behind members can chainDrop an intermedia…
+Testing CRDT merge logic: patterns and pitfalls
+The nested_crdt_merge test module was previously disabled behind a TODO comment.
+It is now unconditionally compiled and runs as part of the default test suite.
+The fixes required to unblock it surface a handful of patterns that apply to any
+test exercising Mergeable::merge on storage-backed collections.
+Call Mergeable::merge on the collection, not on a Root<T> wrapper
+Root<T> drives merge through an app-registered callback, not through Mergeable.
+Wrapping a collection in Root<T> and calling Mergeable::merge on the root does
+not exercise the collection's merge logic. Call Mergeable::merge directly on
+UnorderedMap, Vector, etc.:
+// wrong — Root's merge goes through a different dispatch path
+let mut root_a: Root<UnorderedMap<…>> = …;
+Mergeable::merge(&mut root_a, root_b);
+// correct
+let mut map_a: UnorderedMap<String, LwwRegister<String>> = …;
+Mergeable::merge(&mut map_a, map_b);
+Use LwwRegister<T>, not String, as leaf values
+String intentionally does not implement Mergeable. Leaf values inside a nested
+CRDT must themselves implement Mergeable. Use LwwRegister<String> (or another
+concrete CRDT type) as the leaf:
+// String does not implement Mergeable — compile error at the merge call
+UnorderedMap<String, String>
+// LwwRegister implements Mergeable via Last-Write-Wins semantics
+UnorderedMap<String, LwwRegister<String>>
+Construct two independent instances — don't clone
+Storage-backed collections cannot be cloned. Attempting to clone one collection
+to simulate a second replica does not compile. Instead, construct two completely
+independent instances and populate each with the same baseline data plus node-
+specific divergent writes:
+let mut node_a: UnorderedMap<String, LwwRegister<String>> = UnorderedMap::new();
+node_a.insert("shared".into(), LwwRegister::new("v1".into()))?;
+node_a.insert("only_a".into(), LwwRegister::new("a".into()))?;
+let mut node_b: UnorderedMap<String, LwwRegister<String>> = UnorderedMap::new();
+node_b.insert("shared".into(), LwwRegister::new("v1".into()))?;
+node_b.insert("only_b".into(), LwwRegister::new("b".into()))?;
+Use distinct executor IDs for counter replica tests
+GCounter tracks increments per executor key. If both replicas increment using
+the same executor ID, max-semantics collapse the divergent increments into one,
Why this changed (source: PR #2972)

The nested_crdt_merge module was commented out in lib.rs with a TODO about Clone. It is now unconditionally compiled and included in the test suite.

All five tests previously wrapped collections in Root<T> and called Mergeable::merge on the root. Root<T> uses a special app-registered callback, not Mergeable, so the tests never actually tested merge logic. Tests now call Mergeable::merge directly on UnorderedMap instances.

Nested maps used String as the leaf CRDT value. String intentionally does not implement Mergeable. All leaf values are now LwwRegister<String>, which does implement Mergeable via Last-Write-Wins semantics.

Tests previously attempted to clone one collection to simulate a second replica. Storage-backed collections cannot be cloned. Each test now constructs two completely independent collection instances from scratch, each populated with the same initial data plus node-specific divergent writes.

In test_map_of_counters_merge, node 1 uses executor ID [100;32] and node 2 uses [200;32] before their respective increment calls. GCounter tracks increments per executor key; using the same ID on both nodes would cause max-semantics to yield 3 instead of the correct sum of 6 after merge.

test_map_of_lww_registers_merge previously used std::thread::sleep to create real-time gaps between writes, making timestamp ordering depend on wall-clock behavior. All LwwRegister insertions now use LwwRegister::new_with_metadata with explicit HybridTimestamp values constructed via a ts(time, node) helper function.

A module-private ts(time: u64, node: NonZeroU128) -> HybridTimestamp function was added. It constructs a HybridTimestamp from an explicit logical time and node ID. NonZeroU128 is used for the node parameter to make zero-value mistakes visible at the call site.

The test was renamed to accurately describe its behavior: it validates that merging two maps with completely disjoint keys produces a union (both keys present in result), not that it tests counter merge semantics.

All five tests are now annotated with #[serial] from the serial_test crate, ensuring they run sequentially rather than in parallel.

Five tests that were marked #[ignore] are now active and included in the default test run.


Generated by ai-reviewer update-docs. Nothing was auto-merged.

@meroreviewer meroreviewer Bot added automated-docs documentation Improvements or additions to documentation labels Jun 27, 2026

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 37.2s


✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉


🤖 Generated by AI Code Reviewer | Review ID: review-c0125f15

@chefsale chefsale closed this Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated-docs documentation Improvements or additions to documentation external

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant