Testing sent founds visibility in multi-test#363
Conversation
hashedone
changed the title
ethanfrey
left a comment
ethanfrey
left a comment
There was a problem hiding this comment.
Nice start with code reorg.
Maybe you finish this (if more there) and merge this in just as a reorg.
And do the sent funds tests as a new PR
packages/multi-test/src/app.rs
Outdated
| PayoutCountResponse, PayoutInitMessage, PayoutQueryMsg, PayoutSudoMsg, ReflectMessage, | ||
| ReflectQueryMsg, | ||
| }; | ||
| use crate::test_helpers::contracts::{payout, reflect}; |
| @@ -0,0 +1,94 @@ | |||
| use schemars::JsonSchema; | |||
There was a problem hiding this comment.
nice idea pulling these into another file
| SetAge { age: u32 }, | ||
| } | ||
|
|
||
| fn instantiate_error( |
There was a problem hiding this comment.
Is there a reason you left error top level and didn't pull it into a separate file like payout and reflect?
I know it is smaller, but was there another reason?
(I like your reorg)
There was a problem hiding this comment.
I decided that those functions are just not part of specific contracts, they are generic to be used in other contracts which doesn't implement centrain functionality. I actually used query_error this way in my new contract (not yet here). One thing I am not sure is a COUNTitem - I left it here, but I debate if it belongs to payout contract.
There was a problem hiding this comment.
Forget it - I just missed there is whole contract for errors. I am blind.
|
|
||
| // reflect count updated | ||
| let qres: PayoutCountResponse = app | ||
| let qres: payout::CountResponse = app |
There was a problem hiding this comment.
All of these type names get better
There was a problem hiding this comment.
My hint: if you have 3+ names in your file/module with exactly the same prefix, you want to have a submodule ;) Extrapolation of famous Djiskstra words: "Two or more - use a for".
|
I agree with merging those refactor in separate PR, but I prefer this one to be the feature one (better name, better tracking), so I just created separate one with those changes only: #365 |
hashedone
force-pushed
the
347-test-sent-founds-visibility
branch
from
fba9f32 to
02f4aad
Compare
hashedone
force-pushed
the
347-test-sent-founds-visibility
branch
from
02f4aad to
0b2c4ba
Compare
|
Please ping me on discord when ready for the final review |
| _info: MessageInfo, | ||
| _msg: EmptyMsg, | ||
| ) -> Result<Response, StdError> { | ||
| let init = HACKATOM.load(deps.storage)?; |
There was a problem hiding this comment.
Nice and simple. Anyone can pay out.
Does the job for the test and no other frills.
I like it.
| Ok(resp) | ||
| } | ||
|
|
||
| fn query(_deps: Deps, _env: Env, msg: EmptyMsg) -> Result<Binary, StdError> { |
There was a problem hiding this comment.
I would just return an error here if you don't support any queries.
I would actually add a query to return the beneficiary / InitMsg
| to_binary(&msg) | ||
| } | ||
|
|
||
| pub fn contract() -> Box<dyn Contract<Empty>> { |
|
|
||
| // Check balance of all accounts to ensure no tokens where burned or created, and they are | ||
| // in correct places | ||
| assert_eq!(get_balance(&app, &owner), &[]); |
There was a problem hiding this comment.
Awesome work.
I would usually add such asserts at the state between instantiate and execute, but you are correct, those are totally not needed for such a BDD test like this. If these failed for some reason, I would add some intermediary checks, otherwise it is just noise.
Looks good to me.
There was a problem hiding this comment.
I just try to find a balance between checking everything, and deciding what is relevant to case. Basically it is nice when you can understand the test just looking at it quickly. It would be ideal to test everything (like even to check if some side state didn't change - which there is no reason to happen, but for sake of completeness), but I found this checks should actually cover test case.
2 participants
closes #347