chore(systemtest): More stable fraud test #21949
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📥 CommitsFiles that changed from the base of the PR and between b40c7c2e62e126f2834cd248f51c7bfa679ff4e2 and f89e6ab. 📒 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/fraud_test.go (2)
45-47: Approved: Changes improve test stabilityThe modifications in these lines enhance the test's reliability:
- Increasing the loop count from 20 to 30 allows more time for the validator power to update.
- Initializing
nodePowerAfterto -1 provides a clear initial state.These changes align well with the PR objective of improving the stability of the fraud test.
Consider adding a comment explaining the significance of the -1 initial value for
nodePowerAfter, e.g.:// Initialize nodePowerAfter to -1 to distinguish from the case where power is 0 var nodePowerAfter int64 = -1
49-52: Approved: Improved code clarity and observabilityThe changes in these lines enhance the test's readability and debuggability:
- Moving the
nodePowerAfterassignment into the if condition simplifies the code.- Adding a log statement for the current height improves visibility into the test progress.
These modifications contribute positively to the overall code quality and align with the PR's objectives.
For consistency with the Uber Go Style Guide, consider using
strconv.Itoa()instead of%dfor integer to string conversion in the log statement:t.Logf("wait %s", strconv.Itoa(sut.CurrentHeight()))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between dc09671 and b40c7c2e62e126f2834cd248f51c7bfa679ff4e2.
📒 Files selected for processing (1)
- tests/systemtests/fraud_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/fraud_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
tests/systemtests/fraud_test.go (2)
Line range hint
1-63: Overall assessment: Changes improve test stability and clarityThe modifications in this file successfully address the PR objectives:
- The test's stability is improved by allowing more time for validator power updates.
- Code redundancies have been removed, particularly in the block confirmation process.
- Observability has been enhanced with additional logging.
These changes should result in a more reliable and maintainable fraud test. The suggestions provided in the review comments are minor and aimed at further improving code quality and consistency.
To ensure comprehensive test coverage, consider adding the following test cases if they're not already present elsewhere:
- Test with multiple validators to ensure the correct validator is jailed.
- Test with different network conditions to verify the robustness of the updated timing.
- Verify that a validator can't rejoin the active set after being jailed for double-signing.
62-63: Approved: Streamlined block confirmation processThe changes in these lines improve the test's efficiency and clarity:
- Replacing the loop with a single
AwaitNBlockscall simplifies the code.- Reducing the wait from 10 to 5 blocks could potentially speed up the test.
These modifications align well with the PR's goal of removing redundancies and improving test stability.
To ensure this change doesn't affect the test's reliability, please verify that waiting for 5 blocks is sufficient for all necessary updates to propagate. You can run the following command to check for any timing-related issues in recent test runs:
If no issues are found, the change is likely safe. Otherwise, consider reverting to 10 blocks or finding an optimal number through experimentation.
alpe
force-pushed
the
alex/chore_fraud_test
branch
from
b40c7c2 to
f89e6ab
Compare
|
@mergify backport release/v0.52.x |
✅ Backports have been created
|
(cherry picked from commit dcf00cf)
Co-authored-by: Alexander Peters <[email protected]>
Description
Minor update to have longer pull for validator status and some redundancies removed
Summary by CodeRabbit