(chore) change tests in 27ica/controller to expect specific error #7177
Conversation
| suite.Require().Equal(TestVersion, version) | ||
| suite.Require().NoError(err) | ||
| } else { | ||
| suite.Require().Error(err) | ||
| suite.Require().ErrorContains(err, tc.expErr.Error()) |
There was a problem hiding this comment.
This (and all other occurrences using "ErrorContains") is a bit weird but for some reason using ErrorIs fails here :/
There was a problem hiding this comment.
Yeah, I think we have faced this issue other times as well... Something about how the error is wrapped, I think...
modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go
Outdated
Show resolved
Hide resolved
modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go
Show resolved
Hide resolved
bznein
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
bznein
changed the title
There was a problem hiding this comment.
Great work, @bznein. Thank you so much for taking the initiate to refactor the tests!
| suite.Require().Equal(TestVersion, version) | ||
| suite.Require().NoError(err) | ||
| } else { | ||
| suite.Require().Error(err) | ||
| suite.Require().ErrorContains(err, tc.expErr.Error()) |
There was a problem hiding this comment.
Yeah, I think we have faced this issue other times as well... Something about how the error is wrapped, I think...
| @@ -12,33 +12,33 @@ func (suite *KeeperTestSuite) TestQueryInterchainAccount() { | |||
| testCases := []struct { | |||
| name string | |||
| malleate func() | |||
| expPass bool | |||
| errMsg string | |||
There was a problem hiding this comment.
Is it not possible to use expErr here?
There was a problem hiding this comment.
I wasn't able to. We return errors with status.Errorf which is not the same as wrapping errors. They are not a specific error type
| @@ -25,46 +27,46 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount_MsgServer() { | |||
|
|
|||
| testCases := []struct { | |||
| name string | |||
| expPass bool | |||
| expErr error | |||
There was a problem hiding this comment.
Nit: just for consistency with the other tests, we could just switch the order of the last two and have expErr last.
| func() { | ||
| msg.Ordering = channeltypes.NONE | ||
| expectedOrderding = channeltypes.UNORDERED | ||
| }, | ||
| }, | ||
| { | ||
| "invalid connection id", | ||
| false, | ||
| connectiontypes.ErrConnectionNotFound, | ||
| func() { | ||
| msg.ConnectionId = "connection-100" | ||
| }, | ||
| }, | ||
| { | ||
| "non-empty owner address is valid", |
There was a problem hiding this comment.
Maybe we could also move this test up together with the rest of success cases.
There was a problem hiding this comment.
Yeah I actually wanted to do this but also wanted to keep the diff tidy! Now that it has been reviewed I think it makes more sense to move it :)
|
Description
closes: #7178
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.