Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Loockup msg server test #2205

Merged
merged 11 commits into from
Jul 29, 2022
Merged

Loockup msg server test #2205

merged 11 commits into from
Jul 29, 2022

Conversation

hieuvubk
Copy link
Contributor

@hieuvubk hieuvubk commented Jul 23, 2022 •
edited
Loading

Closes: #2183

What is the purpose of the change

Add tests for lockup module msg_server.test. Add some missing ValidateBasic.

  • MsgBeginUnlockingAll

  • MsgBeginUnlocking

  • MsgExtendLockup

@hieuvubk hieuvubk requested a review from a team July 23, 2022 06:30
@hieuvubk hieuvubk marked this pull request as draft July 23, 2022 06:50
@github-actions github-actions bot added the C:CLI label Jul 23, 2022
@hieuvubk hieuvubk marked this pull request as ready for review July 23, 2022 07:18
@hieuvubk hieuvubk requested a review from czarcas7ic July 28, 2022 07:16
czarcas7ic
czarcas7ic approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

x/lockup/types/msgs.go Outdated Show resolved Hide resolved
x/lockup/types/msgs_test.go Outdated Show resolved Hide resolved
x/lockup/types/msgs_test.go Outdated Show resolved Hide resolved
@hieuvubk hieuvubk requested a review from czarcas7ic July 29, 2022 14:52
@czarcas7ic
Copy link
Member

This all LGTM, much cleaner! Going to wait on one more review in case I missed anything then should be good to merge!

p0mvn
p0mvn requested changes Jul 29, 2022
View reviewed changes
x/lockup/types/msgs_test.go Outdated Show resolved Hide resolved
x/lockup/types/msgs_test.go Outdated Show resolved Hide resolved
p0mvn
p0mvn approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work @hieuvubk

I have 2 follow-up questions. If questions make sense, can you please either address them or create issues for them to be addressed? Thank you

x/lockup/types/msgs_test.go Outdated

// MsgExtendLockup
func generateTestAddrs() (string, string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this helper to test utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'll do it now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is to define it on the app testing suite:

osmosis/app/apptesting/test_suite.go

Line 273 in 450e837

func (s *KeeperTestHelper) LockTokens(addr sdk.AccAddress, coins sdk.Coins, duration time.Duration) (lockID uint64) {

If you go with my other suggestion of utilizing test suite, it would be convenient to reuse

x/lockup/types/msgs_test.go Show resolved Hide resolved
@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Jul 29, 2022
p0mvn
p0mvn approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@p0mvn
Copy link
Member

p0mvn commented Jul 29, 2022

Merging since @czarcas7ic previously approved this and only my comments have been applied since

@p0mvn p0mvn merged commit c13d798 into osmosis-labs:main Jul 29, 2022
@czarcas7ic czarcas7ic mentioned this pull request Aug 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p0mvn p0mvn p0mvn approved these changes

@czarcas7ic czarcas7ic Awaiting requested review from czarcas7ic czarcas7ic is a code owner

Assignees
No one assigned
Labels
C:app-wiring Changes to the app folder C:CLI C:x/lockup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Complete msgs_test.go for the lockup module
3 participants
@hieuvubk @czarcas7ic @p0mvn