Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(systemtests): Remove testutil dependency #21995

Merged
merged 2 commits into from
Oct 3, 2024
Merged

chore(systemtests): Remove testutil dependency #21995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7b186a
Remove testutil dependency
alpe Oct 1, 2024
12cd856
linting
tac0turtle Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions tests/systemtests/authz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package systemtests

import (
"fmt"
"net/http"
"os"
"testing"
"time"
Expand Down Expand Up @@ -670,73 +671,84 @@ func TestAuthzGRPCQueries(t *testing.T) {
invalidMsgTypeOutput := `{"code":2, "message":"codespace authz code 2: authorization not found: authorization not found for invalidMsg type", "details":[]}`
expGrantOutput := fmt.Sprintf(`{"grants":[{%s}],"pagination":null}`, grant1)

grantTestCases := []GRPCTestCase{
grantTestCases := []RestTestCase{
{
"invalid granter address",
fmt.Sprintf(grantURL, "invalid_granter", grantee1Addr, msgSendTypeURL),
http.StatusInternalServerError,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use http.StatusBadRequest instead of http.StatusInternalServerError for client errors

In your test cases, you're using http.StatusInternalServerError (500) when the client provides invalid input, such as invalid addresses or empty parameters. Since these are client-side errors due to bad requests, it's more appropriate to use http.StatusBadRequest (400) to indicate the nature of the error.

Apply this diff to update the status codes:

-http.StatusInternalServerError,
+http.StatusBadRequest,

Also applies to: 684-684, 690-690, 696-696, 702-702, 764-764, 791-791

bech32FailOutput,
},
{
"invalid grantee address",
fmt.Sprintf(grantURL, granterAddr, "invalid_grantee", msgSendTypeURL),
http.StatusInternalServerError,
bech32FailOutput,
},
{
"with empty granter",
fmt.Sprintf(grantURL, "", grantee1Addr, msgSendTypeURL),
http.StatusInternalServerError,
emptyStrOutput,
},
{
"with empty grantee",
fmt.Sprintf(grantURL, granterAddr, "", msgSendTypeURL),
http.StatusInternalServerError,
emptyStrOutput,
},
{
"invalid msg-type",
fmt.Sprintf(grantURL, granterAddr, grantee1Addr, "invalidMsg"),
http.StatusInternalServerError,
invalidMsgTypeOutput,
},
{
"valid grant query",
fmt.Sprintf(grantURL, granterAddr, grantee1Addr, msgSendTypeURL),
http.StatusOK,
expGrantOutput,
},
}

RunGRPCQueries(t, grantTestCases)
RunRestQueries(t, grantTestCases)

// test query grants grpc endpoint
grantsURL := baseurl + "/cosmos/authz/v1beta1/grants?granter=%s&grantee=%s"

grantsTestCases := []GRPCTestCase{
grantsTestCases := []RestTestCase{
{
"expect single grant",
fmt.Sprintf(grantsURL, granterAddr, grantee1Addr),
http.StatusOK,
fmt.Sprintf(`{"grants":[{%s}],"pagination":{"next_key":null,"total":"1"}}`, grant1),
},
{
"expect two grants",
fmt.Sprintf(grantsURL, granterAddr, grantee2Addr),
http.StatusOK,
fmt.Sprintf(`{"grants":[{%s},{%s}],"pagination":{"next_key":null,"total":"2"}}`, grant2, grant3),
},
{
"expect single grant with pagination",
fmt.Sprintf(grantsURL+"&pagination.limit=1", granterAddr, grantee2Addr),
http.StatusOK,
fmt.Sprintf(`{"grants":[{%s}],"pagination":{"next_key":"L2Nvc21vcy5nb3YudjEuTXNnVm90ZQ==","total":"0"}}`, grant2),
},
{
"expect single grant with pagination limit and offset",
fmt.Sprintf(grantsURL+"&pagination.limit=1&pagination.offset=1", granterAddr, grantee2Addr),
http.StatusOK,
fmt.Sprintf(`{"grants":[{%s}],"pagination":{"next_key":null,"total":"0"}}`, grant3),
},
{
"expect two grants with pagination",
fmt.Sprintf(grantsURL+"&pagination.limit=2", granterAddr, grantee2Addr),
http.StatusOK,
fmt.Sprintf(`{"grants":[{%s},{%s}],"pagination":{"next_key":null,"total":"0"}}`, grant2, grant3),
},
}

RunGRPCQueries(t, grantsTestCases)
RunRestQueries(t, grantsTestCases)

// test query grants by granter grpc endpoint
grantsByGranterURL := baseurl + "/cosmos/authz/v1beta1/grants/granter/%s"
Expand All @@ -745,49 +757,55 @@ func TestAuthzGRPCQueries(t *testing.T) {
granterQueryOutput := fmt.Sprintf(`{"grants":[{"granter":"%s","grantee":"%s",%s}],"pagination":{"next_key":null,"total":"1"}}`,
grantee1Addr, grantee2Addr, grant4)

granterTestCases := []GRPCTestCase{
granterTestCases := []RestTestCase{
{
"invalid granter account address",
fmt.Sprintf(grantsByGranterURL, "invalid address"),
http.StatusInternalServerError,
decodingFailedOutput,
},
{
"no authorizations found from granter",
fmt.Sprintf(grantsByGranterURL, grantee2Addr),
http.StatusOK,
noAuthorizationsOutput,
},
{
"valid granter query",
fmt.Sprintf(grantsByGranterURL, grantee1Addr),
http.StatusOK,
granterQueryOutput,
},
}

RunGRPCQueries(t, granterTestCases)
RunRestQueries(t, granterTestCases)

// test query grants by grantee grpc endpoint
grantsByGranteeURL := baseurl + "/cosmos/authz/v1beta1/grants/grantee/%s"
grantee1GrantsOutput := fmt.Sprintf(`{"grants":[{"granter":"%s","grantee":"%s",%s}],"pagination":{"next_key":null,"total":"1"}}`, granterAddr, grantee1Addr, grant1)

granteeTestCases := []GRPCTestCase{
granteeTestCases := []RestTestCase{
{
"invalid grantee account address",
fmt.Sprintf(grantsByGranteeURL, "invalid address"),
http.StatusInternalServerError,
decodingFailedOutput,
},
{
"no authorizations found from grantee",
fmt.Sprintf(grantsByGranteeURL, granterAddr),
http.StatusOK,
noAuthorizationsOutput,
},
{
"valid grantee query",
fmt.Sprintf(grantsByGranteeURL, grantee1Addr),
http.StatusOK,
grantee1GrantsOutput,
},
}

RunGRPCQueries(t, granteeTestCases)
RunRestQueries(t, granteeTestCases)
}

func setupChain(t *testing.T) (*CLIWrapper, string, string) {
Expand Down
37 changes: 23 additions & 14 deletions tests/systemtests/bank_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@ package systemtests

import (
"fmt"
"net/http"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/tidwall/gjson"
"github.com/tidwall/sjson"

"github.com/cosmos/cosmos-sdk/testutil"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

func TestBankSendTxCmd(t *testing.T) {
Expand Down Expand Up @@ -53,7 +51,7 @@ func TestBankSendTxCmd(t *testing.T) {
insufficientCmdArgs = append(insufficientCmdArgs, fmt.Sprintf("%d%s", valBalance, denom), "--fees=10stake")
rsp = cli.Run(insufficientCmdArgs...)
RequireTxFailure(t, rsp)
require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error())
require.Contains(t, rsp, "insufficient funds")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use error constants instead of hardcoded strings

Instead of hardcoding the error message "insufficient funds", consider using the predefined error constant sdkerrors.ErrInsufficientFunds.Error(). This approach enhances maintainability and reduces the risk of errors if the error message changes in the future.

Apply this diff to fix the issue:

-	require.Contains(t, rsp, "insufficient funds")
+	require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error())
📝 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.

Suggested change
require.Contains(t, rsp, "insufficient funds")
require.Contains(t, rsp, sdkerrors.ErrInsufficientFunds.Error())

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Prefer checking error codes over error messages

Directly asserting that the response contains "insufficient funds" is fragile because error messages can change. Instead, consider parsing the response to check the error code, which makes the test more robust and less dependent on the exact error message.

You can create an assertion function similar to assertUnauthorizedErr to check the error code:

assertInsufficientFundsErr := 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())
	// Replace <expected_error_code> with the actual error code for insufficient funds
	require.Equal(t, <expected_error_code>, code.Int())
	return false
}

Then modify your test to use this assertion:

-	require.Contains(t, rsp, "insufficient funds")
+	rsp = cli.WithRunErrorMatcher(assertInsufficientFundsErr).Run(insufficientCmdArgs...)


// test tx bank send with unauthorized signature
assertUnauthorizedErr := func(_ assert.TestingT, gotErr error, gotOutputs ...interface{}) bool {
Expand Down Expand Up @@ -234,23 +232,26 @@ func TestBankGRPCQueries(t *testing.T) {
blockHeight := sut.CurrentHeight()

supplyTestCases := []struct {
name string
url string
headers map[string]string
expOut string
name string
url string
headers map[string]string
expHttpCode int
expOut string
}{
{
"test GRPC total supply",
supplyUrl,
map[string]string{
blockHeightHeader: fmt.Sprintf("%d", blockHeight),
},
http.StatusOK,
expTotalSupplyOutput,
},
{
"test GRPC total supply of a specific denom",
supplyUrl + "/by_denom?denom=" + newDenom,
map[string]string{},
http.StatusOK,
specificDenomOutput,
},
{
Expand All @@ -259,67 +260,75 @@ func TestBankGRPCQueries(t *testing.T) {
map[string]string{
blockHeightHeader: fmt.Sprintf("%d", blockHeight+5),
},
http.StatusInternalServerError,
"invalid height",
},
{
"test GRPC total supply of a bogus denom",
supplyUrl + "/by_denom?denom=foobar",
map[string]string{},
http.StatusOK,
// http.StatusNotFound,
bogusDenomOutput,
},
}

for _, tc := range supplyTestCases {
t.Run(tc.name, func(t *testing.T) {
resp, err := testutil.GetRequestWithHeaders(tc.url, tc.headers)
require.NoError(t, err)
resp := GetRequestWithHeaders(t, tc.url, tc.headers, tc.expHttpCode)
require.Contains(t, string(resp), tc.expOut)
})
}

// test denom metadata endpoint
denomMetadataUrl := baseurl + "/cosmos/bank/v1beta1/denoms_metadata"
dmTestCases := []GRPCTestCase{
dmTestCases := []RestTestCase{
{
"test GRPC client metadata",
denomMetadataUrl,
http.StatusOK,
fmt.Sprintf(`{"metadatas":%s,"pagination":{"next_key":null,"total":"2"}}`, bankDenomMetadata),
},
{
"test GRPC client metadata of a specific denom",
denomMetadataUrl + "/uatom",
http.StatusOK,
fmt.Sprintf(`{"metadata":%s}`, atomDenomMetadata),
},
{
"test GRPC client metadata of a bogus denom",
denomMetadataUrl + "/foobar",
http.StatusNotFound,
`{"code":5, "message":"client metadata for denom foobar", "details":[]}`,
},
}

RunGRPCQueries(t, dmTestCases)
RunRestQueries(t, dmTestCases)

// test bank balances endpoint
balanceUrl := baseurl + "/cosmos/bank/v1beta1/balances/"
allBalancesOutput := `{"balances":[` + specificDenomOutput + `,{"denom":"stake","amount":"10000000"}],"pagination":{"next_key":null,"total":"2"}}`

balanceTestCases := []GRPCTestCase{
balanceTestCases := []RestTestCase{
{
"test GRPC total account balance",
balanceUrl + account1Addr,
http.StatusOK,
allBalancesOutput,
},
{
"test GRPC account balance of a specific denom",
fmt.Sprintf("%s%s/by_denom?denom=%s", balanceUrl, account1Addr, newDenom),
http.StatusOK,
fmt.Sprintf(`{"balance":%s}`, specificDenomOutput),
},
{
"test GRPC account balance of a bogus denom",
fmt.Sprintf("%s%s/by_denom?denom=foobar", balanceUrl, account1Addr),
http.StatusOK,
fmt.Sprintf(`{"balance":%s}`, bogusDenomOutput),
},
}

RunGRPCQueries(t, balanceTestCases)
RunRestQueries(t, balanceTestCases)
}
3 changes: 1 addition & 2 deletions tests/systemtests/bankv2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,9 @@ func TestBankV2SendTxCmd(t *testing.T) {
valBalanceAfer := gjson.Get(valRaw, "balance.amount").Int()

// TODO: Make DeductFee ante handler work with bank/v2
require.Equal(t, valBalanceAfer, valBalance - transferAmount)
require.Equal(t, valBalanceAfer, valBalance-transferAmount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Formatting improvement approved, but fee deduction testing needed.

The removal of the space before the subtraction operator improves code formatting. However, the TODO comment above this line indicates that fee deduction is not being properly handled or tested for bank/v2.

Consider the following actions:

  1. Address the TODO comment by implementing the DeductFee ante handler for bank/v2.
  2. Once implemented, add a test case to verify that fees are correctly deducted from the sender's balance.

Would you like assistance in drafting the additional test case for fee deduction?


receiverRaw := cli.CustomQuery("q", "bankv2", "balance", receiverAddr, denom)
receiverBalance := gjson.Get(receiverRaw, "balance.amount").Int()
require.Equal(t, receiverBalance, transferAmount)

}
Loading
Loading