Fix redeem actions tests to run on Nightly #6602
Conversation
- test_redeem_must_not_work_for_tickets_of_previous_epoch_being_aggregated_and_being_redeemed - test_redeem_must_not_work_for_tickets_of_next_epoch_being_redeemed Closes #6597
📝 WalkthroughWalkthroughThe pull request introduces updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
chain/actions/src/redeem.rs (2)
597-611: Improve variable naming consistency.The variable
ticket_from_previous_epoch_countis used to track tickets from epoch 3, but the test setup comments refer to "Epoch 4" and "Epoch 3". Consider renaming variables to match the epochs they represent for better clarity.-let ticket_from_previous_epoch_count = 2; +let tickets_from_epoch_3_count = 2;
674-674: Fix incorrect comment.The comment states "Insert another 2 tickets in Epoch 3" but the code actually inserts tickets for Epoch 5.
-// Insert another 2 tickets in Epoch 3 +// Insert another 2 tickets in Epoch 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
chain/actions/Cargo.toml(1 hunks)chain/actions/src/redeem.rs(2 hunks)
🔇 Additional comments (3)
chain/actions/Cargo.toml (2)
19-19: LGTM: Feature dependencies are well-structured.
The addition of hopr-chain-rpc runtime features maintains consistency with existing runtime dependencies and supports the enhanced ticket redemption logic.
Also applies to: 24-24
Line range hint 1-70: Verify impact of removing tracing-test dependency.
The AI summary indicates that tracing-test was removed from dev-dependencies. Since this PR focuses on fixing tests for nightly builds, we should verify this won't impact test debugging capabilities.
✅ Verification successful
Let me analyze the test files to check for tracing usage in the HOPR codebase specifically.
Let me check the actual tracing usage in the test files and the alternative tracing mechanisms in place.
Let me check how tracing is used in the chain-actions crate files to understand the impact.
Based on the analysis of the codebase, I can now provide the final response:
Removing tracing-test dependency has no impact on test debugging capabilities
The codebase uses the test-log crate for test logging (as seen in session_integration_tests.rs) and the standard tracing crate for instrumentation. The removed tracing-test dependency was not actively used. The codebase has comprehensive tracing through:
- Standard tracing macros (debug, error, info, warn) used throughout the code
- Instrumented functions with
#[tracing::instrument]for detailed logging - Test-specific logging via
#[test_log::test]attribute
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if tracing-test is used in any test files
# and if there are alternative tracing mechanisms in place
# Search for tracing-test usage in test files
echo "Checking for tracing-test usage in tests:"
rg -l "use.*tracing_test" $(fd -t f -e rs test)
# Check for alternative tracing mechanisms
echo -e "\nChecking for alternative tracing setups:"
rg -l "use.*tracing" $(fd -t f -e rs test)
Length of output: 79946
Script:
#!/bin/bash
# Check for tracing usage in HOPR test files
echo "Checking tracing usage in HOPR tests:"
rg -A 5 "use.*tracing|tracing::" hopr/hopr-lib/tests/chain_integration_tests.rs hopr/hopr-lib/tests/session_integration_tests.rs hopr-socks/hopr-socks-server/tests/socks_server_tests.rs
# Check if tracing is a dependency in other test-related files
echo -e "\nChecking for tracing in test configuration files:"
rg -l "tracing.*=" $(fd -t f "Cargo.toml")
Length of output: 42041
Script:
#!/bin/bash
# Check for test-log dependency which is commonly used for test tracing
echo "Checking for test-log dependency:"
rg "test-log.*=" chain/actions/Cargo.toml
# Check for test logging setup in the test files
echo -e "\nChecking test logging setup:"
rg -A 5 "#\[test_log|use test_log" hopr/hopr-lib/tests/chain_integration_tests.rs hopr/hopr-lib/tests/session_integration_tests.rs
# Check if tracing is used in other test files in the same crate
echo -e "\nChecking tracing usage in other test files:"
fd -t f -e rs . chain/actions/src chain/actions/tests 2>/dev/null | xargs rg -l "use.*tracing|#\[test_log"
Length of output: 1875
Script:
#!/bin/bash
# Check how tracing is used in the chain-actions crate files
echo "Checking tracing usage in chain-actions files:"
rg -A 2 "use.*tracing|tracing::" chain/actions/src/redeem.rs chain/actions/src/node.rs chain/actions/src/channels.rs chain/actions/src/action_queue.rs chain/actions/src/action_state.rs
# Check for any test-specific tracing setup in the crate
echo -e "\nChecking for test-specific tracing setup:"
rg -l "mod test" chain/actions/src/*.rs | xargs rg -A 5 "use.*tracing|#\[test]"
Length of output: 7533
chain/actions/src/redeem.rs (1)
Line range hint 596-720: LGTM! Test improvements enhance clarity and coverage.
The changes successfully improve the test suite by:
- Making tests more focused on specific epoch validation scenarios
- Reducing unnecessary complexity while maintaining coverage
- Properly testing both previous and next epoch validation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
chain/actions/src/redeem.rs (3)
597-599: Consider defining constants for epoch numbers and countsUsing named constants for values like
2,3, and4u32can enhance readability and maintainability.
605-607: Avoid reusing the variable nameticketReusing the
ticketvariable can cause confusion. Consider renaming variables to reflect their unique purposes.Also applies to: 609-611
675-681: Consider defining constants for epoch numbers and countsAs with earlier code, using named constants for
5,0, and1improves code clarity and makes future adjustments easier.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
chain/actions/Cargo.toml(1 hunks)chain/actions/src/redeem.rs(2 hunks)
🔇 Additional comments (2)
chain/actions/Cargo.toml (2)
19-19: LGTM: Runtime feature additions are well-structured.
The addition of runtime-specific features from hopr-chain-rpc aligns with the existing pattern of runtime configuration and maintains consistency with other dependencies.
Also applies to: 24-24
Line range hint 1-70: Verify impact of removing tracing-test dependency.
The removal of tracing-test from dev-dependencies could affect test debugging capabilities. Please ensure that all tests still have adequate logging and debugging support.
Consider documenting the preferred approach for test tracing if alternatives are being used.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
chain/actions/src/redeem.rs (2)
604-611: Refactor duplicated ticket insertion code into a helper function.The code for generating and inserting tickets is duplicated. Extracting this logic into a helper function will improve code maintainability and reduce redundancy.
Consider adding a helper function like this:
async fn insert_tickets( db: &HoprDb, tickets: &mut Vec<AcknowledgedTicket>, counterparty: &ChainKeypair, epoch: u32, indices: &[u64], ) -> anyhow::Result<()> { for &index in indices { let ticket = generate_random_ack_ticket(index, counterparty, epoch)?; db.upsert_ticket(None, ticket.clone()).await?; tickets.insert(0, ticket); } Ok(()) }Then replace the duplicated code with:
// Insert another 2 tickets in Epoch 3 insert_tickets(&db, &mut tickets, &BOB, 3, &[0, 1]).await?;
675-681: Refactor duplicated ticket insertion code into a helper function.Similar to the previous suggestion, the code for generating and inserting tickets is duplicated here as well. Refactor this into a helper function to enhance code reuse and readability.
After adding the helper function, update the code:
// Insert another 2 tickets in Epoch 5 insert_tickets(&db, &mut tickets, &BOB, 5, &[0, 1]).await?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
chain/actions/Cargo.toml(1 hunks)chain/actions/src/redeem.rs(2 hunks)
🔇 Additional comments (3)
chain/actions/Cargo.toml (3)
19-19: LGTM! Runtime feature configuration looks good.
The addition of hopr-chain-rpc with specific runtime features ensures consistent async runtime selection across dependencies, which is crucial for nightly compatibility.
Also applies to: 24-24
Line range hint 27-31: LGTM! Prometheus feature configuration is consistent.
The addition of hopr-chain-rpc/prometheus aligns well with the existing prometheus integrations in other dependencies.
Line range hint 1-100: Verify impact of removing tracing-test dependency.
The AI summary indicates that tracing-test was removed from dev-dependencies. Let's verify if this affects the test fixes mentioned in the PR objectives.
✅ Verification successful
Removal of tracing-test dependency is safe
The git history shows that tracing-test was recently removed from dev-dependencies, and our searches found:
- No test files currently using tracing-test
- No remaining references to tracing-test in the codebase
- The dependency was only added in March 2024 and removed in this PR
Since it was a dev-dependency and there are no usages found, its removal does not impact the codebase or test functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if tracing-test is still used in tests and if there are alternative tracing test utilities
# Test 1: Check for any remaining usage of tracing-test in test files
echo "Checking for tracing-test usage in tests..."
rg -l 'use.*tracing_test' 'chain/actions/src/**/*test*.rs'
# Test 2: Check for alternative tracing test utilities
echo "Checking for alternative tracing test utilities..."
rg -l 'use.*tracing::' 'chain/actions/src/**/*test*.rs'
Length of output: 468
Script:
#!/bin/bash
# Let's first check if test files exist in the correct location
echo "Checking for test files..."
fd -t f ".*test.*\.rs$" chain/actions/src/
# Check for any tracing-test usage in the entire crate
echo "Checking for tracing-test usage in the crate..."
rg -l "tracing.test" chain/actions/
# Check Cargo.toml history for tracing-test
echo "Checking git history for tracing-test..."
git log -p chain/actions/Cargo.toml | grep -A 5 -B 5 "tracing-test"
Length of output: 1669
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
chain/actions/src/redeem.rs (2)
604-605: Improve comment clarity for epoch contextThe comment should clarify that these are tickets from a previous epoch, as it's important context for understanding the test's purpose.
-// Insert another 2 tickets in Epoch 3 +// Insert another 2 tickets from previous Epoch (3)
674-675: Improve comment clarity for epoch contextThe comment should clarify that these are tickets from a future epoch, as it's important context for understanding the test's purpose.
-// Insert another 2 tickets in Epoch 5 +// Insert another 2 tickets from next Epoch (5)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
chain/actions/src/redeem.rs(2 hunks)chain/indexer/src/block.rs(3 hunks)
🔇 Additional comments (4)
chain/actions/src/redeem.rs (1)
Line range hint 596-741: LGTM! Test cases properly verify epoch-based ticket redemption
The test cases effectively verify that:
- Tickets from previous epochs cannot be redeemed
- Tickets from future epochs cannot be redeemed
- Only tickets from the current epoch are eligible for redemption
This aligns well with the PR objective of fixing tests to run on Rust nightly.
chain/indexer/src/block.rs (3)
60-60: LGTM! Good improvement in type constraints.
The removal of the Clone trait bound from type parameter U is a positive change that:
- Simplifies the type constraints
- Removes unnecessary trait requirements
- Better aligns with Rust's trait minimization principles
Also applies to: 76-76
117-117: LGTM! Good use of Arc for thread-safe sharing.
Wrapping db_processor in Arc is a good improvement that:
- Enables safe concurrent access to the processor
- Provides efficient shared ownership across async tasks
- Eliminates the need for the
Clonetrait bound
Line range hint 486-553: Verify test coverage for finalized blocks.
The test test_indexer_should_pass_blocks_that_are_finalized appears to be testing critical functionality. Let's verify that all edge cases are covered.
✅ Verification successful
Let me gather more context about the test implementation and related code.
Let me gather more information about the block finalization logic to better understand the test coverage.
Based on the gathered context, I can now provide a final response about the test coverage for finalized blocks.
Test coverage for block finalization appears adequate
The test test_indexer_should_pass_blocks_that_are_finalized comprehensively verifies the block finalization logic by:
- Testing block finalization with both finalized and unfinalized blocks
- Verifying proper handling of block sequences (head block and previous block)
- Checking event collection for finalized blocks
- Validating the block processing order
The test covers the core finalization scenarios:
- Processing of finalized blocks (block_id: head_block - 1)
- Handling of head blocks that allow finalization
- Event collection from finalized blocks
- Block sequence validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other test cases related to block finalization
# Test: Search for test functions that verify block finalization logic
rg -A 2 "test.*finalized"
Length of output: 586
Script:
#!/bin/bash
# Get the test implementation and surrounding context
rg -B 5 -A 30 "test_indexer_should_pass_blocks_that_are_finalized"
# Check for other related test functions in the same file
ast-grep --pattern 'async fn test_$_($$$) $$$'
Length of output: 4224
Script:
#!/bin/bash
# Search for block finalization related code
rg -B 3 -A 10 "finalized|finalization" --type rust
# Look for other block-related tests
rg -B 2 -A 5 "async fn test.*block" --type rust
Length of output: 59621
Fixes the following tests
test_redeem_must_not_work_for_tickets_of_previous_epoch_being_aggregated_and_being_redeemed
test_redeem_must_not_work_for_tickets_of_next_epoch_being_redeemed
test_indexer_should_pass_blocks_that_are_finalized
Closes #6597