Add update tests #1083
Add update tests #1083
Conversation
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
…cairo-contracts into add-update-tests
| assert_eq!(initial_supply, current_supply, "Incorrect supply before transfer"); | ||
| assert_eq!(state.balance_of(sender), SUPPLY, "Incorrect balance before transfer"); | ||
| assert_eq!(state.balance_of(recipient), 0, "Incorrect balance before transfer"); |
There was a problem hiding this comment.
Minor one:
| assert_eq!(initial_supply, current_supply, "Incorrect supply before transfer"); | |
| assert_eq!(state.balance_of(sender), SUPPLY, "Incorrect balance before transfer"); | |
| assert_eq!(state.balance_of(recipient), 0, "Incorrect balance before transfer"); | |
| assert_eq!(initial_supply, current_supply, "Incorrect supply before transfer"); | |
| assert_eq!(state.balance_of(sender), SUPPLY, "Incorrect sender balance before transfer"); | |
| assert_eq!(state.balance_of(recipient), 0, "Incorrect recipient balance before transfer"); |
There was a problem hiding this comment.
IMO this doesn't add much for the potential inconsistencies it might generate. The rule is to add messages when it very clearly adds clarity. I don't feel this is the case, and if it is not that clear, we should not add them.
| assert_eq!( | ||
| state.balance_of(sender), initial_supply - amount, "Incorrect balance after transfer" | ||
| ); | ||
| assert_eq!(state.balance_of(recipient), amount, "Incorrect balance after transfer"); |
There was a problem hiding this comment.
| assert_eq!( | |
| state.balance_of(sender), initial_supply - amount, "Incorrect balance after transfer" | |
| ); | |
| assert_eq!(state.balance_of(recipient), amount, "Incorrect balance after transfer"); | |
| assert_eq!( | |
| state.balance_of(sender), initial_supply - amount, "Incorrect sender balance after transfer" | |
| ); | |
| assert_eq!(state.balance_of(recipient), amount, "Incorrect recipient balance after transfer"); |
| @@ -98,7 +98,8 @@ fn test_approve() { | |||
| let mut spy = spy_events(); | |||
|
|
|||
| start_cheat_caller_address(contract_address, OWNER()); | |||
| assert!(state.approve(SPENDER(), VALUE)); | |||
| let success = state.approve(SPENDER(), VALUE); | |||
| assert!(success, "Transfer failed"); | |||
There was a problem hiding this comment.
| assert!(success, "Transfer failed"); | |
| assert!(success, "Approval failed"); |
Is this error message adding real value?
There was a problem hiding this comment.
To my mind, it does once you have to debug the failing test
There was a problem hiding this comment.
You can go to the specific line from the test error message, and in this particular case, I think it is quite clear that that line error implies that the approval failed. How exactly does this help debug?
There was a problem hiding this comment.
Foundry output doesn't specify the number of the line that failed. However, it does write the variable names passed to the assertion and their values
For example, let's take the test cases for transfers. Both and are called in a single test cases and both helper functions contain the same assertion assert_eq!(initial_supply, current_supply);
How to find out which one has failed?
There was a problem hiding this comment.
I got your point, but I don't think "Approval failed" is going to help with that if we add the same message. Adding before and after (in the other examples) differentiates and is true it makes it easier to find, but if the message intention is only to find the line easier, we may directly put the line as a message, and I don't think the message should be used for that. We can/should push for snforge to print the line, but I'm still against using messages for this purpose.
There was a problem hiding this comment.
I see your point. Previously some error messages, like "Approval failed" here, did make it more obvious which exact assertion has failed. But after migration things got better since SnFoundry does a pretty good job printing the assertion itself and the names of variables passed to it
I shall agree with you, most of the error messages can be safely omitted. In some cases, thought, we should keep them until SnFoundry starts printing the assertion's line number
There was a problem hiding this comment.
@ericnordelo @immrsd so I changed this back and removed the error messages because whether we need these error messages or not for debugging is clearly debatable. Because it's debatable, I'm favoring consistency with how we've already used the assert_state_x assertions in the erc721 and erc1155 tests (with no error messages).
I propose we create a new issue and continue this discussion there. If we were to conclude that we should include error messages in these assertions, It'd be nice to also update the other token tests as well
|
|
||
| assert_state_before_transfer(OWNER(), RECIPIENT()); | ||
| let success = state.transfer(RECIPIENT(), VALUE); | ||
| assert!(success, "Transfer failed"); |
There was a problem hiding this comment.
I don't think this is error message is adding real value, you guys think it does?
There was a problem hiding this comment.
Some testing practices recommend to follow the rule "one test case — one assertion". If it was the case for our tests, then adding error messages would make no difference. But in most cases we have 3+ assertions and an error occurs in a test, then it may be difficult to find out which assertion has failed
| assert_eq!(initial_supply, current_supply, "Incorrect supply before transfer"); | ||
| assert_eq!(state.balance_of(sender), SUPPLY, "Incorrect balance before transfer"); | ||
| assert_eq!(state.balance_of(recipient), 0, "Incorrect balance before transfer"); |
There was a problem hiding this comment.
IMO this doesn't add much for the potential inconsistencies it might generate. The rule is to add messages when it very clearly adds clarity. I don't feel this is the case, and if it is not that clear, we should not add them.
Fixes #1063.
PR Checklist