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

[feature] #2712: Config proptests #2770

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

ilchu
Copy link
Contributor

@ilchu ilchu commented Sep 21, 2022

Description of the Change

  • Added property testing to the Iroha and Iroha client ConfigurationProxy structs
  • Expanded default config.json to allow them to be built from proxies immediately upon reading
  • Updates some docs, error messages, trait derives

Issue

Closes #2712.

Benefits

More confidence in current proxy implementation's robustness.

Possible Drawbacks

I've struggled a bit with a generation strategy that would be compact yet showing the intended invariant, so I hope nothing's missing.

Usage Examples or Tests

cargo test --package iroha_config --lib -- iroha::tests client::tests --nocapture

Alternate Designs [optional]

@ilchu ilchu self-assigned this Sep 21, 2022
@github-actions github-actions bot added iroha2-dev The re-implementation of a BFT hyperledger in RUST config-changes Changes in configuration and start up of the Iroha labels Sep 21, 2022
@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #2770 (c4cd158) into iroha2-dev (a16d9c3) will decrease coverage by 1.52%.
The diff coverage is 73.36%.

❗ Current head c4cd158 differs from pull request most recent head 1159a61. Consider uploading reports for the commit 1159a61 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #2770      +/-   ##
==============================================
- Coverage       67.61%   66.09%   -1.53%     
==============================================
  Files             140      166      +26     
  Lines           26173    30222    +4049     
==============================================
+ Hits            17696    19974    +2278     
- Misses           8477    10248    +1771     
Impacted Files Coverage Δ
actor/src/actor_id.rs 90.00% <ø> (ø)
actor/src/deadlock.rs 100.00% <ø> (ø)
cli/derive/src/lib.rs 92.30% <ø> (+17.58%) ⬆️
cli/src/event.rs 41.86% <ø> (ø)
cli/src/main.rs 1.09% <0.00%> (-0.26%) ⬇️
cli/src/stream.rs 81.39% <ø> (ø)
cli/src/torii/mod.rs 28.88% <0.00%> (ø)
client/src/http.rs 47.82% <ø> (ø)
client/src/http_default.rs 41.59% <ø> (-18.59%) ⬇️
client_cli/src/main.rs 0.27% <0.00%> (+0.01%) ⬆️
... and 212 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Erigara Erigara self-assigned this Sep 21, 2022
@ilchu ilchu force-pushed the config-proptest branch 3 times, most recently from 1444c86 to 80a0229 Compare September 22, 2022 12:21
Copy link
Contributor

@Erigara Erigara 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 the first steps in direction of property based testing.
IMO if done right (especially something like stateful-testsing) it could help us to find bugs that would be hard to find otherwise.
John Hughes has great talks on this topic.

@appetrosyan
Copy link
Contributor

appetrosyan commented Sep 29, 2022

@Erigara @ilchu Just a vocabulary note. The word finalize has the -ize ending, the spelling of which cannot be agreed upon. If we write ise we'll annoy all American contributors, while writing -ize annoys British English speakers.

Can we come up with a different word? For example: finish, commit, complete, hell even consummate?

appetrosyan
appetrosyan previously approved these changes Oct 4, 2022
Erigara
Erigara previously approved these changes Oct 5, 2022
@ilchu ilchu dismissed stale reviews from Erigara and appetrosyan via 7a14ff3 October 5, 2022 08:37
appetrosyan
appetrosyan previously approved these changes Oct 5, 2022
@ilchu ilchu requested a review from Erigara October 6, 2022 11:05
@ilchu ilchu force-pushed the config-proptest branch 2 times, most recently from 332cc53 to 09aac50 Compare October 7, 2022 12:12
@SamHSmith SamHSmith self-assigned this Oct 10, 2022
Copy link
Contributor

@SamHSmith SamHSmith left a comment

Choose a reason for hiding this comment

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

LGTM

@ilchu ilchu merged commit 6279892 into hyperledger-iroha:iroha2-dev Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-changes Changes in configuration and start up of the Iroha iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants