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

e2e: Replace AssertValidTxResponse and error check with AssertSuccessTx or AssertFailedTx #3435

Closed
3 tasks
colin-axner opened this issue Apr 11, 2023 · 2 comments · Fixed by #3475
Closed
3 tasks
Assignees
Labels
e2e needs discussion Issues that need discussion before they can be worked on type: code hygiene Clean up code but without changing functionality or interfaces

Comments

@colin-axner
Copy link
Contributor

Summary

Instead of asserting that a response from broadcasting a tx is not nil, we should immediately fail if it is nil and provide helper functions to see if the tx was successful or not. See comment:

Instead of:

    transferTxResp, err := s.Transfer(args)
    s.Require().NoError(err)
    s.AssertValidTxResponse(transferTxResp)

use

    resp := s.Transfer(args)
    s.AssertTxSuccess(resp)

or

    resp := s.Transfer(args)
    s.AssertTxFailure(resp, transfertypes.ErrInsufficientBalance) // expected error code 

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added needs discussion Issues that need discussion before they can be worked on type: code hygiene Clean up code but without changing functionality or interfaces e2e labels Apr 11, 2023
@damiannolan
Copy link
Member

I like the idea of AssertTxSuccess and AssertTxFailure. Is the idea here to also remove the error return from BroadcastMessages? E.g. replacing this code block with s.Require().NoError(err)

@chatton
Copy link
Contributor

chatton commented Apr 13, 2023

Updated link to BroadcastMessages and the error return

@DimitrisJim DimitrisJim self-assigned this Apr 13, 2023
@crodriguezvega crodriguezvega moved this to In progress in ibc-go Apr 14, 2023
@DimitrisJim DimitrisJim linked a pull request Apr 18, 2023 that will close this issue
9 tasks
@github-project-automation github-project-automation bot moved this from In progress to Todo in ibc-go May 17, 2023
@damiannolan damiannolan moved this from Todo to Done in ibc-go May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e needs discussion Issues that need discussion before they can be worked on type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants