test: add x/circuit system tests #22331
Conversation
akhilkumarpilli
changed the title
akhilkumarpilli
requested review from
kocubinski,
raynaudoe and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new system test file, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)
🔇 Additional comments (2)
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
|
akhilkumarpilli
requested review from
alpe,
tac0turtle and
julienrbrt
and removed request for
kocubinski and
raynaudoe
akhilkumarpilli
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/circuit_test.go (2)
28-32: Check for empty validator addresses before proceeding.While
require.NotEmpty(t, superAdmin)ensures the addresses are not empty, consider adding explicit error messages for clearer debugging if the test fails.Enhance the
requirestatements with custom error messages:- require.NotEmpty(t, superAdmin) + require.NotEmpty(t, superAdmin, "superAdmin address should not be empty")- require.NotEmpty(t, superAdmin2) + require.NotEmpty(t, superAdmin2, "superAdmin2 address should not be empty")
131-134: Simplify permission JSON construction to avoid redundancy.The construction of
permissionJSONcan be streamlined for better readability.Refactor the code as follows:
permission := map[string]interface{}{ "level": tc.level, } if len(tc.limitTypeURLs) != 0 { permission["limit_type_urls"] = tc.limitTypeURLs } else { permission["limit_type_urls"] = []string{} } permissionJSONBytes, err := json.Marshal(permission) require.NoError(t, err) permissionJSON := string(permissionJSONBytes)This ensures proper JSON formatting and handles empty slices correctly.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/systemtests/circuit_test.go (1 hunks)
- x/circuit/autocli.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/systemtests/circuit_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"x/circuit/autocli.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
x/circuit/autocli.go (1)
46-46: Approve: Corrected JSON string formatting in exampleThe change correctly fixes the formatting of the JSON string in the
Examplefield. This is important because:
- It resolves a potential parsing issue that could occur with the previous incorrect string concatenation.
- It provides a valid example for users of the
authorizecommand, which is crucial for proper usage.- It aligns with the expected format for the
permissionsparameter.The modification adheres to the Uber Golang style guide and improves the overall quality of the documentation.
tests/systemtests/circuit_test.go (1)
86-90: Verify that proposal votes are successfully included in blocks.Ensure that the voting transactions are included in blocks and not only accepted by the CLI.
Run the following script to confirm that the votes are committed:
This script queries the blocks to confirm that the vote transactions were successfully included.
There was a problem hiding this comment.
lgtm! @coderabbitai comments look like a valid improvements btw.
(cherry picked from commit 33e3231)
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Description
Closes: #22301
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Examplefield of theAuthorizeCircuitBreakermethod to prevent parsing issues.