test: migrate e2e/tx tests to systemtest #22152
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve the deletion of an end-to-end test suite for transaction services located in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/genesis_io.go (1)
60-70: LGTM: Well-implemented helper function with a minor suggestion.The
StoreTempFilefunction is well-implemented and follows best practices:
- It's correctly marked as a helper function with
t.Helper().- Error handling is appropriately done using
require.NoError.- The file is properly closed before returning.
Consider adding a comment to explain that the file is closed before returning, as this might not be immediately obvious to all readers:
// StoreTempFile creates a temporary file in the test's temporary directory with the provided content. // It returns a pointer to the created file. The file is closed before returning. // Errors during the process are handled with test assertions. func StoreTempFile(t *testing.T, content []byte) *os.File { // ... (existing implementation) }tests/systemtests/tx_test.go (1)
1077-1081: Use consistent paths for keyring home directoriesThe
--homeflag paths in commands should be consistent. In lines 1077 and 1079, the paths are--home=./testnet, whereas in other places like line 160,--home=./testnet/node0/simdis used. Ensure that the paths point to the correct directories and consider defining a variable for the home path to avoid confusion.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/systemtests/testdata/tx_amino1.binis excluded by!**/*.bin
📒 Files selected for processing (5)
- tests/e2e/tx/service_test.go (0 hunks)
- tests/e2e/tx/testdata/tx_amino1.json (0 hunks)
- tests/systemtests/genesis_io.go (2 hunks)
- tests/systemtests/testdata/tx_amino1.json (1 hunks)
- tests/systemtests/tx_test.go (1 hunks)
💤 Files with no reviewable changes (2)
- tests/e2e/tx/service_test.go
- tests/e2e/tx/testdata/tx_amino1.json
✅ Files skipped from review due to trivial changes (1)
- tests/systemtests/testdata/tx_amino1.json
🧰 Additional context used
📓 Path-based instructions (2)
tests/systemtests/genesis_io.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"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"
🔇 Additional comments (1)
tests/systemtests/genesis_io.go (1)
4-7: LGTM: New imports are correctly placed and necessary.The new imports for "bytes", "io", and "os" are correctly placed and are required for the newly added
StoreTempFilefunction. This adheres to the Uber Golang style guide.
tests/systemtests/tx_test.go
Outdated
| func TestSimulateTx_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| receiverAddr := cli.AddKey("account1") | ||
| denom := "stake" | ||
| var transferAmount int64 = 1000 | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| // create unsign tx | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| res := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| txFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "encode", signedTxFile.Name()) | ||
| txBz, err := base64.StdEncoding.DecodeString(res) | ||
| require.NoError(t, err) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.SimulateRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.SimulateRequest{}, true, "empty txBytes is not allowed"}, | ||
| {"valid request with tx_bytes", &tx.SimulateRequest{TxBytes: txBz}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Broadcast the tx via gRPC via the validator's clientCtx (which goes | ||
| // through Tendermint). | ||
| res, err := qc.Simulate(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| // Check the result and gas used are correct. | ||
| // | ||
| // The 12 events are: | ||
| // - Sending Fee to the pool: coin_spent, coin_received and transfer | ||
| // - tx.* events: tx.fee, tx.acc_seq, tx.signature | ||
| // - Sending Amount to recipient: coin_spent, coin_received and transfer | ||
| // - Msg events: message.module=bank, message.action=/cosmos.bank.v1beta1.MsgSend and message.sender=<val1> (in one message) | ||
| require.Equal(t, 10, len(res.GetResult().GetEvents())) | ||
| require.True(t, res.GetGasInfo().GetGasUsed() > 0) // Gas used sometimes change, just check it's not empty. | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in TestSimulateTx_GRPC and TestSimulateTx_GRPCGateway
The functions TestSimulateTx_GRPC (lines 73-135) and TestSimulateTx_GRPCGateway (lines 137-199) contain duplicated code. Consider refactoring the common setup and logic into helper functions to improve maintainability and reduce redundancy.
Also applies to: 137-199
tests/systemtests/tx_test.go
Outdated
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
|
|
||
| rsp = cli.Run("tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=1stake") | ||
| txResult, found = cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.GetTxsEventRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| expLen int | ||
| }{ | ||
| { | ||
| "nil request", | ||
| nil, | ||
| true, | ||
| "request cannot be nil", | ||
| 0, | ||
| }, | ||
| { | ||
| "empty request", | ||
| &tx.GetTxsEventRequest{}, | ||
| true, | ||
| "query cannot be empty", | ||
| 0, | ||
| }, | ||
| { | ||
| "request with dummy event", | ||
| &tx.GetTxsEventRequest{Query: "foobar"}, | ||
| true, | ||
| "failed to search for txs", | ||
| 0, | ||
| }, | ||
| { | ||
| "request with order-by", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: bankMsgSendEventAction, | ||
| OrderBy: tx.OrderBy_ORDER_BY_ASC, | ||
| }, | ||
| false, | ||
| "", | ||
| 2, | ||
| }, | ||
| { | ||
| "without pagination", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: bankMsgSendEventAction, | ||
| }, | ||
| false, | ||
| "", | ||
| 2, | ||
| }, | ||
| { | ||
| "with pagination", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: bankMsgSendEventAction, | ||
| Page: 1, | ||
| Limit: 1, | ||
| }, | ||
| false, | ||
| "", | ||
| 1, | ||
| }, | ||
| { | ||
| "with multi events", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: fmt.Sprintf("%s AND message.module='bank'", bankMsgSendEventAction), | ||
| }, | ||
| false, | ||
| "", | ||
| 2, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Query the tx via gRPC. | ||
| grpcRes, err := qc.GetTxsEvent(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.GreaterOrEqual(t, len(grpcRes.Txs), 1) | ||
| require.Equal(t, "foobar", grpcRes.Txs[0].Body.Memo) | ||
| require.Equal(t, tc.expLen, len(grpcRes.Txs)) | ||
|
|
||
| // Make sure fields are populated. | ||
| // ref: https://github.com/cosmos/cosmos-sdk/issues/8680 | ||
| // ref: https://github.com/cosmos/cosmos-sdk/issues/8681 | ||
| require.NotEmpty(t, grpcRes.TxResponses[0].Timestamp) | ||
| require.Empty(t, grpcRes.TxResponses[0].RawLog) // logs are empty if the transactions are successful | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in transaction event tests
The functions TestGetTxEvents_GRPC (lines 201-317) and TestGetTxEvents_GRPCGateway (lines 319-415) have significant overlap in their code. Refactoring shared code into helper functions can improve readability and reduce duplication.
Also applies to: 319-415
tests/systemtests/tx_test.go
Outdated
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| // create unsign tx | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| res := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| txFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "encode", signedTxFile.Name()) | ||
| txBz, err := base64.StdEncoding.DecodeString(res) | ||
| require.NoError(t, err) | ||
| invalidTxBytes := append(txBz, byte(0o00)) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.TxDecodeRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.TxDecodeRequest{}, true, "invalid empty tx bytes"}, | ||
| {"invalid tx bytes", &tx.TxDecodeRequest{TxBytes: invalidTxBytes}, true, "tx parse error"}, | ||
| {"valid request with tx bytes", &tx.TxDecodeRequest{TxBytes: txBz}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := qc.TxDecode(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| require.Empty(t, res) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, res.GetTx()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestTxDecode_GRPCGateway(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| receiverAddr := cli.AddKey("account1") | ||
| denom := "stake" | ||
| var transferAmount int64 = 1000 | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| basrUrl := sut.APIAddress() | ||
|
|
||
| // create unsign tx | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| res := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| txFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "encode", signedTxFile.Name()) | ||
| txBz, err := base64.StdEncoding.DecodeString(res) | ||
| require.NoError(t, err) | ||
| invalidTxBytes := append(txBz, byte(0o00)) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.TxDecodeRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"empty request", &tx.TxDecodeRequest{}, true, "invalid empty tx bytes"}, | ||
| {"invalid tx bytes", &tx.TxDecodeRequest{TxBytes: invalidTxBytes}, true, "tx parse error"}, | ||
| {"valid request with tx_bytes", &tx.TxDecodeRequest{TxBytes: txBz}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| reqBz, err := json.Marshal(tc.req) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err := testutil.PostRequest(fmt.Sprintf("%s/cosmos/tx/v1beta1/decode", basrUrl), "application/json", reqBz) | ||
| require.NoError(t, err) | ||
| if tc.expErr { | ||
| require.Contains(t, string(res), tc.expErrMsg) | ||
| } else { | ||
| signatures := gjson.Get(string(res), "tx.signatures").Array() | ||
| require.Equal(t, len(signatures), 1) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in TestTxDecode_GRPC and TestTxDecode_GRPCGateway
The functions TestTxDecode_GRPC (lines 742-855) and TestTxDecode_GRPCGateway (lines 799-855) have similar code structures and duplicated logic. Refactoring shared code into helper functions can improve maintainability.
Also applies to: 799-855
| txJSONBytes, err := os.ReadFile("testdata/tx_amino1.json") | ||
| require.NoError(t, err) | ||
| var stdTx legacytx.StdTx | ||
| err = aminoCodec.UnmarshalJSON(txJSONBytes, &stdTx) | ||
| require.NoError(t, err) | ||
| return txJSONBytes, &stdTx | ||
| } | ||
|
|
||
| func readTestAminoTxBinary(t *testing.T, aminoCodec *codec.LegacyAmino) ([]byte, *legacytx.StdTx) { | ||
| txJSONBytes, err := os.ReadFile("testdata/tx_amino1.bin") | ||
| require.NoError(t, err) | ||
| var stdTx legacytx.StdTx | ||
| err = aminoCodec.Unmarshal(txJSONBytes, &stdTx) | ||
| require.NoError(t, err) | ||
| return txJSONBytes, &stdTx | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Combine similar helper functions for reading test transactions
The functions readTestAminoTxJSON (lines 1093-1100) and readTestAminoTxBinary (lines 1101-1108) are similar in functionality. Consider combining them into a single function that can handle both JSON and binary formats to reduce code duplication.
Also applies to: 1101-1108
|
|
||
| sut.StartChain(t) | ||
|
|
||
| basrUrl := sut.APIAddress() |
There was a problem hiding this comment.
Fix typo in variable name 'basrUrl'
There's a typo in the variable name on line 814. It should be baseUrl instead of basrUrl to maintain consistency with other instances in the code.
Apply this diff to fix the typo:
- basrUrl := sut.APIAddress()
+ baseUrl := sut.APIAddress()📝 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.
| basrUrl := sut.APIAddress() | |
| baseUrl := sut.APIAddress() |
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/tx_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (3)
tests/systemtests/tx_test.go (3)
36-73:TestQueryBySig: Good test coverageThe
TestQueryBySigfunction effectively tests querying transactions by signature. It sets up the necessary environment, constructs a transaction, and verifies that the correct transaction is retrieved based on the signature.
36-1071: Test suite provides comprehensive coverageThe added test functions cover various transaction scenarios, including querying, simulating, encoding, decoding, and multi-signature transactions. This comprehensive test suite aligns with the PR objectives and enhances the reliability of the transaction functionalities.
796-796:⚠️ Potential issueFix typo in variable name
There's a typo on line 796:
basrUrlshould bebaseUrlto maintain consistency and avoid confusion.Apply this diff to correct the typo:
- basrUrl := sut.APIAddress() + baseUrl := sut.APIAddress()Likely invalid or redundant comment.
tests/systemtests/tx_test.go
Outdated
| func TestSimulateTx_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| receiverAddr := cli.AddKey("account1") | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| // create unsign tx | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| res := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| txFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "encode", signedTxFile.Name()) | ||
| txBz, err := base64.StdEncoding.DecodeString(res) | ||
| require.NoError(t, err) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.SimulateRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.SimulateRequest{}, true, "empty txBytes is not allowed"}, | ||
| {"valid request with tx_bytes", &tx.SimulateRequest{TxBytes: txBz}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Broadcast the tx via gRPC via the validator's clientCtx (which goes | ||
| // through Tendermint). | ||
| res, err := qc.Simulate(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| // Check the result and gas used are correct. | ||
| // | ||
| // The 12 events are: | ||
| // - Sending Fee to the pool: coin_spent, coin_received and transfer | ||
| // - tx.* events: tx.fee, tx.acc_seq, tx.signature | ||
| // - Sending Amount to recipient: coin_spent, coin_received and transfer | ||
| // - Msg events: message.module=bank, message.action=/cosmos.bank.v1beta1.MsgSend and message.sender=<val1> (in one message) | ||
| require.Equal(t, 10, len(res.GetResult().GetEvents())) | ||
| require.True(t, res.GetGasInfo().GetGasUsed() > 0) // Gas used sometimes change, just check it's not empty. | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in simulation test functions
The functions TestSimulateTx_GRPC (lines 75-135) and TestSimulateTx_GRPCGateway (lines 137-197) contain significant duplicated code. Consider refactoring common setup and logic into helper functions to reduce redundancy and improve maintainability.
Also applies to: 137-197
tests/systemtests/tx_test.go
Outdated
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
|
|
||
| rsp = cli.Run("tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=1stake") | ||
| txResult, found = cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.GetTxsEventRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| expLen int | ||
| }{ | ||
| { | ||
| "nil request", | ||
| nil, | ||
| true, | ||
| "request cannot be nil", | ||
| 0, | ||
| }, | ||
| { | ||
| "empty request", | ||
| &tx.GetTxsEventRequest{}, | ||
| true, | ||
| "query cannot be empty", | ||
| 0, | ||
| }, | ||
| { | ||
| "request with dummy event", | ||
| &tx.GetTxsEventRequest{Query: "foobar"}, | ||
| true, | ||
| "failed to search for txs", | ||
| 0, | ||
| }, | ||
| { | ||
| "request with order-by", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: bankMsgSendEventAction, | ||
| OrderBy: tx.OrderBy_ORDER_BY_ASC, | ||
| }, | ||
| false, | ||
| "", | ||
| 2, | ||
| }, | ||
| { | ||
| "without pagination", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: bankMsgSendEventAction, | ||
| }, | ||
| false, | ||
| "", | ||
| 2, | ||
| }, | ||
| { | ||
| "with pagination", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: bankMsgSendEventAction, | ||
| Page: 1, | ||
| Limit: 1, | ||
| }, | ||
| false, | ||
| "", | ||
| 1, | ||
| }, | ||
| { | ||
| "with multi events", | ||
| &tx.GetTxsEventRequest{ | ||
| Query: fmt.Sprintf("%s AND message.module='bank'", bankMsgSendEventAction), | ||
| }, | ||
| false, | ||
| "", | ||
| 2, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Query the tx via gRPC. | ||
| grpcRes, err := qc.GetTxsEvent(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.GreaterOrEqual(t, len(grpcRes.Txs), 1) | ||
| require.Equal(t, "foobar", grpcRes.Txs[0].Body.Memo) | ||
| require.Equal(t, tc.expLen, len(grpcRes.Txs)) | ||
|
|
||
| // Make sure fields are populated. | ||
| // ref: https://github.com/cosmos/cosmos-sdk/issues/8680 | ||
| // ref: https://github.com/cosmos/cosmos-sdk/issues/8681 | ||
| require.NotEmpty(t, grpcRes.TxResponses[0].Timestamp) | ||
| require.Empty(t, grpcRes.TxResponses[0].RawLog) // logs are empty if the transactions are successful | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in transaction event test functions
The functions TestGetTxEvents_GRPC (lines 199-313) and TestGetTxEvents_GRPCGateway (lines 315-409) have substantial overlap. Refactoring shared code into reusable helper functions can enhance readability and reduce code duplication.
Also applies to: 315-409
tests/systemtests/tx_test.go
Outdated
| func TestTxDecode_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| receiverAddr := cli.AddKey("account1") | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| // create unsign tx | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| res := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| txFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "encode", signedTxFile.Name()) | ||
| txBz, err := base64.StdEncoding.DecodeString(res) | ||
| require.NoError(t, err) | ||
| invalidTxBytes := append(txBz, byte(0o00)) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.TxDecodeRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.TxDecodeRequest{}, true, "invalid empty tx bytes"}, | ||
| {"invalid tx bytes", &tx.TxDecodeRequest{TxBytes: invalidTxBytes}, true, "tx parse error"}, | ||
| {"valid request with tx bytes", &tx.TxDecodeRequest{TxBytes: txBz}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := qc.TxDecode(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| require.Empty(t, res) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, res.GetTx()) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in transaction decode test functions
The functions TestTxDecode_GRPC (lines 728-781) and TestTxDecode_GRPCGateway (lines 783-837) have similar structures and logic. Consolidate common code into helper functions to improve code maintainability.
Also applies to: 783-837
tests/systemtests/tx_test.go
Outdated
| func TestSimMultiSigTx(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| _ = cli.AddKey("account1") | ||
| _ = cli.AddKey("account2") | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| multiSigName := "multisig" | ||
| cli.RunCommandWithArgs("keys", "add", multiSigName, "--multisig=account1,account2", "--multisig-threshold=2", "--keyring-backend=test", "--home=./testnet") | ||
| multiSigAddr := cli.GetKeyAddr(multiSigName) | ||
|
|
||
| // Send from validator to multisig addr | ||
| rsp := cli.Run("tx", "bank", "send", valAddr, multiSigAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=1stake") | ||
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
|
|
||
| multiSigBalance := cli.QueryBalance(multiSigAddr, denom) | ||
| require.Equal(t, multiSigBalance, transferAmount) | ||
|
|
||
| // Send from multisig to validator | ||
| // create unsign tx | ||
| var newTransferAmount int64 = 100 | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", multiSigAddr, valAddr, fmt.Sprintf("%d%s", newTransferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| res := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| txFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", "account1"), fmt.Sprintf("--multisig=%s", multiSigAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet") | ||
| account1Signed := StoreTempFile(t, []byte(res)) | ||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", "account2"), fmt.Sprintf("--multisig=%s", multiSigAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet") | ||
| account2Signed := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "multisign-batch", txFile.Name(), multiSigName, account1Signed.Name(), account2Signed.Name(), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet") | ||
| txSignedFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.Run("tx", "broadcast", txSignedFile.Name()) | ||
| RequireTxSuccess(t, res) | ||
|
|
||
| multiSigBalance = cli.QueryBalance(multiSigAddr, denom) | ||
| require.Equal(t, multiSigBalance, transferAmount-newTransferAmount-10) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper error handling in multi-signature transaction test
In the TestSimMultiSigTx function (lines 1023-1070), consider adding error checks after running commands with cli.RunCommandWithArgs and cli.Run. While the test may fail if an error occurs, explicit error handling can provide clearer test failures and make debugging easier.
| func TestGetTx_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| receiverAddr := cli.AddKey("account1") | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| rsp := cli.Run("tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=1stake", "--note=foobar") | ||
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
| txHash := gjson.Get(txResult, "txhash").String() | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.GetTxRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.GetTxRequest{}, true, "tx hash cannot be empty"}, | ||
| {"request with dummy hash", &tx.GetTxRequest{Hash: "deadbeef"}, true, "code = NotFound desc = tx not found: deadbeef"}, | ||
| {"good request", &tx.GetTxRequest{Hash: txHash}, false, ""}, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Query the tx via gRPC. | ||
| grpcRes, err := qc.GetTx(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.Equal(t, "foobar", grpcRes.Tx.Body.Memo) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate TestGetTx_GRPC and TestGetTx_GRPCGateway
The functions TestGetTx_GRPC (lines 411-456) and TestGetTx_GRPCGateway (lines 458-519) share similar code for testing transaction retrieval. Refactoring shared logic into helper functions can reduce duplication and improve test maintainability.
Also applies to: 458-519
| func TestGetBlockWithTxs_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| // add new key | ||
| receiverAddr := cli.AddKey("account1") | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| rsp := cli.Run("tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=1stake", "--note=foobar") | ||
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) | ||
| height := gjson.Get(txResult, "height").Int() | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.GetBlockWithTxsRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| expTxsLen int | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil", 0}, | ||
| {"empty request", &tx.GetBlockWithTxsRequest{}, true, "height must not be less than 1 or greater than the current height", 0}, | ||
| {"bad height", &tx.GetBlockWithTxsRequest{Height: 99999999}, true, "height must not be less than 1 or greater than the current height", 0}, | ||
| {"bad pagination", &tx.GetBlockWithTxsRequest{Height: height, Pagination: &query.PageRequest{Offset: 1000, Limit: 100}}, true, "out of range", 0}, | ||
| {"good request", &tx.GetBlockWithTxsRequest{Height: height}, false, "", 1}, | ||
| {"with pagination request", &tx.GetBlockWithTxsRequest{Height: height, Pagination: &query.PageRequest{Offset: 0, Limit: 1}}, false, "", 1}, | ||
| {"page all request", &tx.GetBlockWithTxsRequest{Height: height, Pagination: &query.PageRequest{Offset: 0, Limit: 100}}, false, "", 1}, | ||
| {"block with 0 tx", &tx.GetBlockWithTxsRequest{Height: height - 1, Pagination: &query.PageRequest{Offset: 0, Limit: 100}}, false, "", 0}, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Query the tx via gRPC. | ||
| grpcRes, err := qc.GetBlockWithTxs(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| } else { | ||
| require.NoError(t, err) | ||
| if tc.expTxsLen > 0 { | ||
| require.Equal(t, "foobar", grpcRes.Txs[0].Body.Memo) | ||
| } | ||
| require.Equal(t, grpcRes.Block.Header.Height, tc.req.Height) | ||
| if tc.req.Pagination != nil { | ||
| require.LessOrEqual(t, len(grpcRes.Txs), int(tc.req.Pagination.Limit)) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in block transaction retrieval tests
The functions TestGetBlockWithTxs_GRPC (lines 521-577) and TestGetBlockWithTxs_GRPCGateway (lines 579-637) have overlapping code. Consider extracting common code into reusable helper functions to enhance code clarity and reduce redundancy.
Also applies to: 579-637
| func TestTxEncode_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
|
|
||
| cli := NewCLIWrapper(t, sut, verbose) | ||
| // get validator address | ||
| valAddr := cli.GetKeyAddr("node0") | ||
| require.NotEmpty(t, valAddr) | ||
|
|
||
| sut.StartChain(t) | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
|
|
||
| protoTx := &tx.Tx{ | ||
| Body: &tx.TxBody{ | ||
| Messages: []*codectypes.Any{}, | ||
| }, | ||
| AuthInfo: &tx.AuthInfo{}, | ||
| Signatures: [][]byte{}, | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.TxEncodeRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.TxEncodeRequest{}, true, "invalid empty tx"}, | ||
| {"valid tx request", &tx.TxEncodeRequest{Tx: protoTx}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := qc.TxEncode(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| require.Empty(t, res) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in transaction encode test functions
The functions TestTxEncode_GRPC (lines 639-682) and TestTxEncode_GRPCGateway (lines 684-726) perform similar operations for testing transaction encoding. Refactoring shared code into helper functions can improve code reuse and readability.
Also applies to: 684-726
| func TestTxEncodeAmino_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
| sut.StartChain(t) | ||
|
|
||
| legacyAmino := codec.NewLegacyAmino() | ||
| std.RegisterLegacyAminoCodec(legacyAmino) | ||
| legacytx.RegisterLegacyAminoCodec(legacyAmino) | ||
| legacy.RegisterAminoMsg(legacyAmino, &banktypes.MsgSend{}, "cosmos-sdk/MsgSend") | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
| txJSONBytes, stdTx := readTestAminoTxJSON(t, legacyAmino) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.TxEncodeAminoRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.TxEncodeAminoRequest{}, true, "invalid empty tx json"}, | ||
| {"invalid request", &tx.TxEncodeAminoRequest{AminoJson: "invalid tx json"}, true, "invalid request"}, | ||
| {"valid request with amino-json", &tx.TxEncodeAminoRequest{AminoJson: string(txJSONBytes)}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := qc.TxEncodeAmino(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| require.Empty(t, res) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, res.GetAminoBinary()) | ||
|
|
||
| var decodedTx legacytx.StdTx | ||
| err = legacyAmino.Unmarshal(res.AminoBinary, &decodedTx) | ||
| require.NoError(t, err) | ||
| require.Equal(t, decodedTx.GetMsgs(), stdTx.GetMsgs()) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in Amino transaction encode tests
The functions TestTxEncodeAmino_GRPC (lines 839-881) and TestTxEncodeAmino_GRPCGateway (lines 883-927) contain duplicated code. Consolidate common logic into helper functions to reduce code duplication and enhance maintainability.
Also applies to: 883-927
| func TestTxDecodeAmino_GRPC(t *testing.T) { | ||
| sut.ResetChain(t) | ||
| sut.StartChain(t) | ||
|
|
||
| legacyAmino := codec.NewLegacyAmino() | ||
| std.RegisterLegacyAminoCodec(legacyAmino) | ||
| legacytx.RegisterLegacyAminoCodec(legacyAmino) | ||
| legacy.RegisterAminoMsg(legacyAmino, &banktypes.MsgSend{}, "cosmos-sdk/MsgSend") | ||
|
|
||
| qc := tx.NewServiceClient(sut.RPCClient(t)) | ||
| encodedTx, stdTx := readTestAminoTxBinary(t, legacyAmino) | ||
|
|
||
| invalidTxBytes := append(encodedTx, byte(0o00)) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| req *tx.TxDecodeAminoRequest | ||
| expErr bool | ||
| expErrMsg string | ||
| }{ | ||
| {"nil request", nil, true, "request cannot be nil"}, | ||
| {"empty request", &tx.TxDecodeAminoRequest{}, true, "invalid empty tx bytes"}, | ||
| {"invalid tx bytes", &tx.TxDecodeAminoRequest{AminoBinary: invalidTxBytes}, true, "invalid request"}, | ||
| {"valid request with tx bytes", &tx.TxDecodeAminoRequest{AminoBinary: encodedTx}, false, ""}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res, err := qc.TxDecodeAmino(context.Background(), tc.req) | ||
| if tc.expErr { | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tc.expErrMsg) | ||
| require.Empty(t, res) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, res.GetAminoJson()) | ||
|
|
||
| var decodedTx legacytx.StdTx | ||
| err = legacyAmino.UnmarshalJSON([]byte(res.GetAminoJson()), &decodedTx) | ||
| require.NoError(t, err) | ||
| require.Equal(t, stdTx.GetMsgs(), decodedTx.GetMsgs()) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in Amino transaction decode tests
The functions TestTxDecodeAmino_GRPC (lines 929-973) and TestTxDecodeAmino_GRPCGateway (lines 975-1021) have similar structures. Refactoring shared code into reusable functions can improve code organization and reduce redundancy.
Also applies to: 975-1021
julienrbrt
added
the
backport/v0.52.x
|
Looks like the system tests (v1) are failing here. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/tx_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
| require.NoError(t, err) | ||
| msgResponses := gjson.Get(string(res), "result.msg_responses").Array() | ||
| require.Equal(t, len(msgResponses), 1) | ||
|
|
There was a problem hiding this comment.
Swap arguments in require.Equal assertions for correct order.
In the require.Equal assertions, the expected value should precede the actual value. The arguments are reversed in several places.
Examples:
- Line 190: Change
require.Equal(t, len(msgResponses), 1)torequire.Equal(t, 1, len(msgResponses)). - Line 407: Change
require.Equal(t, len(txs), tc.expLen)torequire.Equal(t, tc.expLen, len(txs)). - Line 835: Change
require.Equal(t, len(signatures), 1)torequire.Equal(t, 1, len(signatures)).
Apply the following diffs:
190
- require.Equal(t, len(msgResponses), 1)
+ require.Equal(t, 1, len(msgResponses))407
- require.Equal(t, len(txs), tc.expLen)
+ require.Equal(t, tc.expLen, len(txs))835
- require.Equal(t, len(signatures), 1)
+ require.Equal(t, 1, len(signatures))Also applies to: 407-407, 835-835
tests/systemtests/tx_test.go
Outdated
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.Run("tx", "broadcast", signedTxFile.Name()) | ||
| fmt.Println("resss", res) |
There was a problem hiding this comment.
Remove unnecessary fmt.Println statements.
The fmt.Println statements on lines 61 and 71 are unnecessary and should be removed to keep the test output clean.
Apply this diff:
61
- fmt.Println("resss", res)
71
- fmt.Println("resppp", resp)Also applies to: 71-71
| // The 12 events are: | ||
| // - Sending Fee to the pool: coin_spent, coin_received and transfer | ||
| // - tx.* events: tx.fee, tx.acc_seq, tx.signature | ||
| // - Sending Amount to recipient: coin_spent, coin_received and transfer | ||
| // - Msg events: message.module=bank, message.action=/cosmos.bank.v1beta1.MsgSend and message.sender=<val1> (in one message) | ||
| require.Equal(t, 10, len(res.GetResult().GetEvents())) |
There was a problem hiding this comment.
Correct the expected number of events in the assertion or comment.
The comment mentions that there are 12 events expected, but the assertion checks for 10 events. This inconsistency should be addressed by updating the comment or the assertion to match the correct number of events.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/tx_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (1)
tests/systemtests/tx_test.go (1)
800-800: Fix typo in variable namebasrUrl.The variable
basrUrlon line 800 is misspelled. It should bebaseUrlto maintain consistency.
tests/systemtests/tx_test.go
Outdated
| // create unsign tx | ||
| bankSendCmdArgs := []string{"tx", "bank", "send", valAddr, receiverAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--fees=10stake", "--sign-mode=direct", "--generate-only"} | ||
| unsignedTx := cli.RunCommandWithArgs(bankSendCmdArgs...) | ||
| fmt.Println("unsignedTx", unsignedTx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary fmt.Println statement.
The fmt.Println statement on line 54 is unnecessary and can be removed to keep the test output clean.
tests/systemtests/tx_test.go
Outdated
| txFile := StoreTempFile(t, []byte(unsignedTx)) | ||
|
|
||
| signedTx := cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| fmt.Println("signedTx", signedTx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary fmt.Println statement.
The fmt.Println statement on line 58 is unnecessary and can be removed to maintain clean test output.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/tx_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (1)
tests/systemtests/tx_test.go (1)
58-58: 🛠️ Refactor suggestionRemove unnecessary
fmt.Printlnstatement.The
fmt.Printlnstatement on line 58 is unnecessary and can be removed to keep the test output clean.Apply this diff to remove the unnecessary print statement:
- fmt.Println("signedTx", signedTx)Likely invalid or redundant comment.
| require.Equal(t, len(msgResponses), 1) | ||
|
|
||
| events := gjson.Get(string(res), "result.events").Array() | ||
| require.Equal(t, len(events), 10) |
There was a problem hiding this comment.
Swap arguments in require.Equal assertion for correct order.
In the require.Equal assertion on line 194, the expected value should precede the actual value. The arguments are currently reversed.
Apply this diff to correct the assertion:
- require.Equal(t, len(events), 10)
+ require.Equal(t, 10, len(events))📝 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.
| require.Equal(t, len(events), 10) | |
| require.Equal(t, 10, len(events)) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/systemtests/tx_test.go (3)
1086-1092: Rename variable 'txJSONBytes' to 'txBytes' for clarityIn the function
readTestAminoTxBinary, the variabletxJSONBytesis misleading because it reads binary data fromtx_amino1.bin. Renaming it totxBytesimproves code clarity.Apply this diff to rename the variable:
func readTestAminoTxBinary(t *testing.T, aminoCodec *codec.LegacyAmino) ([]byte, *legacytx.StdTx) { - txJSONBytes, err := os.ReadFile("testdata/tx_amino1.bin") + txBytes, err := os.ReadFile("testdata/tx_amino1.bin") require.NoError(t, err) var stdTx legacytx.StdTx - err = aminoCodec.Unmarshal(txJSONBytes, &stdTx) + err = aminoCodec.Unmarshal(txBytes, &stdTx) require.NoError(t, err) - return txJSONBytes, &stdTx + return txBytes, &stdTx }
154-154: Remove unnecessary commented-out codeThe commented-out code
// qc := tx.NewServiceClient(sut.RPCClient(t))appears unnecessary and can be removed to keep the codebase clean.- // qc := tx.NewServiceClient(sut.RPCClient(t))
758-758: Use hexadecimal notation for zero byte literalUsing
byte(0x00)instead ofbyte(0o00)for the zero byte literal improves readability and is more commonly used.Apply this diff for clarity:
-invalidTxBytes := append(txBz, byte(0o00)) +invalidTxBytes := append(txBz, byte(0x00))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/tx_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (1)
tests/systemtests/tx_test.go (1)
800-800:⚠️ Potential issueFix typo in variable name 'basrUrl'
There's a typo in the variable name on line 800. It should be
baseUrlinstead ofbasrUrlto maintain consistency with other instances in the code.Apply this diff to correct the variable name:
- basrUrl := sut.APIAddress() + baseUrl := sut.APIAddress()Likely invalid or redundant comment.
it should work now |
tests/systemtests/tx_test.go
Outdated
| // ref: https://github.com/cosmos/cosmos-sdk/issues/8680 | ||
| // ref: https://github.com/cosmos/cosmos-sdk/issues/8681 |
There was a problem hiding this comment.
Seems these links are closed. Please remove if not necessary.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
tests/systemtests/tx_test.go (2)
985-985: Avoid appending null byte to transaction bytesAppending a null byte (
0o00) toinvalidTxBytesmay not effectively simulate an invalid transaction. Consider using invalid or corrupted transaction bytes for more robust testing.You might generate invalid bytes like this:
invalidTxBytes := txBz[:len(txBz)-1]
854-855: Consistent error messages in test casesIn the test cases starting at line 854, the error messages in
expErrMsgfor invalid requests should be consistent and precise to facilitate accurate assertions.Review and standardize the
expErrMsgvalues for clarity and consistency.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/systemtests/tx_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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"
🔇 Additional comments (5)
tests/systemtests/tx_test.go (5)
794-794:⚠️ Potential issueFix typo in variable name
basrUrlThere's a typo in the variable name on line 794. It should be
baseUrlinstead ofbasrUrlto maintain consistency with other instances in the code.Apply this diff to fix the typo:
- basrUrl := sut.APIAddress() + baseUrl := sut.APIAddress()Likely invalid or redundant comment.
832-832:⚠️ Potential issueSwap arguments in
require.Equalassertion for correct orderIn the
require.Equalassertion on line 832, the expected value should precede the actual value. The arguments are currently reversed.Apply this diff to correct the assertion:
- require.Equal(t, len(signatures), 1) + require.Equal(t, 1, len(signatures))Likely invalid or redundant comment.
627-627:⚠️ Potential issueSwap arguments in
require.Equalassertion for correct orderIn the
require.Equalassertion on line 627, the expected value should precede the actual value. The arguments are currently reversed.Apply this diff to correct the assertion:
- require.Equal(t, memo, "foobar") + require.Equal(t, "foobar", memo)Likely invalid or redundant comment.
190-190:⚠️ Potential issueSwap arguments in
require.Equalassertion for correct orderIn the
require.Equalassertion on line 190, the expected value should precede the actual value. The arguments are currently reversed.Apply this diff to correct the assertion:
- require.Equal(t, len(events), 10) + require.Equal(t, 10, len(events))Likely invalid or redundant comment.
628-628:⚠️ Potential issueSwap arguments in
require.Equalassertion for correct orderIn the
require.Equalassertion on line 628, the expected value should precede the actual value. The arguments are currently reversed.Apply this diff to correct the assertion:
- require.Equal(t, respHeight, height) + require.Equal(t, height, respHeight)Likely invalid or redundant comment.
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", "account1"), fmt.Sprintf("--multisig=%s", multiSigAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet") | ||
| account1Signed := StoreTempFile(t, []byte(res)) | ||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", "account2"), fmt.Sprintf("--multisig=%s", multiSigAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet") | ||
| account2Signed := StoreTempFile(t, []byte(res)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for errors after signing transactions
In the TestSimMultiSigTx function, the calls to cli.RunCommandWithArgs on lines 1055 and 1057 are not followed by error checks. Although the test may fail if an error occurs, explicit error handling can provide clearer test failures and make debugging easier.
Consider adding error checks after these commands:
res, err := cli.RunCommandWithArgs(/* arguments */)
require.NoError(t, err)|
|
||
| res = cli.RunCommandWithArgs("tx", "sign", txFile.Name(), fmt.Sprintf("--from=%s", valAddr), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet/node0/simd") | ||
| signedTxFile := StoreTempFile(t, []byte(res)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for errors after signing and encoding transactions
In the TestTxDecode_GRPC function, after running cli.RunCommandWithArgs to sign and encode transactions, there are no error checks to confirm the commands executed successfully. Explicit error handling can improve test reliability.
Add error checks after signing and encoding:
res, err := cli.RunCommandWithArgs("tx", "sign", /* arguments */)
require.NoError(t, err)
res, err = cli.RunCommandWithArgs("tx", "encode", /* arguments */)
require.NoError(t, err)| account2Signed := StoreTempFile(t, []byte(res)) | ||
|
|
||
| res = cli.RunCommandWithArgs("tx", "multisign-batch", txFile.Name(), multiSigName, account1Signed.Name(), account2Signed.Name(), fmt.Sprintf("--chain-id=%s", sut.chainID), "--keyring-backend=test", "--home=./testnet") | ||
| txSignedFile := StoreTempFile(t, []byte(res)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for errors after multisigning transactions
In the TestSimMultiSigTx function, the call to cli.RunCommandWithArgs("tx", "multisign-batch", ...) on line 1061 does not include error handling. Adding an error check can help identify issues with the multisign process.
Add error handling:
res, err := cli.RunCommandWithArgs("tx", "multisign-batch", /* arguments */)
require.NoError(t, err)| fmt.Sprintf("%s/cosmos/tx/v1beta1/txs/block/%d", baseUrl, height), | ||
| false, "", |
There was a problem hiding this comment.
Ensure proper error handling for HTTP requests
In the TestGetBlockWithTxs_GRPCGateway function, after calling testutil.GetRequest, the code checks require.NoError(t, err) but does not handle HTTP error responses that might not set err. Consider checking the HTTP status code or response content for error indications.
Add checks to handle HTTP errors appropriately.
| } else { | ||
| require.NoError(t, err) | ||
| msgResponses := gjson.Get(string(res), "result.msg_responses").Array() | ||
| require.Equal(t, len(msgResponses), 1) |
There was a problem hiding this comment.
Swap arguments in require.Equal assertion for correct order
In the require.Equal assertion on line 187, the expected value should precede the actual value. The arguments are currently reversed.
Apply this diff to correct the assertion:
- require.Equal(t, len(msgResponses), 1)
+ require.Equal(t, 1, len(msgResponses))📝 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.
| require.Equal(t, len(msgResponses), 1) | |
| require.Equal(t, 1, len(msgResponses)) |
| } else { | ||
| require.NoError(t, err) | ||
| txs := gjson.Get(string(res), "txs").Array() | ||
| require.Equal(t, len(txs), tc.expLen) |
There was a problem hiding this comment.
Swap arguments in require.Equal assertion for correct order
In the require.Equal assertion on line 403, the expected value should precede the actual value. The arguments are currently reversed.
Apply this diff to correct the assertion:
- require.Equal(t, len(txs), tc.expLen)
+ require.Equal(t, tc.expLen, len(txs))📝 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.
| require.Equal(t, len(txs), tc.expLen) | |
| require.Equal(t, tc.expLen, len(txs)) |
| reqBz, err := json.Marshal(tc.req) | ||
| require.NoError(t, err) | ||
|
|
||
| res, err := testutil.PostRequest(fmt.Sprintf("%s/cosmos/tx/v1beta1/decode/amino", baseUrl), "application/json", reqBz) |
There was a problem hiding this comment.
Handle potential errors when decoding responses
In the TestTxDecodeAmino_GRPCGateway function, the error returned by json.Unmarshal on line 1009 is being shadowed. Ensure that the error variable is not redeclared to avoid unintended behavior.
Apply this diff to fix the error handling:
- err := json.Unmarshal(res, &result)
+ err = json.Unmarshal(res, &result)Committable suggestion was skipped due to low confidence.
(cherry picked from commit ea219a2)
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Description
Closes: #22038
Migrate
e2e/tx/service_test.goto systemtest.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Chores