refactor(systemtests): Extract system test framework #22578
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve the introduction of a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
|
tests/systemtests/go.mod
Outdated
There was a problem hiding this comment.
Can we delete this go.mod? So we only have the testing go.mod from tests/ ?
There was a problem hiding this comment.
uf, removing the go.mod creates some mean side-effects. The tests/go.mod has a replace statement for the sdk in the go.mod which makes sense to run the tests with the latest versions. But for the systemtests framework, stability is key so it has the sdk and comet versions pinned.
Now I have the situation, that the systemtests framework is fine while the tests/systemtests are not compiling due to breaking changes in the different sdk + comet version.
Do you have any idea now to address this?
There was a problem hiding this comment.
then maybe keeping the go.mod is best yeah. This does mean everyone that uses systemtests with SDK > 0.50 need to do that. If it is added in the docs that the system tests should be in another go.mod I think it is fine.
Let's kill the vanity url from that go.mod however and just call it github.com/cosmos/cosmos-sdk/tests/systemtests.
* main: fix(server/v2/api/telemetry): enable global metrics (#22571)
* main: feat(serverv2): add benchmarks of (old) cacheKV vs branch (#22497) fix: match comets bls implmentation (#22613) feat(math): Upstream GDA based decimal type (#21982) fix(client/v2/autocli): prevent duplicate addition of customCommands (#22576) build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580)
alpe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (58)
tests/systemtests/README.md (5)
3-5: Consider using absolute links for better maintainabilityReplace relative paths with absolute GitHub links to ensure they remain valid even if the directory structure changes. This makes the documentation more resilient to reorganization.
-Go black box tests that setup and interact with a local blockchain. The system test [framework](../../systemtests) +Go black box tests that setup and interact with a local blockchain. The system test [framework](/systemtests) -To get up to speed, checkout the [getting started guide](../../systemtests/getting_started.md). +To get up to speed, checkout the [getting started guide](/systemtests/getting_started.md).
7-7: Fix grammar: "Beside" should be "Besides"The word "Beside" (meaning next to) should be replaced with "Besides" (meaning in addition to) for correct grammar.
-Beside the Go tests and testdata files, this directory can contain the following directories: +Besides the Go tests and testdata files, this directory can contain the following directories:
12-12: Enhance git ignore instructionsThe warning about git could be more specific by mentioning
.gitignore.-Please make sure to not add or push them to git. +Please make sure these directories are in your .gitignore file and not committed to git.
16-16: Fix grammar: Add missing articleAdd "the" for better grammar.
-Build a new binary from current branch and copy it to the `tests/systemtests/binaries` folder by running system tests. +Build a new binary from the current branch and copy it to the `tests/systemtests/binaries` folder by running system tests.🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ... ## Execution Build a new binary from current branch and copy it to the `tests/system...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
43-48: Fix markdown formattingThe numbered list should be surrounded by blank lines, and the file should end with a single newline character.
To circumvent this limitation locally: 1. Checkout and build the older version of the artifact from a specific tag for your OS. 2. Place the built artifact into the `binaries` folder. 3. Ensure that the filename, including the version, is correct. With the cached artifact in place, the test will use this file instead of attempting to pull it from Docker.🧰 Tools
🪛 Markdownlint (0.35.0)
44-44: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
48-48: null
Files should end with a single newline character(MD047, single-trailing-newline)
tests/systemtests/bankv2_test.go (1)
Line range hint
54-54: Address the TODO comment about DeductFee ante handlerThe comment indicates a pending implementation for the DeductFee ante handler with bank/v2.
Would you like me to help create an issue to track the implementation of the DeductFee ante handler for bank/v2?
tests/systemtests/unordered_tx_test.go (2)
17-20: Enhance the skip message for better clarityConsider providing more context in the skip message to help developers understand why unordered tx handling isn't wired in v2.
- t.Skip("The unordered tx handling is not wired in v2") + t.Skip("Skipping test: Unordered transaction handling is not implemented in v2 of the chain")
40-52: Consider enhancing test coverageWhile the test effectively covers the happy and error paths, consider adding these scenarios:
- Test with different timeout values
- Verify account1's balance after transaction
- Add assertions for transaction fees deduction
This would provide more comprehensive coverage of the unordered transaction handling.
Example addition:
// Verify account1's final balance (initial - sent amount - fees) require.Eventually(t, func() bool { expectedBalance := 10000000 - 5000 - 1 // initial - sent - fees return cli.QueryBalance(account1Addr, "stake") == expectedBalance }, 10*systest.Sut.BlockTime(), 200*time.Millisecond, "Unexpected final balance for sender")tests/systemtests/fraud_test.go (2)
23-25: LGTM: Clean migration to systest package with clear setupThe test setup maintains clarity while transitioning to the new framework. The scenario documentation is helpful.
Consider enhancing the test documentation by adding expected outcomes for each step:
// Scenario: // given: a running chain // when: a second instance with the same val key signs a block -// then: the validator is removed from the active set and jailed forever +// then: +// - the validator is removed from the active set (power = 0) +// - the validator is jailed forever +// - the chain continues to produce blocks
37-51: Consider refactoring for improved maintainabilityThe implementation could benefit from several improvements:
- Error handling is suppressed for file operations
- Magic number 30 in the polling loop
- Complex polling logic could be encapsulated
Consider these improvements:
+const maxPollAttempts = 30 // At package level + +// Helper function +func awaitValidatorPowerChange(t *testing.T, rpc *http.Client, pkBz []byte, targetPower int64) { + for i := 0; i < maxPollAttempts; i++ { + systest.Sut.AwaitNextBlock(t) + if power := systest.QueryCometValidatorPower(rpc, pkBz); power == targetPower { + return + } + t.Logf("wait %d", systest.Sut.CurrentHeight()) + } +} func TestValidatorDoubleSign(t *testing.T) { // ... existing setup ... - for i := 0; i < 30; i++ { - systest.Sut.AwaitNextBlock(t) - if nodePowerAfter = systest.QueryCometValidatorPower(rpc, pkBz); nodePowerAfter == 0 { - break - } - t.Logf("wait %d", systest.Sut.CurrentHeight()) - } + awaitValidatorPowerChange(t, rpc, pkBz, 0) + nodePowerAfter = systest.QueryCometValidatorPower(rpc, pkBz)systemtests/rest_support.go (2)
23-31: LGTM! Consider adding input validationThe change to variadic parameters improves usability. The function is well-structured with proper test helper marking and error handling.
Consider adding validation for empty test cases:
func RunRestQueries(t *testing.T, testCases ...RestTestCase) { t.Helper() + if len(testCases) == 0 { + t.Fatal("no test cases provided") + } for _, tc := range testCases {
Line range hint
37-59: Consider performance optimizationsWhile the function works correctly, there are opportunities for performance improvements:
- Compile the regex pattern once at package level:
+var numberRegex = regexp.MustCompile(`"\d+(\.\d+)?"`) + func TestRestQueryIgnoreNumbers(t *testing.T, testCases ...RestTestCase) { t.Helper() for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { resp, err := testutil.GetRequest(tc.Url) require.NoError(t, err) - - // regular expression pattern to match any numeric value in the JSON - numberRegexPattern := `"\d+(\.\d+)?"` - - // compile the regex - r, err := regexp.Compile(numberRegexPattern) - require.NoError(t, err) // replace all numeric values with `NUMBER` text - expectedJSON := r.ReplaceAllString(tc.ExpOut, `"NUMBER"`) - actualJSON := r.ReplaceAllString(string(resp), `"NUMBER"`) + expectedJSON := numberRegex.ReplaceAllString(tc.ExpOut, `"NUMBER"`) + actualJSON := numberRegex.ReplaceAllString(string(resp), `"NUMBER"`)tests/systemtests/staking_test.go (2)
22-28: Consider adding error handling for CLI operationsThe CLI operations could benefit from explicit error handling to make test failures more descriptive and easier to debug.
Example improvement:
- account1Addr := cli.AddKey("account1") + account1Addr, err := cli.AddKey("account1") + if err != nil { + t.Fatalf("failed to add key: %v", err) + }
39-39: Consider expanding test coverageWhile the test covers the basic staking workflow, consider adding test cases for:
- Invalid delegation amounts
- Multiple delegations/undelegations
- Slashing scenarios
Would you like me to help create additional test cases for these scenarios?
Also applies to: 57-57
tests/systemtests/export_test.go (1)
54-55: Consider additional error test casesWhile the test covers basic success and invalid home directory cases, consider adding test cases for:
- File permission issues
- Invalid file paths
- Existing file overwrite scenarios
Also applies to: 58-58, 61-61, 63-63, 72-72
tests/systemtests/mint_test.go (2)
Line range hint
21-33: Consider enhancing error handling in genesis modificationsWhile the genesis modifications work correctly, consider extracting the error handling into a helper function to improve readability and reusability.
+func setGenesisValue(t *testing.T, genesis []byte, path, value string) []byte { + state, err := sjson.Set(string(genesis), path, value) + require.NoError(t, err) + return []byte(state) +} systest.Sut.ModifyGenesisJSON(t, - func(genesis []byte) []byte { - state, err := sjson.Set(string(genesis), "app_state.mint.minter.inflation", "1.00") - require.NoError(t, err) - return []byte(state) - }, - func(genesis []byte) []byte { - state, err := sjson.Set(string(genesis), "app_state.mint.params.inflation_max", "1.00") - require.NoError(t, err) - return []byte(state) - }, + func(genesis []byte) []byte { + return setGenesisValue(t, genesis, "app_state.mint.minter.inflation", "1.00") + }, + func(genesis []byte) []byte { + return setGenesisValue(t, genesis, "app_state.mint.params.inflation_max", "1.00") + }, )
43-45: Track TODOs in issue trackerThere are two important TODOs that should be tracked:
- Investigate value differences when querying with height between v1 and v2 (ref: [Bug]: Inconsistent annual-provisions values between v1 and v2 with
--heightflag #22302)- Implementation of gRPC gateway in v2
Would you like me to create GitHub issues to track these TODOs? This will help ensure these items aren't forgotten and can be properly prioritized.
Also applies to: 83-85
tests/systemtests/snapshots_test.go (1)
Line range hint
1-100: Good architectural direction with framework extractionThe separation of the system test framework from the actual tests is a positive architectural change that will:
- Allow independent evolution of the testing framework
- Prevent tests from depending on internal framework logic
- Improve maintainability through clear separation of concerns
Consider documenting the framework's public API and usage patterns to help other teams adopt it effectively.
systemtests/test_runner.go (2)
17-18: Consider encapsulating global stateWhile exporting these variables aligns with the framework extraction, having global mutable state could lead to issues with concurrent test execution. Consider encapsulating this state within a test context object that can be passed around.
Example approach:
type TestContext struct { Sut *SystemUnderTest Verbose bool } func NewTestContext(sut *SystemUnderTest, verbose bool) *TestContext { return &TestContext{ Sut: sut, Verbose: verbose, } }
67-67: Consider refactoring getters as part of state encapsulationThe changes are correct, but if you implement the suggested TestContext encapsulation, these getters could be methods on that struct instead of package-level functions.
Example approach:
func (tc *TestContext) GetSystemUnderTest() *SystemUnderTest { return tc.Sut } func (tc *TestContext) IsVerbose() bool { return tc.Verbose }Also applies to: 71-71
tests/systemtests/upgrade_test.go (1)
90-94: Consider expanding post-upgrade smoke testsWhile the current smoke tests verify basic functionality, consider adding more comprehensive checks:
- Verify state persistence across upgrade
- Check upgrade-specific changes
- Validate chain parameters
tests/systemtests/group_test.go (3)
48-51: Consider enhancing error handling for file operationsWhile the transition to
systest.StoreTempFileis good, consider adding cleanup of temporary files usingt.Cleanup()to ensure proper resource management.+ t.Cleanup(func() { + _ = os.Remove(validMembersFile.Name()) + })
85-88: LGTM: Consider test isolation improvementThe percentage policy test implementation looks good. Consider extracting this into a separate sub-test using
t.Run()for better test isolation and clarity.
127-130: Consider extracting V2 check logicThe V2 conditional checks are scattered throughout the test. Consider extracting this logic into a helper function to improve maintainability.
+ func getVoteResponse(t *testing.T, cli *systest.CLIWrapper, baseurl, proposalId, valAddr string) string { + if systest.IsV2() { + return cli.CustomQuery("q", "group", "vote", proposalId, valAddr) + } + return string(systest.GetRequest(t, fmt.Sprintf("%s/cosmos/group/v1/vote_by_proposal_voter/%s/%s", + baseurl, proposalId, valAddr))) + }tests/systemtests/account_test.go (1)
Line range hint
58-157: Consider breaking down the TestAccountsMigration into smaller, focused test functionsThe test covers multiple distinct functionalities (migration, transactions, key swapping) in a single test function. Consider splitting it into separate test functions for better maintainability and clarity:
TestAccountMigrationBasicTestMigratedAccountTransactionsTestMigratedAccountKeySwapThis would improve:
- Test failure isolation
- Code readability
- Maintenance of test cases
systemtests/testnet_init.go (2)
16-20: Enhance documentation and robustness of exported functionSince
IsV2is now an exported function, it should have a more comprehensive documentation that follows Go standards. Additionally, consider making the version check more robust.Consider this improvement:
-// IsV2 checks if the tests run with simapp v2 +// IsV2 determines if the tests are running with simulation app version 2. +// It checks the COSMOS_BUILD_OPTIONS environment variable for the "v2" flag. +// Returns true if v2 is enabled, false otherwise. func IsV2() bool { buildOptions := os.Getenv("COSMOS_BUILD_OPTIONS") - return strings.Contains(buildOptions, "v2") + return strings.Contains(strings.ToLower(buildOptions), "v2") }
Line range hint
139-143: Consider extracting duplicated version-specific flag logicThis code block is identical to the one in
SingleHostTestnetCmdInitializer.Initialize(). Consider extracting it to a helper function to avoid duplication.Here's a suggested implementation:
+// getGasPricesFlag returns the appropriate minimum gas prices flag based on version +func getGasPricesFlag(price string) string { + if IsV2() { + return "--server.minimum-gas-prices=" + price + } + return "--minimum-gas-prices=" + price +} func (s ModifyConfigYamlInitializer) Initialize() { // ... existing code ... - if IsV2() { - args = append(args, "--server.minimum-gas-prices="+s.minGasPrice) - } else { - args = append(args, "--minimum-gas-prices="+s.minGasPrice) - } + args = append(args, getGasPricesFlag(s.minGasPrice))This helper function could then be used in both initializer types.
tests/systemtests/circuit_test.go (2)
38-42: Consider extracting voting period as a package-level constantThe voting period configuration looks good, but since it's used in multiple places and represents a test configuration value, consider extracting it as a package-level constant for better maintainability.
+ const defaultVotingPeriod = 5 * time.Second func TestCircuitCommands(t *testing.T) { - votingPeriod := 5 * time.Second + votingPeriod := defaultVotingPeriod
233-242: Consider extracting the fee amount as a constantThe transaction execution and verification logic looks good, but the hardcoded fee amount
"--fees=2stake"should be extracted as a named constant for better maintainability and clarity.+ const defaultTxFee = "2stake" func testCircuitTxCommand(...) { - tx = append(tx, "--fees=2stake") + tx = append(tx, "--fees=" + defaultTxFee)systemtests/getting_started.md (8)
5-18: Improve preparation section clarityThe preparation instructions could be clearer about prerequisites and expected outcomes:
- Add information about required Go version and other dependencies
- Explain what the binary is used for
- Clarify the significance of the
systemtests/binariesdirectory
24-26: Fix grammatical issues in the test file introductionThe sentence structure needs improvement:
-If there is no test file matching your use case, start a new test file here. -for example `bank_test.go` to begin with: +If there is no test file matching your use case, start a new test file here, +for example, `bank_test.go`:🧰 Tools
🪛 LanguageTool
[typographical] ~25-~25: After the expression ‘for example’ a comma is usually used.
Context: ...e case, start a new test file here. for examplebank_test.goto begin with: ```go //...(COMMA_FOR_EXAMPLE)
36-44: Add documentation for test setup functionsThe example code uses
sut.ResetChainandsut.StartChainwithout explaining their purpose. Consider adding comments to clarify:
- What
sutrepresents- The purpose of
ResetChain- What
StartChaininitializes
51-55: Fix sentence structure and add missing detailsThe CLI wrapper explanation needs improvement:
-In this example we want to execute `simd q bank total-supply --output json --node tcp://localhost:26657` which queries -the bank module. -Then print the result to for the next steps +In this example, we execute `simd q bank total-supply --output json --node tcp://localhost:26657` to query +the bank module. +The result is then printed for analysis in the next steps.🧰 Tools
🪛 LanguageTool
[typographical] ~52-~52: It appears that a comma is missing.
Context: ...r to interact or parse results. In this example we want to execute `simd q bank total-s...(DURING_THAT_TIME_COMMA)
92-97: Improve GJSON documentation clarityThe GJSON explanation could be enhanced:
-When we have a json response, the [gjson](https://github.com/tidwall/gjson) lib can shine. It comes with jquery like -syntax that makes it easy to navigation within the document. - -For example `gjson.Get(raw, "supply").Array()` gives us all the childs to `supply` as an array. +When working with JSON responses, the [gjson](https://github.com/tidwall/gjson) library provides jQuery-like +syntax that simplifies document navigation. + +For example, `gjson.Get(raw, "supply").Array()` returns all child elements of `supply` as an array.🧰 Tools
🪛 LanguageTool
[typographical] ~95-~95: After the expression ‘for example’ a comma is usually used.
Context: ...to navigation within the document. For examplegjson.Get(raw, "supply").Array()give...(COMMA_FOR_EXAMPLE)
[style] ~97-~97: Consider a shorter alternative to avoid wordiness.
Context: ...ount of the stake token as int64 type. In order to test our assumptions in the system test...(IN_ORDER_TO_PREMIUM)
131-135: Improve readability of genesis modification sectionThe introduction to genesis modification needs restructuring:
-First step is to disable inflation. This can be done via the `ModifyGenesisJSON` helper. But to add some complexity, -we also introduce a new token and update the balance of the account for key `node0`. -The setup code looks quite big and unreadable now. Usually a good time to think about extracting helper functions for -common operations. +The first step is to disable inflation using the `ModifyGenesisJSON` helper. To demonstrate more complex modifications, +we'll also introduce a new token and update the balance of the `node0` account. + +While the setup code becomes more complex, this is typically a good opportunity to extract helper functions for +common operations.🧰 Tools
🪛 LanguageTool
[style] ~133-~133: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ...t for keynode0. The setup code looks quite big and unreadable now. Usually a good time...(EN_WEAK_ADJECTIVE)
[typographical] ~133-~133: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...ode looks quite big and unreadable now. Usually a good time to think about extracting h...(RB_LY_COMMA)
184-187: Fix typo and improve clarity in transaction sectionThe transaction section contains a typo and unclear wording:
-Complexer workflows and tests require modifying state on a running chain. This works only with builtin logic and operations. -If we want to burn some of our new tokens, we need to submit a bank burn message to do this. -The CLI wrapper works similar to the query. Just pass the parameters. It uses the `node0` key as *default*: +More complex workflows and tests require modifying state on a running chain. This only works with built-in logic and operations. +To burn some of our new tokens, we need to submit a bank burn message. +The CLI wrapper works similarly to queries - just pass the parameters. It uses the `node0` key by default:
207-215: Improve readability of test documentation sectionThe concluding section needs better structure:
-While tests are still more or less readable, it can gets harder the longer they are. I found it helpful to add -some comments at the beginning to describe what the intention is. +As tests grow in complexity, it becomes helpful to add comments at the beginning that describe the test's intention, +following this pattern:🧰 Tools
🪛 LanguageTool
[grammar] ~207-~207: The modal verb ‘can’ requires the verb’s base form.
Context: ...are still more or less readable, it can gets harder the longer they are. I found it ...(MD_BASEFORM)
[typographical] ~207-~207: Consider inserting a comma for improved readability.
Context: ... readable, it can gets harder the longer they are. I found it helpful to add some com...(MISSING_COMMAS)
tests/systemtests/go.mod (1)
Line range hint
1-1: Remove vanity URL from module pathAs discussed in previous reviews, we should use the direct GitHub URL instead of the vanity URL for this module.
Apply this change:
-module cosmossdk.io/tests/systemtests +module github.com/cosmos/cosmos-sdk/tests/systemtestssystemtests/go.mod (1)
5-24: Consider consolidating require blocksThe dependencies are split across multiple require blocks. Consider consolidating them for better maintainability.
You can run
go mod tidyto automatically organize the dependencies into a single block:go mod tidyAlso applies to: 26-33
tests/systemtests/distribution_test.go (3)
Line range hint
130-204: Consider adding negative test cases for GRPC endpointsWhile the current test coverage is good, consider adding more negative test cases for the GRPC endpoints, such as:
- Invalid validator addresses
- Malformed requests
- Edge cases for pagination
222-224: Consider using a helper function for gentx file operationsThe file operations could be encapsulated in a helper function to improve readability and reusability. This would also help standardize temporary file handling across tests.
+ func generateGentxFile(t *testing.T, cli *systest.CLIWrapper, amount string) []byte { + outFile := filepath.Join(t.TempDir(), "gentx.json") + cli.RunCommandWithArgs("genesis", "gentx", "node0", amount, + "--chain-id=" + cli.ChainID(), + "--commission-rate=0.01", + "--home", systest.Sut.NodeDir(0), + "--keyring-backend=test", + "--output-document=" + outFile) + gentxBz, err := os.ReadFile(outFile) + require.NoError(t, err) + return gentxBz + }
Line range hint
1-296: Consider adding package-level documentationWhile the individual tests are well-structured, consider adding package-level documentation that explains:
- The purpose of these system tests
- The test setup requirements
- Common patterns and helper functions used
tests/systemtests/cometbft_client_test.go (2)
36-40: Consider removing TODO comment or creating an issueThe TODO comment about distinguishing v2 system tests should either be addressed now or tracked in an issue.
Would you like me to help create a GitHub issue to track this TODO?
334-339: LGTM: Well-implemented generic helper functionThe
musthelper function is a good addition for simplifying error handling in tests. The generic implementation makes it reusable across different types, and using panic is acceptable in test code.Consider adding a brief doc comment to explain its purpose:
+// must is a helper that wraps a call returning (T, error) and panics if the error is non-nil. +// It's designed for use in tests where we want to fail fast on errors. func must[T any](r T, err error) T { if err != nil { panic(err) } return r }tests/systemtests/bank_test.go (2)
22-23: LGTM: Consistent migration to the new system test frameworkThe changes correctly migrate the test to use the new
systestpackage while maintaining the original test logic. All framework method calls have been properly updated to use the new package.However, there's a minor improvement opportunity in the error assertion logic.
Consider extracting the error assertion logic into a reusable helper function in the
systestpackage:- assertUnauthorizedErr := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool { - require.Len(t, gotOutputs, 1) - code := gjson.Get(gotOutputs[0].(string), "code") - require.True(t, code.Exists()) - require.Greater(t, code.Int(), int64(0), gotOutputs[0]) - return false - } + rsp = invalidCli.WithRunErrorMatcher(systest.AssertUnauthorizedErr).Run(bankSendCmdArgs...)Also applies to: 31-31, 44-44, 54-54, 62-62, 65-67
278-278: Improved structure for REST API testingThe migration to
systest.RestTestCaseand the framework's REST utilities provides a more maintainable and consistent approach to testing REST endpoints.Consider extracting the common test setup (like URL construction) into helper functions to reduce duplication:
func buildBankEndpointURL(baseURL, endpoint string) string { return fmt.Sprintf("%s/cosmos/bank/v1beta1/%s", baseURL, endpoint) }Also applies to: 285-306, 312-333
tests/systemtests/gov_test.go (2)
45-46: Consider adding cleanup in defer statementWhile using
systest.StoreTempFileis correct, consider adding file removal in the defer statement to prevent test artifacts from persisting.- defer invalidPropFile.Close() + defer func() { + invalidPropFile.Close() + os.Remove(invalidPropFile.Name()) + }()Also applies to: 68-69
379-383: Consider extracting magic numbers into named constantsWhile the voting period configuration is correct, consider extracting the magic numbers into named constants for better readability and maintenance:
+ const ( + baseVotingPeriod = 3 * time.Second + expeditedOffset = time.Second + ) - votingPeriod := 3 * time.Second + votingPeriod := baseVotingPeriod systest.Sut.ModifyGenesisJSON( t, systest.SetGovVotingPeriod(t, votingPeriod), - systest.SetGovExpeditedVotingPeriod(t, votingPeriod-time.Second), + systest.SetGovExpeditedVotingPeriod(t, votingPeriod-expeditedOffset),systemtests/cli.go (3)
259-274: Consider extracting flag constants for better maintainabilityThe flag handling is well-organized, but consider extracting the commonly used flags into constants to improve maintainability and reduce the risk of typos.
+const ( + flagBroadcastMode = "--broadcast-mode" + flagOutput = "--output" + flagYes = "--yes" + flagChainID = "--chain-id" +) func (c CLIWrapper) WithTXFlags(args ...string) []string { args = append(args, - "--broadcast-mode", "sync", - "--output", "json", - "--yes", - "--chain-id", c.chainID, + flagBroadcastMode, "sync", + flagOutput, "json", + flagYes, + flagChainID, c.chainID, )
422-424: Add documentation comment for the ChainID methodConsider adding a documentation comment to describe the purpose and return value of the ChainID method.
+// ChainID returns the chain identifier used by the CLI wrapper func (c CLIWrapper) ChainID() string { return c.chainID }
Line range hint
315-321: Consider secure handling of mnemonic seedsThe mnemonic seed is passed as a plain string and could potentially be exposed in logs or error messages. Consider implementing secure string handling for sensitive data.
Consider:
- Using a secure string type that's zeroed after use
- Ensuring the mnemonic isn't logged in error messages
- Adding a warning in the documentation about secure handling of mnemonics
tests/systemtests/auth_test.go (3)
58-59: Consider enhancing error handling for broadcast commandThe broadcast command's error handling could be more specific. Consider checking for particular error conditions that might occur during broadcasting.
-rsp = cli.RunCommandWithArgs(cli.WithTXFlags(broadcastCmd...)...) -systest.RequireTxFailure(t, rsp) +rsp = cli.RunCommandWithArgs(cli.WithTXFlags(broadcastCmd...)...) +systest.RequireTxFailure(t, rsp, "expected error: invalid public key")
Line range hint
343-393: Consider refactoring test cases for better maintainabilityThe test cases in
testMultisigTxBroadcastcould be improved for better maintainability:
- Consider extracting the test case setup into a separate helper function
- The command assembly logic could be simplified
+func setupMultisigTestCase(t *testing.T, cli *systest.CLIWrapper, cmd []string, signers []string) string { + t.Helper() + var signedFiles []string + for _, acc := range signers { + rsp := cli.RunCommandWithArgs(append(cmd, "--from="+acc)...) + signFile := systest.StoreTempFile(t, []byte(rsp)) + signedFiles = append(signedFiles, signFile.Name()) + } + return strings.Join(signedFiles, " ") +}
Line range hint
316-327: Consider adding input validationThe
generateBankSendTxfunction could benefit from input validation to ensure positive amounts and valid addresses.func generateBankSendTx(t *testing.T, cli *systest.CLIWrapper, fromAddr, toAddr string, amount, fees int64, memo string) string { t.Helper() + if amount <= 0 || fees <= 0 { + t.Fatal("amount and fees must be positive") + } + if fromAddr == "" || toAddr == "" { + t.Fatal("addresses cannot be empty") + }tests/systemtests/authz_test.go (2)
276-276: Consider making expiration time calculations more robustThe expiration time calculations could be made more deterministic and clear. Consider extracting the time-related constants and using a more explicit calculation method.
-expirationTime := time.Now().Add(systest.Sut.BlockTime() * 10) +const ( + expirationBlockCount = 10 +) +expirationTime := time.Now().Add(systest.Sut.BlockTime() * time.Duration(expirationBlockCount))Also applies to: 394-404
Line range hint
4-4: Consider adding package documentationWhile the test coverage is comprehensive, consider adding a package-level documentation comment that describes the overall testing strategy and the different authorization scenarios being tested.
+// Package systemtests provides system-level tests for the authorization module. +// It covers various authorization scenarios including granting, executing, and +// revoking different types of authorizations, as well as testing the associated +// API endpoints and error conditions. package systemteststests/systemtests/tx_test.go (2)
1023-1024: Track skipped test.The test is currently skipped waiting for a fix. Consider creating an issue to track this if one doesn't exist already.
Would you like me to help create a GitHub issue to track this skipped test?
Line range hint
1-1090: LGTM: Successfully extracted system test framework.The changes consistently transition all test functions to use the new
systestpackage, achieving the PR's objective of extracting the system test framework. The modifications improve:
- Modularity: Framework code is now centralized in the
systestpackage- Maintainability: Common operations are standardized across tests
- Clarity: Clear separation between test framework and test implementation
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
systemtests/go.sumis excluded by!**/*.sumtests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
systemtests/README.md(1 hunks)systemtests/cli.go(11 hunks)systemtests/getting_started.md(1 hunks)systemtests/go.mod(1 hunks)systemtests/rest_support.go(2 hunks)systemtests/system.go(4 hunks)systemtests/test_runner.go(4 hunks)systemtests/testnet_init.go(3 hunks)tests/systemtests/.gitignore(1 hunks)tests/systemtests/README.md(1 hunks)tests/systemtests/account_test.go(7 hunks)tests/systemtests/auth_test.go(18 hunks)tests/systemtests/authz_test.go(27 hunks)tests/systemtests/bank_test.go(12 hunks)tests/systemtests/bankv2_test.go(3 hunks)tests/systemtests/circuit_test.go(9 hunks)tests/systemtests/cometbft_client_test.go(9 hunks)tests/systemtests/distribution_test.go(8 hunks)tests/systemtests/export_test.go(3 hunks)tests/systemtests/fraud_test.go(2 hunks)tests/systemtests/go.mod(4 hunks)tests/systemtests/gov_test.go(14 hunks)tests/systemtests/group_test.go(6 hunks)tests/systemtests/main_test.go(1 hunks)tests/systemtests/mint_test.go(3 hunks)tests/systemtests/snapshots_test.go(3 hunks)tests/systemtests/staking_test.go(4 hunks)tests/systemtests/tx_test.go(21 hunks)tests/systemtests/unordered_tx_test.go(1 hunks)tests/systemtests/upgrade_test.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/systemtests/.gitignore
🧰 Additional context used
📓 Path-based instructions (28)
systemtests/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
systemtests/cli.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
systemtests/getting_started.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
systemtests/rest_support.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
systemtests/system.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
systemtests/test_runner.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
systemtests/testnet_init.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/README.md (2)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
tests/systemtests/account_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/auth_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/authz_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/bank_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/bankv2_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/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"
tests/systemtests/cometbft_client_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/distribution_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/export_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/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"
tests/systemtests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
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/group_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/main_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/mint_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/snapshots_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/staking_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/tx_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/unordered_tx_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/upgrade_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"
🪛 LanguageTool
systemtests/README.md
[typographical] ~55-~55: After the expression ‘for example’ a comma is usually used.
Context: ...- GRPC * 16656 - 16656+n - P2P For example Node 3 listens on 26660 for RPC cal...
(COMMA_FOR_EXAMPLE)
systemtests/getting_started.md
[typographical] ~25-~25: After the expression ‘for example’ a comma is usually used.
Context: ...e case, start a new test file here. for example bank_test.go to begin with: ```go //...
(COMMA_FOR_EXAMPLE)
[typographical] ~52-~52: It appears that a comma is missing.
Context: ...r to interact or parse results. In this example we want to execute `simd q bank total-s...
(DURING_THAT_TIME_COMMA)
[grammar] ~61-~61: Possible subject-verb agreement error detected.
Context: ...un TestQueryTotalSupply --verbose ``` This give very verbose output. You would see all ...
(THIS_THAT_AGR)
[typographical] ~95-~95: After the expression ‘for example’ a comma is usually used.
Context: ...to navigation within the document. For example gjson.Get(raw, "supply").Array() give...
(COMMA_FOR_EXAMPLE)
[style] ~97-~97: Consider a shorter alternative to avoid wordiness.
Context: ...ount of the stake token as int64 type. In order to test our assumptions in the system test...
(IN_ORDER_TO_PREMIUM)
[style] ~133-~133: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ...t for key node0. The setup code looks quite big and unreadable now. Usually a good time...
(EN_WEAK_ADJECTIVE)
[typographical] ~133-~133: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...ode looks quite big and unreadable now. Usually a good time to think about extracting h...
(RB_LY_COMMA)
[uncategorized] ~167-~167: Possible missing article found.
Context: ...newState }) sut.StartChain(t) ``` Next step is to add the new token to the ass...
(AI_HYDRA_LEO_MISSING_THE)
[grammar] ~207-~207: The modal verb ‘can’ requires the verb’s base form.
Context: ...are still more or less readable, it can gets harder the longer they are. I found it ...
(MD_BASEFORM)
[typographical] ~207-~207: Consider inserting a comma for improved readability.
Context: ... readable, it can gets harder the longer they are. I found it helpful to add some com...
(MISSING_COMMAS)
tests/systemtests/README.md
[grammar] ~6-~6: Did you mean to use ‘besides’ (meaning ‘apart from’, ‘aside from’) instead of the preposition ‘beside’ (meaning ‘next to’)?
Context: ...../../systemtests/getting_started.md). Beside the Go tests and testdata files, this d...
(BESIDES)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ... ## Execution Build a new binary from current branch and copy it to the `tests/system...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint (0.35.0)
systemtests/README.md
9-9: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
10-10: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
11-11: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
12-12: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
16-16: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
17-17: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
18-18: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
tests/systemtests/README.md
44-44: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
48-48: null
Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (69)
tests/systemtests/main_test.go (2)
5-9: LGTM! Clean import organization
The import block is well-structured with standard library imports separated from external packages, and the alias systest clearly indicates its purpose.
11-13: Verify consistent usage of systest.RunTests across all tests
The change to use systest.RunTests aligns with the PR objective of extracting the framework code.
Let's verify that all system tests have been updated to use the new package:
✅ Verification successful
All system tests consistently use systest.RunTests
The verification confirms that there is only one instance of RunTests usage in the codebase, which is the updated systest.RunTests(m) call in tests/systemtests/main_test.go. No direct/old usage of RunTests was found in any test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct RunTests calls in system tests
# that haven't been updated to use the systest package
# Search for direct RunTests calls in test files
rg -g '*_test.go' 'RunTests\(m\)' --type go
# Search for systest.RunTests usage to confirm the pattern
rg -g '*_test.go' 'systest\.RunTests\(m\)' --type go
Length of output: 204
tests/systemtests/README.md (1)
1-48: Documentation effectively supports framework extraction
The README provides clear and comprehensive guidance for using the extracted system test framework, aligning well with the PR objectives. The structure is logical, and the instructions are detailed enough for both standard and macOS users.
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: Did you mean to use ‘besides’ (meaning ‘apart from’, ‘aside from’) instead of the preposition ‘beside’ (meaning ‘next to’)?
Context: ...../../systemtests/getting_started.md). Beside the Go tests and testdata files, this d...
(BESIDES)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ... ## Execution Build a new binary from current branch and copy it to the `tests/system...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint (0.35.0)
44-44: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
48-48: null
Files should end with a single newline character
(MD047, single-trailing-newline)
tests/systemtests/bankv2_test.go (5)
11-12: LGTM: Clean import of the new system test framework
The import of the extracted system test framework package aligns with the PR objectives.
Line range hint 1-60: LGTM: Clean refactoring to use the extracted framework
The changes consistently move framework functionality to the new package while maintaining the same test logic. This aligns well with the PR objective of extracting the system test framework into a separate package.
17-17: Verify the IsV2() function implementation
The version check has been moved to the framework package. Let's verify its implementation.
✅ Verification successful
Function implementation verified and looks correct
The IsV2() function is properly implemented in the systemtests package. It checks the COSMOS_BUILD_OPTIONS environment variable for the presence of "v2", which is consistent with the version checking functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for IsV2 function implementation in the new package
ast-grep --pattern 'func IsV2() bool {
$$$
}'
Length of output: 290
48-48: Verify RequireTxSuccess implementation
The transaction validation has been moved to the framework. Let's verify its implementation.
✅ Verification successful
RequireTxSuccess implementation is verified
The implementation is found in systemtests/cli.go and is correctly used across the codebase. It verifies that the transaction response contains a success code (0) and fails the test if the code is non-zero.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RequireTxSuccess implementation
ast-grep --pattern 'func RequireTxSuccess(t *testing.T, txResult interface{}) {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Search for RequireTxSuccess using ripgrep for broader coverage
rg -A 5 "RequireTxSuccess" --type go
Length of output: 28582
23-24: Verify Sut initialization in the framework
The chain management operations now use systest.Sut. Let's verify its proper initialization.
Also applies to: 33-33
✅ Verification successful
Let me gather more information about the Sut initialization and the overall system test framework structure.
Let me gather more information about the initialization of Sut and its setup.
Let me check the test main function to understand how Sut is initialized.
Let me check the RunTests function to understand the complete initialization flow.
Based on the gathered information, I can now provide a final response about the Sut initialization verification.
Sut initialization is properly implemented in the framework
The system test framework has a robust initialization flow:
- Sut is initialized in
RunTestsusingNewSystemUnderTestwith proper configuration parameters SetupChainis called immediately after initialization to set up the chain and keyring- The implementation includes proper cleanup and chain management operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Sut initialization in the framework
ast-grep --pattern 'var Sut = $$$'
# Search for ResetChain and StartChain implementations
ast-grep --pattern 'func (s $$$) ResetChain(t *testing.T) {
$$$
}'
ast-grep --pattern 'func (s $$$) StartChain(t *testing.T) {
$$$
}'
Length of output: 1290
Script:
#!/bin/bash
# Search for SystemUnderTest struct definition and initialization
ast-grep --pattern 'type SystemUnderTest struct {
$$$
}'
# Search for NewSystemUnderTest or similar initialization functions
ast-grep --pattern 'func New$_($$$) *SystemUnderTest {
$$$
}'
# Search for where Sut is used in test files
rg "systest\.Sut\." -A 2 "tests/"
Length of output: 43934
Script:
#!/bin/bash
# Search for Sut initialization and setup
rg "var Sut" "systemtests/"
# Search for SetupChain implementation
ast-grep --pattern 'func (s *SystemUnderTest) SetupChain() {
$$$
}'
# Search for test initialization
rg "func TestMain" "tests/systemtests/"
Length of output: 2176
Script:
#!/bin/bash
# Check the test main implementation
rg -A 20 "func TestMain" "tests/systemtests/main_test.go"
# Look for any setup functions called before tests
ast-grep --pattern 'func setup$_() {
$$$
}'
Length of output: 158
Script:
#!/bin/bash
# Look for RunTests implementation
ast-grep --pattern 'func RunTests($$$) {
$$$
}'
# Search for any initialization in the package init
rg "func init" "systemtests/"
# Look for NewSystemUnderTest or similar constructor
rg "New.*SystemUnderTest" "systemtests/"
Length of output: 3234
tests/systemtests/unordered_tx_test.go (2)
12-13: LGTM: Clean import organization
The import statement follows Go conventions with proper grouping and clear aliasing.
26-35: LGTM: Improved test setup with systest package
The migration to the systest package methods improves test maintainability and consistency.
tests/systemtests/fraud_test.go (3)
14-15: LGTM: Clean import with appropriate alias
The import statement follows Go conventions and aligns with the PR objective of extracting the system test framework.
62-62: LGTM: Clean migration of block waiting
The migration to systest.Sut.AwaitNBlocks maintains the same functionality while using the new framework.
30-31: Verify error handling in LoadValidatorPubKeyForNode
The migration to systest package looks good, but we should verify the error handling in the new location.
✅ Verification successful
Error handling is properly implemented in validator key loading
The code has proper error handling:
LoadValidatorPubKeyusesrequire.NoError(t, err)to handle errors from bothGetPubKey()andFromCmtPubKeyInterface()- The function is a test helper (marked with
t.Helper()) and uses the testing framework's assertion to fail the test if any error occurs - This is an appropriate approach for test code as any key loading failure should immediately fail the test
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling in the new location
ast-grep --pattern 'func LoadValidatorPubKeyForNode($_, $_, $_) {
$$$
if err != nil {
$$$
}
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Let's try to find the implementation of LoadValidatorPubKeyForNode first
rg "LoadValidatorPubKeyForNode" -A 5
Length of output: 1498
Script:
#!/bin/bash
# Let's check the implementation of LoadValidatorPubKey which is called by LoadValidatorPubKeyForNode
rg "func LoadValidatorPubKey" -A 10
Length of output: 1210
systemtests/rest_support.go (1)
15-18: LGTM! Verify all usages of the exported fields
The struct field exports align with the PR objective of making the framework independently reusable. The naming follows Go conventions.
✅ Verification successful
All usages of RestTestCase struct fields have been properly updated
Based on the search results, all usages of the RestTestCase struct are consistent with the exported field names (Name, Url, ExpCode, ExpOut). The struct is used across multiple test files (distribution_test.go, bank_test.go, authz_test.go) and all instances show proper capitalization of the field names in the struct literals. No lowercase field accesses were found, confirming that the refactoring was complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all usages of RestTestCase have been updated to use the exported fields
# Look for any remaining lowercase field accesses that might have been missed
rg -g '*.go' '\b(name|url|expCode|expOut)\b.*RestTestCase'
Length of output: 60
Script:
#!/bin/bash
# Let's try a different approach to find all usages of RestTestCase
# First, find all references to RestTestCase
rg -g '*.go' 'RestTestCase' -A 5 -B 5
# Also search for any struct field access patterns
ast-grep --pattern 'testCase.$_'
Length of output: 13960
tests/systemtests/staking_test.go (2)
10-11: LGTM: Clean import organization
The imports are well-organized following the style guide, with standard library imports separated from internal packages. The systest alias is clear and meaningful.
20-30: LGTM: Clean migration to systest package
The test setup has been successfully migrated to use the extracted system test framework, maintaining the same functionality while improving code organization.
tests/systemtests/export_test.go (2)
13-14: LGTM: Import changes align with framework extraction
The new import of the systest package aligns well with the PR objective of extracting the system test framework into a separate package.
18-19: LGTM: Clean transition to new framework
The changes consistently replace direct sut references with systest.Sut, maintaining the same functionality while properly utilizing the extracted framework.
Also applies to: 21-21, 24-24, 26-26
tests/systemtests/mint_test.go (4)
10-11: LGTM: Clean import organization
The import statement for the new systest package follows Go conventions and is properly organized.
34-36: LGTM: Clean migration of chain operations
The chain operations have been properly migrated to use the systest package.
Line range hint 91-99: LGTM: Comprehensive test coverage
The CLI tests provide good coverage of mint query functionality:
- Parameters query
- Inflation query
- Annual provisions query with height parameter
This complements the HTTP endpoint tests well, ensuring both CLI and API interfaces are verified.
18-19: Verify the source of systest.Verbose flag
While the migration to systest package looks good, let's verify the source of the systest.Verbose flag to ensure proper configuration.
✅ Verification successful
Let me gather more information about how the Verbose flag is initialized.
Let me try a different search pattern to find where the Verbose flag is set.
Verbose flag is properly configured via command-line arguments
The systest.Verbose flag is correctly initialized in systemtests/test_runner.go using the standard Go flag package. It can be controlled via the -verbose command-line argument during test execution, defaulting to false if not specified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition and usage of Verbose flag
ast-grep --pattern 'var Verbose bool'
rg -A 2 'Verbose.*bool'
Length of output: 637
Script:
#!/bin/bash
# Search for initialization of Verbose flag
rg -A 5 'Verbose\s*=|init\(\).*systemtests'
Length of output: 45
Script:
#!/bin/bash
# Search for flag initialization in test files and main functions
rg -A 5 'func TestMain|func init' systemtests/
rg -A 5 'flag\.Bool.*Verbose'
Length of output: 1772
tests/systemtests/snapshots_test.go (4)
11-12: LGTM: Clean import of the new system test framework
The import of the extracted systest package aligns well with the PR objective of separating the framework code.
16-18: LGTM: Clean transition to the new system test framework
The changes consistently replace direct system test interactions with the new systest package calls while maintaining the same functionality.
Also applies to: 21-21, 23-23, 29-30
78-81: LGTM: Consistent framework usage in pruning tests
The pruning test changes follow the same clean pattern of using the extracted framework while maintaining version-specific behavior.
Also applies to: 84-84, 87-87, 89-89, 93-97
Line range hint 16-67: Verify test coverage after framework transition
Please ensure that the test coverage is maintained after transitioning to the new framework.
✅ Verification successful
Let me gather more information about the test coverage by checking the implementation details of snapshot functionality in both v1 and v2.
Based on the analysis of the implementation and test coverage in both v1 and v2 versions, I can generate the final response:
Test coverage is maintained and enhanced in v2 implementation
The snapshot functionality test coverage is thoroughly maintained in the v2 implementation with equivalent or better coverage:
- Core functionality tests (store, chunk, manager) are preserved with identical test cases and assertions
- Additional test scenarios in v2:
- Parallel snapshot and pruning operations
- Migration scenarios with commit snapshotter
- Extended error handling and recovery tests
- Improved state verification assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for snapshot functionality
# Note: This will help ensure no regression in test coverage
# Find all snapshot-related test files
fd -e go . | rg "snapshot.*_test"
# Find all snapshot-related functionality being tested
rg -A 5 "func.*Test.*Snapshot" tests/
Length of output: 881
Script:
#!/bin/bash
# Check implementation details in v1 and v2 to compare test coverage
# Find snapshot-related functionality in v1
echo "=== V1 Snapshot Implementation ==="
rg -t go "func.*Snapshot" store/snapshots/
# Find snapshot-related functionality in v2
echo -e "\n=== V2 Snapshot Implementation ==="
rg -t go "func.*Snapshot" store/v2/snapshots/
# Check test assertions in both versions
echo -e "\n=== V1 Test Assertions ==="
rg -t go "require\.|assert\." store/snapshots/*_test.go
echo -e "\n=== V2 Test Assertions ==="
rg -t go "require\.|assert\." store/v2/snapshots/*_test.go
Length of output: 60072
systemtests/test_runner.go (1)
28-28: LGTM! Clean and consistent changes
The updates to use the exported variables are consistent throughout the function, maintaining the existing functionality while supporting the framework extraction.
Also applies to: 39-39, 50-51, 57-59
tests/systemtests/upgrade_test.go (4)
15-16: LGTM: Clean import of the new system test framework
The import of the new systest package aligns with the PR objective of extracting the system test framework into a separate package.
29-37: LGTM: Clean refactor of chain management
The chain management logic has been cleanly refactored to use the new systest package while maintaining the same functionality:
- Proper chain shutdown
- Binary management
- Chain initialization
102-116: LGTM: Clean refactor of binary management
The FetchExecutable function has been properly updated to:
- Use
systest.WorkDirfor consistent workspace management - Maintain proper error handling
- Use consistent shell command execution through
systest.MustRunShellCmd
Line range hint 22-24: Verify the skip condition and timeline
The test is currently skipped with a note about waiting for v052 artifacts and proper store upgrade handling. Given this is a critical upgrade path test, we should:
- Track when v052 artifacts will be available
- Ensure there's a plan to address the store upgrade issue
tests/systemtests/group_test.go (2)
11-12: LGTM: Clean import of the new system test framework
The addition of the systest package import aligns with the PR objective of extracting the system test framework into a separate package.
23-26: Verify complete migration to systest package
The chain initialization has been updated to use the new systest package. Let's verify that all sut references have been migrated.
Also applies to: 32-32
✅ Verification successful
Based on the search results, all instances of sut in the codebase are either:
- Part of the
systestpackage's internal implementation (insystemtests/*.go) - Used with the
systest.Sutprefix in test files (intests/systemtests/*.go) - One occurrence in a test log message (
tests/integration/tx/aminojson/aminojson_test.go)
The review comment was concerned about verifying if all sut references have been migrated to use the systest package. The search results confirm that this migration is complete as there are no standalone sut references that need to be updated - they are either part of the package implementation or already using the correct systest.Sut prefix.
All sut references are properly migrated
The codebase shows consistent usage of the systest package, with no remaining direct sut references that need migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct 'sut' references that haven't been migrated
rg -i '\bsut\b' --type go --glob '!vendor/**' --glob '!**/systest/**'
Length of output: 24972
tests/systemtests/account_test.go (3)
11-12: LGTM: Clean import of the new system test framework package
The import of the new systemtests package with the systest alias aligns well with the PR objective of extracting the system test framework.
23-24: LGTM: Clean migration to the new system test framework
The changes consistently update the test to use the new systest package while maintaining the original test logic and coverage. The test continues to effectively verify account creation behavior through fund transfers.
Also applies to: 29-33, 40-40
59-60: LGTM: Consistent migration to the new system test framework
The changes successfully migrate the account migration test to use the new systest package while maintaining comprehensive test coverage of account migration, transaction handling, and key swapping functionality.
Also applies to: 67-71, 104-104, 126-126, 156-156
systemtests/testnet_init.go (1)
Line range hint 80-84: LGTM: Correct version-specific flag handling
The code appropriately handles different flag formats for minimum gas prices based on the version.
tests/systemtests/circuit_test.go (2)
14-15: LGTM: Clean import of the new system test framework package
The import change aligns well with the PR's objective of extracting the system test framework into a separate package.
95-99: LGTM: Excellent improvement in proposal verification logic
The change from a fixed sleep to require.Eventually is a significant improvement that:
- Eliminates potential test flakiness
- Provides better error reporting
- Handles timing variations gracefully
tests/systemtests/go.mod (2)
23-23: Dependencies look good
The changes to dependencies align with the PR objective of extracting the system test framework:
- Marking gRPC as indirect improves dependency clarity
- Adding systemtests package with replace directive supports the framework extraction
Also applies to: 28-28
174-174: Verify systemtests package path
The replace directive points to ../../systemtests. Let's verify this path exists and contains the extracted framework code.
✅ Verification successful
Replace directive path is correct and systemtests package exists
The systemtests directory exists at the repository root and contains the expected framework code including:
- Core system test files (system.go, test_runner.go)
- CLI and RPC support (cli.go, rpc_client.go)
- Network initialization (testnet_init.go)
- Documentation (README.md, getting_started.md)
The go.mod file confirms this is the correct systemtests package with module name cosmossdk.io/systemtests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the systemtests package location and structure
# Check if systemtests directory exists at the specified path
if [ -d "systemtests" ]; then
echo "systemtests directory found"
# List key framework files
ls -la systemtests/
else
echo "systemtests directory not found"
fi
# Check for go.mod in systemtests
if [ -f "systemtests/go.mod" ]; then
echo "go.mod found in systemtests"
cat systemtests/go.mod
fi
Length of output: 9354
systemtests/go.mod (1)
1-171: Update vanity repository
As mentioned in the previous review, please update the vanity repository before merging.
tests/systemtests/distribution_test.go (2)
17-18: LGTM: Clean import of the new system test framework
The import of the new systest package aligns with the PR objective of extracting the system test framework into a separate package.
Line range hint 29-105: LGTM: Clean migration to systest package
The refactoring maintains the original test logic while properly utilizing the new systest package's functionality. The test coverage remains comprehensive, testing both the max-msgs scenarios and the actual reward withdrawal.
tests/systemtests/cometbft_client_test.go (3)
16-17: LGTM: Import changes align with framework extraction
The addition of the systest package import aligns with the PR objective of extracting the system test framework into a separate package.
26-33: LGTM: Clean migration to systest package
The changes properly migrate chain operations to use the extracted system test framework while maintaining the original test logic.
172-177: LGTM: Improved error handling with status codes
The changes properly handle HTTP error responses with appropriate status codes and error message verification.
tests/systemtests/bank_test.go (4)
14-15: LGTM: Clean import of the new system test framework package
The import of the new systest package aligns with the PR objective of extracting the system test framework into a separate package.
103-104: LGTM: Proper framework migration in multi-send test
The changes correctly update all framework method calls to use the new systest package while preserving the test logic and validation.
Also applies to: 114-118, 175-175
196-197: LGTM: Clean setup of GRPC test environment
The changes properly initialize the test environment using the new framework.
Also applies to: 205-205, 215-215, 220-221
Line range hint 1-333: Verify test coverage for bank operations
The test suite provides good coverage of bank operations, including positive and negative scenarios. However, let's verify if we're missing any edge cases.
✅ Verification successful
Test coverage for bank operations is comprehensive
The test suite provides thorough coverage of bank operations including:
- Basic send operations with success and failure cases
- Multi-send operations with various scenarios
- GRPC queries for balances, supply, metadata
- Error cases like insufficient funds and unauthorized signatures
- Edge cases like dry-run and generate-only modes
- Chain-specific validations
- Balance and metadata queries with different parameters
The test coverage aligns well with the bank module's core functionality and error paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any bank-related operations in the codebase that might need testing
ast-grep --pattern 'func $_($$) {
$$$
bank.$$$
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Let's search for bank-related operations using ripgrep
rg -g '*.go' -B 2 -A 2 'bank\.' --no-heading
# Also search for bank message types
rg -g '*.go' 'MsgSend|MsgMultiSend' --no-heading
Length of output: 115794
tests/systemtests/gov_test.go (3)
16-16: LGTM: Import change aligns with framework extraction
The addition of the systest package import aligns with the PR objective of extracting the system test framework into a separate package.
25-26: LGTM: Consistent usage of new framework APIs
The migration to systest.Sut and systest.NewCLIWrapper follows the new framework's patterns consistently.
Also applies to: 32-32
417-421: LGTM: Improved test reliability with assert.Eventually
Good improvement replacing sleep with assert.Eventually for better test reliability and reduced flakiness.
systemtests/cli.go (1)
Line range hint 103-143: Well-implemented clone pattern for immutable state management!
The implementation follows the builder pattern best practices and ensures immutability by creating new instances for state changes. This makes the code more maintainable and state changes more traceable.
tests/systemtests/auth_test.go (3)
13-14: LGTM: Import changes align with framework extraction
The addition of the systest package import aligns with the PR objective of extracting the system test framework into a separate package.
25-28: LGTM: Consistent framework usage
The changes correctly migrate from direct sut usage to the new systest package, maintaining the same functionality while improving code organization.
Line range hint 1-549: Overall changes look good with minor improvement suggestions
The migration to the new systest package is consistent throughout the file and aligns well with the PR objectives. The test coverage remains comprehensive, and the code organization has improved. Consider implementing the suggested improvements for error handling and test case organization in future iterations.
tests/systemtests/authz_test.go (3)
15-16: LGTM: Clean import of the new systemtests package
The import of the new systest package aligns well with the PR objective of extracting the system test framework into a separate package.
207-207: LGTM: Improved error handling with systest utilities
The transition to using systest.RequireTxFailure for error cases improves consistency and readability across the test suite.
Also applies to: 334-334, 490-490, 564-564
820-836: LGTM: Well-structured helper function
The setupChain helper function effectively encapsulates common setup logic and includes proper validation of preconditions. This improves code reuse and maintainability.
systemtests/system.go (5)
40-41: LGTM: Well-defined constant for API port
The DefaultApiPort constant is appropriately defined with a standard port number (1317) for API access.
112-120: LGTM: Well-structured getter/setter for execBinary
The methods are properly documented and follow Go conventions for accessor methods.
122-130: LGTM: Well-structured getter/setter for testnetInitializer
The methods are properly documented and follow Go conventions for accessor methods.
784-786: LGTM: Clear accessor for blockTime
The BlockTime method provides a clean way to access the block time configuration.
94-94: LGTM: Improved constructor with better configurability
The changes enhance the constructor by:
- Using the
DefaultApiPortconstant for API address initialization - Adding flexible testnet initializer configuration with a sensible default
This aligns well with the goal of making the framework more modular and configurable.
Also applies to: 104-108
tests/systemtests/tx_test.go (3)
16-16: LGTM: Import change aligns with framework extraction.
The addition of the systest package import is consistent with the PR's objective of extracting the system test framework into a separate package.
37-39: LGTM: Consistent usage of new system test framework.
The transition to using systest.Sut and systest.NewCLIWrapper improves the modularity of the test framework and aligns with the extraction objective.
54-54: LGTM: Standardized file handling.
The use of systest.StoreTempFile across test functions standardizes temporary file handling in the test framework.
Also applies to: 158-158
| {"should export correct height", []string{"genesis", "export", "--home", systest.Sut.NodeDir(0)}, false}, | ||
| {"should export correct height with --height", []string{"genesis", "export", "--height=5", "--home", systest.Sut.NodeDir(0), "--log_level=disabled"}, false}, | ||
| {"should export height 0 with --for-zero-height", []string{"genesis", "export", "--for-zero-height=true", "--home", systest.Sut.NodeDir(0)}, true}, |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for invalid heights is needed
The test suite lacks specific error cases for the genesis export command with invalid heights, while other parts of the codebase properly test invalid height scenarios. Add test cases for:
- Export with negative height (e.g.,
--height=-1) - Export with future height (e.g., height greater than current chain height)
- Export with non-numeric height value
Example locations where invalid heights are properly tested:
tests/systemtests/cometbft_client_test.go: Tests negative height validationtests/systemtests/distribution_test.go: Tests invalid height boundariestests/integration/server/grpc/server_test.go: Tests invalid height ranges
🔗 Analysis chain
Verify test coverage completeness
The test cases cover essential scenarios (default export, specific height, zero height). Consider adding test cases for:
- Export with invalid height
- Export with future height
Also applies to: 49-49
Let me gather more information about the test coverage and error handling in the codebase.
Let me check one more thing to see if there are any error handling test cases in the entire test suite for invalid heights.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar test cases exist in other test files
rg -A 5 "genesis.*export.*height" --glob '**/*_test.go'
Length of output: 729
Script:
#!/bin/bash
# Check the implementation of export command to understand error handling
ast-grep --pattern 'func $_ ($_, $_) {
$$$
export
$$$
}'
# Also check for any error test cases in the test file
rg -B 5 "Error|error" tests/systemtests/export_test.go
Length of output: 728
Script:
#!/bin/bash
# Search for height-related error test cases in all test files
rg -B 3 "height.*invalid|invalid.*height" --glob '**/*_test.go'
# Also check the test file structure to understand the test setup
rg -A 10 "func Test.*Export" tests/systemtests/export_test.go
Length of output: 7094
| policyFile := systest.StoreTempFile(t, []byte(fmt.Sprintf(`{"@type":"/cosmos.group.v1.ThresholdDecisionPolicy", "threshold":"%d", "windows":{"voting_period":"30000s"}}`, threshold))) | ||
| policyCmd := []string{"tx", "group", "create-group-policy", valAddr, groupId, validMetadata, policyFile.Name(), "--from=" + valAddr} | ||
| rsp = cli.RunAndWait(policyCmd...) | ||
| RequireTxSuccess(t, rsp) | ||
| systest.RequireTxSuccess(t, rsp) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential resource leaks in policy file creation loop
Multiple temporary files are created in the loop without cleanup. Consider managing these resources properly.
+ defer func() {
+ if policyFile != nil {
+ _ = os.Remove(policyFile.Name())
+ }
+ }()Committable suggestion skipped: line range outside the PR's diff.
| @@ -0,0 +1,171 @@ | |||
| module cosmossdk.io/systemtests | |||
|
|
|||
| go 1.23 | |||
There was a problem hiding this comment.
Go 1.23 is not yet released
The specified Go version 1.23 is not yet released (current latest is 1.22). This could cause compatibility issues.
Please update to use the latest stable version:
-go 1.23
+go 1.22📝 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.
| go 1.23 | |
| go 1.22 |
There was a problem hiding this comment.
@julienrbrt, thank you for the correction. I see that Go 1.23 is released and appropriate to use. I'll take this into account for future reviews.
✏️ Learnings added
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#22578
File: systemtests/go.mod:3-3
Timestamp: 2024-11-26T08:14:07.450Z
Learning: Go 1.23 is released and acceptable to use in go.mod files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| require ( | ||
| github.com/cosmos/cosmos-proto v1.0.0-beta.5 // indirect | ||
| github.com/cosmos/cosmos-sdk v0.50.6 |
There was a problem hiding this comment.
💡 Codebase verification
Update cosmos-sdk to v0.50.10 to address security vulnerabilities
The current version v0.50.6 has known security vulnerabilities that have been patched in later versions:
- HIGH severity: ValidateVoteExtensions vulnerability (fixed in v0.50.5)
- LOW severity: Potential slashing evasion during re-delegation (fixed in v0.50.5)
The latest stable version v0.50.10 includes these security fixes and is backward compatible within the v0.50.x series.
🔗 Analysis chain
Verify version constraints for critical dependencies
Critical dependencies like cosmos-sdk and cometbft should have appropriate version constraints to ensure compatibility and security.
Also applies to: 28-28
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for latest versions and security advisories for critical dependencies
# Check latest versions
echo "Checking latest versions for critical dependencies..."
curl -s "https://proxy.golang.org/github.com/cosmos/cosmos-sdk/@v/list"
curl -s "https://proxy.golang.org/github.com/cometbft/cometbft/@v/list"
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/cosmos/cosmos-sdk") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 5736
There was a problem hiding this comment.
ACK! Nice
I'll add adding the backport label. In release/v0.52.x we should delete the backported root systemtests package, and make tests/systemtests import a tagged version cosmossdk.io/systemtests.
Let's maybe tag cosmossdk.io/systemtests v1.0.0-rc.1 after this PR? What do you think?
systemtests/getting_started.md
Outdated
There was a problem hiding this comment.
I'd delete the getting_started from tests/systemtests then. It will just get out of sync otherwise and you've already linked this in the README there
There was a problem hiding this comment.
Could you add in go.work.example /systemtests as well?
julienrbrt
added
the
backport/v0.52.x
systemtests/README.md
Outdated
There was a problem hiding this comment.
Let's add a CHANGELOG.md under this package as well (same templates as the other changelogs)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
systemtests/CHANGELOG.md (2)
1-33: Documentation improvements neededWhile the guidelines are comprehensive, there are several areas that could be improved for clarity:
- The git-cliff reference should include a brief explanation of what the tool does.
- The sentence about conventional commits could be clearer: "Each commit should follow the Conventional Commits specification, with the following supported types:"
- The API/state machine change explanation needs more context about when and why to use the
!suffix.- Consider adding an example of the changelog format structure.
-Changelog entries are generated by git cliff ref: https://github.com/orhun/git-cliff +Changelog entries are generated using git-cliff, a tool that automatically generates changelogs from conventional commit messages. +For more information, see: https://github.com/orhun/git-cliff -Each commit should be conventional, the following message groups are supported. +Each commit should follow the Conventional Commits specification, with the following supported types: -When a change is made that affects the API or state machine, the commit message prefix should be suffixed with `!`. +When a change introduces breaking modifications to the API or state machine, append a `!` suffix to the commit type (e.g., `feat!:` or `fix!:`). + +Example changelog entry format: +```markdown +### Features +* [#123](https://github.com/org/repo/pull/123) Add new feature + +### Bug Fixes +* [#124](https://github.com/org/repo/pull/124) Fix critical issue +```
35-41: Enhance changelog entry detailsThe changelog entry could be more descriptive to better communicate the impact and scope of changes:
## [Unreleased] +<!-- YYYY-MM-DD --> ### Features -* [#22578](https://github.com/cosmos/cosmos-sdk/pull/22578) Extract system test framework +* [#22578](https://github.com/cosmos/cosmos-sdk/pull/22578) Extract system test framework into a separate package to enable independent releases and improve modularity
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
systemtests/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
systemtests/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
go.work.example (2)
Line range hint
14-14: Consider removing the old systemtests pathAs part of the migration to extract the system test framework into a separate package, consider removing the
./tests/systemtestspath. This would complete the migration and prevent confusion about which path should be used.Apply this diff to remove the old path:
- ./tests/systemtests
Remove duplicate module path in go.work.example
The verification reveals that the systemtests package has been fully migrated to a new location:
- All test files in
tests/systemtestsnow importcosmossdk.io/systemtests- The new
systemtestsdirectory contains the framework code- Both locations have their own
go.modfiles, but the old location only contains test filesThe old path
./tests/systemtestsshould be removed fromgo.work.exampleas it's no longer needed, and keeping both paths could lead to module resolution conflicts.🔗 Analysis chain
Line range hint
14-14: Verify potential module path duplicationThere appears to be a duplication between the existing
./tests/systemtestsand the newly added./systemtestspaths. This could lead to module resolution conflicts.Let's verify the usage of both module paths:
Also applies to: 25-25
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for imports and references to both module paths # Check for imports of the old path rg -l "tests/systemtests" --type go # Check for imports of the new path rg -l "systemtests" --type go --glob '!go.work*'Length of output: 947
Script:
#!/bin/bash # Let's check the actual imports in both locations to understand the dependency structure # Check imports in the new systemtests package echo "=== Imports in new systemtests package ===" rg "^import \(" -A 10 systemtests/*.go # Check imports in the test files echo -e "\n=== Imports in test files ===" rg "^import \(" -A 10 tests/systemtests/*_test.go # Check if the old path still exists as a directory echo -e "\n=== Check if old module exists ===" test -d tests/systemtests && echo "Old module directory exists" || echo "Old module directory doesn't exist" # Check for go.mod in both locations echo -e "\n=== Check for go.mod files ===" fd go.modLength of output: 16509
Co-authored-by: marbar3778 <marbar3778@yahoo.com> (cherry picked from commit 14d98d2) # Conflicts: # go.work.example # tests/systemtests/bankv2_test.go # tests/systemtests/cometbft_client_test.go # tests/systemtests/go.mod # tests/systemtests/tx_test.go # tests/systemtests/upgrade_test.go
#22655) Co-authored-by: Alexander Peters <alpe@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
Description
Extract framework code.
Minor refactorings
Closes: #22562
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
README.mdfor thesystemtestspackage, detailing the testing framework and execution instructions.getting_started.mdguide for writing and executing system tests.systestpackage to streamline system test utilities and enhance modularity.CHANGELOG.mdsection outlining guiding principles and usage instructions for maintaining the changelog.go.work.examplefile to include thesystemtestsmodule.Bug Fixes
systestmethods for consistency.Documentation
tests/systemtests/README.mdfor improved usability.systemtestsfiles to reflect changes and provide clearer instructions.Refactor
sutobject to utilizing asystestpackage across multiple test files for better organization and maintainability.