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

Removal of arguments from test_network crate #1414

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

i1i1
Copy link

@i1i1 i1i1 commented Sep 8, 2021

Description of the Change

Removal of arguments from test_network crate

Issue

It is a failure point for us to have arguments in tests, as they force us to read from disk. Also that forces us to take with us 3 configs and put them everywhere in tests.

Benefits

Less errors due to disk failures. Easier genesis usage in tests.

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Sep 8, 2021
@i1i1 i1i1 force-pushed the args_fixup branch 4 times, most recently from 115b716 to 70a83bd Compare September 8, 2021 08:41
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #1414 (ba87e42) into iroha2-dev (326ff44) will increase coverage by 0.70%.
The diff coverage is 65.54%.

Impacted file tree graph

@@              Coverage Diff               @@
##           iroha2-dev    #1414      +/-   ##
==============================================
+ Coverage       75.35%   76.06%   +0.70%     
==============================================
  Files             121      119       -2     
  Lines           17560    17503      -57     
==============================================
+ Hits            13233    13313      +80     
+ Misses           4327     4190     -137     
Impacted Files Coverage Δ
iroha/src/torii/mod.rs 81.48% <ø> (+11.13%) ⬆️
iroha_actor/src/broker.rs 92.94% <ø> (ø)
iroha_actor/src/lib.rs 82.69% <ø> (ø)
iroha_cli/src/main.rs 7.14% <ø> (ø)
iroha_client_cli/src/main.rs 0.40% <0.00%> (ø)
iroha_p2p/src/network.rs 82.26% <ø> (ø)
iroha_schema/src/lib.rs 40.42% <0.00%> (-4.29%) ⬇️
iroha/src/lib.rs 62.02% <33.33%> (-17.29%) ⬇️
iroha/src/smartcontracts/isi/asset.rs 55.15% <50.00%> (-0.74%) ⬇️
iroha_data_model/src/lib.rs 65.82% <60.78%> (-0.27%) ⬇️
... and 12 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 298466b...ba87e42. Read the comment docs.

iroha/test_network/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 124 to 130
G::from_configuration(
submit_genesis,
GENESIS_PATH,
&cfg.genesis_configuration,
cfg.sumeragi_configuration.max_instruction_number,
)
.expect("Failed to")
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think as you mentioned - the purpose was not to load it from file in tests, right?

Copy link
Author

Choose a reason for hiding this comment

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

The purpose was not to load to much files from path. I guess we can read files during compilation of crate. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this way it reads file earlier, which is good if this helps with tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial thoughts were that you would supply genesis Instructions directly in the code. This might be useful for tests and I think our Java client team does this.

Copy link
Author

Choose a reason for hiding this comment

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

Dont see a difference between these 2 things, apart from ide autocompletion.

Copy link
Contributor

Choose a reason for hiding this comment

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

So that we will not create json for each test that wants custom genesis is a big plus to my mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

But anyway we can merge as is. It is just a possible improvement for the future.

@i1i1 i1i1 merged commit 5e5b459 into hyperledger-iroha:iroha2-dev Sep 9, 2021
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.

2 participants