-
Notifications
You must be signed in to change notification settings - Fork 586
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.
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
Add mock test for CheckSubstituteAndUpdateState. #4950
Add mock test for CheckSubstituteAndUpdateState. #4950
Conversation
6862662
to
3c5f5b0
Compare
3c5f5b0
to
0cb3adb
Compare
expErr error | ||
}{ | ||
{ | ||
"success", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding a check for writes/asserting reads from the store passed in will be done in PR for the wrappedStore.
talked with @charleenfei, will mark as draft until #4862 is merged to include additional tests for changes added there |
54c6419
to
e1e9eb0
Compare
@@ -21,7 +21,7 @@ var ( | |||
queryTypes = [...]any{types.StatusMsg{}, types.ExportMetadataMsg{}, types.TimestampAtHeightMsg{}, types.VerifyClientMessageMsg{}, types.CheckForMisbehaviourMsg{}} | |||
|
|||
// sudoTypes contains all the possible sudo message types. | |||
sudoTypes = [...]any{types.UpdateStateMsg{}, types.UpdateStateOnMisbehaviourMsg{}, types.VerifyUpgradeAndUpdateStateMsg{}, types.VerifyMembershipMsg{}, types.VerifyNonMembershipMsg{}} | |||
sudoTypes = [...]any{types.UpdateStateMsg{}, types.UpdateStateOnMisbehaviourMsg{}, types.VerifyUpgradeAndUpdateStateMsg{}, types.VerifyMembershipMsg{}, types.VerifyNonMembershipMsg{}, types.MigrateClientStoreMsg{}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding these as necessary
"failure: code hashes do not match", | ||
func() { | ||
substituteClientState = &types.ClientState{ | ||
CodeHash: []byte("invalid"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about using a mock value here but can't see many occasions where we do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we have many mock code hash instances like in genesis_test
,wasm_test
, but maybe just better to have a separate pr cleaning all these up as we've been doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, can also move all vals to a values.go
file as we do for top level testing/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work @DimitrisJim thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
func() { | ||
substituteClientState = &ibctm.ClientState{} | ||
}, | ||
clienttypes.ErrInvalidClient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could maybe wrap the error with the error string in the code to make it more specific, but fine as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do, this is pretty prevalent currently. Could be nice to open overarching issue and maybe pick it up in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with whatever :)
We can remove the grandpa test as well? That will be handled by e2e's |
Description
closes: #4829
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.