chore: removed skip_mock_implementation_incomplete feature #340
Conversation
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
ZocoLini
force-pushed
the
chore/enabled-skipped-tests
branch
from
c2b0eee to
45b9134
Compare
github-actions
bot
removed
the
merge-conflict
ZocoLini
force-pushed
the
chore/enabled-skipped-tests
branch
from
45b9134 to
5fc263b
Compare
ZocoLini
changed the title
![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 @dash-spv/src/chain/chainlock_test.rs:
- Around line 148-156: The test currently builds a genesis header with
genesis_block(Network::Dash) but constructs a generic ChainState via
ChainState::new() and discards the process result; fix by creating the chain
state for Dash (use ChainState::new_for_network(Network::Dash) or equivalent) to
ensure network consistency with genesis_block, and replace the discarded result
of chainlock_manager.process_chain_lock(...) with an assertion that the call
succeeded (e.g., assert matches Ok/expected variant) so the test fails on errors
instead of hiding them.
🧹 Nitpick comments (2)
dash-spv/src/chain/chainlock_test.rs (2)
11-18: Good helper function, but consider consistent usage across tests.The helper is used in
test_chainlock_superseding(line 62),test_chainlock_queue_and_process_flow(lines 122-124), andtest_chainlock_manager_cache_operations(line 152), buttest_chainlock_processing(lines 29-33) andtest_reorganization_protection(lines 98-102) still constructChainLockinline. Consider refactoring for consistency.
131-138: Remove or update misleading comment.The comment on line 132 states "pending_chainlocks is private, can't access directly" but the code immediately accesses it directly. This comment is confusing and should be removed or clarified (e.g., explain that unit tests have access via
super::super::*).♻️ Proposed fix
// Verify all are queued { - // Note: pending_chainlocks is private, can't access directly + // Unit tests can access internal fields via super::super::* let pending = chainlock_manager.pending_chainlocks.read().unwrap(); assert_eq!(pending.len(), 3); assert_eq!(pending[0].block_height, 100); assert_eq!(pending[1].block_height, 200); assert_eq!(pending[2].block_height, 300); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
dash-spv/Cargo.tomldash-spv/src/chain/chainlock_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/rollback_test.rs
💤 Files with no reviewable changes (6)
- dash-spv/tests/edge_case_filter_sync_test.rs
- dash-spv/tests/block_download_test.rs
- dash-spv/Cargo.toml
- dash-spv/tests/filter_header_verification_test.rs
- dash-spv/tests/chainlock_validation_test.rs
- dash-spv/tests/rollback_test.rs
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/chain/chainlock_test.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/src/chain/chainlock_test.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
dash-spv/src/chain/chainlock_test.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Use conditional compilation with feature flags for optional features
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Run pre-PR checks: `cargo fmt`, `cargo clippy`, `cargo test` (workspace) before submitting pull requests
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Mark network-dependent or long-running tests with `#[ignore]` and run with `-- --ignored`
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Run 'cargo fmt --check' to verify formatting, 'cargo clippy --all-targets --all-features -- -D warnings' for linting, and 'cargo check --all-features' to verify all features compile
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/chain/chainlock_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/chain/chainlock_test.rs
🧬 Code graph analysis (1)
dash-spv/src/chain/chainlock_test.rs (3)
dash/src/blockdata/constants.rs (1)
genesis_block(111-184)test-utils/src/fixtures.rs (1)
test_block_hash(42-46)dash-spv/src/chain/chainlock_manager.rs (2)
new(53-62)has_chain_lock_at_height(349-351)
🔇 Additional comments (3)
dash-spv/src/chain/chainlock_test.rs (3)
20-52: LGTM!The test properly initializes storage, processes a ChainLock, and verifies both storage and retrieval. The assertions are appropriate.
54-87: LGTM!The test correctly validates that multiple ChainLocks at different heights can be stored and that the highest height is tracked properly.
89-115: LGTM!The test covers three important scenarios for reorganization protection: before chainlocks, overlapping chainlocks, and after chainlocks.
| let genesis = genesis_block(Network::Dash).header; | ||
| storage.store_headers_at_height(&[genesis], 0).await.unwrap(); | ||
|
|
||
| // Create and process a ChainLock | ||
| let chain_lock = create_test_chainlock(0, genesis.block_hash()); | ||
| let chain_state = ChainState::new(); | ||
| let _ = chainlock_manager | ||
| .process_chain_lock(chain_lock.clone(), &chain_state, &mut storage) | ||
| .await; |
There was a problem hiding this comment.
Verify network consistency and check process result.
Two concerns:
-
The test uses
genesis_block(Network::Dash)butChainState::new()instead ofChainState::new_for_network(Network::Dash). IfChainState::new()defaults to a different network, this could cause subtle test failures or false positives. -
The result of
process_chain_lockis discarded withlet _ = .... Consider asserting success like other tests do, or at least documenting why the result is intentionally ignored.
🔧 Proposed fix
// Create and process a ChainLock
let chain_lock = create_test_chainlock(0, genesis.block_hash());
- let chain_state = ChainState::new();
- let _ = chainlock_manager
+ let chain_state = ChainState::new_for_network(Network::Dash);
+ chainlock_manager
.process_chain_lock(chain_lock.clone(), &chain_state, &mut storage)
- .await;
+ .await
+ .expect("ChainLock processing should succeed");📝 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.
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_headers_at_height(&[genesis], 0).await.unwrap(); | |
| // Create and process a ChainLock | |
| let chain_lock = create_test_chainlock(0, genesis.block_hash()); | |
| let chain_state = ChainState::new(); | |
| let _ = chainlock_manager | |
| .process_chain_lock(chain_lock.clone(), &chain_state, &mut storage) | |
| .await; | |
| let genesis = genesis_block(Network::Dash).header; | |
| storage.store_headers_at_height(&[genesis], 0).await.unwrap(); | |
| // Create and process a ChainLock | |
| let chain_lock = create_test_chainlock(0, genesis.block_hash()); | |
| let chain_state = ChainState::new_for_network(Network::Dash); | |
| chainlock_manager | |
| .process_chain_lock(chain_lock.clone(), &chain_state, &mut storage) | |
| .await | |
| .expect("ChainLock processing should succeed"); |
🤖 Prompt for AI Agents
In @dash-spv/src/chain/chainlock_test.rs around lines 148 - 156, The test
currently builds a genesis header with genesis_block(Network::Dash) but
constructs a generic ChainState via ChainState::new() and discards the process
result; fix by creating the chain state for Dash (use
ChainState::new_for_network(Network::Dash) or equivalent) to ensure network
consistency with genesis_block, and replace the discarded result of
chainlock_manager.process_chain_lock(...) with an assertion that the call
succeeded (e.g., assert matches Ok/expected variant) so the test fails on errors
instead of hiding them.
I dont really know why this feature exists since there is no incomplete mock implementation but here is a PR removing it and enabling and updating tests in the process:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.