test: simplify process_blocks test-loop setup + enable spice#15630
test: simplify process_blocks test-loop setup + enable spice#15630darioush wants to merge 1 commit intonear:masterfrom
Conversation
darioush
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors process_blocks test-loop tests to use the newer TestLoopBuilder fluent setup APIs, removes a SPICE-specific ignore, and introduces small NodeRunner/TestLoopNode helpers to wait on finality via final_head.
Changes:
- Simplify test setup in
process_blocks.rsby collapsing manual genesis/epoch-config/client wiring intoTestLoopBuilderone-liners. - Add
TestLoopNode::final_head()andNodeRunner::run_until_final_head_height()helpers for finality-based waiting. - Enable
test_produce_block_with_approvals_arrived_earlyunder SPICE by removing thecfg_attr(..., ignore)gate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test-loop-tests/src/utils/node.rs |
Adds final_head() accessors and a NodeRunner helper to wait until a target final-head height. |
test-loop-tests/src/tests/process_blocks.rs |
Refactors test-loop setup and waiting logic to use builder chains and NodeRunner helpers; removes SPICE ignore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn run_until_final_head_height(&mut self, height: BlockHeight) { | ||
| let initial_height = self.final_head().height; | ||
| let height_diff = height.saturating_sub(initial_height) as usize; | ||
| let timeout = self.calculate_block_distance_timeout(height_diff); | ||
| self.run_until(|node| node.final_head().height >= height, timeout); | ||
| } |
There was a problem hiding this comment.
run_until_final_head_height derives its timeout from height - initial_final_head_height, but final_head is expected to lag behind head by (at least) the Doomslug finality window (e.g. chain tests show final_head at height 4 after processing up to height 6). That means reaching a given final-head height typically requires producing additional blocks beyond height_diff, so this timeout can be too short and make callers flaky. Consider adding a small finality slack (e.g. +2/+3 blocks) or deriving the timeout from run_until on head/last_final_block semantics rather than assuming 1:1 final-head progression.
There was a problem hiding this comment.
Seems fine to me, we are starting from the original final_head height. We can add slack here if needed or accept a timeout.
| let mut env = TestLoopBuilder::new().validators(4, 0).num_shards(4).epoch_length(100).build(); | ||
| let epoch_manager = env.node(0).client().epoch_manager.clone(); |
There was a problem hiding this comment.
This refactor changes the shard layout version used by the test: the previous setup used ShardLayout::multi_shard(4, 3), while TestLoopBuilder::num_shards(4) currently hard-codes ShardLayout::multi_shard(.., 1). If the test is meant to exercise shard-layout version 3 behavior (or keep semantics identical while simplifying setup), consider using .shard_layout(ShardLayout::multi_shard(4, 3)) here (or extending num_shards to accept a version).
There was a problem hiding this comment.
I don't think version matters for this test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #15630 +/- ##
=======================================
Coverage 69.38% 69.39%
=======================================
Files 942 942
Lines 213590 213598 +8
Branches 213590 213598 +8
=======================================
+ Hits 148203 148219 +16
+ Misses 59490 59480 -10
- Partials 5897 5899 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collapse the verbose genesis + epoch-config + clients setup in
ban_peer_for_invalid_block_commonandtest_produce_block_with_approvals_arrived_earlyto one-lineTestLoopBuildersetup, and switch their loop-until-height waits to theNodeRunnerhelpers.Adds two small helpers to support the second test:
TestLoopNode::final_head()NodeRunner::run_until_final_head_height()Also drops the
#[cfg_attr(feature = "protocol_feature_spice", ignore)]ontest_produce_block_with_approvals_arrived_early: the test passes under SPICE without modification.