Synchronization test accuracy#1956
Synchronization test accuracy#1956cursor[bot] wants to merge 1 commit intosync/snapshot-merge-protection-testsfrom
Conversation
- Fix divergence formula in negotiation.rs tests to match actual calculate_divergence() implementation (abs_diff / remote.entity_count) instead of the incorrect symmetric formula (1.0 - min/max) - Fix test_protection_holds_across_random_seeds to use RandomScenario::two_nodes_random(seed) instead of the deterministic Scenario::n_nodes_diverged(2), ensuring actual coverage variation - Add handshake_depth_override to SimNode and set_handshake_depth() method to allow scenarios to control reported tree depth independently of entity count - Update force_bloom_filter() and force_levelwise() scenarios to set appropriate depth overrides (3 and 2 respectively) to avoid triggering Rule 4 (SubtreePrefetch) when the scenarios are meant to test Rule 5 (BloomFilter) and Rule 6 (LevelWise)
|
Cursor Agent can help with this pull request. Just |
|
Your PR title does not adhere to the Conventional Commits convention: Common errors to avoid:
|
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 179.8s
💡 1 suggestions, 📝 1 nitpicks. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-4c8d6607
| } else { | ||
| 0.0 | ||
| }; | ||
| // Calculate divergence using the same formula as calculate_divergence(): |
There was a problem hiding this comment.
📝 Nit: Repeated divergence calculation could be extracted
The same divergence formula (abs_diff / remote.max(1)) is now repeated in three places (lines 131-136, 192-197, 429-433); a small helper function would improve DRY and make future formula changes easier.
Suggested fix:
Extract a `fn calculate_test_divergence(local_count: u64, remote_count: u64) -> f64` helper at module level.
|
|
||
| /// Set the max_depth override for handshake generation. | ||
| /// | ||
| /// This allows scenarios to control the reported tree depth independently |
There was a problem hiding this comment.
💡 Consider documenting valid depth ranges
The set_handshake_depth method allows arbitrary u32 values, but the protocol rules only care about specific thresholds (>3 for Rule 4, 1-2 for Rule 6); documenting expected values would help future test authors.
Suggested fix:
Add a brief note like "/// Common values: 2 for LevelWise (Rule 6), 3 for BloomFilter (Rule 5), >3 for SubtreePrefetch (Rule 4)."
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
1 participant
Fixes for Sync Protocol Test and Scenario Bugs
Description
This PR addresses three identified bugs related to the synchronization protocol negotiation tests and simulation scenarios, ensuring test accuracy and coverage.
Test divergence formula differs from actual implementation (bug_id: e2fde381-b08d-467e-95e1-75bea6036db9):
negotiation.rsused a symmetric divergence formula (1.0 - (min_count / max_count)), which differed from the asymmetric formula (abs_diff / remote.entity_count.max(1)) used in the actualcalculate_divergence()function. This could lead to incorrect precondition assertions, especially whenlocal > remote.negotiation.rs(lines 131, 192, 429) to match the asymmetric formula used inprotocol.rs, ensuring consistency between test assertions and production logic.Random seeds test uses fully deterministic identical inputs (bug_id: 24d031e8-3f0c-46fd-9dfd-bbf15ebc00b4):
test_protection_holds_across_random_seedstest insnapshot_merge_protection.rsiterated 100 times with different seeds but usedScenario::n_nodes_diverged(2), which is deterministic. This meant the test effectively ran the same assertion 100 times, providing a false sense of coverage.RandomScenario::two_nodes_random(seed)for node generation, ensuring that each iteration uses genuinely varied node configurations based on the seed.Scenarios don't force named protocols via build_handshake (bug_id: ref1_963e2153-f958-4ee5-870b-8af5474d0f70):
build_handshake()method'smax_depthestimation, based onentity_count, causedScenario::force_bloom_filter()andScenario::force_levelwise()to incorrectly triggerSubtreePrefetch(Rule 4) because the estimated depth was always> 3. This prevented proper testing of Rule 5 (BloomFilter) and Rule 6 (LevelWise).handshake_depth_overridefield toSimNodeand integrated it intobuild_handshake(). Theforce_bloom_filter()scenario now explicitly setsmax_depth = 3andforce_levelwise()setsmax_depth = 2, ensuring they correctly trigger their intended protocols.Test plan
All changes are within the test and simulation crates. The existing
cargo testsuite was run locally and passed successfully, confirming that the fixes do not introduce regressions and that the tests now behave as intended. The changes improve the accuracy and coverage of existing tests.Documentation update
No public or internal documentation updates are required as these changes are confined to test code and simulation utilities.