refactor: storage tests moved into unit tests and dropped duplicated tests #371
Conversation
📝 WalkthroughWalkthroughConsolidates and expands storage tests by moving and refactoring many integration tests into unit tests inside Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv/tests/header_sync_test.rs (1)
31-35:TempDiris dropped immediately, potentially deleting the directory before the test completes.The
TempDiris created inline and not bound to a variable, so it's dropped immediately after.path()returns. WhenTempDirdrops, it deletes the temporary directory. TheDiskStorageManagerwill then be operating on a deleted path.Proposed fix
+ // Create storage manager + let temp_dir = TempDir::new().expect("Failed to create tmp dir"); let storage_manager = - DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path()) + DiskStorageManager::new(temp_dir.path()) .await .expect("Failed to create tmp storage");dash-spv/src/storage/mod.rs (1)
142-148:TempDiris dropped when function returns, deleting the directory.The
TempDiris created locally and dropped whenwith_temp_dir()returns. This deletes the temporary directory whileDiskStorageManageris still expected to use it. The fact thatnew()callscreate_dir_allrecreates the path, but the temp directory cleanup semantics are lost and this is fragile.Consider returning a tuple or wrapping both in a struct so the
TempDirlifetime is tied to the storage lifetime.Proposed fix - return both TempDir and storage
#[cfg(test)] - pub async fn with_temp_dir() -> StorageResult<Self> { + pub async fn with_temp_dir() -> StorageResult<(tempfile::TempDir, Self)> { use tempfile::TempDir; let temp_dir = TempDir::new()?; - Self::new(temp_dir.path()).await + let storage = Self::new(temp_dir.path()).await?; + Ok((temp_dir, storage)) }Then update callers:
- let mut storage = - DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage"); + let (_temp_dir, mut storage) = + DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage");
🧹 Nitpick comments (1)
dash-spv/src/storage/mod.rs (1)
525-525: Unnecessary double reference.
temp_dir.path()already returns&Path, so&temp_dir.path()creates&&Path. While this works due to auto-deref, it's inconsistent with line 506 which correctly usestemp_dir.path().Suggested fix
- let storage = DiskStorageManager::new(&temp_dir.path()).await.unwrap(); + let storage = DiskStorageManager::new(temp_dir.path()).await.unwrap();
ZocoLini
force-pushed
the
refactor/storage-tests
branch
from
192fb56 to
743988d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dash-spv/src/storage/mod.rs`:
- Around line 434-443: The test drops DiskStorageManager while its background
worker may still be running; ensure you call storage.shutdown().await before
drop(storage) to stop the worker and avoid post-test writes. Update the
instances around DiskStorageManager::new(...) and subsequent drop(storage) to
invoke the async shutdown() on the storage handle (e.g., call
storage.shutdown().await) prior to dropping it; apply the same change to the
other similar blocks referenced (around the other occurrences ~lines 523-535 and
575-577).
- Around line 540-552: DiskStorageManager::with_temp_dir() currently drops the
TempDir immediately, risking deletion of the backing directory while storage is
still used; fix the test by creating and retaining an explicit TempDir (e.g.,
via tempfile::TempDir) and passing its path into DiskStorageManager (or into
DiskStorageManager::new) so the TempDir lives for the lifetime of storage;
update the test around DiskStorageManager::with_temp_dir(),
BlockHeader::dummy_batch, storage.store_headers,
storage.get_header_height_by_hash, and storage.clear to hold the TempDir
variable until after storage.clear() completes.
| storage.shutdown().await; | ||
| drop(storage); | ||
| let storage = | ||
| DiskStorageManager::new(temp_dir.path()).await.expect("Unable to open storage"); | ||
|
|
||
| let loaded_headers = storage.load_headers(49_999..50_002).await.unwrap(); | ||
| assert_eq!(loaded_headers.len(), 3); | ||
| assert_eq!(&loaded_headers, &headers[49_999..50_002]); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Stop the background worker before dropping storage instances.
Dropping DiskStorageManager without shutdown() leaves the background task running, which can keep writing to a temp path after the test finishes and cause flaky cleanup. Consider shutting down before drop, even in tests.
🔧 Suggested fix
@@
- let storage =
- DiskStorageManager::new(temp_dir.path()).await.expect("Unable to open storage");
+ let mut storage =
+ DiskStorageManager::new(temp_dir.path()).await.expect("Unable to open storage");
@@
let loaded_headers = storage.load_headers(49_999..50_002).await.unwrap();
assert_eq!(loaded_headers.len(), 3);
assert_eq!(&loaded_headers, &headers[49_999..50_002]);
+ storage.shutdown().await;
@@
- let storage = DiskStorageManager::new(&temp_dir.path()).await.unwrap();
+ let mut storage = DiskStorageManager::new(temp_dir.path()).await.unwrap();
@@
for i in 0..10 {
let stored_header = storage.get_header(i).await.unwrap().unwrap();
let hash = stored_header.block_hash();
let height = storage.get_header_height_by_hash(&hash).await.unwrap();
assert_eq!(height, Some(i), "Height mismatch after reload for header {}", i);
}
+ storage.shutdown().await;
@@
- drop(storage1);
+ storage1.shutdown().await;
+ drop(storage1);Also applies to: 523-535, 575-577
🤖 Prompt for AI Agents
In `@dash-spv/src/storage/mod.rs` around lines 434 - 443, The test drops
DiskStorageManager while its background worker may still be running; ensure you
call storage.shutdown().await before drop(storage) to stop the worker and avoid
post-test writes. Update the instances around DiskStorageManager::new(...) and
subsequent drop(storage) to invoke the async shutdown() on the storage handle
(e.g., call storage.shutdown().await) prior to dropping it; apply the same
change to the other similar blocks referenced (around the other occurrences
~lines 523-535 and 575-577).
| let mut storage = | ||
| DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage"); | ||
|
|
||
| // Store some headers | ||
| let header = BlockHeader::dummy_batch(0..1); | ||
| storage.store_headers(&header).await.unwrap(); | ||
|
|
||
| let hash = header[0].block_hash(); | ||
| assert!(storage.get_header_height_by_hash(&hash).await.unwrap().is_some()); | ||
|
|
||
| // Clear storage | ||
| storage.clear().await.unwrap(); | ||
|
|
There was a problem hiding this comment.
Keep the TempDir alive for the storage lifetime.
DiskStorageManager::with_temp_dir() drops the TempDir immediately, which can delete the backing directory while the storage is still active. Use an explicit TempDir in the test to keep the directory alive.
🔧 Suggested fix
@@
- let mut storage =
- DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage");
+ let temp_dir = TempDir::new().expect("Failed to create temp directory");
+ let mut storage =
+ DiskStorageManager::new(temp_dir.path()).await.expect("Failed to create tmp storage");
@@
// Verify index is cleared
assert!(storage.get_header_height_by_hash(&hash).await.unwrap().is_none());
+ storage.shutdown().await;🤖 Prompt for AI Agents
In `@dash-spv/src/storage/mod.rs` around lines 540 - 552,
DiskStorageManager::with_temp_dir() currently drops the TempDir immediately,
risking deletion of the backing directory while storage is still used; fix the
test by creating and retaining an explicit TempDir (e.g., via tempfile::TempDir)
and passing its path into DiskStorageManager (or into DiskStorageManager::new)
so the TempDir lives for the lifetime of storage; update the test around
DiskStorageManager::with_temp_dir(), BlockHeader::dummy_batch,
storage.store_headers, storage.get_header_height_by_hash, and storage.clear to
hold the TempDir variable until after storage.clear() completes.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.