test(types/address): add unit tests for the file types/address.go #20237
Conversation
WalkthroughThe changes encompass enhancing fuzz testing capabilities by adding a new native Go fuzzer, Changes
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 (
|
There was a problem hiding this comment.
Thanks for the PR! Lot of work to improve coverage! 💪
I have some preferences on standard Go table tests as I find them more readable but this is just me and not blocker.
I would like to ask you to simplify the address generation towards a simple cast on bytes and to not replicate the code that is tested in the tests. Just pin a concrete example address and output if possible
types/address_test.go
Outdated
| @@ -58,6 +63,13 @@ func (s *addressTestSuite) testMarshal(original, res interface{}, marshal func() | |||
| s.Require().Equal(original, res) | |||
| } | |||
|
|
|||
| func (s *addressTestSuite) testMarshalYAML(original, res interface{}, marshal func() (interface{}, error), unmarshal func([]byte) error) { | |||
There was a problem hiding this comment.
personal preference: the addressTestSuite does not setup any state. IMHO standard Go table tests are much better to read.
There was a problem hiding this comment.
I agree with you. I think that the test suite has its place in the unit tests as all, however, in this case, it is useless because it doesn't maintain any state. I only used it for my tests because previous tests had already used the test suite and for consistency I decided to use it too. Here, table tests would be more appropriate. I can remove the test suite and implement table tests in a separate PR because the file is almost 1000 rows. What do you think about that?
types/address_test.go
Outdated
| @@ -58,6 +63,13 @@ func (s *addressTestSuite) testMarshal(original, res interface{}, marshal func() | |||
| s.Require().Equal(original, res) | |||
| } | |||
|
|
|||
| func (s *addressTestSuite) testMarshalYAML(original, res interface{}, marshal func() (interface{}, error), unmarshal func([]byte) error) { | |||
| bz, err := marshal() | |||
| s.Require().Nil(err) | |||
There was a problem hiding this comment.
nit: here and others .NoError(err) gives a better failure output
There was a problem hiding this comment.
Done. I replace it in the all test file
types/address_test.go
Outdated
| accAddress1, err := types.AccAddressFromBech32(address) | ||
| s.Require().NoError(err) | ||
|
|
||
| accAddress2 := types.MustAccAddressFromBech32(address) |
There was a problem hiding this comment.
why not use a valid and an invalid bech32 address string here instead of building it. You are covering the happy path only?
There was a problem hiding this comment.
I test only the happy path. As I write in my previously comment I can refactor the tests file in a separate PR. I can replace testSuite with table tests and I will use valid and invalid bech32.
types/address_test.go
Outdated
| bech32PrefixAccAddr := types.GetConfig().GetBech32AccountAddrPrefix() | ||
| addr20byte := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19} | ||
| address := types.MustBech32ifyAddressBytes(bech32PrefixAccAddr, addr20byte) | ||
| accAddr := types.MustAccAddressFromBech32(address) |
There was a problem hiding this comment.
this whole block can be simplified by:
accAddr := types.AccAddress([]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19})The AccAddress is just bytes
types/address_test.go
Outdated
| } | ||
|
|
||
| func (s *addressTestSuite) TestFormatConsAddressWhenVerbIsDifferentFromSOrP() { | ||
| bech32PrefixAccAddr := types.GetConfig().GetBech32ConsensusAddrPrefix() |
There was a problem hiding this comment.
Here and a above: this is a lot of code. How about using a loop instead?
myAddr := types.AccAddress([]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19})
exp := "000102030405060708090A0B0C0D0E0F10111213"
spec := []string{"%v", "%#v"} //...
for _, v := range spec {
assert.Equal(t, exp, fmt.Sprintf(v, myAddr), v)
}
|
|
||
| ptrAddr := &accAddr | ||
| actual := fmt.Sprintf("%p", ptrAddr) | ||
| expected := uintptr(unsafe.Pointer(&accAddr)) |
There was a problem hiding this comment.
nit: expected should be the full string in this case
types/address_test.go
Outdated
| @@ -133,6 +145,45 @@ func (s *addressTestSuite) TestRandBech32AccAddrConsistency() { | |||
| s.Require().Equal(types.ErrEmptyHexAddress, err) | |||
| } | |||
|
|
|||
| func (s *addressTestSuite) TestRandBech32AccAddrConsistencyYAML() { | |||
There was a problem hiding this comment.
I have some personal preference on table tests. But it is fine to do it differently.
For regression, some concrete byte examples would be better so that we have confidence that it is backwards compatible and nothing is broken. You would use fix AccAddresses and their yaml, hex and bech32 representation instead of random addresses.
There was a problem hiding this comment.
I used random addresses with Go Fuzzer
types/address_test.go
Outdated
| pubBz := make([]byte, ed25519.PubKeySize) | ||
| pub := &ed25519.PubKey{Key: pubBz} | ||
|
|
||
| for i := 0; i < 1000; i++ { |
There was a problem hiding this comment.
Why 1000? With random addresses the Go Fuzzer may be more interesting to navigate the code tree for edge cases.
|
@alpe thank you for your time and your comments. |
types/address_test.go
Outdated
| func testMarshalYAML(t *testing.T, original, res interface{}, marshal func() (interface{}, error), unmarshal func([]byte) error) { | ||
| bz, err := marshal() | ||
|
|
||
| assert.NoError(t, err) |
There was a problem hiding this comment.
nit: here and others: use require.NoError instead so that your test can stop early.
types/address_test.go
Outdated
| s.Require().NotNil(err) | ||
|
|
||
| err = (*types.AccAddress)(nil).UnmarshalYAML([]byte("\"" + str + "\"")) | ||
| s.Require().NotNil(err) |
There was a problem hiding this comment.
nit: here and others: prefer NoError for better output
| bech32PrefixValAddr := types.GetConfig().GetBech32ValidatorAddrPrefix() | ||
| accAddress1 := types.AccAddress([]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}) | ||
| accAddress2 := types.MustAccAddressFromBech32(accAddress1.String()) | ||
| s.Require().Equal(accAddress1, accAddress2) |
There was a problem hiding this comment.
I would use the full bech32 string here to ensure that the decoding works. Otherwise you can not really know what encoding is used.
func (s *addressTestSuite) TestMustAccAddressFromBech32() {
src := types.MustAccAddressFromBech32("cosmos1qqqsyqcyq5rqwzqfpg9scrgwpugpzysnrk363e")
exp := types.AccAddress([]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19})
s.Assert().Equal(exp, src)
}There was a problem hiding this comment.
@aple now I remember why I did this. The prefix is always different randomly generated string. If I use "cosmos" as a prefix the test always fail when I run all the tests.
| actual := fmt.Sprintf("%s", consAddr) | ||
|
|
||
| hrp := types.GetConfig().GetBech32ConsensusAddrPrefix() | ||
| expected, err := bech32.ConvertAndEncode(hrp, consAddr) |
There was a problem hiding this comment.
Why not use the string value here instead of converting. It helps with regression is much more readable to humans as well.
types/address_test.go
Outdated
| @@ -133,6 +145,52 @@ func (s *addressTestSuite) TestRandBech32AccAddrConsistency() { | |||
| s.Require().Equal(types.ErrEmptyHexAddress, err) | |||
| } | |||
|
|
|||
| func FuzzBech32AccAddrConsistencyYAML(f *testing.F) { | |||
There was a problem hiding this comment.
please move fuzzer tests to to a address_fuzz_test.go file. I always find it confusing to see them with others.
|
argh, now the linter is complaining: You can check with |
types/address_fuzz_test.go
Outdated
| res := &types.AccAddress{} | ||
|
|
||
| testMarshalYAML(t, &acc, res, acc.MarshalYAML, res.UnmarshalYAML) | ||
| testMarshalYAML(t, &acc, res, acc.MarshalYAML, res.UnmarshalYAML) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- fuzz/oss-fuzz-build.sh (1 hunks)
Additional comments not posted (1)
fuzz/oss-fuzz-build.sh (1)
28-28: New fuzzing target added correctly.The new fuzzing target
FuzzBech32AccAddrConsistencyYAMLhas been added correctly and follows the same pattern as the existing targets.
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (9)
types/address_fuzz_test.go (1)
1-1: Package name should match the directory structure and convention.Ensure the package name aligns with the directory structure and follows Go conventions. Typically, test files should be in the same package as the code they test.
types/address_test.go (8)
Line range hint
72-89: Consider adding comments to explain the purpose of each test case.Adding comments will help other developers understand the intent behind each test case.
func (s *addressTestSuite) TestEmptyAddresses() { s.T().Parallel() // Test empty string representation s.Require().Equal((types.AccAddress{}).String(), "") s.Require().Equal((types.ValAddress{}).String(), "") s.Require().Equal((types.ConsAddress{}).String(), "") // Test empty Bech32 address conversion accAddr, err := types.AccAddressFromBech32("") s.Require().True(accAddr.Empty()) s.Require().Error(err) valAddr, err := types.ValAddressFromBech32("") s.Require().True(valAddr.Empty()) s.Require().Error(err) consAddr, err := types.ConsAddressFromBech32("") s.Require().True(consAddr.Empty()) s.Require().Error(err) }
Line range hint
111-146: Consider reducing the number of iterations for the loop to speed up the test.Reducing the number of iterations can help speed up the test without compromising its effectiveness.
for i := 0; i < 100; i++ { // Test logic }
Line range hint
159-187: Consider adding comments to explain the purpose of each test case.Adding comments will help other developers understand the intent behind each test case.
func (s *addressTestSuite) TestAddrCache() { // Use a random key pubBz := make([]byte, ed25519.PubKeySize) pub := &ed25519.PubKey{Key: pubBz} _, err := rand.Read(pub.Key) s.Require().NoError(err) // Set SDK bech32 prefixes to 'osmo' prefix := "osmo" conf := types.GetConfig() conf.SetBech32PrefixForAccount(prefix, prefix+pubStr) conf.SetBech32PrefixForValidator(prefix+valoper, prefix+valoperpub) conf.SetBech32PrefixForConsensusNode(prefix+valcons, prefix+valconspub) acc := types.AccAddress(pub.Address()) osmoAddrBech32 := acc.String() // Set SDK bech32 to 'cosmos' prefix = "cosmos" conf.SetBech32PrefixForAccount(prefix, prefix+pubStr) conf.SetBech32PrefixForValidator(prefix+valoper, prefix+valoperpub) conf.SetBech32PrefixForConsensusNode(prefix+valcons, prefix+valconspub) // We name this 'addrCosmos' to prove a point, but the bech32 address will still begin with 'osmo' due to the cache behavior. addrCosmos := types.AccAddress(pub.Address()) cosmosAddrBech32 := addrCosmos.String() // The default behavior will retrieve the bech32 address from the cache, ignoring the bech32 prefix change. s.Require().Equal(osmoAddrBech32, cosmosAddrBech32) s.Require().True(strings.HasPrefix(osmoAddrBech32, "osmo")) s.Require().True(strings.HasPrefix(cosmosAddrBech32, "osmo")) }
Line range hint
189-217: Consider adding comments to explain the purpose of each test case.Adding comments will help other developers understand the intent behind each test case.
func (s *addressTestSuite) TestAddrCacheDisabled() { types.SetAddrCacheEnabled(false) // Use a random key pubBz := make([]byte, ed25519.PubKeySize) pub := &ed25519.PubKey{Key: pubBz} _, err := rand.Read(pub.Key) s.Require().NoError(err) // Set SDK bech32 prefixes to 'osmo' prefix := "osmo" conf := types.GetConfig() conf.SetBech32PrefixForAccount(prefix, prefix+pubStr) conf.SetBech32PrefixForValidator(prefix+valoper, prefix+valoperpub) conf.SetBech32PrefixForConsensusNode(prefix+valcons, prefix+valconspub) acc := types.AccAddress(pub.Address()) osmoAddrBech32 := acc.String() // Set SDK bech32 to 'cosmos' prefix = "cosmos" conf.SetBech32PrefixForAccount(prefix, prefix+pubStr) conf.SetBech32PrefixForValidator(prefix+valoper, prefix+valoperpub) conf.SetBech32PrefixForConsensusNode(prefix+valcons, prefix+valconspub) addrCosmos := types.AccAddress(pub.Address()) cosmosAddrBech32 := addrCosmos.String() // retrieve the bech32 address from the cache, respecting the bech32 prefix change. s.Require().NotEqual(osmoAddrBech32, cosmosAddrBech32) s.Require().True(strings.HasPrefix(osmoAddrBech32, "osmo")) s.Require().True(strings.HasPrefix(cosmosAddrBech32, "cosmos")) }
Line range hint
219-268: Consider reducing the number of iterations for the loop to speed up the test.Reducing the number of iterations can help speed up the test without compromising its effectiveness.
for i := 0; i < 100; i++ { // Test logic }
Line range hint
270-308: Consider reducing the number of iterations for the loop to speed up the test.Reducing the number of iterations can help speed up the test without compromising its effectiveness.
for i := 0; i < 100; i++ { // Test logic }
Line range hint
348-391: Consider reducing the number of iterations for the loop to speed up the test.Reducing the number of iterations can help speed up the test without compromising its effectiveness.
for length := 1; length < 5; length++ { for times := 1; times < 10; times++ { // Test logic } }
Line range hint
393-406: Consider adding comments to explain the purpose of each test case.Adding comments will help other developers understand the intent behind each test case.
func (s *addressTestSuite) TestAddressInterface() { pubBz := make([]byte, ed25519.PubKeySize) pub := &ed25519.PubKey{Key: pubBz} _, err := rand.Read(pub.Key) s.Require().NoError(err) addrs := []types.Address{ types.ConsAddress(pub.Address()), types.ValAddress(pub.Address()), types.AccAddress(pub.Address()), } for _, addr := range addrs { switch addr := addr.(type) { case types.AccAddress: // Test account address _, err := types.AccAddressFromBech32(addr.String()) s.Require().NoError(err) case types.ValAddress: // Test validator address _, err := types.ValAddressFromBech32(addr.String()) s.Require().NoError(err) case types.ConsAddress: // Test consensus address _, err := types.ConsAddressFromBech32(addr.String()) s.Require().NoError(err) default: s.T().Fail() } } }
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- types/address_fuzz_test.go (1 hunks)
- types/address_test.go (8 hunks)
Additional Context Used
Path-based Instructions (2)
types/address_fuzz_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/address_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (3)
types/address_fuzz_test.go (1)
21-37: Ensure proper error handling and validation for theinputparameter.Verify that the
inputparameter is valid and meets the expected format before proceeding with the test. This can help catch potential issues early.f.Fuzz(func(t *testing.T, input []byte) { if len(input) == 0 { t.Skip("input is empty") } acc := types.AccAddress(input) res := &types.AccAddress{} testMarshalYAML(t, &acc, res, acc.MarshalYAML, res.UnmarshalYAML) str := acc.String() var err error *res, err = types.AccAddressFromBech32(str) require.NoError(t, err) require.Equal(t, acc, *res) str = hex.EncodeToString(acc) *res, err = types.AccAddressFromHexUnsafe(str) require.NoError(t, err) require.Equal(t, acc, *res) })types/address_test.go (2)
64-71: Good use oft.Helper()to mark the function as a test helper.This helps in better error reporting by pointing to the actual test case that failed.
Line range hint
91-109: Consider adding error handling for theyaml.Marshalcalls.Adding error handling will ensure that any issues with marshaling are caught and reported.
func (s *addressTestSuite) TestYAMLMarshalers() { addr := secp256k1.GenPrivKey().PubKey().Address() acc := types.AccAddress(addr) val := types.ValAddress(addr) cons := types.ConsAddress(addr) got, err := yaml.Marshal(&acc) s.Require().NoError(err) s.Require().Equal(acc.String()+"\n", string(got)) got, err = yaml.Marshal(&val) s.Require().NoError(err) s.Require().Equal(val.String()+"\n", string(got)) got, err = yaml.Marshal(&cons) s.Require().NoError(err) s.Require().Equal(cons.String()+"\n", string(got)) }
| func FuzzBech32AccAddrConsistencyYAML(f *testing.F) { | ||
| if testing.Short() { | ||
| f.Skip("running in -short mode") | ||
| } |
There was a problem hiding this comment.
Consider adding a comment to explain the purpose of skipping the test in short mode.
Adding a comment will help other developers understand why the test is skipped in short mode.
if testing.Short() {
// Skip this test in short mode to save time
f.Skip("running in -short mode")
}| func (s *addressTestSuite) TestUnmarshalYAMLWithInvalidInput() { | ||
| for _, str := range invalidStrs { | ||
| _, err := types.AccAddressFromHexUnsafe(str) | ||
| s.Require().Error(err) | ||
|
|
||
| _, err = types.AccAddressFromBech32(str) | ||
| s.Require().Error(err) | ||
|
|
||
| err = (*types.AccAddress)(nil).UnmarshalYAML([]byte("\"" + str + "\"")) | ||
| s.Require().Error(err) |
There was a problem hiding this comment.
Consider adding comments to explain the purpose of each test case.
Adding comments will help other developers understand the intent behind each test case.
func (s *addressTestSuite) TestUnmarshalYAMLWithInvalidInput() {
for _, str := range invalidStrs {
// Test invalid hex input
_, err := types.AccAddressFromHexUnsafe(str)
s.Require().Error(err)
// Test invalid Bech32 input
_, err = types.AccAddressFromBech32(str)
s.Require().Error(err)
// Test invalid YAML input
err = (*types.AccAddress)(nil).UnmarshalYAML([]byte("\"" + str + "\""))
s.Require().Error(err)
}
// Test empty hex input
_, err := types.AccAddressFromHexUnsafe("")
s.Require().Equal(types.ErrEmptyHexAddress, err)
}
EmilGeorgiev
changed the title
I thank you too for your time. |
* main: feat(tx): port simappv2 changes (#20648) build(deps): Bump github.com/spf13/cobra from 1.8.0 to 1.8.1 (#20682) chore(proto): fix comments (#20679) (serverv2/cometbft) Commands support both json and yaml output (#20670) fix(x/staking,x/auth): regenerate mock to fix test (#20684) docs: ADR 074: Msg v2 (#20618) fix: nested multisig signatures using CLI (#20438) chore: fix spelling errors (#20674) fix: align Dockerfile for build-push-action (#20672) fix: avoid build fail when make localnet-build-env (#20671) build(deps): Bump bufbuild/buf-setup-action from 1.32.2 to 1.33.0 (#20669) chore: make function comment match function names (#20666) chore(consensus): add cometInfo to consensus (#20615) chore: fix typos (#20662) fix: Properly parse json in the wait-tx command. (#20631) fix(sims): check before sending RotateConsPubKey (#20659) test(types/address): add unit tests for the file types/address.go (#20237)
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...
!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