epoching: test infra and unit tests of keeper/types #47
Conversation
SebastianElvis
changed the title
x/epoching/keeper/grpc_query_test.go
Outdated
| func() { | ||
| req = &types.QueryParamsRequest{} | ||
| }, |
There was a problem hiding this comment.
I don't understand the motivation for this "malleate" pattern. It makes things harder to understand in the general case as you have to consider not just the given test case but all test cases that came before it that have potentially modified the common variable in an accumulative way. That would make it difficult for example to comment out all but a failing test case to concentrate on a single problem.
It could make sense in stateful testing where I want to test that each additional step in a series of operations keeps the model in a valid state.
However in this instance it looks completely unnecessary as the tests do nothing but assign the empty request. They could just return it, or even just use the same instance since it doesn't change.
It's not entirely clear what is being tested here either. The test seems to do nothing more than send a request for params and check that the default is returned, then it checks that for the same request it's not something else that is returned. Is this like a 4 eyes principle 👀 👀 ? That x == y && x != !y? 🙂
I could imagine using a fuzz test in the following way:
- Generate some random params and perhaps a flag
- If the flag is set, set the params it in the keeper
- Send the query to get the current state
- If the flag was set, verify that the generated parameter was returned; otherwise verify that the default params are returned
That would verify that the application doesn't always return default, it respects the settings.
There was a problem hiding this comment.
Fully agree with this one. I copied the tests from Cosmos SDK but malleate and req are indeed unnecessary here as we don't have any parameter in QueryParamsRequest...
There was a problem hiding this comment.
It's not entirely clear what is being tested here either. The test seems to do nothing more than send a request for params and check that the default is returned, then it checks that for the same request it's not something else that is returned. Is this like a 4 eyes principle 👀 👀 ? That x == y && x != !y? 🙂
This is indeed a trivial test. It merely ensures that one can query the parameters via GRPC. However both Cosmos SDK (https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/slashing/keeper/grpc_query_test.go#L58-L64) and Osmosis (https://github.com/osmosis-labs/osmosis/blob/v10.0.0/x/superfluid/keeper/grpc_query_test.go#L10-L15) have this one. Perhaps let's keep it and add an extra fuzz test that you proposed?
x/epoching/keeper/grpc_query_test.go
Outdated
| func() { | ||
| suite.keeper.SetEpochNumber(suite.ctx, sdk.NewUint(0)) | ||
| req = &types.QueryCurrentEpochRequest{} | ||
| }, |
There was a problem hiding this comment.
Right, here it's more obvious why you want to have the malleate function, to mutate the state.
I would still remove the req from here as it never changes. IIRC it doesn't even have parameters.
There was a problem hiding this comment.
These are nice tests by the way for testing the database over a series of calls!
Combined with fuzzing it can be very powerful, with the only difference being that you'd generate these steps of increments/sets randomly and calculate the expected state in the test on an idealised model, and check the real implementation against that.
There was a problem hiding this comment.
Combined with fuzzing it can be very powerful, with the only difference being that you'd generate these steps of increments/sets randomly and calculate the expected state in the test on an idealised model, and check the real implementation against that.
That's a good idea! Will do
x/epoching/keeper/grpc_query_test.go
Outdated
| } else { | ||
| suite.NotEqual(tc.epochNumber.Uint64(), resp.CurrentEpoch) | ||
| suite.NotEqual(tc.epochBoundary.Uint64(), resp.EpochBoundary) | ||
| } |
There was a problem hiding this comment.
Never used. I understand why you want to keep the test structure the same, easier to copy-paste, but if some code is never actually run, it's just extra cruft to maintain and review.
For example here, it could be that only one of the two doesn't match, it's not necessary for both be wrong. But it doesn't matter because all test cases are expected to pass. Better remove it and keep it real.
There was a problem hiding this comment.
At the moment this code is unreachable indeed. Will we have some extra test cases that we want to fail?
There was a problem hiding this comment.
Going by the idea of state-machine tests (or model tests as they are also called), a more realistic scenario is not to check that the result doesn't equal some random rubbish, but rather to generate an invalid input, try to use it as input, then check that it has been rejected, as expected.
So, we either:
- generate a valid input, update our ideal model with it, update the system under test with it, and compare the results are the same
- generate a known invalid input (perhaps by perturbing a valid one), try to update the system under test, check that it rejected the input
Sometimes the operations can't fail, there are no invalid inputs, in which case there's no reason to test failures, only that the state is always correct. One example I did in the past is testing a database, which was based on this example.
On another case, testing a consensus protocol I did generate invalid input in a controlled fashion. By controlled I mean that I knew that the input was going to be invalid, I didn't have to code logic in the test to decide about an arbitrary input whether it was valid or not, replicating much of what the system would have to do as well.
x/epoching/keeper/grpc_query_test.go
Outdated
| req = &types.QueryEpochMsgsRequest{ | ||
| Pagination: &query.PageRequest{ | ||
| Limit: 100, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Once more, the req is the same across all tests. It would reduce code duplication and make it easier to review without having to look closely for differences only to find nothing 🔍
There was a problem hiding this comment.
I was planning to add test cases on the pagination with different Limit, but yeah req is still not necessary and can be merged into individual test cases. 👍
x/epoching/keeper/grpc_query_test.go
Outdated
| false, | ||
| []*types.QueuedMessage{ | ||
| {TxId: []byte{0x01}}, | ||
| }, |
There was a problem hiding this comment.
This again looks like the 👀 👀 in action. The previous test already checked what the response should equal, there's no point testing with an arbitrary example that it should not equal, once we established what it is.
x/epoching/keeper/grpc_query_test.go
Outdated
| if len(tc.epochMsgs) != len(resp.Msgs) { | ||
| suite.T().Skip() | ||
| } |
There was a problem hiding this comment.
This is a bit confusing. We are expecting the test to fail, but we skip it. For this it's enough that it doesn't equal in length to what we are not expecting it to be.
Again, this arm of the test is just a lot of code for something that doesn't add much value, as you can always test for the positive case.
Things would be different if for example the call could return an error, and you could check that it indeed fails.
x/epoching/keeper/grpc_query_test.go
Outdated
| suite.Equal(len(tc.epochMsgs), len(resp.Msgs)) | ||
| suite.Equal(uint64(len(tc.epochMsgs)), suite.keeper.GetQueueLength(suite.ctx).Uint64()) | ||
| for idx := range tc.epochMsgs { | ||
| suite.Equal(tc.epochMsgs[idx].MsgId, resp.Msgs[idx].MsgId) | ||
| suite.Equal(tc.epochMsgs[idx].TxId, resp.Msgs[idx].TxId) | ||
| } |
There was a problem hiding this comment.
This would be another great use case for fuzz testing. You could also randomly generate the limit in the request, to check that it is respected. Something like this:
suite.fuzz(fun(seed uint64) {
rand.Seed(seed)
numOps := rand.Int() % 10
k := NewKeeper()
expTxids := [][]byte{}
for i in range numOps {
op := rand.Int() % 10
if op == 0 {
k.ClearEpochMsgs()
expTxids = [][]byte{}
} else if op <= 6 {
txid := genBytes(32)
k.EnqueueMsg(types.QueueMsg(txid)
expTxids = append(expTxids, txid)
} else {
limit := rand.Int() % 5
req := types.QueryEpochMsgsRequest{
Pagination: &query.PageRequest{
Limit: limit,
},
}
resp, err := queryClient.EpochMsgs(wctx, req)
suite.NoError(err)
suite.Equal(math.min(len(expTxids), limit), len(resp.Msgs))
for idx := range resp.Msgs {
suite.Equal(expTxids[idx], resp.Msgs[idx].TxId)
}
}
}
})If it fails, the framework would save the seed for you to repeat the experiment.
The benefit is that while this takes longer to write and often requires more thought, it's much easier to add features to it, such as varying the limit. In your test, if you wanted to test it, you would have to create more test cases and make sure in some instances the limit is less, in other instances greater than the queued items, and keep it in sync by hand. Not nice.
x/epoching/keeper/msg_server.go
Outdated
| @@ -43,6 +43,21 @@ func (k msgServer) WrappedDelegate(goCtx context.Context, msg *types.MsgWrappedD | |||
| // enqueue msg | |||
| k.EnqueueMsg(ctx, queuedMsg) | |||
|
|
|||
| // emit event | |||
There was a problem hiding this comment.
Just noting that this comment, and the // enqueue msg below, don't add anything because it literally says the same thing as the code (.EmitEvents and .EnqueueMsg).
Comments are great to remind you of the rationale you are doing things, or to clarify some hard to read logic, and to make sure people can spot inconsistencies, but they are not needed to decorate the obvious.
In this instance, I'm not sure what method we're in from the PR diff and why we are emitting this event, or what the event is; for all that I have to carefully read all the attributes. The comment doesn't help with this, but TBH I doubt any comment would in this case.
Sorry, I'm not trying to be pedantic or obnoxious. You can leave the comment in if it helps you. I only mentioned it because I also used to get into the habit of adding comments to every line because after a while the ones without comments looked naked and cold. But it's okay not to comment as well 😌
x/epoching/keeper/msg_server_test.go
Outdated
| func() *types.MsgWrappedDelegate { | ||
| return &types.MsgWrappedDelegate{ | ||
| Msg: &stakingtypes.MsgDelegate{}, | ||
| } | ||
| }, | ||
| false, |
There was a problem hiding this comment.
Great that you are testing individual requests, but this looks dubious. We have a completely empty delegate request, and it doesn't fail. We don't know who wants to delegate to whom and how much, but that's okay?
Do the staking messages have some validation logic that could filter this out?
There was a problem hiding this comment.
This test case merely ensures that WrappedDelegate won't panic. Perhaps I will add another Require() to ensure that the msg is indeed enqueued.
Do the staking messages have some validation logic that could filter this out?
There is. For each sdk.Msg it has to implement a function ValidateBasic() that does some basic stateless checks, e.g., https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/x/staking/types/msg.go#L89-L144. However, ValidateBasic() will be invoked only when a user uses the client to submit a message and when a validator executes runTx. If we submit the message via MsgServer then ValidateBasic() won't be invoked. The logic of ValidateBasic() is tested under x/epoching/types/msg_test.go, and the logic of MsgServer will be tested in x/epoching/keeper/msg_server_test.go.
There was a problem hiding this comment.
Okay, maybe this is worth a comment to say even though these are not valid messages, the service does no validation.
x/epoching/types/msg_test.go
Outdated
| {"basic good", sdk.AccAddress(valAddr1), valAddr2, coinPos, true, false}, | ||
| {"self bond", sdk.AccAddress(valAddr1), valAddr1, coinPos, true, false}, | ||
| {"empty delegator", sdk.AccAddress(emptyAddr), valAddr1, coinPos, false, false}, | ||
| {"empty validator", sdk.AccAddress(valAddr1), emptyAddr, coinPos, false, false}, | ||
| {"empty bond", sdk.AccAddress(valAddr1), valAddr2, coinZero, false, false}, | ||
| {"nil bold", sdk.AccAddress(valAddr1), valAddr2, sdk.Coin{}, false, false}, |
There was a problem hiding this comment.
No test case in the whole file has expectErr as true. Remove?
There was a problem hiding this comment.
And I mean all the other test cases don't hit expectErr ever.
|
Thanks for the comments Akosh! I have fixed the comments and added fuzz tests. Feel free to review again. A thing is that Cosmos SDK's query response with pagination is out-of-order. You can see from the log below, where Another thing is that the Testify library does not support Golang's fuzzing at the moment, and thus I wrote fuzz tests outside the suite. |
| // 2. If the flag is true, set the param in the keeper | ||
| // 3. Send the query to get the current param | ||
| // 4. If the flag is true, verify if the returned param is the set one, otherwise the default one | ||
| func FuzzParamsQuery(f *testing.F) { |
There was a problem hiding this comment.
Do we need both a FuzzParamsQuery and a TestParamsQuery method? From my understanding, a fuzzing method can run as a normal test case which would be equivalent if the base cases (added using f.Add()) are the same. However, I can see that the TestParamsQuery uses more complex types than fuzzing allows, so I'm trying to understand whether we can replicate the behavior on FuzzParamsQuery.
There was a problem hiding this comment.
I'm not sure. I was following Akosh's comment on this one. It seems to be more versatile than TestParamsQuery given the random flag here.
x/epoching/keeper/grpc_query_test.go
Outdated
| f.Add(uint64(11111), true) | ||
| f.Add(uint64(22222), false) | ||
|
|
||
| f.Fuzz(func(t *testing.T, epochInterval uint64, flag bool) { |
There was a problem hiding this comment.
Instead of a flag, you can pass along a seed int64 as a parameter and inside the function do:
rand.Seed(seed)
flag := rand.Intn(2)
A seed might be more useful for other random data, while you can easily extend it to more complex stuff instead of true/false.
There was a problem hiding this comment.
Do we need to generate such a random flag by ourselves? IIUC flag in f.Fuzz will be given a randomly generated value with the corpus as input.
x/epoching/keeper/grpc_query_test.go
Outdated
| req := types.QueryParamsRequest{} | ||
| resp, err := queryClient.Params(wctx, &req) | ||
| require.NoError(t, err) | ||
| // if the random flag is true, then resp.Params should be changed, otherwise default |
There was a problem hiding this comment.
I'm not entirely sure about the usage of the flag. Given that there's a 50% chance that the flag would be set to false, for 50% of the test cases we are testing the same thing, wasting processing power.
Maybe we could use the test case of epochInterval being 0 as the case where the default parameters should be used, since either way the epochInterval should not be allowed to be less than 1, right?
There was a problem hiding this comment.
Maybe we could use the test case of epochInterval being 0 as the case where the default parameters should be used, since either way the epochInterval should not be allowed to be less than 1, right?
Good catch, we definitely need to test this case! Will do.
There was a problem hiding this comment.
Yeah, please, go creative with fuzzing, my flag suggestion was just to make sure that default and non-default are both tested. I usually do something like p := rand.Int() % 100 and then say if p < 10 then do something 10% of the times, etc.
| true, | ||
| types.DefaultParams(), | ||
| sdk.NewUint(1), | ||
| sdk.NewUint(suite.keeper.GetParams(suite.ctx).EpochInterval * 1), |
There was a problem hiding this comment.
I wanted to highlight that EpochBoundary == EpochInterval * <epoch_number>. It can be omitted though.
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { | ||
| tc.malleate() | ||
| wctx := sdk.WrapSDKContext(ctx) | ||
| resp, err := queryClient.CurrentEpoch(wctx, &req) | ||
| suite.NoError(err) |
There was a problem hiding this comment.
What would be an error condition here? Could we test that?
There was a problem hiding this comment.
Typically no error will be returned in this query. The reason of this is that Cosmos SDK enforces each query to return a response and an error.
| Limit: 100, | ||
| }, | ||
| }, | ||
| []*types.QueuedMessage{}, |
There was a problem hiding this comment.
Same here. You could pass the TxId as a parameter and construct inside the test case to keep things simple.
| } else { | ||
| suite.NotEqual(&types.QueryParamsResponse{Params: tc.params}, resp) | ||
| resp, err := queryClient.EpochMsgs(wctx, tc.req) | ||
| suite.NoError(err) |
There was a problem hiding this comment.
What would be an error condition here? Can we test for it?
| wctx := sdk.WrapSDKContext(ctx) | ||
| // enque a random number of msgs with random txids | ||
| for i := uint64(0); i < numMsgs; i++ { | ||
| txid := genRandomByteSlice(32) |
There was a problem hiding this comment.
I implemented a similar function here. Maybe we can add those functions in a global testutils space?
There was a problem hiding this comment.
Also, you are generating a random slice, but you do not set a randomness seed anywhere. I would suggest that you pass a seed as a parameter like the btclightclient fuzz tests (see here)
There was a problem hiding this comment.
In fact I was copy/pasting your implementation 🤣 perhaps I can start adding these functions to such testutils in this PR. 👍
There was a problem hiding this comment.
Yes it is imperative that you use a seed, otherwise these random generations are not repeatable!
I updated my comment to unify requests and state mutation, so multiple queries can be sent along the way. Note that only the seed is the input parameter. #47 (comment)
x/epoching/keeper/msg_server_test.go
Outdated
| { | ||
| "MsgWrappedDelegate", | ||
| &types.MsgWrappedDelegate{ | ||
| Msg: &stakingtypes.MsgDelegate{}, |
There was a problem hiding this comment.
Maybe pass the Msg itself and construct the wrapped message in the test case?
x/epoching/keeper/msg_server_test.go
Outdated
| { | ||
| "MsgWrappedDelegate", | ||
| &types.MsgWrappedUndelegate{ | ||
| Msg: &stakingtypes.MsgUndelegate{}, |
|
That is one helluva cryptic output from the fuzzer 😳 |
|
Thanks for the insightful comments Akosh and Vitalis! I have updated the PR w.r.t. the comments. This PR now also fixes an issue to disallow wrapped msgs that point to no inside unwrapped msgs and adds corresponding tests. Feel free to have a look again.
After a second thought I found we will need to keep the |
x/epoching/keeper/grpc_query_test.go
Outdated
| } | ||
|
|
||
| // FuzzParamsQuery fuzzes queryClient.Params | ||
| // 1. Generate random param and a flag |
x/epoching/keeper/grpc_query_test.go
Outdated
| params.EpochInterval = epochInterval | ||
|
|
||
| // test the case of EpochInterval == 0 | ||
| zeroIntervalFlag := rand.Intn(2) |
There was a problem hiding this comment.
Why have this flag? Maybe you can do
if epochInterval == 0 {
// do the check on the below if condition
// set epoch interval into a random int
}
| // 1. generate a random number of epochs to increment | ||
| // 2. query the current epoch and boundary | ||
| // 3. compare them with the correctly calculated ones | ||
| func FuzzCurrentEpoch(f *testing.F) { |
There was a problem hiding this comment.
They do not ignore fuzz tests, they just don't do the fuzzing if I understand correctly. In order for fuzzing to be performed you need to ask for a particular function to be fuzzed.
Fixes BM-62 and BM-64
Depend on #32 and #35
This PR introduces test infra for the epoching module, and adds some basic unit tests on the types and the keeper. Tests on keeper functionalities (such as maintaining epoch number, msg queues and slashed validators) and module/app will be done in another PR.
I also marked @vitalis as a reviewer as Vitalis merged a PR on tests so may have the corresponding experience. Thanks :)