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!: black-box integration tests #5124

Merged
merged 22 commits into from
Oct 18, 2024

Conversation

0x009922
Copy link
Contributor

@0x009922 0x009922 commented Oct 4, 2024

Re-opening of #5087

Build on top of, and should be undrafted after merging of:


Context

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. Close Proper black-box integration tests MVP #4500
    • Create temp dir for every randomly generated peer and store there its state, configs, and logs.
    • Configuration through raw TOML - feel your users! Close Refactor config usage in integration tests #4383
    • 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 test) and inter-process (cargo nextest Use Nextest #4987).
  • Higher-level API suitable 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:

  • Rewrite many tests, reduce flakiness to the minumum. Turns out some were broken, but passing for other reasons. It was primarily a cause of a messy test network API. Partially address Optimise CI - reduce flakyness, unify workflows #4516
  • 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.
  • Make irohad a closed binary; don't expose Iroha; remove samples from it. Close [suggestion] Refactor Iroha CLI #4136
  • Minor changes in iroha_core.

Further steps

Flaky tests

Migration Guide

For devs: you need to have irohad available in $PATH before running tests using iroha_test_network. The easiest way to achieve this is by running:

cargo install --path crates/irohad --locked

Review notes

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.
  • Upload test network artifacts

@0x009922 0x009922 added Refactor Improvement to overall code quality Tests blocked this problem can't be fixed yet labels Oct 4, 2024
@0x009922 0x009922 self-assigned this Oct 4, 2024
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Oct 4, 2024
@0x009922 0x009922 changed the title fix(iroha_config): broken trusted peers check refactor!: black-box integration tests Oct 4, 2024
@0x009922 0x009922 linked an issue Oct 8, 2024 that may be closed by this pull request
@0x009922 0x009922 removed api-changes Changes in the API for client libraries blocked this problem can't be fixed yet labels Oct 9, 2024
@0x009922 0x009922 linked an issue Oct 9, 2024 that may be closed by this pull request
6 tasks
@0x009922 0x009922 marked this pull request as ready for review October 9, 2024 04:05
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Oct 9, 2024
@0x009922
Copy link
Contributor Author

0x009922 commented Oct 9, 2024

test_network_runs artifact takes 70mb. Is this okay?

Copy link
Contributor Author

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

Self-review

crates/iroha_crypto/src/lib.rs Outdated Show resolved Hide resolved
crates/iroha_torii/src/lib.rs Outdated Show resolved Hide resolved
crates/iroha_torii/src/utils.rs Outdated Show resolved Hide resolved
crates/iroha_wasm_builder/src/lib.rs Outdated Show resolved Hide resolved
hooks/pre-commit.sample Outdated Show resolved Hide resolved
crates/iroha/tests/integration/queries/mod.rs Outdated Show resolved Hide resolved
crates/iroha/tests/integration/set_parameter.rs Outdated Show resolved Hide resolved
crates/iroha/tests/integration/permissions.rs Outdated Show resolved Hide resolved
crates/iroha/tests/integration/extra_functional/normal.rs Outdated Show resolved Hide resolved
crates/iroha/tests/integration/extra_functional/mod.rs Outdated Show resolved Hide resolved
@Erigara Erigara self-assigned this Oct 14, 2024
nxsaken
nxsaken previously approved these changes Oct 18, 2024
mversic
mversic previously approved these changes Oct 18, 2024
@mversic mversic dismissed stale reviews from nxsaken and themself via b729267 October 18, 2024 08:36
@mversic mversic disabled auto-merge October 18, 2024 08:36
@mversic mversic merged commit bc79734 into hyperledger:main Oct 18, 2024
13 checks passed
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 Refactor Improvement to overall code quality Tests
Projects
Status: Done
7 participants