test(gov): Migrate e2e to system test #21927
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces system tests for the governance module of a blockchain application, validating proposal submissions, voting mechanisms, and deposit management. It modifies the genesis block configuration to set a short voting period. Additionally, it removes existing end-to-end tests related to governance, consolidating the testing framework to focus on system tests. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (4)
tests/systemtests/system.go (1)
135-139: Governance voting period set to a short duration.The change introduces a short voting period of 8 seconds for the governance module in the genesis configuration. This aligns with the PR objective of converting e2e tests to system tests, as it allows for faster testing of governance-related functionalities.
However, there are a few points to consider:
- The error message in the panic is incorrect. It mentions "failed set block max gas" instead of something related to the voting period.
- The hard-coded duration might make the tests less flexible. Consider making this value configurable or using a constant.
Consider applying the following improvements:
- Fix the error message:
- panic(fmt.Sprintf("failed set block max gas: %s", err)) + panic(fmt.Sprintf("failed to set governance voting period: %s", err))
- Use a constant for the voting period:
+const defaultTestVotingPeriod = "8s" - genesisBz, err = sjson.SetRawBytes(genesisBz, "app_state.gov.params.voting_period", []byte(fmt.Sprintf(`"%s"`, "8s"))) + genesisBz, err = sjson.SetRawBytes(genesisBz, "app_state.gov.params.voting_period", []byte(fmt.Sprintf(`"%s"`, defaultTestVotingPeriod)))These changes will improve error handling and make the code more maintainable.
tests/systemtests/gov_test.go (3)
220-220: Remove unnecessary debuggingfmt.Printlnstatement.The line
fmt.Println("gotOut", gotOutputs)appears to be leftover debugging code. Removing it will clean up the test output and adhere to best practices.Apply this diff to remove the print statement:
- fmt.Println("gotOut", gotOutputs)
269-269: Remove unnecessaryfmt.Printlnstatement.The
fmt.Println("first proposal", proposal1)statement is unnecessary for the test's functionality and may clutter the output. Consider removing it to keep the test output clean.Apply this diff to remove the print statement:
- fmt.Println("first proposal", proposal1)
323-323: Clarify test case name for better readability.The test case name
"invalid valid split vote string"is confusing. Renaming it to"invalid split vote string"improves clarity and accurately reflects the test scenario.Apply this diff to correct the test case name:
- "invalid valid split vote string", + "invalid split vote string",
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/systemtests/gov_test.go (1 hunks)
- tests/systemtests/system.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/systemtests/gov_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"tests/systemtests/system.go (2)
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"
🔇 Additional comments (1)
tests/systemtests/gov_test.go (1)
327-327:⚠️ Potential issueCorrect the invalid split vote string format.
The split vote string
"yes/0.6,no/0.3,abstain/0.05,no_with_veto/0.05"uses/instead of=to separate options and weights, which is invalid. The correct format should use=. This misformatting is intentional for the test case; however, ensure that the error message accurately reflects this.Apply this diff if the misformatting was unintentional:
- "yes/0.6,no/0.3,abstain/0.05,no_with_veto/0.05", + "yes=0.6,no=0.3,abstain=0.05,no_with_veto=0.05",Likely invalid or redundant comment.
| { | ||
| "invalid proposal", | ||
| []string{ | ||
| "tx", "gov", "submit-proposal", | ||
| invalidPropFile.Name(), | ||
| fmt.Sprintf("--%s=%s", flags.FlagFrom, valAddr), | ||
| fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), | ||
| fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), | ||
| fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(10))).String()), | ||
| }, | ||
| true, | ||
| "invalid character in coin string", | ||
| }, | ||
| { | ||
| "valid proposal", | ||
| []string{ | ||
| "tx", "gov", "submit-proposal", | ||
| validPropFile.Name(), | ||
| fmt.Sprintf("--%s=%s", flags.FlagFrom, valAddr), | ||
| fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), | ||
| fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), | ||
| fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(10))).String()), | ||
| }, | ||
| false, | ||
| "", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage by adding more test cases.
Currently, the TestSubmitProposal function includes only one valid and one invalid proposal test case. To ensure comprehensive coverage, consider adding additional test cases, such as:
- Proposals with missing required fields (e.g., missing
descriptionordeposit). - Proposals with invalid or malformed metadata.
- Proposals with oversized deposit amounts to test boundary conditions.
- Proposals with invalid message types or unsupported content.
| time.Sleep(time.Second * 8) | ||
| resp = cli.CustomQuery("q", "gov", "deposits", "1") | ||
| deposits = gjson.Get(resp, "deposits").Array() | ||
| require.Equal(t, len(deposits), 0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using time.Sleep in tests; implement a dynamic wait mechanism.
Using time.Sleep(time.Second * 8) can make tests slower and less reliable due to fixed wait times. Instead, implement a polling mechanism that periodically checks if the deposits have been cleared, up to a maximum timeout. This approach reduces test duration and improves reliability.
tests/systemtests/gov_test.go
Outdated
| "deposit": "-324foocoin" | ||
| }` | ||
| invalidPropFile := testutil.WriteToNewTempFile(t, invalidProp) | ||
| os.WriteFile("test", []byte(invalidProp), 0o600) |
There was a problem hiding this comment.
Remove unnecessary file write operation to 'test'.
The call to os.WriteFile("test", []byte(invalidProp), 0o600) is unnecessary because invalidProp is already written to invalidPropFile. Additionally, writing to a hardcoded file named "test" may cause conflicts or unintended side effects. Consider removing this line to clean up the code.
Apply this diff to remove the unnecessary file write:
- os.WriteFile("test", []byte(invalidProp), 0o600)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.WriteFile("test", []byte(invalidProp), 0o600) |
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit d493145)
* main: docs: amend docs for 52 changes (#21992) test: migrate e2e/authz to system tests (#21819) refactor(runtime/v2): use StoreBuilder (#21989) feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482) docs: add instructions to change DefaultGenesis (#21680) feat(x/staking)!: Add metadata field to validator info (#21315) chore(x/authz)!: Remove account keeper dependency (#21632) chore(contributing): delete link (#21990) test(gov): Migrate e2e to system test (#21927) test: e2e/client to system tests (#21981)
Description
Closes: #21872
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
Chores