test(ledger-suite-orchestrator): migrate integration tests to PocketIC

[gregorydemay]

Migrates the ledger-suite-orchestrator (LSO) integration tests from the internal ic-state-machine-tests harness to PocketIC, following the pattern already used for the EVM-RPC canister (dfinity/evm-rpc-canister#294). This is DEFI-2262: a pure refactor of the test harness — same tests, same coverage, same semantics.

CkErc20Setup in rs/ethereum/cketh embeds LSO's fixture and shares a StateMachine with it, so flipping LSO's fixture in place would break cketh in the same commit. To keep this change single-component and the tree green, this PR adds a PocketIC fixture in LSO's test_utils alongside the existing StateMachine one (precedent: rs/ledger_suite/icp/test_utils's state_machine_helpers/pocket_ic_helpers coexisting) and migrates only ledger-suite-orchestrator/tests/tests.rs to it. The StateMachine fixture stays untouched at its current paths and is still exercised by cketh; it is deleted only in the last PR of this stack, once cketh no longer needs it.

Summary of changes

• New pocket_ic module in ledger-suite-orchestrator/test_utils (LedgerSuiteOrchestrator, AddErc20TokenFlow, ManagedCanistersAssert, UniversalCanister, new_pocket_ic()), mirroring the existing StateMachine-based types and method names one-for-one so later PRs in the stack only need to swap import paths.
• new_pocket_ic() builds a system subnet (LSO tests assert exact cycle balances, which only hold on a non-charging subnet) with canister-execution rate limiting disabled (LSO repeatedly installs/upgrades ledger, index and archive canisters).
• Metrics assertions now go through ic-metrics-assert's PocketIcHttpQuery (feature pocket_ic) instead of a hand-rolled CanisterHttpQuery impl.
• tests/tests.rs fully migrated to the new fixture.
• Cargo.toml/BUILD.bazel updated accordingly; test_utils keeps ic-state-machine-tests for now (still needed by the old fixture).

Coverage

• All spec requirements from the PR are covered: PocketIC fixture added alongside the old one, tests/tests.rs fully migrated, old fixture/paths untouched, cketh still builds against the untouched fixture.

📚 PR stack

1. test(ledger-suite-orchestrator): migrate integration tests to PocketIC #10949 — test(ledger-suite-orchestrator): migrate integration tests to PocketIC 👈 you are here
2. test(cketh): deduplicate signed-transaction literals in integration tests #10950 — test(cketh): deduplicate signed-transaction literals in integration tests
3. test(cketh): migrate integration tests to PocketIC #10955 — test(cketh): migrate integration tests to PocketIC
4. test(ledger-suite-orchestrator): drop the StateMachine test fixture #10956 — test(ledger-suite-orchestrator): drop the StateMachine test fixture

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR migrates the ledger-suite-orchestrator integration tests from the ic-state-machine-tests harness to PocketIC by introducing a parallel PocketIC-based fixture in test_utils and switching tests/tests.rs to use it, while leaving the existing StateMachine fixture in place for downstream users (e.g., ckETH) until later PRs in the stack.

Changes:

• Added a new PocketIC-based test fixture module (test_utils/src/pocket_ic) including orchestrator setup, flows/assert helpers, and a PocketIC universal canister helper.
• Migrated rs/ethereum/ledger-suite-orchestrator/tests/tests.rs to use the new PocketIC fixture and PocketIC error types/APIs.
• Updated Cargo/Bazel dependencies to include PocketIC and PocketIC-enabled metrics assertions, and wired PocketIC server binary into the Bazel rust_ic_test.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rs/ethereum/ledger-suite-orchestrator/tests/tests.rs	Migrates the integration tests to the new PocketIC fixture and updates calls/assertions accordingly.
rs/ethereum/ledger-suite-orchestrator/test_utils/src/pocket_ic/mod.rs	Introduces the PocketIC-based LedgerSuiteOrchestrator fixture and PocketIC builder/helpers.
rs/ethereum/ledger-suite-orchestrator/test_utils/src/pocket_ic/flow.rs	Adds PocketIC-based test flows and managed-canister assertions used by the migrated tests.
rs/ethereum/ledger-suite-orchestrator/test_utils/src/pocket_ic/universal_canister.rs	Adds a PocketIC-based universal canister helper for management-canister interactions in tests.
rs/ethereum/ledger-suite-orchestrator/test_utils/src/lib.rs	Exposes the new pocket_ic module alongside existing test utilities.
rs/ethereum/ledger-suite-orchestrator/test_utils/Cargo.toml	Enables ic-metrics-assert PocketIC feature and adds a pocket-ic dependency.
rs/ethereum/ledger-suite-orchestrator/test_utils/BUILD.bazel	Switches to the PocketIC-enabled ic-metrics-assert Bazel target and adds //packages/pocket-ic.
rs/ethereum/ledger-suite-orchestrator/Cargo.toml	Drops ic-state-machine-tests from test deps and adds pocket-ic.
rs/ethereum/ledger-suite-orchestrator/BUILD.bazel	Adds PocketIC server binary + env wiring for the Bazel test target and updates deps accordingly.
Cargo.lock	Updates lockfile for the new pocket-ic dependency wiring.

Comments suppressed due to low confidence (1)

rs/ethereum/ledger-suite-orchestrator/tests/tests.rs:93

• This expectation hardcodes a specific principal string for the index canister. To keep the test invariant across PocketIC ID allocation changes, assert the metadata equals the actual index_id obtained from ManagedCanisterIds.

                MetadataKey::parse(MetadataKey::ICRC106_INDEX_PRINCIPAL).unwrap(),
                LedgerMetadataValue::from("xbze6-vd777-77777-aaaba-cai"),
            ),

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gregorydemay]

🤖 Leaving this as-is. The two literals are not vacuous: they are independently cross-checked against each other (the index_id returned by the orchestrator on line 46, and the ledger's own ICRC106_INDEX_PRINCIPAL metadata on line 91-93 must both equal this value), so the assertion still fails if the wiring is wrong even though the concrete text is PocketIC-version-dependent. This is the same fragility class as the StateMachine-era literal it replaces (ryjl3-tyaaa-aaaaa-aaaba-cai), and the migration plan for this PR stack explicitly accepts pocket-ic-server-version-dependent constants (see PR 3's derived-key-constant regeneration). Asserting against the live index_id instead would make the test tautological for that half of the check.

[gregorydemay]

🤖🧐 VERDICT: CHANGES_REQUESTED — 0 blockers, 2 mediums, 6 nits; CI pending (22 pass / 7 pending / 0 red — Bazel Test All, Bazel Test arm64-{linux,darwin}, Cargo Build/Lint Linux, Build IC, RBE Bazel Test All still running).

Neither medium is a semantics problem: the port itself is faithful. Both are the same finding — two AsRef<PocketIc> impls that lost their only callers in the translation and would otherwise be carried through to PR 4.

Review details

Verified by running

Command	Result
bazel test //rs/ethereum/ledger-suite-orchestrator:integration_tests --nocache_test_results (in dev container)	PASSED in 16.4s, 1/1
cargo check --all-targets --all-features -p ic-ledger-suite-orchestrator-test-utils -p ic-ledger-suite-orchestrator	clean
gh pr checks 10949	22 pass, 7 pending, 10 skipping, 0 failing

(Plain bazel test on the host fails in jemalloc's autoconf step — pre-existing local-infra issue, unrelated to this PR.)

Faithfulness of the port

• Test set unchanged: 21 #[test] before and after, identical function-name sets (diff of extracted fn names is empty). No test dropped, none weakened.
• API mapping matches the spec table throughout: execute_ingress→update_call(_, Principal::anonymous(), …), execute_ingress_as(pid, …)→update_call(c, pid.0, …), query→query_call(_, anonymous, …), send_ingress/await_ingress(id, MAX_TICKS)→submit_call/await_call, e.code()/e.description()→e.error_code/e.reject_message, create_canister_with_cycles(u128::MAX)→create_canister()+add_cycles(_, u128::MAX), execute_ingress_as(ctrl, ic_00, "install_code", …)→update_call_with_effective_principal(management_canister, RawEffectivePrincipal::CanisterId(target), ctrl, …), canister_status_as→canister_status(id, Some(sender)).
• Tick semantics preserved: advance_time_for_periodic_tasks / advance_time_for_upgrade keep the identical 6-tick sequences; wait_for still bounds at MAX_TICKS; the "tick before upgrade to finish current timers" tick is retained in both upgrade paths.
• Assertion strength preserved where the shape changed: assert_matches!(stop_res, Ok(WasmResult::Reply(_))) → .expect("failed to stop canister") and assert_reply(res.unwrap()) → .expect(…) are equivalent under pocket-ic's Result<Vec<u8>, RejectResponse> (a canister reject surfaces as Err, so the reject case still fails the test).
• Metrics: the hand-rolled CanisterHttpQuery<UserError> impls are replaced by PocketIcHttpQuery, whose blanket impl is exactly query_call(canister_id, Principal::anonymous(), "http_request", request) — same endpoint, same sender, same canister (orchestrator) for both LedgerSuiteOrchestrator and ManagedCanistersAssert.
• Hard-coded canister ID ryjl3-…-aaaba-cai → xbze6-vd777-77777-aaaba-cai: not vacuous. The value is compared against the index ID the orchestrator actually returns, and independently against the ledger's ICRC106_INDEX_PRINCIPAL metadata — two live assertions that cross-check each other. Same fragility class as the value it replaced.
• Explicit controller in should_upgrade_canisters_managed_but_not_installed_by_orchestrator: pre_existing_controller is captured from the temporary orchestrator that created those canisters, so it is genuinely a controller — this correctly replaces StateMachine's auto-pick now that PocketIC enforces the check.
• Nat cycle arithmetic in should_top_up_spawned_canisters: the deltas assert the same TEN_TRILLIONS increments; the added .clone()s are required because the balances are reused as the baseline for the second top-up.

Scope / boundary

• The single commit touches only Cargo.lock + rs/ethereum/ledger-suite-orchestrator/**. ✔
• Old StateMachine fixture: test_utils/src/flow.rs and test_utils/src/universal_canister.rs are byte-identical to master; test_utils/src/lib.rs gains exactly one line (pub mod pocket_ic;). ✔
• Build config matches the spec: integration_tests drops //rs/state_machine_tests, gains POCKET_IC_BIN + //packages/pocket-ic; test_utils keeps ic-state-machine-tests and moves to ic-metrics-assert_pocket_ic (the pocket_ic feature is purely additive, so the old fixture's CanisterHttpQuery impl still compiles). ✔

Maintainability accounting

• Duplication: ~915 lines of fixture are duplicated against test_utils/src/{lib,flow,universal_canister}.rs. This is the plan's deliberate, documented trade-off (CkErc20Setup shares an Arc<StateMachine> with LSO, so an in-place flip would break cketh in the same commit) with deletion scheduled for PR 4 — accepted, not counted against the verdict. I considered the alternative of abstracting both harnesses behind an env trait to avoid the copy, and it is clearly worse here: it would rewrite fixture code that cketh currently depends on, and the abstraction would itself be deleted in PR 4.
• Structural duplication vs. the wider repo: none beyond the intended mirror; the shape follows the established rs/ledger_suite/icp/test_utils precedent.
• Unused derives: none — no new types with derives are introduced.
• Primitive-obsession parameters: none introduced; the CanisterId/PrincipalId → candid::Principal change is the harness's own domain type, not a de-typing.
• Divergent invariant handling: cleared — .expect(…) is used uniformly on the new call paths; no site panics where a sibling swallows.
• Silent fallbacks: none — no unwrap_or_default(), Result::ok() discard or let _ = on a failure path. PocketIc::install_canister returns () and panics internally, so dropping its result is not a swallowed error.
• Test-only code in production modules: n/a, this is all test_utils.
• Comment minimalism: one stale comment carried over (flagged inline at flow.rs:94); no requirement-ID tags, no JIRA references, no commented-out code.

Test pyramid

Not applicable as a coverage question — this is a harness migration with no production-behaviour change, so "a new test per behaviour change" is satisfied by the existing suite continuing to pass unmodified in content.

[gregorydemay]

🤖🧐 VERDICT: READY (re-review of de18dda2f9) — 0 blockers, 0 mediums, 0 nits outstanding; all 8 findings resolved. CI pending, nothing red (13 pass / 11 pending / 9 skipping / 0 failing) — ⚠️ merge only once CI completes green; I have not verified the pending jobs.

Re-review details

Per-finding resolution

#	Sev	Finding	Commit	Status
1	🟠	Dead AsRef<PocketIc> for LedgerSuiteOrchestrator	223a4482b3	✅ impl removed; PocketIc import still earns its place (Arc<PocketIc> field, get_pocket_ic, new_pocket_ic, out_of_band_upgrade)
2	🟠	Dead AsRef<PocketIc> for ManagedCanistersAssert	223a4482b3	✅ impl removed; PocketIc still used by get_pocket_ic
3	🔵	13 inline crate:: paths + positional panic message	759c6466e8	✅ single use crate::{...} block; wait_for back to inline-capture "…{MAX_TICKS} ticks…"
4	🔵	ic_cdk::…::CanisterStatusResult spelled twice	759c6466e8	✅ aliased CdkCanisterStatusResult, used in both the return type and the Decode! arm
5	🔵	Redundant .clone() on module_hash	d0af1f47a5	✅ both dropped; module_hash moves out of the owned temporary
6	🔵	Fully-qualified ic_management_canister_types::CanisterId	759c6466e8	✅ imported by name, matching mod.rs
7	🔵	Stale "state machine tests" comment	52b82fe587	✅ parenthetical dropped; remaining comment reads cleanly and states no harness-specific timing
8	🔵	//:pocket-ic-server in both data and deps	de18dda2f9	✅ kept in data only; POCKET_IC_BIN still resolves (test re-run confirms)

Copilot's thread on tests.rs:46 (fixed canister principal): the "leave as-is" reply matches what I independently validated in the first pass — the two literals are cross-checked against each other (orchestrator-returned index ID vs. the ledger's ICRC106_INDEX_PRINCIPAL metadata), so neither is vacuous, and the coupling is the same class the StateMachine fixture already had. No objection.

Verified by running (on de18dda2f9)

Command	Result
bazel test //rs/ethereum/ledger-suite-orchestrator:integration_tests --nocache_test_results (dev container)	PASSED in 15.2s, 1/1
cargo check --all-targets --all-features -p ic-ledger-suite-orchestrator-test-utils -p ic-ledger-suite-orchestrator	clean, no warnings
git diff --stat 522634fb44 de18dda2f9	10 files, all under rs/ethereum/ledger-suite-orchestrator/ + Cargo.lock — component boundary still respected
old-fixture check	test_utils/src/{flow,universal_canister}.rs still byte-identical to master; lib.rs still +1 line (pub mod pocket_ic;)

Nothing new introduced

The five commits are strictly subtractive or rename-only — no call sites, senders, tick sequences, or assertions were touched. The faithfulness conclusions from the first pass (21/21 tests, identical function-name set, API mapping per spec, preserved tick semantics and assertion strength) still hold; the net effect on the diff is −14 lines.

One forward-looking note for PR 4: consolidating into a single use crate::{...} block makes the hoist easier than the scattered crate:: prefixes did — if pocket_ic/mod.rs is merged into lib.rs, that one import line is deleted rather than 13 path prefixes rewritten. No action needed here.

Outstanding

Only CI. Nothing is red, but Bazel Test All, Bazel Test arm64-{linux,darwin}, Cargo Build/Lint Linux, Build IC, Lock Generate, Candid compatibility checks, Autofix, Bazel Run Fuzzers and RBE / Infer Bazel Targets were still running when this verdict was written. Final approval and merge are the human's call.