Problem: iavl rollback not tested #1693
Conversation
yihuang
requested review from
JayT106 and
thomas-nguy
and removed request for
a team
WalkthroughThe pull request modifies the configuration of validators in Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@yihuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 13 seconds before requesting another review. ⌛ 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. 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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1693 +/- ##
===========================================
- Coverage 35.47% 11.63% -23.85%
===========================================
Files 124 72 -52
Lines 11893 6163 -5730
===========================================
- Hits 4219 717 -3502
+ Misses 7251 5395 -1856
+ Partials 423 51 -372 |
![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
🧹 Outside diff range and nitpick comments (1)
integration_tests/test_rollback.py (1)
85-107: Consider enhancing error reporting in the testWhile the test logic is thorough, consider adding more detailed error messages to help diagnose failures. For example:
- assert cli.block_height() == 10 + height = cli.block_height() + assert height == 10, f"Expected node {i} to be stuck at height 10, but got height {height}"The overall test implementation is well-structured and properly tests the rollback functionality for both memiavl and iavl nodes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
integration_tests/configs/rollback.jsonnet(1 hunks)integration_tests/test_rollback.py(3 hunks)
🔇 Additional comments (5)
integration_tests/configs/rollback.jsonnet (2)
6-18: LGTM! Well-structured validator configurations for rollback testing.
The configuration properly sets up two validators with opposite memiavl settings, enabling comprehensive rollback testing for both memiavl and traditional iavl scenarios. The naming is clear and purpose-driven.
6-18: Verify integration with test files.
Let's verify that the test files correctly reference these new validator configurations.
✅ Verification successful
Let me gather more information about the test files to ensure proper integration.
Integration with test files is properly configured
The verification confirms proper integration:
- The test file
test_rollback.pycorrectly loads the configuration fromrollback.jsonnet - The test's docstring explicitly documents the validator roles: "node2: test memiavl node, node3: test iavl node"
- The
rollback.jsonnetconfiguration properly defines both validators with correct memiavl settings - There are no lingering references to old configurations
The new validator configurations are properly referenced and integrated within the test infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that test files correctly reference the new validator names
# and that there are no lingering references to the old 'fullnode' validator
# Check for references to the new validator names
echo "Checking references to new validator names:"
rg -l "rollback-test-(mem)?iavl" integration_tests/
# Check for any remaining references to the old validator name
echo "Checking for lingering references to 'fullnode':"
rg -l "fullnode" integration_tests/
# Look for the test file mentioned in the summary
echo "Checking test_rollback.py contents:"
rg -A 5 "update_node_cmd" integration_tests/test_rollback.py
Length of output: 1112
Script:
#!/bin/bash
# Check the content of test_rollback.py to see how validators are referenced
echo "Checking test_rollback.py for validator configuration:"
rg -A 10 "def test_rollback" integration_tests/test_rollback.py
# Check the content of rollback.jsonnet to understand the complete configuration
echo "Checking complete rollback.jsonnet configuration:"
cat integration_tests/configs/rollback.jsonnet
# Look for any test configuration loading or parsing
echo "Checking for configuration loading in test files:"
rg -A 5 "rollback.jsonnet" integration_tests/
Length of output: 1586
integration_tests/test_rollback.py (3)
Line range hint 15-28: Well-structured generalization of the node command update function!
Good improvement in making the function reusable across different nodes. The implementation correctly handles configuration updates and properly manages the supervisor settings.
34-35: Good addition of iavl node testing!
The initialization of both memiavl (node2) and iavl (node3) nodes aligns well with the PR's objective to test iavl rollback functionality.
70-74: Clear documentation and efficient test setup!
Good job on:
- Clearly documenting the role of each node (memiavl vs iavl)
- Using efficient dictionary comprehension for CLI setup
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: yihuang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
integration_tests/test_rollback.py (2)
34-35: Consider using a loop for better maintainability.While the implementation is correct, consider using a loop to make it more maintainable:
- update_node_cmd(path / chain_id, broken_binary, 2) - update_node_cmd(path / chain_id, broken_binary, 3) + for node_id in [2, 3]: + update_node_cmd(path / chain_id, broken_binary, node_id)
85-107: Consider extracting magic numbers as constants.The implementation correctly handles rollback and recovery for multiple nodes. However, consider extracting the block heights (10, 13, 15) as named constants for better maintainability:
+# At the top of the file +STUCK_BLOCK_HEIGHT = 10 +HEALTHY_BLOCK_HEIGHT = 13 +RECOVERY_BLOCK_HEIGHT = 15 def test_rollback(custom_cronos): # ... - wait_for_block(cli, 10) + wait_for_block(cli, STUCK_BLOCK_HEIGHT) # ... - wait_for_block(cli, 13) + wait_for_block(cli, HEALTHY_BLOCK_HEIGHT) # ... - wait_for_block(cli, 15) + wait_for_block(cli, RECOVERY_BLOCK_HEIGHT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
integration_tests/test_rollback.py(3 hunks)
🔇 Additional comments (3)
integration_tests/test_rollback.py (3)
Line range hint 15-28: LGTM! Good function generalization.
The function rename from update_node2_cmd to update_node_cmd improves code reusability and makes the implementation more generic by accepting the node index as a parameter.
70-74: LGTM! Clear documentation and well-structured setup.
The documentation clearly explains the role of each node (memiavl vs iavl), and the setup code is well organized with proper initialization of node list and CLI dictionary.
75-78: LGTM! Proper synchronization handling for multiple nodes.
The implementation correctly handles port waiting and block synchronization for multiple nodes with proper logging.
f1ee213
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor