Remove x/* from testutil #20121
Remove x/* from testutil #20121
Conversation
WalkthroughWalkthroughThe changes involve updates to import paths and functions across various test and simulation files in the Cosmos SDK. Key updates include replacing Changes
Possibly related issues
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional Context UsedPath-based Instructions (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
C:CLI
C:x/gov
C:x/slashing
C:Simulations
C:x/bank
C:x/auth
C:x/genutil
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
simapp/sims/simulation_helpers.go
Outdated
| @@ -49,6 +49,7 @@ func SetupSimulation(config simtypes.Config, dirPrefix, dbName string, verbose, | |||
| return db, dir, logger, false, nil | |||
| } | |||
|
|
|||
| // Can move to simapp | |||
There was a problem hiding this comment.
we broke everyone two versions ago (v0.47) to move all of those from simapp to the sdk and made simapp as hard as possible to import (as it is our testing and demo app, so it shouldn't be imported).
I am not sure we should move this back.
There was a problem hiding this comment.
hmm the only reason I moved it to simapp is we just use SetupSimulation for integration & some simapp tests.
Let me see if I can move to x/simulate. How does it sound?
There was a problem hiding this comment.
Right, but those helpers should be used by chains
depinject/container.go
Outdated
| @@ -395,6 +395,7 @@ func (c *container) resolve(in providerInput, moduleKey *moduleKey, caller Locat | |||
| } | |||
|
|
|||
| vr, err := c.getResolver(in.Type, moduleKey) | |||
| fmt.Println("getResolver", vr, err, in.Type, moduleKey, in.Optional) | |||
Try some hacks to move all x/ related to genutil. Move |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -1047,6 +1047,7 @@ func (s *E2ETestSuite) TestCLIMultisign() { | |||
| } | |||
| return clientCtx.Codec.UnmarshalJSON(resp, &balRes) | |||
| }, 3) | |||
| fmt.Println("RetryForBlocks err", err) | |||
There was a problem hiding this comment.
Consider handling the error from RetryForBlocks more robustly.
The current implementation prints the error but does not handle it, which could lead to further execution with an undefined state. It would be better to assert no error or handle it appropriately to ensure the test's integrity.
| genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types" | ||
| simtestutil "github.com/cosmos/cosmos-sdk/x/simulation/helper" |
There was a problem hiding this comment.
Replace simtestutil with the correct import from github.com/cosmos/cosmos-sdk/x/simulation/helper as per PR objectives.
| @@ -221,3 +232,37 @@ | |||
| func createConfigFolder(dir string) error { | |||
| return os.Mkdir(path.Join(dir, "config"), 0o700) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Replace simtestutil.NewAppOptionsWithFlagHome with the correct utility from github.com/cosmos/cosmos-sdk/x/simulation/helper as per PR objectives.
x/auth/depinject.go
Outdated
| "fmt" | ||
|
|
There was a problem hiding this comment.
Consider removing the fmt import and fmt.Println statement from production code to maintain cleanliness and performance.
- import (
- "fmt"
- )
- fmt.Println("Account Keeper", k)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.
| "fmt" |
| ) | ||
|
|
||
| // Can move to tests/integration |
There was a problem hiding this comment.
The comment about moving the test function to a different directory suggests a potential improvement in test organization. Consider creating a GitHub issue to track this suggestion.
Would you like me to open a GitHub issue to track the potential reorganization of test functions as suggested in the comment?
|
|
||
| appGenStateJSON, err := json.MarshalIndent(cfg.GenesisState, "", " ") | ||
| func initGenFiles(cfg Config, genAccs []sdk.AccAddress, genFiles []string) error { | ||
| genState, err := genutil.InitGenFileFromAddrs(genAccs, cfg.GenesisState, cfg.Codec, cfg.ChainID, cfg.BondDenom, cfg.StakingTokens, cfg.AccountTokens) |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
x/genutil/utils.go (2)
17-33: Consider organizing imports according to the Uber Golang style guide.Imports should be grouped in the following order: standard library, third-party packages, and then internal packages. This helps in improving readability and maintainability of the code.
Line range hint
63-65: Reduce the permissions for directory creation to enhance security.Using
0o777permissions allows all users to read, write, and execute files in this directory, which can lead to security vulnerabilities. Consider using more restrictive permissions, such as0o750.
| codec.MustUnmarshalJSON(genState[authtypes.ModuleName], &authGenState) | ||
|
|
||
| accounts, err := authtypes.PackAccounts(genAccounts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| authGenState.Accounts = append(authGenState.Accounts, accounts...) | ||
| genState[authtypes.ModuleName] = codec.MustMarshalJSON(&authGenState) | ||
|
|
||
| // set the balances in the genesis state | ||
| codec.MustUnmarshalJSON(genState[banktypes.ModuleName], &bankGenState) | ||
|
|
||
| bankGenState.Balances = append(bankGenState.Balances, genBalances...) | ||
| genState[banktypes.ModuleName] = codec.MustMarshalJSON(&bankGenState) |
There was a problem hiding this comment.
Consider handling JSON operations errors gracefully instead of using methods that can panic.
Using MustUnmarshalJSON and MustMarshalJSON can cause the application to panic if there's an error in JSON operations. It's safer to handle these errors gracefully to prevent the application from crashing unexpectedly.
|
closing this for now. We will go a different approach to try to avoid breaking the apis |
Description
Closes: #18290
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.
I have...
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation