-
Notifications
You must be signed in to change notification settings - Fork 581
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
style: address forced type assertions in tests #5116
Conversation
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.
Thanks! Left a few comments:
Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com>
Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com>
Thanks for the review @pysel :) |
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, nice work. Only nits
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: Roman <roman@osmosis.team>
@@ -1056,12 +1056,34 @@ func (s *IntegrationTestSuite) TestIBCWasmHooks() { | |||
var response map[string]interface{} | |||
s.Require().Eventually(func() bool { | |||
response, err = nodeA.QueryWasmSmartObject(contractAddr, fmt.Sprintf(`{"get_total_funds": {"addr": "%s"}}`, senderBech32)) | |||
totalFunds := response["total_funds"].([]interface{})[0] | |||
amount := totalFunds.(map[string]interface{})["amount"].(string) | |||
denom := totalFunds.(map[string]interface{})["denom"].(string) |
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 find the old versions much more readable here (since it's just a test). I'm not strongly against the type assertion checks and get the point in chain code, but in tests I tend to favor making the test shorted since robustness doesn't matter.
For json in particular, we may want to use https://github.com/tidwall/gjson
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.
looks good to me. I have a stylistic preference that I left a comment for, but happy either way.
@nicolaslara is it okay if I try to get your style preference into the final PR for #5061 ? makes sense to me actually |
sounds good! :) |
* fix forced type assertions * revert change to linter config * Update tests/e2e/e2e_test.go Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com> * Update x/concentrated-liquidity/pool_test.go Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com> * skip pool = poolI * update swap_test.go to test that cl type assertions fail * trial * we needed pool because it's used to CalcInGivenOut * Update x/concentrated-liquidity/pool_test.go Co-authored-by: Roman <roman@osmosis.team> * Update x/concentrated-liquidity/pool_test.go Co-authored-by: Roman <roman@osmosis.team> * Update x/gamm/keeper/swap_test.go Co-authored-by: Roman <roman@osmosis.team> --------- Co-authored-by: Ruslan Akhtariev <46343690+pysel@users.noreply.github.com> Co-authored-by: Roman <roman@osmosis.team>
Brief Changelog
This PR addresses forced type assertions in the Osmosis test suite.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no