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!: re-implement iroha_test_network #5087

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

0x009922
Copy link
Contributor

@0x009922 0x009922 commented Sep 19, 2024

Context

Very early draft, work in progress!

iroha_test_network has a few problems:

  • It doesn't test Iroha as a black box, but runs directly using internal implementation details.
  • Execution flow of peers is messy, hard to debug & control. (e.g. for cases of restarts). Logs from all peers are all over the place, very noisy.
  • Messy API: hard to fine tune the network & peers for specific cases; high coupling with defaults; thread::sleeps with pipeline time instead of providing precise lifecycle hooks
  • Non-optimal defaults: most of the tests don't need 4 peers with 4s of pipeline time. A single peer with close-to-zero block time works significantly faster.

Black boxing would mean to run Iroha through its CLI as a dedicated process, just as users would do.

Solution

Re-implement iroha_test_network:

  • Black-boxing: run irohad as a direct child process.
    • Create temp dir for every randomly generated peer and store there its state, configs, and logs. It could be observed (and maybe even replayed??) after tests run.
    • Configuration through raw TOML - feel your users!
    • Can work with any irohad target (e.g. debug, `release)
  • Allocate ports automatically (fslock-based solution) - no more manual ports setting. Works intra-process (cargo tests) and inter-process (cargo nextest Use Nextest #4987).
  • Higher-level, dynamic, flexible, async-first API, helpful for writing expressive tests.
  • Optimal defaults: a single peer with very short timings.
  • Faster!
  • Bonus: test execution can now be terminated immediately with SIGINT (Ctrl + C). It used to be suspended.

Other changes:

  • Turns out some integration tests were broken, but passing for other reasons. It was primarily a cause of a messy test network API.
  • Make irohad a closed binary; don't expose Iroha; remove samples from it. Closes [suggestion] Refactor Iroha CLI #4136
  • Minor changes in iroha_core and iroha_torii.
  • remove unstable_network tests: they rely on a direct violation of black boxing - the use of FreezeStatus to make peers faulty. To be re-implemented in some other way.

Further steps

Migration Guide (optional)

TODO


Review notes (optional)

Due to Client still being blocking, there is some ugly code with spawn_blocking.

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

Undraft:

  • Update all integration tests
  • Update benches and examples
  • Support running from CI (set irohad binary path via ENV?)

@0x009922 0x009922 added Enhancement New feature or request Tests labels Sep 19, 2024
@0x009922 0x009922 self-assigned this Sep 19, 2024
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Sep 19, 2024
@Erigara Erigara assigned Erigara and unassigned Erigara Sep 19, 2024
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
@@ -83,11 +83,6 @@ toml = { workspace = true }
nonzero_ext = { workspace = true }

[dev-dependencies]
# FIXME: These three activate `transparent_api` but client should never activate this feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this line relate to the imports below irohad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh, I don't see how it was related to it before. Seemed an odd outdated comment for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries Enhancement New feature or request Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[suggestion] Refactor Iroha CLI
3 participants