Skip to content

test: clarify qbft_manager harness setup - #917

Draft
diegomrsantos wants to merge 5 commits into
sigp:unstablefrom
diegomrsantos:refactor/qbft-test-harness
Draft

test: clarify qbft_manager harness setup#917
diegomrsantos wants to merge 5 commits into
sigp:unstablefrom
diegomrsantos:refactor/qbft-test-harness

Conversation

@diegomrsantos

Copy link
Copy Markdown
Contributor

Problem, Evidence, and Context (Required)

  • The qbft_manager tests currently hide simple scenarios behind generic setup and generic harness entrypoints, which makes behavior-focused reviews harder than they need to be.
  • This is worth doing now because follow-up QBFT lifecycle and retention work is easier to review once the harness reads in terms of duties, clusters, and expected outcomes.
  • Evidence: a reader has to jump through setup_test(...), raw TestContext::new(...), and verify_consensus() to understand straightforward scenarios like "start one instance and assert consensus".
  • Relevant links: related review context in refactor: use beacon chain deadlines for QBFT instance cleanup #719.

Change Overview (Required)

  • Rework the test helper surface so the high-level qbft_manager tests use intent-named helpers for runtime setup, beacon-vote cluster startup, and consensus assertions.
  • Keep the existing harness behavior intact by preserving the executor lifetime semantics inside the test context instead of relying on an opaque setup bag.
  • Update the existing manager tests to read as scenario-driven Arrange/Act/Assert flows instead of harness plumbing.
  • Intentionally did not change any runtime QBFT behavior, protocol semantics, or production code paths.

Risks, Trade-offs, and Mitigations (Required)

  • Main risk: accidentally changing test-harness behavior while simplifying names and setup flow.
  • Trade-off: the test module grows a few thin wrapper helpers in exchange for much clearer behavior-level tests.
  • Mitigation: the diff stays in a single test file, the underlying harness remains the same, and the full qbft_manager test crate still passes after the refactor.

Validation (Required)

  • cargo test -p qbft_manager
  • cargo fmt --all --check
  • Commit hook checks: cargo fmt --all, cargo clippy --all, cargo sort workspace

Rollback (Required for behavior or runtime changes; optional otherwise)

  • Safe to revert as a normal code-only rollback.
  • No config, data, or operational impact.

Blockers / Dependencies (Optional)

  • No functional blockers.
  • This branch is currently 4 commits behind unstable; rebasing before merge should be straightforward because the PR only touches one test file.

Additional Info / Next Steps (Optional)

  • This is intended as a prep refactor so follow-up qbft_manager test and lifecycle changes can build on clearer test intent.

@diegomrsantos diegomrsantos self-assigned this Apr 3, 2026
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.

1 participant