chore: fix test TestSetGetTotalEscrowForDenom, use expected errors #8282
Conversation
hungdinh82
requested review from
AdityaSripal,
gjermundgaraba and
srdtrk
as code owners
|
can you review it? @gjermundgaraba @srdtrk @AdityaSripal |
|
How about this PR? @gjermundgaraba |
| expAmount = sdkmath.NewInt(-1) | ||
| }, | ||
| false, | ||
| errors.New("negative coin amount: -1"), |
There was a problem hiding this comment.
A bit unsure why this passes 🤔 From the code it calls, it looks like the error should be different.
There was a problem hiding this comment.
I want to break it down comparing the old code and the new code to make it easier to understand. @gjermundgaraba
Old code:
expPass bool
// true => Pass
// false => Panic with message "negative coin amount: -1"
New code:
expError error
// nil => Pass
// not nill =>errors.New("negative coin amount: -1") => Same error as before (panic)
There was a problem hiding this comment.
No, I get that part. But if you look at the actual code that is being called, it looks like it should be failing with a different error message. So I wonder if that message is checked at all.
There was a problem hiding this comment.
In this case, the panic message "negative coin amount: -1" is actually raised by sdk.NewCoin(...), not by SetTotalEscrowForDenom(...). @gjermundgaraba
There was a problem hiding this comment.
I added a comment in the test code to clarify that the panic is expected from sdk.NewCoin.
There was a problem hiding this comment.
Well, in that case, the test is actually wrong. We're just testing our test code :)
Can you instead change it so that we manually create a Coin struct with the expAmount, so we test the actual code?
There was a problem hiding this comment.
ok I created a coin struct, can you review it again? @gjermundgaraba
|
"PanicsWithError(...) is still being used → the test is still expecting a panic, not switching the logic to a returned error." |
|
can you review it again? @gjermundgaraba |
…8282) * fix TestSetGetTotalEscrowForDenom * fix TestSetGetTotalEscrowForDenom * add comment to explain where is panic * test(transfer): construct Coin manually to test SetTotalEscrowForDenom panic logic * lint --------- Co-authored-by: Gjermund Garaba <gjermund@garaba.net> (cherry picked from commit c6d52d0)
…8282) (#8308) * fix TestSetGetTotalEscrowForDenom * fix TestSetGetTotalEscrowForDenom * add comment to explain where is panic * test(transfer): construct Coin manually to test SetTotalEscrowForDenom panic logic * lint --------- Co-authored-by: Gjermund Garaba <gjermund@garaba.net> (cherry picked from commit c6d52d0) Co-authored-by: Hung Dinh | Decentrio <76930315+hungdinh82@users.noreply.github.com>
…8282) * fix TestSetGetTotalEscrowForDenom * fix TestSetGetTotalEscrowForDenom * add comment to explain where is panic * test(transfer): construct Coin manually to test SetTotalEscrowForDenom panic logic * lint --------- Co-authored-by: Gjermund Garaba <gjermund@garaba.net>
2 participants
Description
ref: #7175
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.