test: add test for encoding.go #3018
Conversation
|
|
WalkthroughThe pull request introduces a new test file, Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[config_reader] The configuration option 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ethereum/eip712/encoding_test.go (1)
17-20: Consider using a test fixture instead of global variablesGlobal variables in tests can make them harder to maintain and potentially cause issues with parallel test execution. Consider using a test fixture struct:
-var ( - aminoCodec *codec.LegacyAmino - interfaceRegistry types.InterfaceRegistry -) +type testFixture struct { + aminoCodec *codec.LegacyAmino + interfaceRegistry types.InterfaceRegistry +} + +func newTestFixture() *testFixture { + return &testFixture{ + aminoCodec: codec.NewLegacyAmino(), + interfaceRegistry: types.NewInterfaceRegistry(), + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ethereum/eip712/encoding_test.go(1 hunks)
🔇 Additional comments (2)
ethereum/eip712/encoding_test.go (2)
44-69: 🛠️ Refactor suggestion
Enhance test coverage and structure
The test function needs several improvements:
- Remove debug Printf statement
- Add table-driven tests for different scenarios< 8000 /li>
- Add error cases
- Validate the actual EIP712 typed data structure
-func TestGetEIP712TypedDataForMsg_AminoSuccess(t *testing.T) {
+func TestGetEIP712TypedDataForMsg(t *testing.T) {
+ tests := []struct {
+ name string
+ msg sdk.Msg
+ signDoc legacytx.StdSignDoc
+ wantErr bool
+ }{
+ {
+ name: "valid MsgSend",
+ msg: banktypes.MsgSend{
+ FromAddress: "cosmos1qperwt9wrnkg6kzfj7s9wf69w5gk3ya6r7l273",
+ ToAddress: "cosmos1q5x4yng8x6f5v45m59wvndkgj7c32c7dpw2crz",
+ Amount: sdk.Coins{sdk.NewInt64Coin("stake", 1000)},
+ },
+ signDoc: legacytx.StdSignDoc{
+ AccountNumber: 1,
+ ChainID: "evmos_9001-1",
+ Sequence: 1,
+ Fee: json.RawMessage(`{"amount":[{"denom":"stake","amount":"10"}],"gas":"200000"}`),
+ Memo: "test memo",
+ },
+ wantErr: false,
+ },
+ // Add more test cases here
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ fixture := newTestFixture()
+ require.NoError(t, fixture.initCodecs(t))
+
+ msgBytes, err := fixture.aminoCodec.MarshalJSON(tt.msg)
+ if tt.wantErr {
+ require.Error(t, err)
+ return
+ }
+ require.NoError(t, err)
+
+ tt.signDoc.Msgs = []json.RawMessage{msgBytes}
+ signDocBytes, err := fixture.aminoCodec.MarshalJSON(tt.signDoc)
+ require.NoError(t, err)
+
+ // Validate the actual EIP712 typed data structure
+ typedData, err := eip712.GetEIP712TypedDataForMsg(signDocBytes)
+ require.NoError(t, err)
+ require.NotNil(t, typedData)
+ // Add specific assertions for the typedData structure
+ })
+ }
+}Let's verify the test coverage:
1-69: Verify test coverage across related packages
Since this is testing critical encoding functionality, we should ensure comprehensive coverage across related packages.
✅ Verification successful
Test coverage appears sufficient and well-structured
The verification shows that the test coverage for EIP712 encoding is comprehensive:
- There are dedicated test suites (
EIP712TestSuite) covering both current and legacy implementations - The new test
TestGetEIP712TypedDataForMsg_AminoSuccesscomplements existing tests by covering Amino JSON marshaling - The encoding functionality is tested across both
encoding.goandencoding_legacy.gowith proper error handling
The test structure aligns well with the codebase's needs and doesn't require additional coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar encoding tests in related packages
echo "Searching for related encoding tests..."
rg --type go "TestGetEIP712" --stats
rg --type go "encoding.*_test" --stats
# Check for any TODOs or missing test coverage
echo "Checking for TODO comments in related files..."
rg --type go "TODO.*test" ethereum/
Length of output: 775
Script:
#!/bin/bash
# Let's check for all EIP712 related tests and their coverage
echo "Checking EIP712 related tests..."
rg --type go "func Test.*EIP712" -A 5
# Check for encoding related functions and their tests
echo "Checking encoding related functions..."
ast-grep --pattern 'func $_($$$) {
$$$
encoding.$$$
$$$
}'
# Check for any existing test files in the ethereum/eip712 directory
echo "Checking test files in ethereum/eip712..."
fd -e go -e test.go$ ethereum/eip712/
# Look for related encoding test patterns
echo "Looking for encoding test patterns..."
rg --type go "MarshalJSON|UnmarshalJSON" "ethereum/eip712/" -B 1 -A 3
Length of output: 4672
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ethereum/eip712/encoding_test.go(1 hunks)
🔇 Additional comments (3)
ethereum/eip712/encoding_test.go (3)
1-15: LGTM: Package declaration and imports are well-structured.
The package name follows Go test conventions, and imports are properly organized.
22-28: Improve configuration handling.
The previous review comment about improving configuration handling is still valid. The function modifies global SDK configuration which could affect other tests.
30-42: Refactor initialization for better separation of concerns.
The previous review comment about refactoring initialization is still valid. The function handles multiple initialization tasks without proper error handling.
|
Thanks for the contribution @keke-ka!! |
ok, thank you for your notice, I just have time to fix it. |
|
@keke-ka the test is failing. Please address the error |
Description
Closes: #XXXX
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...
Reviewers 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...
Unreleasedsection inCHANGELOG.mdSummary by CodeRabbit