test(drive): add error path tests for document CRUD and cache lifecycle#3129
test(drive): add error path tests for document CRUD and cache lifecycle#3129thepastaclaw wants to merge 3 commits intodashpay:v3.1-devfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThese changes expand test coverage for document operations and cache functionality. New tests verify error handling for non-existent and duplicate document operations, while additional cache tests validate global vs. block cache behavior and cache clearing scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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🧪 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.
🧹 Nitpick comments (4)
packages/rs-drive/src/drive/document/insert/mod.rs (1)
158-226: LGTM — the test correctly targets the C6 error path and theassert!(matches!(...))assertion on the specificDriveError::CorruptedDocumentAlreadyExists(_)variant is idiomatic and well-scoped.One optional cleanup: Lines 159–198 are nearly verbatim copies of
test_add_dashpay_documents_no_transaction's setup and first insert. Consider extracting the shared setup into a small helper (returning(Drive, DataContract, DocumentType, Document, [u8; 32], &PlatformVersion)) to reduce the copy-paste surface in this test module.♻️ Sketch of a shared setup helper
+ fn setup_dashpay_contact_request_first_version( + label: &str, + ) -> ( + crate::drive::Drive, + dpp::data_contract::DataContract, + dpp::version::PlatformVersion, + ) { + let (drive, dashpay) = setup_dashpay(label, true); + let platform_version = PlatformVersion::first(); + (drive, dashpay, *platform_version) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/insert/mod.rs` around lines 158 - 226, The test test_add_dashpay_document_duplicate_insert_returns_error duplicates setup logic from test_add_dashpay_documents_no_transaction; extract the common setup into a small helper (e.g., fn setup_dashpay_test_context()) that returns the Drive, DataContract (dashpay), DocumentType, prepared Document (dashpay_cr_document), owner id (random_owner_id) and PlatformVersion; update both tests to call this helper and use the returned values when calling add_document_for_contract and asserting the duplicate error (keep the existing assertion on DriveError::CorruptedDocumentAlreadyExists).packages/rs-drive/src/drive/document/update/mod.rs (1)
254-299: LGTM — well-structured error-path test.A few observations:
- Correctly uses
mutable_contact_requests: trueso the error is about the document not existing rather than the type being immutable — the samecontactRequestfixture is used bytest_modify_dashpay_contact_requestwith the non-mutable contract, so they exercise complementary paths cleanly.- The dual-error assertion (
UpdatingDocumentThatDoesNotExistORPathKeyNotFound) is an acceptable safety net for an error-path test, consistent with the pattern used for the delete nonexistent test described in the PR.- The
"update-nonexistent"prefix string passed tosetup_dashpayis silently discarded (_prefix). Since all othersetup_dashpaycall-sites already pass"", consider using""here too for consistency, though this has zero functional impact.🧹 Nit: use consistent empty prefix
- let (drive, contract) = setup_dashpay("update-nonexistent", true); + let (drive, contract) = setup_dashpay("", true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/update/mod.rs` around lines 254 - 299, The test test_update_nonexistent_document_returns_error passes a non-empty prefix string ("update-nonexistent") to setup_dashpay even though that parameter is ignored by the function; change the call in test_update_nonexistent_document_returns_error to pass "" for consistency with other tests (i.e., replace setup_dashpay("update-nonexistent", true) with setup_dashpay("", true)) while leaving all other logic, assertions, and references to document, contract, and update_document_for_contract unchanged.packages/rs-drive/src/cache/data_contract.rs (2)
186-198:test_insert_into_block_cachedoesn't verify cache isolation
get(contract_id, true)falls back toglobal_cachewhen the block cache misses, so this assertion would pass even ifinsert(..., true)accidentally routed toglobal_cache. A directglobal_cache.getabsence check removes that ambiguity. The same gap exists symmetrically intest_insert_into_global_cache(no assertion that block_cache is empty).🔧 Proposed fix
let from_cache = data_contract_cache.get(contract_id, true); assert_eq!(from_cache, Some(fetch_info)); + + // Verify the entry went to block_cache and not global_cache + assert!(data_contract_cache.global_cache.get(&contract_id).is_none()); } }And symmetrically for
test_insert_into_global_cache:let from_cache = data_contract_cache.get(contract_id, false); assert_eq!(from_cache, Some(fetch_info)); + + // Verify the entry went to global_cache and not block_cache + assert!(data_contract_cache.block_cache.get(&contract_id).is_none()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/cache/data_contract.rs` around lines 186 - 198, The test test_insert_into_block_cache currently can pass due to fallback to global_cache; update the test (and symmetrically test_insert_into_global_cache) to assert cache isolation: after calling data_contract_cache.insert(fetch_info, true) assert that data_contract_cache.global_cache.get(&contract_id) is None (or equivalent) to ensure the entry was placed only in block_cache, and in test_insert_into_global_cache assert data_contract_cache.block_cache.get(&contract_id) is None after inserting with false; keep existing assertions that get(contract_id, true/false) returns the correct fetch_info.
237-267: Missing test: block cache entry overwrites stale global cache entry on mergeBoth
test_merge_moves_block_items_to_global_cacheandtest_merge_clears_block_cachestart with an empty global cache. The more important invariant — thatmerge_and_clear_block_cachelets a newer block-cache version win over a stale global-cache version — has no coverage. This is the primary production scenario (a block update supersedes the cached global state).🧪 Suggested additional test
#[test] fn test_merge_block_cache_overwrites_global_cache() { use dpp::data_contract::accessors::v0::DataContractV0Setters; let data_contract_cache = DataContractCache::new(10, 10); let protocol_version = PlatformVersion::latest().protocol_version; // Insert a "stale" version into global cache let fetch_info_global = Arc::new(DataContractFetchInfo::dpns_contract_fixture(protocol_version)); let contract_id = fetch_info_global.contract.id().to_buffer(); data_contract_cache.insert(Arc::clone(&fetch_info_global), false); // Insert an updated version into block cache let mut fetch_info_block = DataContractFetchInfo::dpns_contract_fixture(protocol_version); fetch_info_block.contract.increment_version(); let fetch_info_block = Arc::new(fetch_info_block); data_contract_cache.insert(Arc::clone(&fetch_info_block), true); data_contract_cache.merge_and_clear_block_cache(); // Global cache should now hold the block cache version let from_global = data_contract_cache .global_cache .get(&contract_id) .expect("should be present in global cache"); assert_eq!(from_global, fetch_info_block); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/cache/data_contract.rs` around lines 237 - 267, The tests miss verifying that merge_and_clear_block_cache lets a newer block-cache entry overwrite a stale global-cache entry; add a test (e.g., test_merge_block_cache_overwrites_global_cache) that 1) creates a DataContractCache, 2) inserts a "stale" DataContractFetchInfo into the global cache via DataContractCache::insert(..., false), 3) creates a newer block version by calling increment_version on a DataContract (use DataContractV0Setters) and inserts it into the block cache via insert(..., true), 4) calls DataContractCache::merge_and_clear_block_cache(), and 5) asserts that DataContractCache::global_cache.get(contract_id) returns the block-cache version (compare to the Arc of the updated fetch info). Use the existing symbols DataContractCache, insert, merge_and_clear_block_cache, and DataContractV0Setters to locate code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-drive/src/cache/data_contract.rs`:
- Around line 186-198: The test test_insert_into_block_cache currently can pass
due to fallback to global_cache; update the test (and symmetrically
test_insert_into_global_cache) to assert cache isolation: after calling
data_contract_cache.insert(fetch_info, true) assert that
data_contract_cache.global_cache.get(&contract_id) is None (or equivalent) to
ensure the entry was placed only in block_cache, and in
test_insert_into_global_cache assert
data_contract_cache.block_cache.get(&contract_id) is None after inserting with
false; keep existing assertions that get(contract_id, true/false) returns the
correct fetch_info.
- Around line 237-267: The tests miss verifying that merge_and_clear_block_cache
lets a newer block-cache entry overwrite a stale global-cache entry; add a test
(e.g., test_merge_block_cache_overwrites_global_cache) that 1) creates a
DataContractCache, 2) inserts a "stale" DataContractFetchInfo into the global
cache via DataContractCache::insert(..., false), 3) creates a newer block
version by calling increment_version on a DataContract (use
DataContractV0Setters) and inserts it into the block cache via insert(...,
true), 4) calls DataContractCache::merge_and_clear_block_cache(), and 5) asserts
that DataContractCache::global_cache.get(contract_id) returns the block-cache
version (compare to the Arc of the updated fetch info). Use the existing symbols
DataContractCache, insert, merge_and_clear_block_cache, and
DataContractV0Setters to locate code.
In `@packages/rs-drive/src/drive/document/insert/mod.rs`:
- Around line 158-226: The test
test_add_dashpay_document_duplicate_insert_returns_error duplicates setup logic
from test_add_dashpay_documents_no_transaction; extract the common setup into a
small helper (e.g., fn setup_dashpay_test_context()) that returns the Drive,
DataContract (dashpay), DocumentType, prepared Document (dashpay_cr_document),
owner id (random_owner_id) and PlatformVersion; update both tests to call this
helper and use the returned values when calling add_document_for_contract and
asserting the duplicate error (keep the existing assertion on
DriveError::CorruptedDocumentAlreadyExists).
In `@packages/rs-drive/src/drive/document/update/mod.rs`:
- Around line 254-299: The test test_update_nonexistent_document_returns_error
passes a non-empty prefix string ("update-nonexistent") to setup_dashpay even
though that parameter is ignored by the function; change the call in
test_update_nonexistent_document_returns_error to pass "" for consistency with
other tests (i.e., replace setup_dashpay("update-nonexistent", true) with
setup_dashpay("", true)) while leaving all other logic, assertions, and
references to document, contract, and update_document_for_contract unchanged.
thepastaclaw
force-pushed
the
test/rs-drive-document-crud-error-paths
branch
from
22a0889 to
a817e14
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
|
||
| let mut filter = | ||
| CoreBloomFilter::from_bytes(vec![0; 256], 3, 0, BloomFlags::None).unwrap(); | ||
| let mut filter = CoreBloomFilter::from_bytes(vec![0; 256], 3, 0, BloomFlags::None).unwrap(); |
There was a problem hiding this comment.
please remove these unrelated fmt changes
There was a problem hiding this comment.
Done — reverted the bloom.rs fmt changes from this PR. They're a pre-existing cargo fmt non-conformance; if we want to fix them, it should be a separate PR.
thepastaclaw
force-pushed
the
test/rs-drive-document-crud-error-paths
branch
from
0e730e3 to
b0ed8ea
Compare
|
Addressed CodeRabbit review feedback: Implemented:
Skipped (nitpick, low value):
|
thepastaclaw
force-pushed
the
test/rs-drive-document-crud-error-paths
branch
from
b0ed8ea to
96702d8
Compare
thepastaclaw
force-pushed
the
test/rs-drive-document-crud-error-paths
branch
from
96702d8 to
940aedb
Compare
2 participants
Summary
Add error path tests for core document CRUD operations in
rs-drive, and add comprehensive lifecycle tests forDataContractCache.Tracker: thepastaclaw/tracker#17
Parent audit: thepastaclaw/tracker#11
What's Changed
Error Path Tests for Document Operations
C6 — Duplicate Insert:
test_add_dashpay_document_duplicate_insert_returns_errorDriveError::CorruptedDocumentAlreadyExistsC7 — Update Nonexistent:
test_update_nonexistent_document_returns_errorDriveError::UpdatingDocumentThatDoesNotExistorGroveDB::PathKeyNotFoundC8 — Delete Nonexistent:
test_delete_nonexistent_document_returns_errorDriveError::DeletingDocumentThatDoesNotExistorGroveDB::PathKeyNotFoundCache Lifecycle Tests (C10)
Added test submodules for
DataContractCachecovering the full lifecycle:insert— Insert into global cache, insert into block cache, verify retrievalremove— Insert then remove, verify entry is gone from both cachesmerge_and_clear_block_cache— Insert into block cache, merge to global, verify promotion and block cache clearanceclear— Insert into both caches, clear all, verify everything is goneDeferred
DataContractCacheusesmoka::sync::Cacheinternally which is thread-safe, but adding proper concurrent tests is out of scope for this PR.Test Results
All new tests pass:
cargo test -p drive -- test_add_dashpay_document_duplicate✅cargo test -p drive -- test_update_nonexistent✅cargo test -p drive -- test_delete_nonexistent✅cargo test -p drive -- cache::data_contract::tests✅ (10 tests total)Summary by CodeRabbit