Unit tests for platform-subnet-manager#27
Conversation
📝 WalkthroughWalkthroughAdds extensive unit and integration tests across the subnet-manager crate, implements directory deserialization for snapshots, exposes test-only HealthMonitor accessors, and derives PartialEq for UpdateType. No production API signatures were removed or otherwise changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (8)
crates/subnet-manager/src/snapshot.rs (2)
324-331: Asymmetric serialization/deserialization pair may cause confusion.
serialize_directory(lines 316-321) stores the path as bytes, butdeserialize_directorywrites the input data todata.binrather than interpreting it as a path. While both are marked as simplified placeholders, this asymmetry means round-tripping won't work as expected.Consider either:
- Adding a clarifying comment about this intentional mismatch, or
- Aligning the implementations for consistency
807-823: Test may be flaky due to timing dependency.The 10ms sleep between snapshot creations (line 812) might not provide sufficient time separation on heavily loaded CI systems, potentially causing intermittent test failures if timestamps collide.
Consider using a more deterministic approach:
♻️ Suggested improvement
// Create snapshots in order for i in 0..3 { manager .create_snapshot(&format!("snap{}", i), (i + 1) * 100, i + 1, &state, "test", false) .unwrap(); - std::thread::sleep(std::time::Duration::from_millis(10)); + std::thread::sleep(std::time::Duration::from_millis(50)); }crates/subnet-manager/src/recovery.rs (2)
938-993: Test lacks meaningful assertions.The test captures
attempt1,attempt2, andattempt3but doesn't assert their results. The comments suggest it should verify max attempts limiting and fallback behavior, but the test only checks thatcheck_and_recoverdoesn't panic.Consider adding explicit assertions:
♻️ Suggested improvement
// First recovery attempt let attempt1 = manager.check_and_recover(&health).await; - // Might succeed based on health status + assert!(attempt1.is_some(), "First attempt should trigger recovery"); // Second recovery attempt let attempt2 = manager.check_and_recover(&health).await; + assert!(attempt2.is_some(), "Second attempt should trigger recovery"); // Third attempt should be limited let attempt3 = manager.check_and_recover(&health).await; - // Should eventually stop or trigger fallback + assert!(attempt3.is_none(), "Third attempt should be blocked by max_attempts limit");
1246-1250: Minor style: preferassert!for boolean checks.- assert_eq!(decoded.success, true); + assert!(decoded.success);crates/subnet-manager/src/health.rs (1)
1058-1080: Duplicate test:test_worse_status_orderingduplicatestest_worse_status_priority_ordering.Lines 894-900 already test the same
worse_statuslogic with identical assertions. Consider removing this duplicate to reduce test maintenance overhead.crates/subnet-manager/src/commands.rs (3)
1536-1545: Tautological assertion provides no verification.
assert!(result.success || !result.success)is always true and doesn't test anything meaningful. If the expected behavior is unclear, consider either:
- Documenting the expected behavior and adding a proper assertion
- Removing the test if it doesn't verify meaningful behavior
♻️ Suggested improvement
async fn test_remove_nonexistent_challenge() { let (executor, _dir) = create_test_executor(); let result = executor.execute_command(&SubnetCommand::RemoveChallenge { challenge_id: "nonexistent".into(), }).await; - // Should handle gracefully (may succeed or fail depending on implementation) - assert!(result.success || !result.success); + // Removing a non-existent challenge is a no-op and succeeds + assert!(result.success); }
1692-1714: Tests with no assertions don't verify behavior.
test_remove_nonexistent_challenge_error,test_pause_resume_challenge_errors, and similar tests captureresultbut never assert on it. These tests only verify the code doesn't panic, but don't validate the expected outcomes.Consider adding assertions:
♻️ Suggested improvement
async fn test_pause_resume_challenge_errors() { let (executor, _dir) = create_test_executor(); - // Paths for lines 332, 381 let result = executor.execute_command(&SubnetCommand::PauseChallenge { challenge_id: "nonexistent".into(), }).await; + assert!(result.success, "PauseChallenge on nonexistent should succeed as no-op"); let result = executor.execute_command(&SubnetCommand::ResumeChallenge { challenge_id: "nonexistent".into(), }).await; + assert!(result.success, "ResumeChallenge on nonexistent should succeed as no-op"); }
1716-1732: Test lacks assertions for error path verification.The test captures results for unbanning non-existent entities but doesn't assert on them. Based on the implementation, these should return errors, so add appropriate assertions:
♻️ Suggested improvement
async fn test_unban_nonexistent_entities() { let (executor, _dir) = create_test_executor(); - // Paths for lines 416-417, 425-426 let result = executor.execute_command(&SubnetCommand::UnbanValidator { hotkey: Hotkey([99u8; 32]), }).await; + assert!(!result.success, "Unbanning non-existent validator should fail"); let result = executor.execute_command(&SubnetCommand::UnbanHotkey { hotkey: Hotkey([88u8; 32]), }).await; + assert!(!result.success, "Unbanning non-existent hotkey should fail"); let result = executor.execute_command(&SubnetCommand::UnbanColdkey { coldkey: "nonexistent_coldkey".into(), }).await; + assert!(!result.success, "Unbanning non-existent coldkey should fail"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/subnet-manager/src/commands.rscrates/subnet-manager/src/config.rscrates/subnet-manager/src/health.rscrates/subnet-manager/src/recovery.rscrates/subnet-manager/src/snapshot.rscrates/subnet-manager/src/update.rs
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Test
🔇 Additional comments (7)
crates/subnet-manager/src/recovery.rs (1)
319-335: LGTM!Well-structured test helper that encapsulates the common setup pattern, reducing boilerplate across tests. Returning the
TempDirensures cleanup happens at the right time.crates/subnet-manager/src/update.rs (2)
15-15: LGTM!Adding
PartialEqderive toUpdateTypeis appropriate for enabling test assertions. This is a minimal, non-breaking change that improves testability.
415-432: LGTM!Good serialization round-trip tests that verify all
UpdateTypevariants can be serialized and deserialized correctly.crates/subnet-manager/src/config.rs (2)
374-388: LGTM!Good round-trip test that verifies config serialization/deserialization preserves all fields by comparing JSON representations.
390-406: LGTM!Comprehensive validation error tests covering all three validation conditions (
epoch_length,max_validators,weight_interval). The use of pattern matching with guards effectively verifies both the error type and message content.crates/subnet-manager/src/health.rs (1)
571-582: LGTM!Test-only accessors are appropriately scoped with
#[cfg(test)]andpub(crate), enabling internal state inspection for tests without exposing internals in production builds.crates/subnet-manager/src/commands.rs (1)
609-650: LGTM!Well-organized test helpers that encapsulate common setup patterns. The separation between
create_executor_with_keypair(returns keypair for signature tests) andcreate_test_executor(simpler for non-signature tests) is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/subnet-manager/src/commands.rscrates/subnet-manager/src/health.rscrates/subnet-manager/src/recovery.rscrates/subnet-manager/src/snapshot.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/subnet-manager/src/commands.rs (4)
crates/subnet-manager/src/recovery.rs (1)
new(87-103)crates/subnet-manager/src/snapshot.rs (1)
new(77-90)crates/subnet-manager/src/config.rs (6)
new(249-251)default(59-76)default(99-107)default(136-146)load(179-183)load(254-262)crates/subnet-manager/src/update.rs (1)
new(127-135)
⏰ 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 (8)
crates/subnet-manager/src/snapshot.rs (1)
343-823: Excellent test coverage for snapshot functionality.The test suite comprehensively covers:
- Index loading and persistence
- Snapshot creation with config and challenge data
- Hash validation and corruption detection
- Snapshot restoration and application
- Pruning and retention policies
- Edge cases (nonexistent snapshots, empty data, etc.)
The tests are well-structured, isolated using tempdir, and verify both in-memory state and filesystem changes.
crates/subnet-manager/src/recovery.rs (2)
319-353: Well-designed test helper functions.The helper functions provide:
create_manager_with_config: Consistent RecoveryManager setup with custom configcreate_aggressive_health_monitor: Easy testing of recovery triggers with tight thresholdsbase_metrics: Baseline metrics with reasonable defaults (5 peers, etc.)These reduce duplication and make tests more readable.
355-1254: Comprehensive recovery test coverage.The test suite thoroughly covers:
- All recovery actions and serialization
- Health status transitions (healthy → degraded → unhealthy → critical)
- Cooldown and attempt limiting logic
- Rollback scenarios (to last snapshot, to specific snapshot, with/without snapshots)
- Component-specific recovery paths (job_queue, network, evaluations)
- Pause/resume functionality
- History tracking and attempt counting
The tests properly verify both success and failure paths, making extensive use of the helper functions for readability.
crates/subnet-manager/src/health.rs (2)
571-582: Properly scoped test-only accessors.The test accessors are correctly implemented:
- Marked with
#[cfg(test)]to exclude from production buildspub(crate)visibility restricts access to crate-level tests- Enable direct manipulation of internal state for testing edge cases
This is a standard pattern for testing state transitions without exposing internal details to production code.
588-1058: Thorough health monitoring test coverage.The test suite comprehensively validates:
- Health status comparisons and transitions
- Alert severity levels and lifecycle (creation, acknowledgement, auto-resolution)
- Component-specific health checks with configurable thresholds
- Metrics tracking and default values
- Edge cases (zero totals, empty queues, boundary conditions)
- Recovery signaling and worst-component identification
- History management and trimming (100 entry limit)
Tests properly verify both the happy path and various degraded/unhealthy scenarios.
crates/subnet-manager/src/commands.rs (3)
609-651: Well-structured test helper functions.The helper functions provide flexible executor setup:
build_executor_with_sudo: Core setup with custom sudo keycreate_executor_with_keypair: Returns keypair for signature testingcreate_test_executor: Simplified version for tests not needing the keypairThese reduce duplication while providing appropriate abstractions for different test scenarios.
652-1133: Comprehensive command execution test coverage.The tests thoroughly validate:
- CommandResult constructors (ok, ok_with_data, error)
- Signature verification (valid sudo key, wrong signer, invalid signature)
- Command serialization/deserialization for all variants
- Query commands with data verification
- Snapshot creation and rollback (including error paths)
- Config updates with filesystem persistence
- Challenge lifecycle (deploy, update, pause, resume, remove)
- Edge cases (missing data, nonexistent entities, both fields None)
Tests properly verify both in-memory state changes and side effects (file writes, update queueing).
1135-1806: Excellent coverage of remaining command scenarios.The tests comprehensively cover:
- Ban/unban operations for all entity types (validator, hotkey, coldkey)
- Validator management (kick when exists/doesn't exist, sync)
- Recovery triggering (all action types, error paths)
- Hard reset with different configurations
- Serialization of all SubnetCommand variants
- Edge cases (zero epoch length/min stake, nonexistent entities, multiple operations)
- Error paths (invalid signatures, deserialize failures, missing snapshots)
The sha256_hex utility tests verify consistency and collision resistance. Overall, the test suite provides thorough coverage of the command execution system.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.