Check for error returned in channel msg_tests

[expertdicer]

Description

closes: #4304

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

---

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.

• Targeted PR against the correct branch (see CONTRIBUTING.md).
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[expertdicer]

@DimitrisJim Can you take a brief look, is this okay so that I can continue for others Msgs ?

[DimitrisJim]

@expertdicer thanks!

I've pushed a change for the first validate test func as a small template. We can drop the expPass if we use expError and just assert on the final err message generated.

[expertdicer]

Or should we implement the error check like in:

ibc-go/modules/core/04-channel/types/msgs_test.go

Line 1252 in 947b15e

suite.Require().ErrorIs(err, tc.expErr)

[DimitrisJim]

Or should we implement the error check like in:

We should also see what others think on this. In my view, we should be checking the error message that is going to be presented even if this does lead to more verbose set-ups (wrapping the error and formatting in values).

Can leave it as is for now we'll soon get some other opinions. At some point I'll open a bigger issue to ideally get all tests checking for the same thing.

[AdityaSripal]

i personally prefer error code checking as we wouldn't have to change tests if the error string gets rewritten. However, i think this PR can be merged as-is and we standardize on best practices internally.

Thanks @expertdicer !!

[chatton]

I think it does add value to check the error message when the same error type is returned in multiple cases, but my only concern is the additional maintenance overhead (debugging and refactoring tests as error messages change).

I think I am actually happy just checking for the error type in most cases.

In the case of state machine breaking error messages changing it's definitely worth while, but for validate basic error messages maybe not so much.

[DimitrisJim]

I think it does add value to check the error message when the same error type is returned in multiple cases

yea, that was what my thinking was. I do recal seeing these in ValidateBasic tests but could be mistaken. Either way, it def makes more sense to only do in those cases. 👍

[crodriguezvega]

Amazing! Thank you for adding all the errors in the tests!