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

Refactor of cw1-whitelist unit tests #343

Merged
merged 10 commits into from
Jul 26, 2021
Merged

Refactor of cw1-whitelist unit tests #343

merged 10 commits into from
Jul 26, 2021

Conversation

hashedone
Copy link
Contributor

As there is a request to create additional UTs for some contracts: #105, I went through the whitelist contract UT and found that kind of problem with adding many tests there is repetitive boilerplate in every test, which is not actually easy to read. Also because of some edge cases, some test may be actually incorrect (eg. if in future some items would be returned in different order because of any reason, UTs would start failing - while ordering in many cases shouldn't matter).

Therefore I came up with proposal to kind of unify the UTs structure to have visible setup => execute => verify structure, with - what is important - clear setup, so it is easier to figure out what is the test about.

Additional changes I am considering to make execution and possibly verify part simpler is to add execute/query methods directly on Suite (query via router), so the deps and mock_env are hidden. Additionally it would make testing queries through whole ecosystem, not just locally which might be more complete.

At the end I think, that the Suite structure could be partially generalized and extracted to multi-test, to encourage having clean an uniform test through the smart contracts.

@hashedone hashedone requested review from ethanfrey and maurolacy July 22, 2021 09:44
@CLAassistant
Copy link

CLAassistant commented Jul 22, 2021

CLA assistant check
All committers have signed the CLA.

@hashedone hashedone changed the title Refactor of cw1-whitelist unit tests WIP: Refactor of cw1-whitelist unit tests Jul 22, 2021
@hashedone hashedone marked this pull request as draft July 22, 2021 09:48
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice initiative and you seem to be finding your way around the repo quite well.
Added some comments for a few requested changes, otherwise looks good.

contracts/cw1-subkeys/src/contract.rs Show resolved Hide resolved
@@ -130,7 +130,7 @@ where
pub fn check_staking_permissions(
staking_msg: &StakingMsg,
permissions: Permissions,
) -> Result<bool, ContractError> {
) -> Result<(), ContractError> {
Copy link
Member

Choose a reason for hiding this comment

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

Why return a bool here if we only have Ok(true) (never Ok(false)).

It suggests there are 3 possible cases to handle: Ok(true), Ok(false), Err(_), when there are only 2: Ok(_) and Err(_)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is exactly why I changed it from bool to ZST - it was hurting my eyes (and also optimizer - in case when one type of Result is ZST, the Result itself becomes transparent).

Copy link
Member

Choose a reason for hiding this comment

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

I returned Result<(), ContractError> as it captured more failure cases than 1 (a bool would just capture one success and one failure case).

We have 1 success case and 4 failure cases.

Ahhh.... I misread the diff... I thought you made the bool. You removed it. 👀
Very good change

Copy link
Contributor

@maurolacy maurolacy Jul 22, 2021

Choose a reason for hiding this comment

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

👍🏼

}

pub fn check_distribution_permissions(
distribution_msg: &DistributionMsg,
permissions: Permissions,
) -> Result<bool, ContractError> {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you want to rename to assert_*_permissions? Then the return type would make more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean I find naming "check_" functions returning Result on failure handy (if I would like to return bool then I would call it more like question, let say - has_distribution_permissions). assert_* suggests, panic on failure from my POV (which basically happens here in one branch which I don't like - I would prefer debug assertion to check invalid branch as prerequirement, but on release I would just return error here).

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, remove this suggestion. I misread the diff and the comments up here are wrong.
Your way is better (and what I was arguing for)

&initial_allowances,
&initial_expirations,
);
let Suite { deps, .. } = SuiteConfig {
Copy link
Member

Choose a reason for hiding this comment

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

I see the idea of the Suite to handle many things more clearly.
Somehow the usage seems more verbose than the older setup code.

I wonder if it is possible to make usage more compact. With eg. 2 helpers:

Spender::with_allowance and Spender::with_expiring_allowance. That would reduce those 4-5 line struct inits into 1 line, and then the usage of the new Suite would really shine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually thinking about this - this is very generic proposal, but suggestions are very much welcome. I even considered to split initialization and early execution, adding utility for executes, to achieve something like:

let Suite { deps, owner_info: owner, .. } = SuiteConfig {
  // Only minimal config, or maybe just `Suite::init(...)` with args required to setup properly
}
.init()
.execute_increase_allowance(&owner, SENDER1, vec![coin(1111, TOKEN)], None)
.execute_increase_allowance(&owner, SENDER2, vec![coin(2222, TOKEN)], None);

This would benefit also redability of test logic imho. There can be argument, that this thing hides what is actually called (the execute itself), but wrapping it in init hides it even more, so I am ok with that.

},
];

assert_sorted_eq!(
Copy link
Member

Choose a reason for hiding this comment

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

I like the check here to sort before compare.

///
/// This is implemented as a macro instead of function to throw panic in the place of macro
/// usage instead of from function called inside test.
macro_rules! assert_sorted_eq {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting macro trick.

I write a number of helper functions and always re-run with RUST_BACKTRACE=1 when they fail to find the real location. This does seem more ergonomic.

We need to reconsider the macro-resistance in this codebase.

&initial_allowances,
&initial_expirations,
);
// let's try pagination.
Copy link
Member

Choose a reason for hiding this comment

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

Nice comment

contracts/cw1-whitelist/src/msg.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member

For some reason I cannot reply to this, but I will inline it:

I am actually thinking about this - this is very generic proposal, but suggestions are very much welcome. I even considered to split initialization and early execution, adding utility for executes, to achieve something like:

let Suite { deps, owner_info: owner, .. } = SuiteConfig {
  // Only minimal config, or maybe just `Suite::init(...)` with args required to setup properly
}
.init()
.execute_increase_allowance(&owner, SENDER1, vec![coin(1111, TOKEN)], None)
.execute_increase_allowance(&owner, SENDER2, vec![coin(2222, TOKEN)], None);

This would benefit also redability of test logic imho. There can be argument, that this thing hides what is actually called (the execute itself), but wrapping it in init hides it even more, so I am ok with that.

I am happy adding all kinds of magic helpers to SuiteConfig / init. Anything to build up the Suite. This is just to set up initial state, we trust that code works.

The code that is later being tested (execute), I would like to explicitly call.
The test body and the expected results should be explicit. The setup code is more or less boilerplate we can optimize away somehow

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Misread. I agree with the points on top.
Only comments relate to test setup helpers

@@ -130,7 +130,7 @@ where
pub fn check_staking_permissions(
staking_msg: &StakingMsg,
permissions: Permissions,
) -> Result<bool, ContractError> {
) -> Result<(), ContractError> {
Copy link
Member

Choose a reason for hiding this comment

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

I returned Result<(), ContractError> as it captured more failure cases than 1 (a bool would just capture one success and one failure case).

We have 1 success case and 4 failure cases.

Ahhh.... I misread the diff... I thought you made the bool. You removed it. 👀
Very good change

}

pub fn check_distribution_permissions(
distribution_msg: &DistributionMsg,
permissions: Permissions,
) -> Result<bool, ContractError> {
Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, remove this suggestion. I misread the diff and the comments up here are wrong.
Your way is better (and what I was arguing for)

@@ -149,13 +149,13 @@ pub fn check_staking_permissions(
}
s => panic!("Unsupported staking message: {:?}", s),
Copy link
Member

Choose a reason for hiding this comment

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

This should return Err().
Good eye

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.

I like these test helper structs / macros.

What we miss in being able to reference existing variables in test assertions, we gain in clarity and conciseness. Good work.

contracts/cw1-subkeys/src/contract.rs Show resolved Hide resolved
@@ -130,7 +130,7 @@ where
pub fn check_staking_permissions(
staking_msg: &StakingMsg,
permissions: Permissions,
) -> Result<bool, ContractError> {
) -> Result<(), ContractError> {
Copy link
Contributor

@maurolacy maurolacy Jul 22, 2021

Choose a reason for hiding this comment

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

👍🏼

contracts/cw1-subkeys/src/contract.rs Show resolved Hide resolved
contracts/cw1-subkeys/src/state.rs Outdated Show resolved Hide resolved
contracts/cw1-subkeys/src/state.rs Outdated Show resolved Hide resolved
contracts/cw1-whitelist/src/msg.rs Show resolved Hide resolved
contracts/cw1-whitelist/src/msg.rs Show resolved Hide resolved
contracts/cw1-whitelist/src/msg.rs Show resolved Hide resolved
contracts/cw1-whitelist/src/msg.rs Show resolved Hide resolved
assert_sorted_eq!(
expected,
[batch1, batch2].concat(),
|l: &AllowanceInfo, r: &AllowanceInfo| l.spender.cmp(&r.spender)
Copy link
Contributor

@maurolacy maurolacy Jul 22, 2021

Choose a reason for hiding this comment

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

Nice trick relying on PartialEq for sorting, instead of requiring Ord here.

I understand it would make sense to just add Ord (and Eq) to AllowanceInfo, and simplify this.

As that can require adding them to other stuff, including some structs in cosmwasm-std, I would say, let's create an issue to do just this; at least for cosmwasm-std. And then we can revisit this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defienetely implementing Ord for AllowanceInfo does not make sense - Ord is for types with reasonable sorting, and I find implementing it for this kind of type a missuse. Note that even standard library doesn't implement sort for types like floats (even if you could technically provide implementation which would somehow properly sort it). Actually it is questionable if there is proper way to pick proper key for comparison - is this anyhow intuitive that allowance with sender earlier in alphabet is in any way smaller than the other one?

My personal opinion is, that even if this adds some code, it avoids any confusion how things are sorted (and why). I think about two possible simplifications:

  1. try to write macro in the way, that it would be more elision friendly so type hints are not needed (actually type hints are half the length of the closure!) - unfortunatelly I have no idea how to approach this
  2. extract the comparison function to AllowanceInfo, but not as an implementation of Ord, but rather testing purpose method like AllowanceInfo::cmp_by_spender - actually I like the idea

Copy link
Contributor

@maurolacy maurolacy Jul 23, 2021

Choose a reason for hiding this comment

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

2. extract the comparison function to AllowanceInfo, but not as an implementation of Ord, but rather testing purpose method like AllowanceInfo::cmp_by_spender - actually I like the idea

Sounds good. That, and implementing these helpers only for tests. That could include deriving something like Ord, which does not make sense, or adds unnecessary complexity, only for tests using conditional compilation.

@hashedone hashedone force-pushed the test-refactor branch 5 times, most recently from 836160a to 90285f5 Compare July 25, 2021 13:12
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Very good stuff! Do you want to take it out of draft?

I made a few minor points, but the code looks great. The only concern I raise is two test cases are marked ignore. If you aren't sure of the expected conditions to finish them, please add a comment on them and either Mauro or I can help.

Other than that, I'd approve it.

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct PermissionsInfo {
pub spender: String,
pub permissions: Permissions,
}

#[cfg(any(test, feature = "test-utils"))]
Copy link
Member

Choose a reason for hiding this comment

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

Nice feature flag for imports


#[test]
fn query() {
let Suite { deps, .. } = SuiteConfig::new()
Copy link
Member

Choose a reason for hiding this comment

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

I'm loving the API design here.
Super concise and flexible. The builder-pattern SuiteConfig is working out very well. Much better than a number of setup functions with different args... so much more power here.

self
}

fn expire_allowances(mut self, spender: &'static str, expires: Expiration) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Okay, so these are different calls that can be combined.

with_allowance(SPENDER2, ..).expire_allowances(SPENDER2, ..) works.
And expire_allowances(SPENDER2, ..).with_allowance(SPENDER2, ..) should also work, right?

That is a very nice API avoiding N^2 init function issues. I would love a test case (on SuiteConfig) showing this works regardless of ordering. But design is 🥇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally don't like to test this simple functions as it turns out to be testing language instead of logic, but possibly it could be tested. The order should not matter as it just sets state, but making API so it matters is obviously possible, I just don't see a point to complicate testing utilities.

Copy link
Member

Choose a reason for hiding this comment

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

If there is a bug in that testing utilities, then some test case below will surely fail.
I would only write them if a test fails unexpectedly and it seems to be a util bug.

},
];

assert_sorted_eq!(
Copy link
Member

Choose a reason for hiding this comment

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

This concat (after we paginate through, we get all the values exactly once) is a nice test-case.
Cleaner to read and more stable if we change any implementation details, but captures business requirements

assert_eq!(rsp.data, None);

assert_eq!(
query_all_allowances(deps.as_ref(), None, None)
Copy link
Member

Choose a reason for hiding this comment

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

since there is only one SPENDER here, why not use query_allowance?
It seems like it would be a bit simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because as I am touching allowance, I want to be sure that only my specific allowance changed, and no side effect occurred. It is surprisingly almost the same (just one encapsulation more), and more generic test.

balance: NativeBalance(vec![allow1.clone(), allow2.clone()]),
expires: expires_height,
#[test]
#[ignore]
Copy link
Member

Choose a reason for hiding this comment

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

why ignore? does this fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it fails. I am pretty sure this test is correct (but double check if I understand logic), and it should be implemented, and then test would be "unignored".

balance: NativeBalance(vec![allow1.clone()]),
expires: expires_never,
#[test]
#[ignore]
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

@hashedone hashedone changed the title WIP: Refactor of cw1-whitelist unit tests Refactor of cw1-whitelist unit tests Jul 26, 2021
@hashedone hashedone marked this pull request as ready for review July 26, 2021 11:18
Co-authored-by: DzikiChrzan <dzikichrzan@tuta.io>
@hashedone
Copy link
Contributor Author

I just removed draft flag. One thing I wanted to have before merging is getting rid of prepare_test_case function to avoid two different approaches of setup in single test suite. One test I didn't clean up is the test of independence, but honestly my idea of how to improve this kind of testing is out of scope of this. Basically I am sure that introducing proptest would allow many test cases to be generated and not overflow the reader, but it is some work to do to introduce it, and it has its downsides.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Let's merge it and I can inspect the item about independence when I have a bit of space and comment on that. (And then have a smaller PR to review there).

Maybe you can pull that out into another mod (mod old_tests) before merging, so it is clear which are using which util functions? Or just leave it as is, it should be clear enough what is missing still

@hashedone hashedone merged commit d4ec341 into main Jul 26, 2021
@hashedone hashedone deleted the test-refactor branch July 26, 2021 13:54
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.

5 participants