-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor: Use mocks for x/nft testing #12407
Conversation
@facundomedica can you reference the issue/epic in the PR body pls? :) |
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.
Hell yeah! Nice work @facundomedica
testutil/context.go
Outdated
CMS store.CommitMultiStore | ||
} | ||
|
||
func DefaultContextWithDB(key storetypes.StoreKey, tkey storetypes.StoreKey) TestContext { |
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.
sorry one more thing. Why not have this take a *testing.T
and that way we can do an assertion and avoid the silly panic?
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.
lgtm!
_ "github.com/cosmos/cosmos-sdk/x/params" | ||
_ "github.com/cosmos/cosmos-sdk/x/staking" | ||
) | ||
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" |
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.
once cli tests are migrated to mocks, we can remove these right?.
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.
It's not really clear to me how to migrate the cli tests to mocks, can you share some pointers? To me it seems that it will be a big-ish rewrite, will merge this and come to it when I have more ideas on how to make it happen
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.
So we have to re-write CLI integration tests such that we can finally remove the dependency on in-process Tendermint. We can achieve this one of two ways:
- Mock Tendermint, i.e. create a super dumb and simple ABCI wrapper.
- Mock the commands themselves, via outputs/inputs.
I'd rather go with option (1) as that'll be much less of a massive re-write. In fact, CLI tests wont have to change at all!
We spoke about this in Lisbon. When Marko is referring to mocks, he's referring to mocking Tendermint.
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.
Gotcha, I'll start looking into that
Please merge this when ready @facundomedica |
Description
Ref: #12398
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change