[feature] #2053: Add tests to 'private_blockchain' #2332

pesterev · 2022-06-07T17:58:12Z

Signed-off-by: Vladimir Pesterev pesterev@pm.me

Description of the Change

I added tests for permissions related to account and some asset queries and also for permissions related to register. ~~But the OnlyAccountsDomain and OnlyAccountsData permissions need to be tested on many other queries.~~(will be in the next PRs) The tests are very different and it is very difficult to write a macro that would generate that tests.

register module
tests permissions with all account related and some asset related queries
tests permissions with other queries (resolved: will be in the next PRs)
weird impl IsAllowed for ProhibitRegisterDomains

Issue

Partially resolves #2053

Benefits

Improved coverage

Possible Drawbacks

None

Alternate Designs [optional]

Using macros would reduce the number of tests, but the tests are too different.

Also we can just write tests only for queries with non-trivial logic.

permissions_validators/src/private_blockchain/mod.rs

codecov · 2022-06-07T18:21:43Z

Codecov Report

Merging #2332 (07e62bd) into iroha2-dev (6973f71) will increase coverage by 1.52%.
The diff coverage is 69.35%.

@@              Coverage Diff               @@
##           iroha2-dev    #2332      +/-   ##
==============================================
+ Coverage       65.50%   67.02%   +1.52%     
==============================================
  Files             133      140       +7     
  Lines           24697    26282    +1585     
==============================================
+ Hits            16177    17616    +1439     
- Misses           8520     8666     +146

Impacted Files	Coverage Δ
cli/derive/src/lib.rs	`74.72% <ø> (ø)`
cli/src/torii/mod.rs	`28.88% <ø> (ø)`
config/src/lib.rs	`15.38% <0.00%> (+1.09%)`	⬆️
core/src/lib.rs	`100.00% <ø> (ø)`
core/src/smartcontracts/isi/account.rs	`12.30% <ø> (ø)`
core/src/smartcontracts/isi/block.rs	`100.00% <ø> (ø)`
core/src/smartcontracts/isi/domain.rs	`39.82% <ø> (ø)`
core/src/smartcontracts/isi/triggers.rs	`0.00% <0.00%> (ø)`
core/src/smartcontracts/isi/tx.rs	`25.00% <ø> (ø)`
core/src/smartcontracts/isi/world.rs	`9.52% <0.00%> (+0.43%)`	⬆️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67f8fc4...07e62bd. Read the comment docs.

appetrosyan · 2022-06-08T07:49:36Z

permissions_validators/src/private_blockchain/mod.rs

+
+    use super::*;
+
+    type TestEnv = (


Doesn't matter here, but this should be a struct. You can bind directly to names using Rust's pattern matching syntax.

appetrosyan · 2022-06-10T15:10:41Z

permissions_validators/src/private_blockchain/mod.rs

+
+        {
+            let only_accounts_domain: IsQueryAllowedBoxed<World> = query::OnlyAccountsDomain.into();
+


There should be a negative test-case (with is_ok()).

Can't allow to find any accounts with OnlyAccountsDomain. That's why there are no is_ok() cases here.

Why not to use another query to check is_ok() variant?

Oh I see, it tests queries, not validators

appetrosyan · 2022-06-10T15:11:36Z

permissions_validators/src/private_blockchain/mod.rs

+    }
+
+    impl TestEnv {
+        fn new() -> Self {


With rust 1.62, we should be able to turn this into a const function.

I'd remove the comments above blocks (but keep the grouping). Instead I'd add a doc-comment above the function, so people could hover and see both the assets, the domains and the relationships.

With rust 1.62, we should be able to turn this into a const function.

It is not possible to turn fn new() into a const fn because the functions called inside fn new() are not const.

Yes. const new for some types is going to be stabilised soon, so it should be possible to propagate those changes to here.

appetrosyan · 2022-06-10T15:13:21Z

permissions_validators/src/private_blockchain/mod.rs

+
+            let wsv = WorldStateView::<World>::new(World::with(
+                [first_domain.clone(), second_domain.clone()],
+                vec![],


Style point: prefer Vec::new() for empty immutable vectors. This conveys to the reader that what you made empty is meant to remain empty.

appetrosyan · 2022-06-10T15:15:39Z

permissions_validators/src/private_blockchain/mod.rs

+
+        {
+            let only_accounts_domain: IsQueryAllowedBoxed<World> = query::OnlyAccountsDomain.into();
+


This is an example of a good assertion block. You need to be able to demonstrate when it works and when it doesn't.

appetrosyan · 2022-06-10T15:16:13Z

permissions_validators/src/private_blockchain/mod.rs

+            ..
+        } = TestEnv::new();
+
+        let op1 = QueryBox::FindAssetById(FindAssetById::new(gold_asset_id));


I'd give these operations more descriprtive names. e.g. find_gold, find_bronze etc.

appetrosyan · 2022-06-10T15:22:08Z

permissions_validators/src/private_blockchain/mod.rs

+            assert!(only_accounts_data.check(&alice_id, &op3, &wsv).is_err());
+        }
+    }
+


Group the tests that check the queries into mod queries. It should end about here.

Also add mod revoke_and_grant_tests and include a few of the following tests.

appetrosyan · 2022-06-10T15:25:02Z

permissions_validators/src/private_blockchain/mod.rs

+    }
+
+    #[test]
+    fn granted_allowed_register_domains_success() {


I'd combine the test cases. I'd also give them better names: this one for example is closer to
add_register_domains_permission_allows_registering_domain. You're documenting an invariant, in this case that it A) isn't possible to register the domain without this permission, B) that it becomes possible after adding the permission, C) that it becomes impossible again when you revoke it.

All three should be checked in a single test.

appetrosyan · 2022-06-13T05:39:39Z

permissions_validators/src/private_blockchain/mod.rs

+    }
+
+    impl TestEnv {
+        fn new() -> Self {


If you have time, please add doc comments, which explain who has which assets to this associated function, or to the struct (preferably both, preferably using #[doc(include...].

I couldn't think of anything better than describing relationships in the comments to the fields of TestEnv structure. Comments are prompted when you hover over the field. I wanted to use something like mermaid but rustdoc doesn't support it. Creating a separate markdown file is probably too much.

Sounds good.

Arjentix · 2022-06-14T13:29:19Z

permissions_validators/src/private_blockchain/register.rs

+        let register = if let Instruction::Register(register) = instruction {
            register
        } else {
            return Ok(());
        };
-        Err("Domain registration is prohibited.".to_owned())
+
+        if let Ok(RegistrableBox::Domain(_)) = register.object.evaluate(wsv, &Context::new()) {
+            Err("Domain registration is prohibited.".to_owned())
+        } else {
+            Ok(())
+        }


I'd rewrite something like this:

if let Instruction::Register(register) = instruction { if let Ok(RegistrableBox::Domain(_)) = register.object.evaluate(wsv, &Context::new()) { return Err("Domain registration is prohibited.".to_owned()); } } Ok(())

Easier to read for me

Probably with the merge of #2284 you'll have to add into() after to_owned()

Arjentix · 2022-06-14T14:00:49Z

permissions_validators/src/private_blockchain/mod.rs

+            let TestEnv {
+                alice_id,
+                bob_id,
+                carol_id,
+                wsv,
+                ..
+            } = TestEnv::new();


Interesting idea

Arjentix · 2022-06-14T14:03:47Z

permissions_validators/src/private_blockchain/mod.rs

+
+        {
+            let only_accounts_domain: IsQueryAllowedBoxed<World> = query::OnlyAccountsDomain.into();
+


Why not to use another query to check is_ok() variant?

Arjentix · 2022-06-14T14:04:55Z

permissions_validators/src/private_blockchain/mod.rs

+
+        {
+            let only_accounts_domain: IsQueryAllowedBoxed<World> = query::OnlyAccountsDomain.into();
+


Oh I see, it tests queries, not validators

Arjentix · 2022-06-14T14:14:12Z

permissions_validators/src/private_blockchain/mod.rs

+        }
+    }
+
+    mod revoke_and_grant {


I'd suggest to add more negative tests scenarios in this module tests.
I.e. add_register_domains_permission_allows_registering_domain_with_right_token() checks that Alice can register new domainsm, but doesn't check if anyone without permission token can do it too.

appetrosyan

Excellent!

appetrosyan · 2022-06-17T16:51:33Z

permissions_validators/src/private_blockchain/mod.rs

+    }
+
+    impl TestEnv {
+        fn new() -> Self {


Sounds good.

Signed-off-by: Vladimir Pesterev <pesterev@pm.me>

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jun 7, 2022

pesterev commented Jun 7, 2022

View reviewed changes

permissions_validators/src/private_blockchain/mod.rs Outdated Show resolved Hide resolved

pesterev force-pushed the private_blockchain_permissions_tests branch from 5cca2c0 to 8d8e9bc Compare June 7, 2022 18:10

pesterev changed the title ~~[test] #2053: Add tests to 'private_blockchain'~~ [feature] #2053: Add tests to 'private_blockchain' Jun 7, 2022

pesterev force-pushed the private_blockchain_permissions_tests branch from 8d8e9bc to 5d2d14a Compare June 7, 2022 20:37

appetrosyan reviewed Jun 8, 2022

View reviewed changes

pesterev force-pushed the private_blockchain_permissions_tests branch 3 times, most recently from f762fc5 to 636c667 Compare June 10, 2022 14:47

pesterev requested a review from appetrosyan June 10, 2022 14:49

pesterev marked this pull request as ready for review June 10, 2022 15:15

pesterev requested review from s8sato, mversic, Arjentix, SamHSmith, QuentinI, outoftardis, ilchu and Erigara as code owners June 10, 2022 15:15

appetrosyan reviewed Jun 10, 2022

View reviewed changes

pesterev force-pushed the private_blockchain_permissions_tests branch from 636c667 to b63607f Compare June 10, 2022 16:22

pesterev requested a review from appetrosyan June 10, 2022 16:26

appetrosyan previously approved these changes Jun 13, 2022

View reviewed changes

Arjentix suggested changes Jun 14, 2022

View reviewed changes

pesterev dismissed appetrosyan’s stale review via d1fff25 June 16, 2022 19:27

pesterev force-pushed the private_blockchain_permissions_tests branch 3 times, most recently from 8b7fd59 to 14fafcf Compare June 16, 2022 19:43

pesterev force-pushed the private_blockchain_permissions_tests branch from 14fafcf to 8a6f32d Compare June 16, 2022 19:50

pesterev requested a review from Arjentix June 16, 2022 19:51

pesterev force-pushed the private_blockchain_permissions_tests branch 2 times, most recently from 1ab9bf8 to 57b6c0b Compare June 17, 2022 16:27

pesterev requested a review from appetrosyan June 17, 2022 16:27

appetrosyan approved these changes Jun 17, 2022

View reviewed changes

permissions_validators/src/private_blockchain/mod.rs

}

impl TestEnv {

fn new() -> Self {

Copy link

Contributor

appetrosyan Jun 17, 2022

Choose a reason for hiding this comment

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

Sounds good.

appetrosyan previously approved these changes Jun 17, 2022

View reviewed changes

Arjentix previously approved these changes Jun 20, 2022

View reviewed changes

pesterev enabled auto-merge (rebase) June 20, 2022 14:35

pesterev disabled auto-merge June 20, 2022 14:36

pesterev force-pushed the private_blockchain_permissions_tests branch from 57b6c0b to d9ef279 Compare June 20, 2022 14:36

[feature] hyperledger#2053: Add tests to 'private_blockchain'

07e62bd

Signed-off-by: Vladimir Pesterev <pesterev@pm.me>

pesterev dismissed stale reviews from Arjentix and appetrosyan via 07e62bd June 20, 2022 14:53

pesterev force-pushed the private_blockchain_permissions_tests branch from d9ef279 to 07e62bd Compare June 20, 2022 14:53

pesterev requested review from appetrosyan and Arjentix June 20, 2022 16:05

Arjentix approved these changes Jun 20, 2022

View reviewed changes

appetrosyan approved these changes Jun 21, 2022

View reviewed changes

appetrosyan merged commit 4412892 into hyperledger:iroha2-dev Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] #2053: Add tests to 'private_blockchain' #2332

[feature] #2053: Add tests to 'private_blockchain' #2332

pesterev commented Jun 7, 2022 •

edited

codecov bot commented Jun 7, 2022 •

edited

appetrosyan Jun 8, 2022

appetrosyan Jun 10, 2022

pesterev Jun 10, 2022 •

edited

Arjentix Jun 14, 2022

Arjentix Jun 14, 2022

appetrosyan Jun 10, 2022

appetrosyan Jun 10, 2022

pesterev Jun 10, 2022

appetrosyan Jun 13, 2022

appetrosyan Jun 10, 2022

appetrosyan Jun 10, 2022

appetrosyan Jun 10, 2022

appetrosyan Jun 10, 2022

appetrosyan Jun 10, 2022

appetrosyan Jun 13, 2022

pesterev Jun 17, 2022

appetrosyan Jun 17, 2022

Arjentix Jun 14, 2022

Arjentix Jun 14, 2022

pesterev Jun 16, 2022

Arjentix Jun 14, 2022

Arjentix Jun 14, 2022

Arjentix Jun 14, 2022

Arjentix Jun 14, 2022

pesterev Jun 16, 2022

appetrosyan left a comment

appetrosyan Jun 17, 2022


		{
		let only_accounts_domain: IsQueryAllowedBoxed<World> = query::OnlyAccountsDomain.into();

[feature] #2053: Add tests to 'private_blockchain' #2332

[feature] #2053: Add tests to 'private_blockchain' #2332

Conversation

pesterev commented Jun 7, 2022 • edited

Description of the Change

Issue

Benefits

Possible Drawbacks

Alternate Designs [optional]

codecov bot commented Jun 7, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pesterev Jun 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appetrosyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pesterev commented Jun 7, 2022 •

edited

codecov bot commented Jun 7, 2022 •

edited

pesterev Jun 10, 2022 •

edited