(chore): use expected errors in 29fee #7191
Conversation
| false, | ||
| nil, |
There was a problem hiding this comment.
Moved this up as it's a success case
| @@ -135,7 +138,7 @@ func (suite *FeeTestSuite) TestOnChanOpenInit() { | |||
|
|
|||
| suite.Require().NoError(err, "unexpected error from version: %s", tc.version) | |||
| } else { | |||
| suite.Require().Error(err, "error not returned for version: %s", tc.version) | |||
| suite.Require().True(errors.Is(err, tc.expErr) || strings.Contains(err.Error(), tc.expErr.Error()), "error no returned for version: %s or error %s is not %s", tc.version, err, tc.expErr) | |||
There was a problem hiding this comment.
This line will come up several times in the PR. It's ugly, but the issue is that some of the errors that we get (especially from sdk) are not wrapped, so we can't check for a specific error but rather we have to rely on the error string. It's not ideal, but the other option would be to catch every usage of those errors and wrap them ourselves with something else
| return fmt.Errorf("application callback fails") | ||
| } | ||
| }, false, | ||
| "fee module is not enabled", func() { |
There was a problem hiding this comment.
I moved stuff around here to have success cases before failures
| @@ -24,7 +24,7 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPackets() { | |||
| testCases := []struct { | |||
| name string | |||
| malleate func() | |||
| expPass bool | |||
| errMsg string | |||
There was a problem hiding this comment.
This file relies entirely on error messages as all the errors we get from GRPC queries are not wrapped
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func (suite *KeeperTestSuite) TestQueryFeeEnabledChannelsWithPagination() { |
There was a problem hiding this comment.
This was really interesting.
In this test, that was before a test case of another one, we create 10 extra channel, then get the first 5 from pagination and compare them. However, when building them, we loop over an integer and create them.
BUT, the logic that handles pagination paginates them based on their lexicographical order.
This means that if our channels are all after 10 (and before 100, so as to have 2 digits), the two orders are equal.
But in our case, depending on how many tests were ran before that, we could have the case in which our channels were 1,2,3,4,5,6,7,8,9,10, but the pagination would return 1, 10, 2,3,4,5,6,7,8,9, making the test fail!
Thanks @chatton for helping me debug this!
There was a problem hiding this comment.
Very interesting! And thank you both for debugging and fixing it!
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
|
|
||
| suite.SetupTest() | ||
| suite.path.Setup() | ||
| suite.Run(tc.name, func() { |
There was a problem hiding this comment.
There are a few instances in this PR where I introduced t.Run or suite.Run. It helped me debug things but I also think it's better to have it this way, even though it increases the diff. Happy to revert if people find it would be better suited for another PR
bznein
requested review from
AdityaSripal,
colin-axner,
damiannolan and
chatton
as code owners
| require.NoError(t, err, tc.name) | ||
| } else { | ||
| require.True(t, errors.Is(err, tc.expErr) || strings.Contains(err.Error(), tc.expErr.Error()), err.Error()) | ||
| } |
There was a problem hiding this comment.
I wonder would it be worth while turning this into some helper within the tests, seems like we need it in a good few places where errors aren't wrapped.
There was a problem hiding this comment.
yeah makes sense, done!
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func (suite *KeeperTestSuite) TestQueryFeeEnabledChannelsWithPagination() { |
There was a problem hiding this comment.
Very interesting! And thank you both for debugging and fixing it!
Co-authored-by: Carlos Rodriguez <[email protected]>
|
Description
closes: #7190
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.