-
Notifications
You must be signed in to change notification settings - Fork 650
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
(tests): e2e test for failed forwarding #6876
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.
Great work! LGTM
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.
Thank you, @bznein. I think there is an error on the denomination used to query the balance on chain B and C...
|
||
s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks") | ||
|
||
chainBstartingBalance, err := testsuite.GetChainBalanceForDenom(ctx, chainB, chainADenom, chainBWallet) |
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.
Usage of chainADenom
here and for the balance on chain C is not correct, right? We should be using the IBC denom con chain B and C respectively, right?
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.
you're absolutely right, I'll fix this, thanks for catching it!
}) | ||
|
||
t.Run("balances for B and C have not changed", func(t *testing.T) { | ||
chainBBalance, err := testsuite.GetChainBalanceForDenom(ctx, chainB, chainADenom, chainBWallet) |
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.
chainADenom
here an below should also be the IBC denoms on each chain.
Thanks Carlos for spotting this, would you mind taking another look? I'm still not entirely sure how to compose the right denom in E2E tests, so pleaes let me know if this seems right :) |
Quality Gate passed for 'ibc-go'Issues Measures |
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.
The fix looks correct to me. Thank you for quickly fixing it, @bznein!
Description
ref: #6578
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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.