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.

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

test/devux: helper function for assertions conditional on panics #1964

Merged
merged 3 commits into from
Jul 5, 2022

Conversation

stackman27
Copy link
Contributor

Closes: #1946

What is the purpose of the change

Adds a helper function for conditioning on Panics
Realized that there is already a helper function in x/gamm/pool-models/balancer/util_test.go so i just moved that under osmoutils

Brief Changelog

n/a

Testing and Verifying

n/a

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/mint labels Jul 4, 2022
@stackman27
Copy link
Contributor Author

stackman27 commented Jul 4, 2022

We could use something similar for conditionalErrors, but it is going to be a little more complex. These are 4 ways that i've found expectError being used ;

  1. Basic implementation
if tc.expectErr {
    require.Error(t, err)
} else {
    require.NoError(t, err)
}
  1. Error with message
if test.expectPass {
   require.NoError(t, test.msg.ValidateBasic(), "test: %v", test.name)
} else {
   require.Error(t, test.msg.ValidateBasic(), "test: %v", test.name)
}
  1. Checks after NoError
if tc.expectErr {
	require.Error(t, err)
} else {
	require.NoError(t, err)
	require.NotNil(t, resp)
	require.Equal(t, tc.expectAdmin, resp.Admin)
}
  1. Check that halts after error
if tc.expErr {
    require.Error(t, gotErr)
    return
}
require.NoError(t, gotErr)

If this is something that we should add. Please let me know and i can add it in this PR :)

@stackman27 stackman27 marked this pull request as ready for review July 4, 2022 23:27
@stackman27 stackman27 requested a review from a team July 4, 2022 23:27
@ValarDragon
Copy link
Member

Thanks for adding this! Would recommend we don't do this for expect pass atm

@stackman27
Copy link
Contributor Author

Thanks for adding this! Would recommend we don't do this for expect pass atm

Thank you @ValarDragon! Resolved your comments

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Logic and feature LGTM!

I'd prefer creating a new file test_helpers.go in osmo utils instead of having the method within cli_helpers

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.

Sweet, LGTM once Matt's suggestion to move to a different file is addressed

osmoutils/cli_helpers.go Outdated Show resolved Hide resolved
@stackman27
Copy link
Contributor Author

stackman27 commented Jul 5, 2022

Addressed all the comments. Good to go!

@ValarDragon ValarDragon merged commit 298d7cf into osmosis-labs:main Jul 5, 2022
@p0mvn p0mvn added the A:backport/v10.x backport patches to v10.x branch label Jul 19, 2022
mergify bot pushed a commit that referenced this pull request Jul 19, 2022
* test: helper function for assertions conditional on panics

* added devs comments

* matts comment

(cherry picked from commit 298d7cf)

# Conflicts:
#	x/gamm/pool-models/balancer/pool_suite_test.go
#	x/mint/keeper/genesis_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v10.x backport patches to v10.x branch C:x/gamm Changes, features and bugs related to the gamm module. C:x/mint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test/devux: helper function for assertions conditional on error
5 participants