Flaky test fix: Reduce unnecessary reads from live ledgers, and log a warning when they do occur#8047
Open
eddyashton wants to merge 3 commits into
Open
Conversation
…ish_chunk_flush_from_state_get
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces test flakiness by avoiding unnecessary parsing of ledger data from live nodes. It introduces safer helpers to force ledger chunk creation and wait until the corresponding committed chunk exists on disk, and adds warnings when tests still copy/parse ledgers from live nodes.
Changes:
- Add
Network.create_and_wait_for_chunk()to force a ledger chunk and wait until a committed chunk covering a target seqno is present. - Add
Node.get_committed_ledger_dirs()/Node.get_committed_ledger()helpers and emit warnings when copying/parsing ledger files from a live node. - Update multiple tests to use the new chunk/committed-ledger helpers instead of directly reading/parsing live ledger directories.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/recovery.py | Switch snapshot/ledger-offset recovery flow to wait for committed chunks via new helper. |
| tests/reconfiguration.py | Replace live-ledger parsing with committed-ledger helpers and chunk waiting. |
| tests/lts_compatibility.py | Simplify “fresh public state” retrieval and avoid direct in-flight chunk parsing. |
| tests/jwt_test.py | Replace direct ledger construction with primary.get_committed_ledger() and chunk waiting. |
| tests/infra/node.py | Add committed-ledger discovery/copy helpers and warnings when operating on live nodes. |
| tests/infra/network.py | Add create_and_wait_for_chunk() and refactor ledger-public-state helpers to use committed ledger copies. |
| tests/governance_history.py | Update ledger reads to use committed-ledger helpers and chunk waiting. |
| tests/e2e_operations.py | Update forced-ledger-chunk test to use new helper and committed ledger reads. |
achamayou
reviewed
Jul 9, 2026
| node, _ = self.find_primary() | ||
|
|
||
| proposal = self.consortium.force_ledger_chunk(node) | ||
| target_seqno = seqno if seqno is not None else proposal.completed_seqno |
Member
There was a problem hiding this comment.
Suggested change
| target_seqno = seqno if seqno is not None else proposal.completed_seqno | |
| target_seqno = seqno or proposal.completed_seqno |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lots of test cases were caling
network.get_latest_ledger_public_state()(dangerous to call on a live node, as it involves reading ledger state), when all they really wanted wasnetwork.force_ledger_chunk()and wait. This renames some helpers, and where possible makes them all do the (simpler, safer) latter.Since this touches lots of call-points which are still constructing a
Ledgerfrom a live node, we at leastLOG.warningthat this is dangerous, hopefully pointing at the root cause for future flaky failures.