test: revive node+wallet multi-process integration suite#772
Conversation
Replace the bitrotted 1.1.0-era integration crate with modern tests that run a real grin_servers node against grin-wallet foreign/owner listeners (mine to wallet, HTTP slate send/receive). Node-only multi-server coverage now lives in mimblewimble/grin#3910. Wire the crate into the workspace and Linux CI (serial execution).
|
rebase to staging! Fix merge conflicts, please. |
| #grin_p2p = { git = "https://github.com/mimblewimble/grin", branch = "milestone/1.1.0" } | ||
| #grin_servers = { git = "https://github.com/mimblewimble/grin", branch = "milestone/1.1.0" } | ||
| # Align with workspace grin crates (node multi-server stack). | ||
| grin_core = "5.4.1" |
There was a problem hiding this comment.
The integration crate still pulls the node crates from crates.io, while staging now uses the pinned grin submodule. In a local merge this resolved two different Grin stacks, and the test build then failed against the current APIs. Could we use the same path dependencies as staging here?
|
|
||
| pub fn start_node(cfg: servers::ServerConfig) -> servers::Server { | ||
| // grin_servers 5.4.1: (config, stop_state, api_chan) | ||
| servers::Server::new(cfg, None, leak_api_chan()).expect("node starts") |
There was a problem hiding this comment.
After rebasing, this helper module still does not compile against current staging: Server::new needs another argument, DefaultLCProvider no longer has a lifetime parameter, HTTPNodeClient::new expects a timeout, and foreign_listener requires a concrete TorConfig. Could we update the complete helper against the submodule API and run cargo test afterwards?
| @@ -0,0 +1,191 @@ | |||
| // Copyright 2021 The Grin Developers | |||
| api.tx_lock_outputs(m, &slate)?; | ||
|
|
||
| // Receive on B (in-process foreign; same process as HTTP listener but direct call). | ||
| controller::controller::foreign_single_use(wallet_b.clone(), mask_b_clone.clone(), |fapi| { |
There was a problem hiding this comment.
The slate is passed directly to wallet B in-process here instead of being sent through its HTTP listener. I removed wallet B’s listener locally and the test still passed. Could we actually send the slate to rec_addr so the test covers the HTTP path it describes?
| pub fn start_foreign_listener(wallet: WalletHandle, mask: Option<grin_util::secp::key::SecretKey>, addr: &str) { | ||
| let keychain_mask = Arc::new(Mutex::new(mask)); | ||
| let listen = addr.to_string(); | ||
| thread::spawn(move || { |
There was a problem hiding this comment.
The listener result is discarded and readiness is only assumed after a fixed sleep. A bind or startup failure therefore appears later as a timeout or may remain unnoticed. Could we report startup readiness through a channel and shut down the thread cleanly at the end of the test?
| @@ -0,0 +1,20 @@ | |||
| # Wallet + node integration tests | |||
|
|
|||
| Multi-process tests that run a real Grin node (`grin_servers`) together with | |||
There was a problem hiding this comment.
These tests run as threads inside one process, and the owner HTTP listener is currently not used at all. Could we describe this as live node-and-wallet integration with foreign HTTP, or actually add the missing owner and multi-process coverage?
| Multi-process tests that run a real Grin node (`grin_servers`) together with | ||
| `grin-wallet` foreign/owner HTTP listeners. | ||
|
|
||
| **Node-only** multi-server suites (seeding, propagation, body sync, stratum) |
There was a problem hiding this comment.
The revived node suites are currently still part of the open mimblewimble/grin #3910 PR and do not live in grin yet. Could we link the open PR and phrase this accordingly until it is merged?
|
|
||
| //! Helpers for multi-process node + wallet integration tests. | ||
|
|
||
| #![allow(dead_code)] |
There was a problem hiding this comment.
The blanket allow(dead_code) hides the unused owner listener as well as peer_addr and seeds. Could we remove or actually use these helpers and keep the compiler warning enabled?
| } | ||
|
|
||
| /// Start foreign HTTP listener (coinbase / slate receive) on a background thread. | ||
| pub fn start_foreign_listener(wallet: WalletHandle, mask: Option<grin_util::secp::key::SecretKey>, addr: &str) { |
There was a problem hiding this comment.
cargo fmt --all -- --check currently fails in this file and in wallet_node.rs. Could we run cargo fmt before merging?
| - name: Test ${{ matrix.job_args }} | ||
| working-directory: ${{ matrix.job_args }} | ||
| run: cargo test --release | ||
| # Multi-process node+wallet tests need serial execution (ports / RocksDB). |
There was a problem hiding this comment.
Grin uses LMDB here, not RocksDB, and the same wording also appears in the README. Could we simply say “fixed ports and data directories” instead?
Grin++ use RocksDB
2 participants
Summary
integrationcrate (commented-out 1.1.0 deps / pre-split APIs).grin_servers5.4.1 node with wallet foreign listeners:simulnet,stratum, v1 REST API probes). Those now live in mimblewimble/grin#3910 (#2957).--test-threads=1).In-process mock-node wallet coverage remains under
controller/tests/.Test plan
cd integration && cargo test --release -- --test-threads=1(2 tests pass locally)integrationjob green