Improve tests #864
Improve tests #864
Conversation
| use core::fmt::{Debug, Formatter, Error}; | ||
| use starknet::{ContractAddress, ClassHash}; | ||
|
|
||
| impl DebugContractAddress of core::fmt::Debug<ContractAddress> { | ||
| fn fmt(self: @ContractAddress, ref f: Formatter) -> Result<(), Error> { | ||
| let address: felt252 = (*self).into(); | ||
| write!(f, "{address:?}") | ||
| } | ||
| } | ||
|
|
||
| impl DebugClassHash of core::fmt::Debug<ClassHash> { | ||
| fn fmt(self: @ClassHash, ref f: Formatter) -> Result<(), Error> { | ||
| let hash: felt252 = (*self).into(); | ||
| write!(f, "{hash:?}") | ||
| } | ||
| } |
There was a problem hiding this comment.
Also, should we document this addition in the CHANGELOG?
There was a problem hiding this comment.
Not sure if we should add test stuff in the changelog. Actually, I think we shouldn't, even when I added a couple of entries already. I think changelog should be for API-related stuff of the library, not including testing, what do you think?
There was a problem hiding this comment.
If you agree I will remove the test-related entries already added.
There was a problem hiding this comment.
Yeah, I agree with not including testing
There was a problem hiding this comment.
agreed offline to focus on relevant API changes, albeit with some flexibility analyzing case by case
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
…tracts into feat/improve-tests
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
| use core::fmt::{Debug, Formatter, Error}; | ||
| use starknet::{ContractAddress, ClassHash}; | ||
|
|
||
| impl DebugContractAddress of core::fmt::Debug<ContractAddress> { | ||
| fn fmt(self: @ContractAddress, ref f: Formatter) -> Result<(), Error> { | ||
| let address: felt252 = (*self).into(); | ||
| write!(f, "{address:?}") | ||
| } | ||
| } | ||
|
|
||
| impl DebugClassHash of core::fmt::Debug<ClassHash> { | ||
| fn fmt(self: @ClassHash, ref f: Formatter) -> Result<(), Error> { | ||
| let hash: felt252 = (*self).into(); | ||
| write!(f, "{hash:?}") | ||
| } | ||
| } |
There was a problem hiding this comment.
agreed offline to focus on relevant API changes, albeit with some flexibility analyzing case by case
src/tests/presets/test_account.cairo
Outdated
| assert!(supports_isrc5); | ||
| assert!(supports_isrc6); |
There was a problem hiding this comment.
why not asserting right after each query? this is the least important optimization but we can save us a call in case isrc5 fails early -- that's actually a guideline we try to follow in our contracts: fail early and loud
There was a problem hiding this comment.
Good point, I will update it.
| let supports_isrc5 = dispatcher.supports_interface(ISRC5_ID); | ||
| let supports_isrc6 = dispatcher.supports_interface(ISRC6_ID); | ||
| let doesnt_support_0x123 = !dispatcher.supports_interface(0x123); | ||
| assert!(supports_isrc5); | ||
| assert!(supports_isrc6); | ||
| assert!(doesnt_support_0x123); |
There was a problem hiding this comment.
we should settle on a query/assert order pattern. in here it's all queries then asserts, in the previous test it's mixed, and there's also query/assert pairs
There was a problem hiding this comment.
Asserting right after each query as you suggested looks like the best approach to me.
There was a problem hiding this comment.
added a comment in the style guidelines issue
Fixes #670 #820
See this comment for assertion guidelines.
This PR also removes the
#[available_gas]attributes.PR Checklist