feat(halo/evmstaking2): unit tests for unhappy paths #2591
Conversation
| @@ -88,18 +88,112 @@ func TestInsertAndDeleteEVMEvents(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestHappyPathDelivery(t *testing.T) { | |||
| func TestDeliveryWithBrokenServer(t *testing.T) { | |||
There was a problem hiding this comment.
we don't really do this for any tests in the code base. So not sure we can start a new convention simply in the middle of a PR review?
There was a problem hiding this comment.
I did some quick googling and it does not look like this convention is a thing, in general...
I think comments inside the test doing the "handholding" and explaining the steps are more useful, aren't they? If the godoc would contain all these details instead, you'd still need to do mental work by mapping the docs to the actual code. But if we remove the details, then the test name is already explaining the high level idea, doesn't it?
There was a problem hiding this comment.
we don't really do this for any tests in the code base. So not sure we can start a new convention simply in the middle of a PR review?
I think this is fine starting good standards as we go along, especially in small PR's. wydt ?
I did some quick googling and it does not look like this convention is a thing, in general...
I think comments inside the test doing the "handholding" and explaining the steps are more useful, aren't they? If the godoc would contain all these details instead, you'd still need to do mental work by mapping the docs to the actual code. But if we remove the details, then the test name is already explaining the high level idea, doesn't it?
Brief godoc that concisely describes the test and expected behavior is very useful. Example geth. That way without any context a dev can see what the test does, just looking at the code is not enough. I dont think there is any cost to adding godocs for all tests (atleast the more involved ones) and all exported funcs.
chmllr
changed the title
|
|
||
| for _, event := range events { | ||
| err := keeper.parseAndDeliver(ctx, &event) | ||
| require.True(t, strings.Contains(err.Error(), sServer.err.Error())) |
There was a problem hiding this comment.
nit: Incorrect syntax usage of require , update all usages.
| require.True(t, strings.Contains(err.Error(), sServer.err.Error())) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(),"create validator: pubkey to cosmos") |
Using require.Error and require.Contains is preferred as it provides clearer intent, better failure messages, and avoids potential panics if err is nil. It also aligns with idiomatic testing practices for improved readability and debugging.
halo/evmstaking2/keeper/keeper.go
Outdated
| @@ -240,6 +240,10 @@ func (k Keeper) deliverDelegate(ctx context.Context, ev *bindings.StakingDelegat | |||
| return errors.New("validator does not exist", "validator", valAddr.String()) | |||
| } | |||
|
|
|||
| if ev.Amount == nil { | |||
There was a problem hiding this comment.
lets add a "verifyStakingDelegatefunction. Also, we can rename theev` variable, it was copy paste bug.
| @@ -88,18 +88,112 @@ func TestInsertAndDeleteEVMEvents(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestHappyPathDelivery(t *testing.T) { | |||
| func TestDeliveryWithBrokenServer(t *testing.T) { | |||
There was a problem hiding this comment.
we don't really do this for any tests in the code base. So not sure we can start a new convention simply in the middle of a PR review?
Co-authored-by: Khalil Claybon <[email protected]>
Co-authored-by: Khalil Claybon <[email protected]>
chmllr
changed the title
| @@ -1,3 +1,4 @@ | |||
| //go:generate mockgen -source ./interfaces.go -package testutil -destination ../testutil/mock_interfaces.go | |||
There was a problem hiding this comment.
Add a new command to the Makefile where we can put all the generate mock commands. Also mocks tend to go in a mocks dir so that the import is more explicit mocks.MockStakingMsgServer
.PHONY: generate-mocks
generate-mocks:
mockgen -source ./halo/evmstaking2/types/interfaces.go -package mocks -destination halo/evmstaking2/mocks/interfaces.go/mocks.go
There was a problem hiding this comment.
I would leave this out here and instead create one PR doing this for all other tests later.
More unit tests for unhappy paths.
issue: #2525