test(drive-abci): improve abci handler test coverage#3326
Merged
Conversation
Contributor
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds unit test modules to five ABCI handler files (extend_vote, finalize_block, info, init_chain, verify_vote_extension). Tests cover block execution context validation, height/round/hash/withdrawal mismatch cases, and success paths; no public APIs or core logic were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/abci/handler/finalize_block.rs (1)
132-138: Prefer matching error variants instead of message substrings in assertions.These assertions are brittle to wording changes. Matching
Error::Execution(ExecutionError::...)keeps tests stable while still validating behavior.♻️ Proposed test assertion refactor
- let result = finalize_block::<_, MockCoreRPCLike>(&app, request); - assert!(result.is_err()); - let err_string = result.unwrap_err().to_string(); - assert!( - err_string.contains("not in transaction") - || err_string.contains("trying to finalize block without"), - "Expected 'not in transaction' error, got: {}", - err_string - ); + let result = finalize_block::<_, MockCoreRPCLike>(&app, request); + assert!( + matches!(result, Err(Error::Execution(ExecutionError::NotInTransaction(_)))), + "Expected NotInTransaction error, got: {result:?}" + ); @@ - let result = finalize_block::<_, MockCoreRPCLike>(&app, request); - assert!(result.is_err()); - let err_string = result.unwrap_err().to_string(); - assert!( - err_string.contains("block execution context must be set"), - "Expected 'block execution context must be set' error, got: {}", - err_string - ); + let result = finalize_block::<_, MockCoreRPCLike>(&app, request); + assert!( + matches!( + result, + Err(Error::Execution(ExecutionError::CorruptedCodeExecution(_))) + ), + "Expected CorruptedCodeExecution error, got: {result:?}" + );Also applies to: 161-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/abci/handler/finalize_block.rs` around lines 132 - 138, The test currently converts the error to a string and asserts on substrings (using result.unwrap_err().to_string() and err_string.contains(...)); replace this brittle check by matching the concrete error variant returned (e.g., match on the error from result.unwrap_err() and assert it is Error::Execution(ExecutionError::NotInTransaction) or the appropriate variant), doing the same refactor for the similar block around lines 161-166; reference the result variable and the existing unwrap_err() call and replace the substring checks with pattern matching against the Error and ExecutionError enum variants to make the assertion robust.packages/rs-drive-abci/src/abci/handler/extend_vote.rs (1)
165-189: Tighten the height-mismatch assertion to avoid false positives.At Line 188,
assert!(result.is_err())will pass for any error path. Assert the specific wrong-block signal so this test only validates the intended branch.Suggested test assertion refinement
- let result = extend_vote::<_, MockCoreRPCLike>(&app, request); - assert!(result.is_err()); + let result = extend_vote::<_, MockCoreRPCLike>(&app, request); + assert!(result.is_err()); + let err_string = result.unwrap_err().to_string(); + assert!( + err_string.contains("request does not match current block"), + "Expected wrong block error, got: {}", + err_string + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/abci/handler/extend_vote.rs` around lines 165 - 189, The test extend_vote_fails_when_height_does_not_match currently asserts any error (assert!(result.is_err())); change it to assert the specific wrong-block error returned by extend_vote so the test only passes for the intended branch: call result.unwrap_err() (or match result) and assert it equals or matches the wrong-block variant/error code emitted by extend_vote (e.g., the WrongBlock/WrongHeight signal from the ABci error type), referencing the extend_vote::<_, MockCoreRPCLike> call and the RequestExtendVote used in the test to locate the check.packages/rs-drive-abci/src/abci/handler/info.rs (1)
153-156: Use exact handshake expectations instead of broad checks.At Line 154-155,
!is_empty()and> 0are permissive. Asserting the concrete expected values makes this test more regression-resistant.Suggested assertion strengthening
assert_eq!(response.last_block_height, 0); - assert!(!response.version.is_empty()); - assert!(response.app_version > 0); + assert_eq!(response.version, env!("CARGO_PKG_VERSION")); + assert_eq!( + response.app_version, + DESIRED_PLATFORM_VERSION.protocol_version as u64 + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/abci/handler/info.rs` around lines 153 - 156, The test currently uses permissive checks for handshake values; replace assert!(!response.version.is_empty()) and assert!(response.app_version > 0) with exact equality checks against the known expected handshake constants: assert_eq!(response.version, <expected_version>) and assert_eq!(response.app_version, <expected_app_version>), where <expected_version> should be the package/version constant (e.g. env!("CARGO_PKG_VERSION") or your crate's VERSION) and <expected_app_version> the agreed app version constant used by the ABCI handshake (or a literal expected value); keep the existing assert_eq!(response.last_block_height, 0) as-is. Ensure you reference the constants you use (e.g., env!("CARGO_PKG_VERSION") or crate::APP_VERSION) so the test asserts the concrete expected handshake values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-drive-abci/src/abci/handler/init_chain.rs`:
- Around line 117-124: After calling init_chain::<_, MockCoreRPCLike>(&app,
request) replace the unused _result with a captured result and add an explicit
assertion that the block execution context was cleared—e.g., call the
accessor/take method or inspect the field (block_execution_context or
take_block_execution_context()) and assert it is None/empty
(assert!(app.block_execution_context.is_none()) or
assert!(app.take_block_execution_context().is_none())). Keep the init_chain call
but add this concrete assertion immediately after to fail the test if context
clearing regresses.
---
Nitpick comments:
In `@packages/rs-drive-abci/src/abci/handler/extend_vote.rs`:
- Around line 165-189: The test extend_vote_fails_when_height_does_not_match
currently asserts any error (assert!(result.is_err())); change it to assert the
specific wrong-block error returned by extend_vote so the test only passes for
the intended branch: call result.unwrap_err() (or match result) and assert it
equals or matches the wrong-block variant/error code emitted by extend_vote
(e.g., the WrongBlock/WrongHeight signal from the ABci error type), referencing
the extend_vote::<_, MockCoreRPCLike> call and the RequestExtendVote used in the
test to locate the check.
In `@packages/rs-drive-abci/src/abci/handler/finalize_block.rs`:
- Around line 132-138: The test currently converts the error to a string and
asserts on substrings (using result.unwrap_err().to_string() and
err_string.contains(...)); replace this brittle check by matching the concrete
error variant returned (e.g., match on the error from result.unwrap_err() and
assert it is Error::Execution(ExecutionError::NotInTransaction) or the
appropriate variant), doing the same refactor for the similar block around lines
161-166; reference the result variable and the existing unwrap_err() call and
replace the substring checks with pattern matching against the Error and
ExecutionError enum variants to make the assertion robust.
In `@packages/rs-drive-abci/src/abci/handler/info.rs`:
- Around line 153-156: The test currently uses permissive checks for handshake
values; replace assert!(!response.version.is_empty()) and
assert!(response.app_version > 0) with exact equality checks against the known
expected handshake constants: assert_eq!(response.version, <expected_version>)
and assert_eq!(response.app_version, <expected_app_version>), where
<expected_version> should be the package/version constant (e.g.
env!("CARGO_PKG_VERSION") or your crate's VERSION) and <expected_app_version>
the agreed app version constant used by the ABCI handshake (or a literal
expected value); keep the existing assert_eq!(response.last_block_height, 0)
as-is. Ensure you reference the constants you use (e.g.,
env!("CARGO_PKG_VERSION") or crate::APP_VERSION) so the test asserts the
concrete expected handshake values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1284f068-80f1-49f7-b5ff-5f68b2728f65
📒 Files selected for processing (5)
packages/rs-drive-abci/src/abci/handler/extend_vote.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/abci/handler/info.rspackages/rs-drive-abci/src/abci/handler/init_chain.rspackages/rs-drive-abci/src/abci/handler/verify_vote_extension.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3326 +/- ##
============================================
+ Coverage 73.13% 73.26% +0.13%
============================================
Files 3291 3291
Lines 271091 272261 +1170
============================================
+ Hits 198261 199479 +1218
+ Misses 72830 72782 -48
🚀 New features to boost your workflow:
|
Add unit tests for ABCI handler error branches and edge cases: - info: ABCI version mismatch error, successful handshake response - extend_vote: missing block context, block hash mismatch, height mismatch, success path - verify_vote_extension: no block context, height mismatch, withdrawal mismatch, different round acceptance, success path - finalize_block: no transaction error, missing block execution context error - init_chain: existing block execution context drop path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address CodeRabbit review feedback: replace comment-only verification with an explicit assertion that block_execution_context is cleared after init_chain processes an existing context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- extend_vote: tighten height-mismatch test to assert specific error message - finalize_block: use matches! with error variants instead of string contains - info: assert exact expected values for version and app_version Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
test/drive-abci-handler-coverage
branch
from
de72caa to
0c65541
Compare
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
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.
Issue being fixed or feature implemented
Improve test coverage for the
packages/rs-drive-abci/src/abci/handler/module by adding unit tests for previously uncovered error branches and edge cases.What was done?
Added targeted unit tests to five handler files that previously lacked test coverage for error paths:
info.rs (2 tests):
extend_vote.rs (4 tests):
verify_vote_extension.rs (5 tests):
finalize_block.rs (2 tests):
init_chain.rs (1 test):
How Has This Been Tested?
All 47 handler tests pass:
Code passes clippy with no warnings:
Breaking Changes
None. This PR only adds test code.
Checklist:
Summary by CodeRabbit