Merge pull request #132 from sdmg15/fix/rpc-tests

fix unreliable rpc tests due to race condition issue on get_available_port

Author: ChrisSon15

Cargo.lock

@@ -4,7 +4,7 @@ use std::time::Duration;
 use assert_cmd::assert::Assert;
 use assert_cmd::cargo::cargo_bin_cmd;
 use bdk_wallet::bitcoin::hex::test_hex_unwrap as hex;
-use bdk_wallet::bitcoin::{consensus, OutPoint, Transaction};
+use bdk_wallet::bitcoin::{OutPoint, Transaction, consensus};
 use bdk_wallet::chain::{ChainPosition, ConfirmationBlockTime};
 use bdk_wallet::{KeychainKind, LocalOutput};
 use const_format::str_replace;
@@ -13,10 +13,11 @@ use predicates::str;
 use rpc::server::{WalletImpl, WalletServer};
 use rpc::wallet::{TxConfidence, WalletService, WalletServiceImpl, WalletServiceMock, WalletTx};
 use testenv::TestEnv;
+use tokio::net::TcpListener;
 use tokio::task::{self, JoinHandle};
 use tonic::transport::server::TcpIncoming;
 use tonic::transport::{self, Server};
-use unimock::{matching, MockFn as _, Unimock};
+use unimock::{MockFn as _, Unimock, matching};
 
 const CLI_TIMEOUT: Duration = Duration::from_millis(200);
 
@@ -100,8 +101,11 @@ fn test_cli_no_connection() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
 async fn test_cli_wallet_balance() {
     let testenv = TestEnv::new().expect("testEnv could not start");
-    let port = testenv::get_available_port().expect("port");
-    spawn_wallet_grpc_service(port, WalletServiceImpl::create_with_rpc_params(testenv.bitcoin_core_rpc_client().unwrap()));
+    let (port, listener) = TestEnv::get_bound_port().await.expect("listener");
+    spawn_wallet_grpc_service(
+        listener,
+        WalletServiceImpl::create_with_rpc_params(testenv.bitcoin_core_rpc_client().unwrap()),
+    );
 
     task::spawn_blocking(move || assert_cli_with_port(port, ["wallet-balance"]))
         .await.unwrap()
@@ -113,8 +117,11 @@ async fn test_cli_wallet_balance() {
 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
 async fn test_cli_new_address() {
     let testenv = TestEnv::new().expect("testEnv could not start"); // TODO: this doesnt make sense as a CLI make only sense if the bitcoind is
-    let port = testenv::get_available_port().expect("port");
-    spawn_wallet_grpc_service(port, WalletServiceImpl::create_with_rpc_params(testenv.bitcoin_core_rpc_client().unwrap()));
+    let (port, listener) = TestEnv::get_bound_port().await.expect("listener");
+    spawn_wallet_grpc_service(
+        listener,
+        WalletServiceImpl::create_with_rpc_params(testenv.bitcoin_core_rpc_client().unwrap()),
+    );
 
     task::spawn_blocking(move || assert_cli_with_port(port, ["new-address"]))
         .await.unwrap()
@@ -135,8 +142,8 @@ async fn test_cli_list_unspent() {
         .some_call(matching!()).returns(vec![mock_utxo()]);
     let mock_wallet_service = Unimock::new(clause).no_verify_in_drop();
 
-    let port = testenv::get_available_port().expect("port");
-    spawn_wallet_grpc_service(port, mock_wallet_service);
+    let (port, listener) = TestEnv::get_bound_port().await.expect("listener");
+    spawn_wallet_grpc_service(listener, mock_wallet_service);
 
     task::spawn_blocking(move || assert_cli_with_port(port, ["list-unspent"]))
         .await.unwrap()
@@ -153,8 +160,8 @@ async fn test_cli_notify_confidence() {
         .answers(&|_, _| mock_confidence_stream());
     let mock_wallet_service = Unimock::new(clause).no_verify_in_drop();
 
-    let port = testenv::get_available_port().expect("port");
-    spawn_wallet_grpc_service(port, mock_wallet_service);
+    let (port, listener) = TestEnv::get_bound_port().await.expect("listener");
+    spawn_wallet_grpc_service(listener, mock_wallet_service);
 
     task::spawn_blocking(move || assert_cli_with_port(port, ["notify-confidence",
         "37b560334094515cfdaa0146bfd4ce19e940064c505082031858b0aba3218990"]))
@@ -229,10 +236,12 @@ fn assert_cli_with_port<'a>(port: u16, args: impl IntoIterator<Item=&'a str>) ->
     assert_cli(args)
 }
 
-fn spawn_wallet_grpc_service(port: u16, wallet_service: impl WalletService + Send + Sync + 'static)
-                             -> JoinHandle<Result<(), transport::Error>> {
+fn spawn_wallet_grpc_service(
+    listener: TcpListener,
+    wallet_service: impl WalletService + Send + Sync + 'static,
+) -> JoinHandle<Result<(), transport::Error>> {
     let wallet = WalletImpl { wallet_service: Arc::new(wallet_service) };
-    let incoming = TcpIncoming::bind(format!("127.0.0.1:{port}").parse().unwrap()).unwrap();
+    let incoming = TcpIncoming::from(listener);
 
     task::spawn(async move {
         Server::builder()

rpc/tests/cli.rs

@@ -15,6 +15,7 @@ secp = { workspace = true }
 simple-semaphore = { workspace = true }
 tempfile = { workspace = true }
 bmp_tracing = { workspace = true }
+tokio = { workspace = true }
 
 electrsd = { version = "0.36.1", features = [
 "esplora_a33e97e1",

testenv/Cargo.toml

@@ -14,7 +14,7 @@ use bdk_wallet::bitcoin::key::Secp256k1;
 use bdk_wallet::bitcoin::secp256k1::All;
 use bdk_wallet::bitcoin::{Address, Amount, BlockHash, Network, Transaction, Txid};
 use bmp_tracing::tracing;
-pub use corepc_node::get_available_port;
+use tokio::net::TcpListener;
 use electrsd::corepc_node::Node;
 use electrsd::electrum_client::{Client, ElectrumApi};
 use electrsd::{ElectrsD, corepc_node};
@@ -235,7 +235,8 @@ impl TestEnv {
     pub fn start_explorer_in_container(&mut self) -> Result<()> {
         // this start a container for debugging
         let bitcoind_rpc_port = self.bitcoin_rpc_port();
-        let browser_port = get_available_port()?;
+        let listener = std::net::TcpListener::bind("127.0.0.1:0")?;
+        let browser_port = listener.local_addr()?.port();
 
         let electrum_port = self.electrsd.electrum_url.split(':').next_back()
                 .context("Failed to parse electrum port")?;
@@ -267,6 +268,9 @@ impl TestEnv {
         self.explorer_process = Some(child);
         self.container_name = Some(container_name);
 
+        // Drop the listener to free the port for the container
+        drop(listener);
+
         tracing::info!(
             "Starting explorer in container, access it at http://127.0.0.1:{browser_port}/blocks"
         );
@@ -458,6 +462,14 @@ impl TestEnv {
     pub fn p2p_socket_addr(&self) -> Option<SocketAddrV4> {
         self.bitcoind.params.p2p_socket
     }
+
+    /// Returns a TcpListener bound to an available port (port 0 lets OS assign).
+    /// This avoids race conditions by keeping the port bound until used.
+    pub async fn get_bound_port() -> Result<(u16, TcpListener)> {
+        let listener = TcpListener::bind("127.0.0.1:0").await?;
+        let port = listener.local_addr().unwrap().port();
+        Ok((port, listener))
+    }
 }
 
 impl Drop for TestEnv {