Multi test example #137
Multi test example #137
Conversation
There was a problem hiding this comment.
Please take into account that these are my first impressions. I would probably have a better opinion when using this API to write some integration tests myself.
I like it! It makes writing complex integration tests not only possible, but also relatively straightforward.
|
|
||
| use crate::msg::{CreateMsg, DetailsResponse, HandleMsg, InitMsg, QueryMsg, ReceiveMsg}; | ||
|
|
||
| fn mock_router() -> Router { |
There was a problem hiding this comment.
From an usability point of view, the name Router is a little confusing to me. I understand this is just the (mock) blockchain. Or, a high level description of it? I'm not sure about this, but maybe MockChain, something like that?
Something that makes clear this is a framework to write better / more versatile integration tests.
There was a problem hiding this comment.
Good Point. I will rename it.
There was a problem hiding this comment.
How about BaseApp or Application? That is what they call the equivalent in the Cosmos SDK.
The object that holds all state and the various handlers.
There was a problem hiding this comment.
I think Application or App is better than Router. In the end, it's a matter of documenting it.
| assert_ne!(cash_addr, escrow_addr); | ||
|
|
||
| // set up cw20 helpers | ||
| let cash = Cw20Contract(cash_addr.clone()); |
There was a problem hiding this comment.
This was confusing to me.
Maybe Cw20Addr would be a better name? Or even, following the usual api helpers: deps.api.cw20_address().
Update: I see now this is more than an address (more like a collection of helper functions?). Maybe what was confusing was the instantiation from an HumanAddr. Will revisit later, in any case.
There was a problem hiding this comment.
Can you propose better naming or docs for this struct? We will be using this pattern a lot more and this should not be confusing.
I would be happy for a PR or an issue with a clearly proposed design. I think these helpers are only used in this new multi-test
There was a problem hiding this comment.
OK, I'll think about this, and play with it using the multi-test code base, to see if I can come up with a better name / design.
| assert_eq!(6, res.attributes.len()); | ||
|
|
||
| // ensure balances updated | ||
| let owner_balance = cash.balance(&router, owner.clone()).unwrap(); |
There was a problem hiding this comment.
This is totally accessory, but maybe doing a query for the balance here using router would be clearer. I mean, given that this is testing the integration, it would be nice to test the queries too.
There was a problem hiding this comment.
It does the query via router.
I really need to rename/document the cash: CW20Contract type better. Rather than manually building QueryRequests for this contract, cash.balance will do that for you, a short-cut for 6 lines or so of boilerplate.
All the actual work happens via the router, but this provides a nice helper wrapper.
We could add one explicit query to show that. But mainly I think we need to start documenting and improving naming of these contract-specific helpers. That would make it much easier for all the tests to just call methods rather than build up the raw request and message types.
There was a problem hiding this comment.
I agree. Some documentation / usage examples will solve this. Explicitly mentioning that these are wrappers / helpers on top of Router / App is a good idea.
|
|
||
| // ensure escrow properly created | ||
| let details: DetailsResponse = router | ||
| .wrap() |
There was a problem hiding this comment.
It would be nice if this could be avoided, i.e. handled internally or so. In any case, get_wrapper() or get_querier() would be a better name? This is exposing something, not wrapping it, AFAIK.
There was a problem hiding this comment.
router.as_querier_wrapper() would be the most explicit. I just felt it was too verbose.
I am happy for better naming, but take a look at some of the big changes in cosmwasm-std for 0.12 when we took in dyn Querier and such instead of generics. (And I am happy for better approaches/naming).
Basically, the Querier trait only required one method (raw_query) and had a lot of default generic implementations. As I made this &dyn Querier, we could no longer include generics and defaults on the trait. You can see how QuerierWrapper assumed most of the functionality that used to be in Querier. And can easily be created from a Querier.
I am not sure what a better way to handle this is. I cannot see how to use "deref" magic here to allow router.query_wasm_smart(...) directly, which is what I wanted. Please take a look at that wrapper struct (now you understand why) and I am happy for a concrete suggestion to make this cleaner.
There was a problem hiding this comment.
I see, thanks for the explanation about the need for Wrapper. Naming is tricky... will play with the code base to see if I can propose something concrete.
|
Thank you for the feedback. I will make the adjustments on the two actionable items (rename and argument reordering). The other comments highlight some places where better naming/docs/usability would be nice, but it is unclear how exactly it could be improved. A clear proposal for any of those points as issue or PR would be very well received. |
This adds a new "multi test" for sending cw20 tokens to an escrow and releasing it.
This should demonstrate how to use multi-test. We can also use this to see how to adjust the high-level APIs.
Please let me know how we can improve the APIs. But this should be much easier than everything we had until now.