Make snapshot generation explicit in test setup - #8329
Open
cjen1-msft wants to merge 11 commits into
Open
Conversation
Add node-local committed snapshot listing and passive target-seqno waiting. Migrate timing, election, and persistence callers without changing generation or recovery-copy behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the helper-only snapshot file tests and their CMake registration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the existing snapshot trigger's target when reading governance history. Compare recovery snapshot files in place, including read-only inputs, without copying into common_dir. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace eight combined snapshot-helper calls with explicit triggers and passive waits. Use committed application transactions as snapshot targets and read generated files directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use explicit snapshot triggers and passive waits for large snapshots, access and digest checks, selection, and fetch-size limits. Preserve intentional backup copies and assert the oversized snapshot precondition. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Snapshot selection currently drops the view component when sorting snapshots, which can mis-order candidates and select the wrong “best” snapshot when multiple views share a seqno.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates CCF’s Python end-to-end tests to make snapshot creation/selection explicit during test setup, replacing implicit snapshot generation/copying with explicit trigger_snapshot() calls and passive wait_for_snapshot() waits, and reading node-owned snapshot files directly where needed.
Changes:
- Replace uses of
network.get_committed_snapshots(...)(and ad-hoc directory scans) withprimary.trigger_snapshot()+primary.wait_for_snapshot(...)in snapshot-driven scenarios. - Update snapshot inspection tests to open the returned snapshot file directly (rather than scanning/copying from a committed snapshots directory).
- Refactor snapshot-selection setup logic to build candidate lists from
primary.get_snapshots().
Custom instructions used:
- None (no additional repository instruction files or Skills were loaded for this review).
File summaries
| File | Description |
|---|---|
| tests/recovery_snapshot_endorsements.py | Switch one setup path to explicit snapshot trigger + wait before continuing the recovery endorsement flow. |
| tests/e2e_operations.py | Migrate multiple snapshot-related scenarios to trigger_snapshot()/wait_for_snapshot() and node-owned snapshot file reads, removing implicit snapshot directory polling/copying. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+684
to
+685
| seqno, _ = ccf.ledger.snapshot_index_from_filename(snapshot_path) | ||
| src_snapshots.append((seqno, os.path.basename(snapshot_path), snapshot_path)) |
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.
Replace implicit snapshot generation and copying in test setup with explicit triggers and passive waits.
Builds on the disk-backed helpers from #8317, now merged.
Migrate generation and inspection scenarios in
e2e_operations.pyandrecovery_snapshot_endorsements.py. Manual snapshot tests retain their explicit triggers and assert the returned snapshot's contents. Fetch scenarios replace interval-sized transaction batches with a committed application transaction,trigger_snapshot(), andwait_for_snapshot(). The target is the application transaction, not the trigger request's TxID, which may be newer than the snapshot boundary.Large-snapshot, snapshot access, and representation-digest checks read node-owned files directly. Snapshot selection waits for distinct committed generations instead of sleeping, while retaining its intentional copies to backups. The fetch-size-limit case writes a 2 KB payload and asserts that the generated snapshot exceeds the 1 KB limit before checking rejection.
Generated files are read directly rather than copied through
common_dir. No new infrastructure helper is needed. Recovery fixture copying, other join paths, corruption, and LTS callers remain for later slices.scripts/ci-checks.shpasses. The affectedschema_testscenarios andrecovery_snapshot_endorsements_testcould not start locally because theloggingapplication executable is missing. End-to-end execution remains outstanding.