test: add comprehensive unit tests across Platform v2 modules #3
Conversation
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
Add extensive test coverage across multiple crates: - p2p-consensus: network.rs with 419 lines of tests - challenge-registry: version, lifecycle, health, and error modules - rpc-server: RpcConfig and RpcServer tests - distributed-storage: StorageError type tests - core: Challenge and ChallengeConfig tests All 609+ tests pass with cargo test --workspace
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across multiple crates and modules, including error handling, health monitoring, lifecycle management, versioning, challenge configuration, distributed storage, P2P networking, and RPC server functionality. No production code or public APIs were modified. 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
🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In `@crates/distributed-storage/src/error.rs`:
- Around line 184-232: The three tests (test_from_sled_error,
test_from_bincode_error, test_from_serde_json_error) use `if let Err` which can
silently skip assertions if the call unexpectedly succeeds; change each to force
the error path by calling unwrap_err() or expect_err() on the failing operation
(e.g., replace `let sled_result = sled::open("/\0invalid"); if let Err(sled_err)
= sled_result { ... }` with `let sled_err =
sled::open("/\0invalid").unwrap_err()` and similarly use
`bincode::deserialize(invalid_data).unwrap_err()` and
`serde_json::from_str(invalid_json).unwrap_err()`), then convert the obtained
error into a `StorageError` and keep the existing to_string() starts_with
assertions.
In `@crates/p2p-consensus/src/network.rs`:
- Around line 962-982: The test shows PeerMapping::insert currently leaves a
stale entry in hotkey_to_peer when overwriting a peer's hotkey; update
PeerMapping::insert to first check peer_to_hotkey for an existing hotkey for the
given peer_id, and if found remove that old mapping from hotkey_to_peer before
inserting the new hotkey (ensure peer_to_hotkey is then updated to the new
hotkey and hotkey_to_peer points to the peer_id). Alternatively, if stale
mappings are intended, add a comment in PeerMapping::insert explaining why
hotkey_to_peer is not cleaned on overwrite.
🧹 Nitpick comments (3)
crates/rpc-server/src/server.rs (1)
746-753: Weak assertion does not verify route registration.The assertion
std::mem::size_of_val(&router) > 0is always true for any non-zero-sized type and doesn't actually verify that routes are registered. The comment acknowledges this limitation, but the test provides minimal value beyond verifyingrouter()doesn't panic.Consider either:
- Accepting this as a smoke test (rename to
test_rpc_server_router_creation_does_not_panic)- Using
axum_testor similar to actually invoke routes and verify responsescrates/challenge-registry/src/version.rs (1)
200-209: Consider documenting the "1.2.3.4" parsing behavior.Line 208 tests that
"1.2.3.4"parses successfully with extra parts ignored. While the comment notes this, the production code's behavior of silently ignoring extra version components might be surprising. Consider whether this should returnNonefor stricter semver compliance, or add documentation to theparsefunction explaining this lenient behavior.crates/p2p-consensus/src/network.rs (1)
1083-1084: Consider using#[test]instead of#[tokio::test]for synchronous tests.These tests don't contain any
.awaitcalls, so they don't require the tokio runtime. Using plain#[test]avoids the overhead of spawning a runtime.Example change for one test
- #[tokio::test] - async fn test_replay_attack_detection() { + #[test] + fn test_replay_attack_detection() {This applies to:
test_replay_attack_detection,test_rate_limit_enforcement,test_clean_old_nonces,test_clean_rate_limit_entries,test_network_connected_peer_count,test_network_has_min_peers.Also applies to: 1128-1129, 1171-1172, 1214-1215, 1265-1266, 1302-1303
Comment on lines
+184
to
+232
| #[test] | ||
| fn test_from_sled_error() { | ||
| // Create a sled error by opening an invalid path scenario | ||
| // sled::Error doesn't have public constructors, so we trigger a real error | ||
| let sled_result = sled::open("/\0invalid"); | ||
| if let Err(sled_err) = sled_result { | ||
| let storage_err: StorageError = sled_err.into(); | ||
| let display = storage_err.to_string(); | ||
| assert!( | ||
| display.starts_with("Database error:"), | ||
| "Expected 'Database error:' prefix, got: {}", | ||
| display | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_bincode_error() { | ||
| // Create a bincode error by attempting to deserialize invalid data | ||
| let invalid_data: &[u8] = &[0xff, 0xff, 0xff, 0xff]; | ||
| let bincode_result: Result<String, bincode::Error> = | ||
| bincode::deserialize(invalid_data); | ||
| if let Err(bincode_err) = bincode_result { | ||
| let storage_err: StorageError = bincode_err.into(); | ||
| let display = storage_err.to_string(); | ||
| assert!( | ||
| display.starts_with("Serialization error:"), | ||
| "Expected 'Serialization error:' prefix, got: {}", | ||
| display | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_from_serde_json_error() { | ||
| // Create a serde_json error by parsing invalid JSON | ||
| let invalid_json = "{invalid json}"; | ||
| let json_result: Result<serde_json::Value, serde_json::Error> = | ||
| serde_json::from_str(invalid_json); | ||
| if let Err(json_err) = json_result { | ||
| let storage_err: StorageError = json_err.into(); | ||
| let display = storage_err.to_string(); | ||
| assert!( | ||
| display.starts_with("Serialization error:"), | ||
| "Expected 'Serialization error:' prefix, got: {}", | ||
| display | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
From conversion tests may silently pass if error creation fails.
The if let Err pattern means these tests will silently pass without assertions if the error creation unexpectedly succeeds. Consider using unwrap_err() or expect_err() to ensure the error path is always exercised:
🔧 Suggested fix for test_from_sled_error
#[test]
fn test_from_sled_error() {
- // Create a sled error by opening an invalid path scenario
- // sled::Error doesn't have public constructors, so we trigger a real error
- let sled_result = sled::open("/\0invalid");
- if let Err(sled_err) = sled_result {
- let storage_err: StorageError = sled_err.into();
- let display = storage_err.to_string();
- assert!(
- display.starts_with("Database error:"),
- "Expected 'Database error:' prefix, got: {}",
- display
- );
- }
+ // sled::Error doesn't have public constructors, so we trigger a real error
+ let sled_err = sled::open("/\0invalid").expect_err("Expected sled::open to fail");
+ let storage_err: StorageError = sled_err.into();
+ let display = storage_err.to_string();
+ assert!(
+ display.starts_with("Database error:"),
+ "Expected 'Database error:' prefix, got: {}",
+ display
+ );
}The same pattern should be applied to test_from_bincode_error and test_from_serde_json_error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn test_from_sled_error() { | |
| // Create a sled error by opening an invalid path scenario | |
| // sled::Error doesn't have public constructors, so we trigger a real error | |
| let sled_result = sled::open("/\0invalid"); | |
| if let Err(sled_err) = sled_result { | |
| let storage_err: StorageError = sled_err.into(); | |
| let display = storage_err.to_string(); | |
| assert!( | |
| display.starts_with("Database error:"), | |
| "Expected 'Database error:' prefix, got: {}", | |
| display | |
| ); | |
| } | |
| } | |
| #[test] | |
| fn test_from_bincode_error() { | |
| // Create a bincode error by attempting to deserialize invalid data | |
| let invalid_data: &[u8] = &[0xff, 0xff, 0xff, 0xff]; | |
| let bincode_result: Result<String, bincode::Error> = | |
| bincode::deserialize(invalid_data); | |
| if let Err(bincode_err) = bincode_result { | |
| let storage_err: StorageError = bincode_err.into(); | |
| let display = storage_err.to_string(); | |
| assert!( | |
| display.starts_with("Serialization error:"), | |
| "Expected 'Serialization error:' prefix, got: {}", | |
| display | |
| ); | |
| } | |
| } | |
| #[test] | |
| fn test_from_serde_json_error() { | |
| // Create a serde_json error by parsing invalid JSON | |
| let invalid_json = "{invalid json}"; | |
| let json_result: Result<serde_json::Value, serde_json::Error> = | |
| serde_json::from_str(invalid_json); | |
| if let Err(json_err) = json_result { | |
| let storage_err: StorageError = json_err.into(); | |
| let display = storage_err.to_string(); | |
| assert!( | |
| display.starts_with("Serialization error:"), | |
| "Expected 'Serialization error:' prefix, got: {}", | |
| display | |
| ); | |
| } | |
| } | |
| #[test] | |
| fn test_from_sled_error() { | |
| // sled::Error doesn't have public constructors, so we trigger a real error | |
| let sled_err = sled::open("/\0invalid").expect_err("Expected sled::open to fail"); | |
| let storage_err: StorageError = sled_err.into(); | |
| let display = storage_err.to_string(); | |
| assert!( | |
| display.starts_with("Database error:"), | |
| "Expected 'Database error:' prefix, got: {}", | |
| display | |
| ); | |
| } | |
| #[test] | |
| fn test_from_bincode_error() { | |
| // Create a bincode error by attempting to deserialize invalid data | |
| let invalid_data: &[u8] = &[0xff, 0xff, 0xff, 0xff]; | |
| let bincode_err = bincode::deserialize::<String>(invalid_data) | |
| .expect_err("Expected bincode::deserialize to fail"); | |
| let storage_err: StorageError = bincode_err.into(); | |
| let display = storage_err.to_string(); | |
| assert!( | |
| display.starts_with("Serialization error:"), | |
| "Expected 'Serialization error:' prefix, got: {}", | |
| display | |
| ); | |
| } | |
| #[test] | |
| fn test_from_serde_json_error() { | |
| // Create a serde_json error by parsing invalid JSON | |
| let invalid_json = "{invalid json}"; | |
| let json_err = serde_json::from_str::<serde_json::Value>(invalid_json) | |
| .expect_err("Expected serde_json::from_str to fail"); | |
| let storage_err: StorageError = json_err.into(); | |
| let display = storage_err.to_string(); | |
| assert!( | |
| display.starts_with("Serialization error:"), | |
| "Expected 'Serialization error:' prefix, got: {}", | |
| display | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@crates/distributed-storage/src/error.rs` around lines 184 - 232, The three
tests (test_from_sled_error, test_from_bincode_error,
test_from_serde_json_error) use `if let Err` which can silently skip assertions
if the call unexpectedly succeeds; change each to force the error path by
calling unwrap_err() or expect_err() on the failing operation (e.g., replace
`let sled_result = sled::open("/\0invalid"); if let Err(sled_err) = sled_result
{ ... }` with `let sled_err = sled::open("/\0invalid").unwrap_err()` and
similarly use `bincode::deserialize(invalid_data).unwrap_err()` and
`serde_json::from_str(invalid_json).unwrap_err()`), then convert the obtained
error into a `StorageError` and keep the existing to_string() starts_with
assertions.
Comment on lines
+962
to
+982
| #[test] | ||
| fn test_peer_mapping_overwrite() { | ||
| let mapping = PeerMapping::new(); | ||
| let peer_id = PeerId::random(); | ||
| let hotkey1 = Hotkey([1u8; 32]); | ||
| let hotkey2 = Hotkey([2u8; 32]); | ||
|
|
||
| // Insert with first hotkey | ||
| mapping.insert(peer_id, hotkey1.clone()); | ||
| assert_eq!(mapping.get_hotkey(&peer_id), Some(hotkey1.clone())); | ||
| assert_eq!(mapping.get_peer(&hotkey1), Some(peer_id)); | ||
|
|
||
| // Overwrite with second hotkey | ||
| mapping.insert(peer_id, hotkey2.clone()); | ||
| assert_eq!(mapping.get_hotkey(&peer_id), Some(hotkey2.clone())); | ||
| assert_eq!(mapping.get_peer(&hotkey2), Some(peer_id)); | ||
|
|
||
| // Old hotkey should still point to the peer (due to current impl not cleaning old entry) | ||
| // This tests the actual behavior - hotkey_to_peer is not cleaned on overwrite | ||
| assert_eq!(mapping.get_peer(&hotkey1), Some(peer_id)); | ||
| } |
There was a problem hiding this comment.
Test documents potential data inconsistency in PeerMapping::insert().
The test correctly captures the current behavior where overwriting a peer's hotkey leaves a stale entry in hotkey_to_peer. This means get_peer(&old_hotkey) will return the peer even after their hotkey changed.
If this is unintended, the production insert() method should be updated to remove the old hotkey mapping:
Proposed fix for production code (lines 113-116)
pub fn insert(&self, peer_id: PeerId, hotkey: Hotkey) {
+ // Remove old hotkey mapping if peer already exists
+ if let Some(old_hotkey) = self.peer_to_hotkey.read().get(&peer_id).cloned() {
+ self.hotkey_to_peer.write().remove(&old_hotkey);
+ }
self.peer_to_hotkey.write().insert(peer_id, hotkey.clone());
self.hotkey_to_peer.write().insert(hotkey, peer_id);
}If the current behavior is intentional, consider updating the comment to explain why stale mappings are acceptable.
🤖 Prompt for AI Agents
In `@crates/p2p-consensus/src/network.rs` around lines 962 - 982, The test shows
PeerMapping::insert currently leaves a stale entry in hotkey_to_peer when
overwriting a peer's hotkey; update PeerMapping::insert to first check
peer_to_hotkey for an existing hotkey for the given peer_id, and if found remove
that old mapping from hotkey_to_peer before inserting the new hotkey (ensure
peer_to_hotkey is then updated to the new hotkey and hotkey_to_peer points to
the peer_id). Alternatively, if stale mappings are intended, add a comment in
PeerMapping::insert explaining why hotkey_to_peer is not cleaned on overwrite.
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.
Summary
This PR adds extensive unit test coverage across multiple Platform v2 crates to ensure all core modules function correctly.
Changes
Test Coverage Added
Total: 1,753 lines of new test code
Testing
All 609+ tests pass with
cargo test --workspaceVerification
cargo check --workspace- Compiles successfullycargo test --workspace- All tests passSummary by CodeRabbit