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

[test] #2272: Add tests for 'FindAssetDefinitionById' query #2273

Merged

Conversation

pesterev
Copy link
Contributor

Description of the Change

Add some tests for 'FindAssetDefinitionById' query.

Issue

Resolves #2272

Benefits

It tests the query.

Possible Drawbacks

None

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 26, 2022
@pesterev pesterev force-pushed the asset-definition-by-id-tests branch from 1969079 to 27d52bf Compare May 26, 2022 14:40
Comment on lines 20 to 29
test_client.submit_blocking(RegisterBox::new(asset_definition.clone()))?;
test_client.submit_blocking(MintBox::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is also worth querying and checking the result

Suggested change
test_client.submit_blocking(RegisterBox::new(asset_definition.clone()))?;
test_client.submit_blocking(MintBox::new(
test_client.submit_blocking(RegisterBox::new(asset_definition.clone()))?;
// Here
test_client.submit_blocking(MintBox::new(

@s8sato s8sato self-assigned this May 26, 2022
@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #2273 (118dc37) into iroha2-dev (1f1c8d7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           iroha2-dev    #2273      +/-   ##
==============================================
+ Coverage       64.67%   64.68%   +0.01%     
==============================================
  Files             131      131              
  Lines           24659    24664       +5     
==============================================
+ Hits            15947    15953       +6     
+ Misses           8712     8711       -1     
Impacted Files Coverage Δ
cli/src/torii/tests.rs 94.42% <100.00%> (+0.20%) ⬆️
actor/src/deadlock.rs 98.63% <0.00%> (-1.37%) ⬇️
data_model/src/domain.rs 77.77% <0.00%> (-1.15%) ⬇️
core/src/kura.rs 92.19% <0.00%> (-0.16%) ⬇️
core/src/smartcontracts/isi/asset.rs 33.84% <0.00%> (ø)
core/src/smartcontracts/isi/query.rs 88.92% <0.00%> (ø)
actor/src/lib.rs 84.09% <0.00%> (+0.30%) ⬆️
data_model/src/asset.rs 51.88% <0.00%> (+0.41%) ⬆️
data_model/src/query.rs 24.95% <0.00%> (+0.57%) ⬆️

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 1f1c8d7...118dc37. Read the comment docs.

@pesterev pesterev force-pushed the asset-definition-by-id-tests branch from 27d52bf to ca3670e Compare May 26, 2022 15:13
@pesterev pesterev requested a review from s8sato May 26, 2022 15:13
s8sato
s8sato previously approved these changes May 27, 2022
use test_network::Peer as TestPeer;

#[test]
fn find_asset_definition_by_id() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is not what I asked you to do, I would like to know why you decided not to add the query to an existing test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, now I got you. So I'm going to move this test into the closest module (account.rs) like this:

    // Registering accounts
    let register_accounts = accounts
        .iter()
        .skip(1) // Alice has already been registered in genesis
        .cloned()
        .map(|account_id| RegisterBox::new(Account::new(account_id, [])).into())
        .collect::<Vec<_>>();
    test_client.submit_all_blocking(register_accounts)?;

   // HERE
===>  let received_asset_definition = test_client.request(client::asset::definition_by_id(definition_id))?;
===>  assert_eq!(received_asset_definition, asset_definition.build());

    let mint_asset = accounts
        .iter()
        .cloned()
        .map(|account_id| <Asset as Identifiable>::Id::new(definition_id.clone(), account_id))
        .map(|asset_id| MintBox::new(1_u32, asset_id).into())
        .collect::<Vec<_>>();
    test_client.submit_all_blocking(mint_asset)?;

   // HERE AFTER MINTING
===>   let received_asset_definition = test_client.request(client::asset::definition_by_id(definition_id))?;
===>   assert_eq!(received_asset_definition, asset_definition.build());

    let accounts = HashSet::from(accounts);

Copy link
Contributor

Choose a reason for hiding this comment

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

I now realize that this PR only needs to replace FindAllAssetsDefinitions with FindAssetDefinitionById in this Torii unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah seems this test needs to replace but its only on the server-side. I also added a new client-side method so I think it also should be tested.

…' query

Signed-off-by: Vladimir Pesterev <pesterev@pm.me>
@pesterev pesterev force-pushed the asset-definition-by-id-tests branch from 4cd57ba to 118dc37 Compare May 27, 2022 11:33
@pesterev
Copy link
Contributor Author

@appetrosyan I've removed asset_definition.rs module and moved the test into an existing test in account.rs module.

@s8sato I've replaced FindAllAssetsDefinitions with FindAssetDefinitionById in find_asset_definition.

@pesterev pesterev requested a review from s8sato May 27, 2022 12:28
@s8sato s8sato merged commit 2ba9c8f into hyperledger-iroha:iroha2-dev May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants