Test(storage): More unit tests #24
Conversation
📝 WalkthroughWalkthroughRemoved the public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @crates/storage/src/dynamic.rs:
- Around line 813-879: Tests show list_push and map_set silently replace
existing non-collection values (e.g., StorageValue::U64) which can silently lose
data; update list_push and map_set to check the existing value (via get_value /
the same retrieval path used by set) and return a clear error (e.g.,
TypeMismatch) when the stored type is not a list/map instead of overwriting, or
alternatively add explicit methods like replace_with_list/convert_to_map and
only perform conversion there; update unit tests to expect the new error
behavior (or add new tests for the explicit-conversion methods) and keep
references to the functions list_push, map_set, set, StorageValue, and get_value
when making changes.
🧹 Nitpick comments (2)
crates/storage/src/dynamic.rs (2)
1090-1099: Consider enhancing the flush verification test.The test currently only verifies that
flush()succeeds without error. While verifying actual disk persistence is challenging in unit tests, consider adding verification such as:
- Dropping the storage instance and re-opening to confirm data persisted
- Checking that sled's internal state indicates flush completion
However, the current test provides basic smoke test coverage, which may be sufficient.
1139-1185: Consider removing specific line number references in test comments.Several test comments reference specific line numbers in the production code (e.g., "lines 367-374", "line 386", "line 441"). While these help readers understand which code paths are being tested, they become stale and incorrect when the production code is refactored. Consider replacing with descriptive comments about the behavior being tested instead:
// Tests parsing keys with validator (validator byte array parsing logic) // Tests parsing invalid key format (returns None for malformed keys) // Tests stats aggregation across namespacesThis makes the tests more maintainable while preserving the intent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/storage/src/distributed.rscrates/storage/src/dynamic.rscrates/storage/src/lib.rscrates/storage/src/migration.rscrates/storage/src/optimized.rscrates/storage/src/types.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/storage/src/distributed.rs (3)
crates/storage/src/dynamic.rs (3)
test_get_nonexistent(760-766)create_test_storage(594-599)new(44-59)crates/storage/src/types.rs (2)
new(264-274)is_expired(282-289)crates/storage/src/lib.rs (2)
db(134-136)open(73-99)
crates/storage/src/dynamic.rs (1)
crates/storage/src/types.rs (7)
validator(41-51)as_u64(119-125)system(23-29)as_list(173-178)as_str(152-157)as_map(180-185)challenge(32-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (10)
crates/storage/src/distributed.rs (1)
1299-2001: LGTM! Excellent test coverage for distributed storage.The 34 new test functions provide comprehensive validation of:
- Entry operations (compression, verification, expiry, ACKs, serialization)
- Consensus mechanism (voting, self-votes, 50% threshold)
- Storage CRUD operations (propose, vote, commit, get, list, cleanup)
- Edge cases (size limits, duplicate votes, corruption, pending ops)
- Error handling and display
The tests are well-structured, assertions are correct, and boundary conditions are properly tested (e.g., TTL expiry at exact block height).
crates/storage/src/types.rs (1)
373-632: LGTM! Comprehensive type conversion and accessor tests.The 23 new tests thoroughly validate:
- StorageKey construction (system, challenge, validator, global_validator) and serialization
- StorageValue type conversions with proper boundary checks (e.g., u64::MAX to i64 returns None, negative i64 to u64 returns None)
- All accessor methods (as_bool, as_u64, as_i64, as_u128, as_f64, as_str, as_bytes, as_json, as_list, as_map, is_null)
- From trait implementations for all value types
- StorageEntry lifecycle (creation, updates, TTL expiry)
- StorageChange and Stats structures
The edge case coverage is excellent, particularly the overflow/underflow checks in numeric conversions.
crates/storage/src/lib.rs (1)
340-478: LGTM! Solid test coverage for Storage API.The 11 new tests validate essential Storage behaviors:
- Challenge CRUD operations (delete, list, save/load)
- Loading non-existent entities correctly returns None
- Empty state handling
- Persistence and flush operations
- Dynamic storage access consistency (dynamic() and dynamic_arc())
- Database handle access
- Migration runner creation and execution with built-in migrations
The tests are clear, assertions are correct, and they provide good coverage of the public API surface.
crates/storage/src/optimized.rs (1)
395-718: LGTM! Thorough test coverage for optimization features.The 24 new tests comprehensively validate:
- BatchWriter: auto-flush on buffer capacity, flush on drop
- LruCache: LRU eviction policy (oldest first), explicit removal, clear, emptiness checks, TTL-based cleanup, TTL expiry timing
- CachedTree: cache hit/miss tracking, CRUD operations, cache invalidation on remove, flush behavior
- CacheStats: hit rate calculation including zero-ops edge case
- PrefixScanner: count, keys, values extraction, for_each iteration with early termination (correctly tests break on false return)
- StorageMetrics: average latency calculations with proper zero-division handling
The arithmetic in tests is correct (e.g., ASCII sum: 49 + 50 = 99), and edge cases are well covered.
crates/storage/src/migration.rs (1)
559-1068: LGTM! Comprehensive migration system test coverage.The 34 new tests thoroughly validate:
- MigrationContext operations: get/set/delete, scan_prefix, raw state access, change tracking
- MigrationRunner orchestration: registration, version tracking, applied/pending queries, run_pending, rollback
- Built-in migrations: InitialMigration and AddChallengeMetricsMigration up/down behavior, properties, reversibility
- Serialization: MigrationRecord round-trip
- Edge cases: duplicate registration (correctly ignored), non-reversible rollback (correctly errors), non-existent key access (correctly returns None), idempotent run_pending when no migrations pending
- Default trait methods validation
The rollback test correctly validates that non-reversible migrations (like InitialMigration) cannot be rolled back, and the early-return path for empty pending migrations is properly tested.
crates/storage/src/dynamic.rs (5)
726-757: LGTM! Well-structured basic functionality tests.The tests for
set_block_height,with_cache, and global validator storage are clear and correctly verify the expected behavior.
759-793: LGTM! Comprehensive edge case coverage.These tests appropriately verify the handling of non-existent keys across multiple operations, including the correct default behavior for
incrementstarting from 0.
881-930: LGTM! Effective tests for scanning and maintenance operations.The tests appropriately verify namespace scanning, expired entry cleanup, and statistics gathering functionality.
932-1031: LGTM! Comprehensive scoped storage coverage.These tests thoroughly exercise challenge-scoped and validator-scoped storage operations, including deletion, TTL, scanning, incrementing, and map operations. The TTL tests verify that values can be set with TTL; actual expiration behavior is covered by
test_ttlat line 645.
1101-1137: LGTM! Good coverage of existence checks and update semantics.The
test_existsverifies proper existence tracking, andtest_set_with_options_update_existingspecifically exercises the update path for existing entries with TTL propagation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/storage/src/dynamic.rs (3)
298-319: Consider performance optimization for list operations.The current implementation clones the entire list on every
list_pushoperation, which has O(n) complexity. For large lists or frequent pushes, this could impact performance.While the type-safety check is excellent and prevents bugs, consider optimizing the list update strategy if this becomes a hot path.
Potential optimization approaches
- Batch operations: Add a
list_push_batchmethod for multiple values- Copy-on-write: Use
Arcwith copy-on-write semantics- Incremental serialization: Store list metadata separately and append without full deserialization
Note: Current implementation is correct and acceptable for typical use cases. Only optimize if profiling shows this as a bottleneck.
329-347: Similar performance consideration for map operations.Like
list_push, themap_setimplementation clones the entire HashMap on every field update. The type-safety check is well-implemented and prevents bugs. Consider the same optimization strategies mentioned for list operations if this becomes a performance bottleneck.
684-684: Potential timing flakiness in TTL tests.Tests using
std::thread::sleepwith hardcoded durations (50-100ms) may be flaky on slow CI environments or under heavy load. While these specific durations are reasonable, consider adding margin or using more robust time-based testing patterns if flakiness occurs.Also applies to: 935-935
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/core/src/error.rscrates/storage/src/dynamic.rs
🔇 Additional comments (3)
crates/core/src/error.rs (1)
49-51: LGTM! Clean addition of TypeMismatch error variant.The new error variant is well-integrated and supports the type-aware storage operations in the dynamic storage module. The error message format is clear and consistent with other variants.
crates/storage/src/dynamic.rs (2)
747-1211: Excellent test coverage!The test suite is comprehensive and well-structured, covering:
- Happy paths and error paths (including TypeMismatch scenarios)
- Edge cases (nonexistent keys, expired entries, invalid formats)
- All major APIs (challenge storage, validator storage, scoped access)
- TTL, caching, change listeners, statistics, and cleanup operations
Tests are focused, well-named, and use proper test utilities.
1-23: No action needed. The dependencies incrates/storage/Cargo.tomluse workspace inheritance ({ workspace = true }), not "latest" markers. This approach ensures reproducible builds across the workspace by centralizing version management.Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
Refactor
New Features / Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.