Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi test example #137

Merged
merged 5 commits into from
Nov 16, 2020
Merged

Multi test example #137

merged 5 commits into from
Nov 16, 2020

Conversation

ethanfrey
Copy link
Member

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.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a look at this now. Look goods, will try to finish reviewing it by tomorrow.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Point. I will rename it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Application or App is better than Router. In the end, it's a matter of documenting it.

contracts/cw20-escrow/src/integration_test.rs Show resolved Hide resolved
contracts/cw20-escrow/src/integration_test.rs Show resolved Hide resolved
assert_ne!(cash_addr, escrow_addr);

// set up cw20 helpers
let cash = Cw20Contract(cash_addr.clone());
Copy link
Contributor

@maurolacy maurolacy Nov 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

contracts/cw20-escrow/src/integration_test.rs Outdated Show resolved Hide resolved
assert_eq!(6, res.attributes.len());

// ensure balances updated
let owner_balance = cash.balance(&router, owner.clone()).unwrap();
Copy link
Contributor

@maurolacy maurolacy Nov 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Contributor

@maurolacy maurolacy Nov 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@maurolacy maurolacy Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

contracts/cw20-escrow/src/integration_test.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

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.

@ethanfrey ethanfrey merged commit 564817d into master Nov 16, 2020
@ethanfrey ethanfrey deleted the multi-test-example branch November 16, 2020 16:46
@ethanfrey ethanfrey mentioned this pull request Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants