-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor TestGetNodeHeight #1545
Refactor TestGetNodeHeight #1545
Conversation
Warning Rate Limit Exceeded@Manav-Aggarwal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 10 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. WalkthroughThis update focuses on enhancing the test infrastructure for a node system by introducing a unified approach to node creation and configuration. The main goal is to simplify the test setup and make the codebase more maintainable by reducing redundancy and consolidating common setup tasks into single, reusable functions. Changes
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1545 +/- ##
==========================================
+ Coverage 50.23% 52.01% +1.77%
==========================================
Files 52 52
Lines 6762 5516 -1246
==========================================
- Hits 3397 2869 -528
+ Misses 3024 2321 -703
+ Partials 341 326 -15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR still includes the commit which has the start stop logic changes.
either we should mark this PR as draft and dependent on #1544 or we should remove that commit and just review the changes to TestGetNodeHeight
I created this branch on top of #1544 branch, to avoid rebase conflicts later on 😅 . Poor choice on my part. I'll remove that first commit, and push only relevant changes here. |
c2d8c86
to
f774c26
Compare
makes sense. since the changes can be pretty cleanly separated I don't suspect there to be much in terms of conflict. looks good now! |
There might be an unrelated flaky test https://github.com/rollkit/rollkit/actions/runs/7891260736/job/21536776255?pr=1545#step:4:2712 cc @Manav-Aggarwal have we seen this one yet? EDIT: |
f774c26
to
5da92ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- node/full_node_integration_test.go (3 hunks)
- node/helpers_test.go (2 hunks)
Additional comments: 4
node/helpers_test.go (2)
- 48-48: The function
createAndConfigureNode
is used here but its definition or modification is not visible within this file. Ensure its implementation correctly handles the newdalc
parameter and other setup logic as intended.- 60-84: Refactoring test cases into a loop over different cases improves readability and maintainability. Ensure that each test case is independent and does not affect the others due to shared state or side effects.
node/full_node_integration_test.go (2)
- 548-549: Usage of
createAndConfigureNode
in test setup is consistent with the objective to streamline node creation and configuration. Verify that thedalc
parameter is correctly utilized within this function to configure the nodes as intended.- 723-730: The implementation of
createAndConfigureNode
is provided here. Ensure that this function correctly encapsulates the creation and configuration logic, including setting thedalc
for both theFullNode
and itsblockManager
.
This was a flaky test on main that was reverted: #1521, so rebasing should fix it. |
5da92ad
to
a007130
Compare
Overview
I've refactored the testcases in
TestGetNodeHeight
into a struct-list format for a cleaner testing interface. And taken out theSetDalc
pattern, repeated in the codes, out to a separate helper function.Checklist
Summary by CodeRabbit