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] #2238: Add peer builder for tests #2304

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

pesterev
Copy link
Contributor

@pesterev pesterev commented Jun 1, 2022

Description of the Change

This replaces nested methods which creates Peer with PeerBuilder structure (Builder pattern).

  • MVP
  • CI checks pass
  • Add the docs/comments

Issue

Resolves #2238

Benefits

Makes creating a test environment more flexible and convenient.

Possible Drawbacks

Idk may increase test run time.

Alternate Designs [optional]

Make Peer structure a builder itself. But in that design, will need to keep parameters (genesis, configuration, instruction_validator, query_validator, temp_dir) inside the Peer structure to start the peer's task - it will makes Peer structure a huge.

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #2304 (84289e8) into iroha2-dev (6973f71) will decrease coverage by 0.11%.
The diff coverage is 54.50%.

@@              Coverage Diff               @@
##           iroha2-dev    #2304      +/-   ##
==============================================
- Coverage       65.50%   65.38%   -0.12%     
==============================================
  Files             133      133              
  Lines           24697    24801     +104     
==============================================
+ Hits            16177    16216      +39     
- Misses           8520     8585      +65     
Impacted Files Coverage Δ
client/src/client.rs 44.04% <0.00%> (-0.81%) ⬇️
core/src/block.rs 70.17% <ø> (ø)
core/src/smartcontracts/isi/mod.rs 52.45% <0.00%> (-0.14%) ⬇️
core/src/smartcontracts/isi/world.rs 9.52% <0.00%> (+0.43%) ⬆️
data_model/src/query.rs 39.34% <0.00%> (-1.90%) ⬇️
telemetry/src/retry_period.rs 68.18% <0.00%> (-10.77%) ⬇️
telemetry/src/ws.rs 86.97% <0.00%> (ø)
telemetry/src/config.rs 64.70% <33.33%> (ø)
core/test_network/src/lib.rs 46.97% <57.44%> (+0.86%) ⬆️
core/src/block_sync.rs 44.51% <75.00%> (ø)
... and 13 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 8cdda32...84289e8. Read the comment docs.

@pesterev
Copy link
Contributor Author

pesterev commented Jun 2, 2022

@Arjentix seems you know the context so can you please review it? Would be great if you leave some feedback on the PeerBuilder design in general.

Copy link
Contributor

@Arjentix Arjentix left a comment

Choose a reason for hiding this comment

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

Pretty the same I was thinking about!

core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
client/tests/integration/queries/account.rs Show resolved Hide resolved
@@ -16,22 +16,22 @@ use super::Configuration;
fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> {
let temp_dir = Arc::new(TempDir::new()?);

let mut configuration = Configuration::test();
let mut peer = <TestPeer>::new()?;
configuration.sumeragi.trusted_peers.peers = std::iter::once(peer.id.clone()).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this was removed?

client/tests/integration/restart_peer.rs Show resolved Hide resolved
Comment on lines 38 to 39
.expect("genesis creation failed")
.expect("genesis created");
Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange... Looks like it should be two different expectations but the message is pretty the same...

client/benches/torii.rs Show resolved Hide resolved
client/benches/torii.rs Show resolved Hide resolved
@pesterev pesterev force-pushed the peer-builder branch 3 times, most recently from acc0305 to 94f5732 Compare June 3, 2022 11:12
@Arjentix Arjentix self-assigned this Jun 4, 2022
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
client/benches/torii.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
@pesterev pesterev force-pushed the peer-builder branch 2 times, most recently from cca42bc to 24ad0a4 Compare June 6, 2022 07:45
@pesterev pesterev marked this pull request as ready for review June 6, 2022 07:55
outoftardis
outoftardis previously approved these changes Jun 6, 2022
Copy link
Contributor

@outoftardis outoftardis left a comment

Choose a reason for hiding this comment

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

docs ok

Copy link
Contributor

@Arjentix Arjentix left a comment

Choose a reason for hiding this comment

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

When all tests will pass, I'll approve

core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
core/test_network/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Vladimir Pesterev <pesterev@pm.me>
@pesterev pesterev requested a review from Arjentix June 6, 2022 15:45
@pesterev pesterev merged commit fc14018 into hyperledger:iroha2-dev Jun 6, 2022
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.

None yet

3 participants