test(node): improve unit test coverage for node module (#2260)

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: sanity

crates/core/src/node/mod.rs

@@ -1591,6 +1591,7 @@ mod tests {
     use std::net::{Ipv4Addr, Ipv6Addr};
 
     use super::*;
+    use crate::operations::OpError;
 
     #[tokio::test]
     async fn test_hostname_resolution() {
@@ -1612,4 +1613,160 @@ mod tests {
         let socket_addr = NodeConfig::parse_socket_addr(&addr).await.unwrap();
         assert_eq!(socket_addr.port(), 8080);
     }
+
+    #[tokio::test]
+    async fn test_hostname_resolution_with_trailing_dot() {
+        // DNS names with trailing dot should be handled
+        let addr = Address::Hostname("localhost.".to_string());
+        let result = NodeConfig::parse_socket_addr(&addr).await;
+        // This should either succeed or fail gracefully
+        if let Ok(socket_addr) = result {
+            assert!(
+                socket_addr.ip() == IpAddr::V4(Ipv4Addr::LOCALHOST)
+                    || socket_addr.ip() == IpAddr::V6(Ipv6Addr::LOCALHOST)
+            );
+        }
+    }
+
+    #[tokio::test]
+    async fn test_hostname_resolution_direct_socket_addr() {
+        let socket = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1)), 8080);
+        let addr = Address::HostAddress(socket);
+        let resolved = NodeConfig::parse_socket_addr(&addr).await.unwrap();
+        assert_eq!(resolved, socket);
+    }
+
+    #[tokio::test]
+    async fn test_hostname_resolution_invalid_port() {
+        let addr = Address::Hostname("localhost:not_a_port".to_string());
+        let result = NodeConfig::parse_socket_addr(&addr).await;
+        assert!(result.is_err());
+    }
+
+    // PeerId tests
+    #[test]
+    fn test_peer_id_equality_based_on_addr() {
+        let keypair1 = TransportKeypair::new();
+        let keypair2 = TransportKeypair::new();
+        let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080);
+
+        let peer1 = PeerId::new(addr, keypair1.public().clone());
+        let peer2 = PeerId::new(addr, keypair2.public().clone());
+
+        // PeerId equality is based on address, not public key
+        assert_eq!(peer1, peer2);
+    }
+
+    #[test]
+    fn test_peer_id_inequality_different_addr() {
+        let keypair = TransportKeypair::new();
+        let addr1 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080);
+        let addr2 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8081);
+
+        let peer1 = PeerId::new(addr1, keypair.public().clone());
+        let peer2 = PeerId::new(addr2, keypair.public().clone());
+
+        assert_ne!(peer1, peer2);
+    }
+
+    #[test]
+    fn test_peer_id_ordering() {
+        let keypair = TransportKeypair::new();
+        let addr1 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080);
+        let addr2 = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8081);
+
+        let peer1 = PeerId::new(addr1, keypair.public().clone());
+        let peer2 = PeerId::new(addr2, keypair.public().clone());
+
+        // Ordering should be consistent
+        assert!(peer1 < peer2);
+        assert!(peer2 > peer1);
+    }
+
+    #[test]
+    fn test_peer_id_hash_consistency() {
+        use std::collections::hash_map::DefaultHasher;
+        use std::hash::{Hash, Hasher};
+
+        let keypair1 = TransportKeypair::new();
+        let keypair2 = TransportKeypair::new();
+        let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080);
+
+        let peer1 = PeerId::new(addr, keypair1.public().clone());
+        let peer2 = PeerId::new(addr, keypair2.public().clone());
+
+        let mut hasher1 = DefaultHasher::new();
+        let mut hasher2 = DefaultHasher::new();
+        peer1.hash(&mut hasher1);
+        peer2.hash(&mut hasher2);
+
+        // Same address should produce same hash
+        assert_eq!(hasher1.finish(), hasher2.finish());
+    }
+
+    #[test]
+    fn test_peer_id_random_produces_unique() {
+        let peer1 = PeerId::random();
+        let peer2 = PeerId::random();
+
+        // Random peers should have different addresses (with high probability)
+        assert_ne!(peer1.addr, peer2.addr);
+    }
+
+    #[test]
+    fn test_peer_id_serialization() {
+        let peer = PeerId::random();
+        let bytes = peer.clone().to_bytes();
+        assert!(!bytes.is_empty());
+
+        // Should be deserializable
+        let deserialized: PeerId = bincode::deserialize(&bytes).unwrap();
+        assert_eq!(peer.addr, deserialized.addr);
+    }
+
+    #[test]
+    fn test_peer_id_display() {
+        let peer = PeerId::random();
+        let display = format!("{}", peer);
+        let debug = format!("{:?}", peer);
+
+        // Display and Debug should produce the same output
+        assert_eq!(display, debug);
+        // Should not be empty
+        assert!(!display.is_empty());
+    }
+
+    // InitPeerNode tests
+    #[test]
+    fn test_init_peer_node_construction() {
+        let keypair = TransportKeypair::new();
+        let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1)), 8080);
+        let peer_key_location = PeerKeyLocation::new(keypair.public().clone(), addr);
+        let location = Location::new(0.5);
+
+        let init_peer = InitPeerNode::new(peer_key_location.clone(), location);
+
+        assert_eq!(init_peer.peer_key_location, peer_key_location);
+        assert_eq!(init_peer.location, location);
+    }
+
+    // is_operation_completed tests
+    #[test]
+    fn test_is_operation_completed_with_none() {
+        let result: Result<Option<OpEnum>, OpError> = Ok(None);
+        assert!(!is_operation_completed(&result));
+    }
+
+    #[test]
+    fn test_is_operation_completed_with_error() {
+        let result: Result<Option<OpEnum>, OpError> =
+            Err(OpError::OpNotAvailable(super::OpNotAvailable::Running));
+        assert!(!is_operation_completed(&result));
+    }
+
+    #[test]
+    fn test_is_operation_completed_with_state_pushed_error() {
+        let result: Result<Option<OpEnum>, OpError> = Err(OpError::StatePushed);
+        assert!(!is_operation_completed(&result));
+    }
 }

crates/core/src/node/network_bridge.rs

@@ -283,4 +283,156 @@ mod tests {
 
         assert!(result.is_err(), "Send should fail when receiver is dropped");
     }
+
+    // Note: Tests that require NetMessage creation are omitted because
+    // constructing valid NetMessage instances requires complex setup with
+    // cryptographic keys, proper state management, and specific operation states.
+    // The core channel functionality is already well-tested by the NodeEvent tests above.
+
+    /// Test channel capacity doesn't block under normal load
+    #[tokio::test]
+    async fn test_channel_capacity() {
+        let (notification_channel, notification_tx) = event_loop_notification_channel();
+        let mut rx = notification_channel.notifications_receiver;
+
+        // Send multiple messages without reading
+        for _ in 0..50 {
+            let test_event = crate::message::NodeEvent::Disconnect { cause: None };
+            notification_tx
+                .notifications_sender()
+                .send(Either::Right(test_event))
+                .await
+                .expect("Should not block with capacity of 100");
+        }
+
+        // Now read them all
+        let mut count = 0;
+        while count < 50 {
+            match timeout(Duration::from_millis(10), rx.recv()).await {
+                Ok(Some(_)) => count += 1,
+                _ => break,
+            }
+        }
+
+        assert_eq!(count, 50, "Should receive all 50 messages");
+    }
+
+    /// Test EventLoopNotificationsSender clone works correctly
+    #[tokio::test]
+    async fn test_sender_clone() {
+        let (notification_channel, notification_tx) = event_loop_notification_channel();
+        let mut rx = notification_channel.notifications_receiver;
+
+        // Clone the sender
+        let cloned_tx = notification_tx.clone();
+
+        // Send from original
+        let test_event1 = crate::message::NodeEvent::Disconnect { cause: None };
+        notification_tx
+            .notifications_sender()
+            .send(Either::Right(test_event1))
+            .await
+            .expect("Should send from original");
+
+        // Send from clone
+        let test_event2 = crate::message::NodeEvent::Disconnect {
+            cause: Some("cloned".into()),
+        };
+        cloned_tx
+            .notifications_sender()
+            .send(Either::Right(test_event2))
+            .await
+            .expect("Should send from clone");
+
+        // Both should be received
+        let mut received = 0;
+        for _ in 0..2 {
+            if timeout(Duration::from_millis(100), rx.recv()).await.is_ok() {
+                received += 1;
+            }
+        }
+        assert_eq!(received, 2, "Should receive both messages");
+    }
+}
+
+// ConnectionError tests
+#[cfg(test)]
+mod connection_error_tests {
+    use super::*;
+
+    #[test]
+    fn test_connection_error_clone() {
+        let errors = vec![
+            ConnectionError::LocationUnknown,
+            ConnectionError::SendNotCompleted("127.0.0.1:8080".parse().unwrap()),
+            ConnectionError::UnexpectedReq,
+            ConnectionError::Serialization(None),
+            ConnectionError::TransportError("test error".to_string()),
+            ConnectionError::FailedConnectOp,
+            ConnectionError::UnwantedConnection,
+            ConnectionError::AddressBlocked("127.0.0.1:8080".parse().unwrap()),
+            ConnectionError::IOError("io error".to_string()),
+            ConnectionError::Timeout,
+        ];
+
+        for error in errors {
+            let cloned = error.clone();
+            // Verify clone produces equivalent error
+            assert_eq!(format!("{}", error), format!("{}", cloned));
+        }
+    }
+
+    #[test]
+    fn test_connection_error_from_io_error() {
+        let io_error = std::io::Error::other("test io error");
+        let conn_error: ConnectionError = io_error.into();
+
+        match conn_error {
+            ConnectionError::IOError(msg) => {
+                assert!(msg.contains("test io error"));
+            }
+            _ => panic!("Expected IOError variant"),
+        }
+    }
+
+    #[test]
+    fn test_connection_error_display() {
+        let error = ConnectionError::LocationUnknown;
+        let display = format!("{}", error);
+        assert!(!display.is_empty());
+        assert!(display.contains("location unknown"));
+
+        let error = ConnectionError::SendNotCompleted("127.0.0.1:8080".parse().unwrap());
+        let display = format!("{}", error);
+        assert!(display.contains("127.0.0.1:8080"));
+
+        let error = ConnectionError::AddressBlocked("192.168.1.1:9000".parse().unwrap());
+        let display = format!("{}", error);
+        assert!(display.contains("192.168.1.1:9000"));
+
+        let error = ConnectionError::Timeout;
+        let display = format!("{}", error);
+        assert!(display.contains("timeout"));
+    }
+
+    #[test]
+    fn test_serialization_error_clone_loses_inner() {
+        // When cloning a Serialization error, the inner error is lost
+        let inner = Box::new(bincode::ErrorKind::SizeLimit);
+        let original = ConnectionError::Serialization(Some(inner));
+        let cloned = original.clone();
+
+        match cloned {
+            ConnectionError::Serialization(None) => {} // Expected - inner is lost
+            _ => panic!("Expected Serialization(None) after clone"),
+        }
+    }
+
+    #[test]
+    fn test_connection_error_debug() {
+        let error = ConnectionError::FailedConnectOp;
+        let debug = format!("{:?}", error);
+        assert!(!debug.is_empty());
+        assert!(debug.contains("FailedConnectOp"));
+    }
 }