From 7cae62fa74f5cb58ddc2de993b40a5056ab3d4f8 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Mon, 31 Aug 2026 19:40:20 +0800 Subject: [PATCH 01/10] fix: propagate node domain and key-expression format to built-in services Fixes circle/hiroz#143 (D2). TypeDescriptionService::new and ParameterService::new each built their own NodeEntity with domain_id hardcoded to 0 and the default KeyExprFormat, instead of the owning node's actual domain and format. A node on a non-zero ROS_DOMAIN_ID had its built-in services (get_type_description, the six parameter services) always announce domain 0. --- .../src/dynamic/type_description_service.rs | 49 +++++++--- crates/hiroz/src/node.rs | 9 +- crates/hiroz/src/parameter/service.rs | 22 +++-- crates/hiroz/tests/graph.rs | 97 +++++++++++++++++++ 4 files changed, 151 insertions(+), 26 deletions(-) diff --git a/crates/hiroz/src/dynamic/type_description_service.rs b/crates/hiroz/src/dynamic/type_description_service.rs index f863c09b3..4de8a6fb2 100644 --- a/crates/hiroz/src/dynamic/type_description_service.rs +++ b/crates/hiroz/src/dynamic/type_description_service.rs @@ -435,7 +435,11 @@ pub struct TypeDescriptionService { } impl TypeDescriptionService { - /// Create a new TypeDescriptionService. + /// Create a domain-0 TypeDescriptionService using the default key-expression format. + /// + /// Nodes created by [`ZNodeBuilder`](crate::node::ZNodeBuilder) inherit their context via + /// [`Self::new_with_node`]. Use that constructor when creating this service manually for a + /// non-default domain or key-expression format. /// /// # Arguments /// @@ -455,6 +459,34 @@ impl TypeDescriptionService { node_id: usize, counter: &crate::context::GlobalCounter, clock: &crate::time::ZClock, + ) -> ZResult { + let node = crate::entity::NodeEntity::new( + 0, + session.zid(), + node_id, + node_name.to_string(), + namespace.to_string(), + String::new(), + ); + Self::new_with_node( + session, + node, + counter, + clock, + hiroz_protocol::KeyExprFormat::default(), + ) + } + + /// Create a TypeDescriptionService from an existing node identity and key-expression format. + /// + /// `node.z_id` must identify `session`. Passing the owning node's entity avoids silently + /// resetting its domain, namespace, enclave, or other discovery identity. + pub fn new_with_node( + session: Arc, + node: crate::entity::NodeEntity, + counter: &crate::context::GlobalCounter, + clock: &crate::time::ZClock, + keyexpr_format: hiroz_protocol::KeyExprFormat, ) -> ZResult { let schemas: Arc>> = Arc::new(RwLock::new(HashMap::new())); @@ -464,19 +496,12 @@ impl TypeDescriptionService { // which expands to /{namespace}/{node_name}/get_type_description let service_name = "~get_type_description"; - // Create the node entity for the service - let node_entity = crate::entity::NodeEntity::new( - 0, // domain_id - session.zid(), - node_id, - node_name.to_string(), - namespace.to_string(), - String::new(), // enclave (empty, normalized to "%" in liveliness token) - ); + let node_name = node.name.clone(); + let namespace = node.namespace.clone(); let entity = crate::entity::EndpointEntity { id: counter.increment(), - node: Some(node_entity), + node: Some(node), kind: crate::entity::EndpointKind::Service, topic: service_name.to_string(), type_info: Some(GetTypeDescription::service_type_info()), @@ -488,7 +513,7 @@ impl TypeDescriptionService { entity, session, clock: clock.clone(), - keyexpr_format: hiroz_protocol::KeyExprFormat::default(), + keyexpr_format, _phantom_data: Default::default(), }; diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index e640a2191..0f0fd86c9 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -241,13 +241,12 @@ impl Builder for ZNodeBuilder { // Create type description service if enabled let type_desc_service = if self.enable_type_desc_service { debug!("[NOD] Creating type description service"); - let service = TypeDescriptionService::new( + let service = TypeDescriptionService::new_with_node( self.session.clone(), - &self.name, - &self.namespace, - id, + node.clone(), &self.counter, &self.clock, + self.keyexpr_format.clone(), )?; info!("[NOD] TypeDescriptionService created (callback mode)"); @@ -263,6 +262,8 @@ impl Builder for ZNodeBuilder { let service = ParameterService::new(ParameterServiceConfig { session: self.session.clone(), graph: self.graph.clone(), + domain_id: self.domain_id, + keyexpr_format: self.keyexpr_format.clone(), node_name: &self.name, namespace: &self.namespace, node_id: id, diff --git a/crates/hiroz/src/parameter/service.rs b/crates/hiroz/src/parameter/service.rs index 15db88d81..8257845f4 100644 --- a/crates/hiroz/src/parameter/service.rs +++ b/crates/hiroz/src/parameter/service.rs @@ -40,6 +40,8 @@ type BoxedServer = Arc; pub(crate) struct ParameterServiceConfig<'a> { pub session: Arc, pub graph: Arc, + pub domain_id: usize, + pub keyexpr_format: hiroz_protocol::KeyExprFormat, pub node_name: &'a str, pub namespace: &'a str, pub node_id: usize, @@ -284,6 +286,8 @@ impl ParameterService { let ParameterServiceConfig { session, graph, + domain_id, + keyexpr_format, node_name, namespace, node_id, @@ -297,7 +301,7 @@ impl ParameterService { wire_types::register_parameter_schemas(tds); } let node_entity = NodeEntity::new( - 0, + domain_id, session.zid(), node_id, node_name.to_string(), @@ -323,8 +327,6 @@ impl ParameterService { qos: Default::default(), }; - let ke_format = hiroz_protocol::KeyExprFormat::default(); - // ── /parameter_events publisher ─────────────────────────────────────── let pub_entity = EndpointEntity { id: counter.increment(), @@ -351,7 +353,7 @@ impl ParameterService { clock: clock.clone(), with_attachment: true, shm_config: None, - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), dyn_schema: None, encoding: None, _phantom_data: Default::default(), @@ -380,7 +382,7 @@ impl ParameterService { entity, session: session.clone(), clock: clock.clone(), - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), _phantom_data: Default::default(), }; builder.build_with_callback(move |query| { @@ -400,7 +402,7 @@ impl ParameterService { entity, session: session.clone(), clock: clock.clone(), - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), _phantom_data: Default::default(), }; builder.build_with_callback(move |query| { @@ -420,7 +422,7 @@ impl ParameterService { entity, session: session.clone(), clock: clock.clone(), - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), _phantom_data: Default::default(), }; builder.build_with_callback(move |query| { @@ -440,7 +442,7 @@ impl ParameterService { entity, session: session.clone(), clock: clock.clone(), - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), _phantom_data: Default::default(), }; builder.build_with_callback(move |query| { @@ -460,7 +462,7 @@ impl ParameterService { entity, session: session.clone(), clock: clock.clone(), - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), _phantom_data: Default::default(), }; builder.build_with_callback(move |query| { @@ -480,7 +482,7 @@ impl ParameterService { entity, session: session.clone(), clock: clock.clone(), - keyexpr_format: ke_format.clone(), + keyexpr_format: keyexpr_format.clone(), _phantom_data: Default::default(), }; builder.build_with_callback(move |query| { diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index 518cb33ac..533f333eb 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -15,6 +15,38 @@ use hiroz::{ entity::{EndpointKind, NodeKey}, }; use hiroz_msgs::{example_interfaces::srv::AddTwoInts, std_msgs::String as RosString}; +use zenoh::{Wait, config::WhatAmI}; + +struct DomainTestRouter { + endpoint: String, + _session: zenoh::Session, +} + +impl DomainTestRouter { + fn new() -> Self { + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind test port"); + let port = listener.local_addr().expect("test address").port(); + drop(listener); + + let endpoint = format!("tcp/127.0.0.1:{port}"); + let mut config = zenoh::Config::default(); + config.set_mode(Some(WhatAmI::Router)).unwrap(); + config + .insert_json5("listen/endpoints", &format!("[\"{endpoint}\"]")) + .unwrap(); + config + .insert_json5("scouting/multicast/enabled", "false") + .unwrap(); + let session = zenoh::open(config).wait().expect("open test router"); + std::thread::sleep(Duration::from_millis(300)); + + Self { + endpoint, + _session: session, + } + } +} + /// Helper to create a test context and node async fn setup_test_node( node_name: &str, @@ -74,6 +106,71 @@ async fn wait_for_subscribers( mod tests { use super::*; + /// Built-in services (type description + the six parameter services) must + /// announce the node's actual domain, not a hardcoded domain 0. + #[tokio::test(flavor = "multi_thread")] + async fn built_in_services_inherit_context_domain() -> Result<()> { + const DOMAIN_ID: usize = 123; + let router = DomainTestRouter::new(); + let observer_ctx = ZContextBuilder::default() + .with_domain_id(DOMAIN_ID) + .disable_multicast_scouting() + .with_connect_endpoints([router.endpoint.as_str()]) + .with_mode("client") + .build()?; + let observer = observer_ctx + .create_node("domain_123_observer") + .without_parameters() + .build()?; + let producer_ctx = ZContextBuilder::default() + .with_domain_id(DOMAIN_ID) + .disable_multicast_scouting() + .with_connect_endpoints([router.endpoint.as_str()]) + .with_mode("client") + .build()?; + let _producer = producer_ctx + .create_node("domain_123_builtins") + .with_type_description_service() + .build()?; + let node_key: NodeKey = (String::new(), "domain_123_builtins".to_string()); + + let deadline = tokio::time::Instant::now() + Duration::from_secs(2); + let services = loop { + let services = observer + .graph() + .get_entities_by_node(EndpointKind::Service, node_key.clone()); + if services.len() >= 7 || tokio::time::Instant::now() >= deadline { + break services; + } + tokio::time::sleep(Duration::from_millis(10)).await; + }; + + assert_eq!( + services.len(), + 7, + "six parameter services and get_type_description must be discoverable" + ); + assert!(services.iter().all(|endpoint| { + endpoint + .node + .as_ref() + .is_some_and(|owner| owner.domain_id == DOMAIN_ID) + })); + + let parameter_events = observer + .graph() + .get_entities_by_node(EndpointKind::Publisher, node_key); + assert!(parameter_events.iter().any(|endpoint| { + endpoint.topic == "/parameter_events" + && endpoint + .node + .as_ref() + .is_some_and(|owner| owner.domain_id == DOMAIN_ID) + })); + + Ok(()) + } + /// Tests getting topic names and types from the graph #[tokio::test(flavor = "multi_thread")] async fn test_get_topic_names_and_types() -> Result<()> { From 169f5cd9c0043f2290504379dd0e0c7f66cfd5d8 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 17:37:37 +0800 Subject: [PATCH 02/10] fix: propagate node enclave to built-in parameter services Crane review of circle/hiroz#150: ParameterService::new still hardcoded enclave to "" while this PR threaded domain_id and keyexpr_format through ParameterServiceConfig for exactly this reason -- its sibling fix, TypeDescriptionService::new_with_node, already documents avoiding silently resetting a node's domain, namespace, or enclave. A node built with a non-default enclave had it correctly on its own liveliness token and get_type_description, but all six parameter services and /parameter_events still reported enclave "". --- crates/hiroz/src/node.rs | 1 + crates/hiroz/src/parameter/service.rs | 4 +++- crates/hiroz/tests/graph.rs | 8 +++++--- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 0f0fd86c9..86fe8f5e3 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -266,6 +266,7 @@ impl Builder for ZNodeBuilder { keyexpr_format: self.keyexpr_format.clone(), node_name: &self.name, namespace: &self.namespace, + enclave: &node.enclave, node_id: id, counter: &self.counter, clock: &self.clock, diff --git a/crates/hiroz/src/parameter/service.rs b/crates/hiroz/src/parameter/service.rs index 8257845f4..23cf67edf 100644 --- a/crates/hiroz/src/parameter/service.rs +++ b/crates/hiroz/src/parameter/service.rs @@ -44,6 +44,7 @@ pub(crate) struct ParameterServiceConfig<'a> { pub keyexpr_format: hiroz_protocol::KeyExprFormat, pub node_name: &'a str, pub namespace: &'a str, + pub enclave: &'a str, pub node_id: usize, pub counter: &'a GlobalCounter, pub clock: &'a crate::time::ZClock, @@ -290,6 +291,7 @@ impl ParameterService { keyexpr_format, node_name, namespace, + enclave, node_id, counter, clock, @@ -306,7 +308,7 @@ impl ParameterService { node_id, node_name.to_string(), namespace.to_string(), - String::new(), + enclave.to_string(), ); // Compute node fully-qualified name for parameter events diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index 533f333eb..fe0d361ec 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -107,10 +107,11 @@ mod tests { use super::*; /// Built-in services (type description + the six parameter services) must - /// announce the node's actual domain, not a hardcoded domain 0. + /// announce the node's actual domain and enclave, not hardcoded defaults. #[tokio::test(flavor = "multi_thread")] async fn built_in_services_inherit_context_domain() -> Result<()> { const DOMAIN_ID: usize = 123; + const ENCLAVE: &str = "/sros2/enclave"; let router = DomainTestRouter::new(); let observer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) @@ -124,6 +125,7 @@ mod tests { .build()?; let producer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) + .with_enclave(ENCLAVE) .disable_multicast_scouting() .with_connect_endpoints([router.endpoint.as_str()]) .with_mode("client") @@ -154,7 +156,7 @@ mod tests { endpoint .node .as_ref() - .is_some_and(|owner| owner.domain_id == DOMAIN_ID) + .is_some_and(|owner| owner.domain_id == DOMAIN_ID && owner.enclave == ENCLAVE) })); let parameter_events = observer @@ -165,7 +167,7 @@ mod tests { && endpoint .node .as_ref() - .is_some_and(|owner| owner.domain_id == DOMAIN_ID) + .is_some_and(|owner| owner.domain_id == DOMAIN_ID && owner.enclave == ENCLAVE) })); Ok(()) From 782cc3d0741edfdb2979cf89460a08ae8c4a405f Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 17:40:20 +0800 Subject: [PATCH 03/10] fix: use a single-segment enclave value in the domain/enclave test The previous run's whippet job showed the real defect: the chosen enclave "/sros2/enclave" contains an internal slash, and the liveliness key expression places enclave as exactly one slash-delimited field (format/rmw_zenoh.rs) with no escaping for that -- a separate, pre-existing wire-format limitation this PR does not touch. Use a single-segment value instead. --- crates/hiroz/tests/graph.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index fe0d361ec..23a7dea86 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -111,7 +111,13 @@ mod tests { #[tokio::test(flavor = "multi_thread")] async fn built_in_services_inherit_context_domain() -> Result<()> { const DOMAIN_ID: usize = 123; - const ENCLAVE: &str = "/sros2/enclave"; + // A single path segment: the liveliness key expression places the + // enclave as exactly one slash-delimited field + // (`@ros2_lv//...////...`, + // format/rmw_zenoh.rs), with no escaping for an internal `/` -- + // that's a separate, pre-existing wire-format limitation, not + // something this fix changes. + const ENCLAVE: &str = "/test_enclave"; let router = DomainTestRouter::new(); let observer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) From d2a1f28a7f41e9ccd77351c954c7e1b6942d47ec Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 17:44:44 +0800 Subject: [PATCH 04/10] fix: stop asserting enclave in the domain-propagation test format/rmw_zenoh.rs's liveliness encoder hardcodes the enclave segment to the empty placeholder for every entity type -- the decoder's own comment says "Enclave (not supported yet)". No entity's enclave round-trips today, not even a plain ZNode's own liveliness token, so the earlier commit's assertion was testing something the wire format cannot carry regardless of construction site. That's a separate, pre-existing gap, not something this PR touches. The production fix (threading the node's enclave into ParameterServiceConfig instead of hardcoding "") stays: it's still correct and consistent with TypeDescriptionService, and will matter the day the encoder is fixed. --- crates/hiroz/tests/graph.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index 23a7dea86..515ef33a6 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -107,17 +107,19 @@ mod tests { use super::*; /// Built-in services (type description + the six parameter services) must - /// announce the node's actual domain and enclave, not hardcoded defaults. + /// announce the node's actual domain, not a hardcoded domain 0. + /// + /// Does not assert `enclave`: `format/rmw_zenoh.rs`'s liveliness + /// encoder hardcodes the enclave segment to the empty placeholder for + /// every entity type (`// Enclave (not supported yet)` on the decode + /// side), so no entity's enclave round-trips today -- not the node's + /// own token, not a plain `ZNode`'s, not these. That is a separate, + /// pre-existing wire-format gap this PR does not touch. This test only + /// covers what production `ParameterService`/`TypeDescriptionService` + /// construction actually controls: domain_id. #[tokio::test(flavor = "multi_thread")] async fn built_in_services_inherit_context_domain() -> Result<()> { const DOMAIN_ID: usize = 123; - // A single path segment: the liveliness key expression places the - // enclave as exactly one slash-delimited field - // (`@ros2_lv//...////...`, - // format/rmw_zenoh.rs), with no escaping for an internal `/` -- - // that's a separate, pre-existing wire-format limitation, not - // something this fix changes. - const ENCLAVE: &str = "/test_enclave"; let router = DomainTestRouter::new(); let observer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) @@ -131,7 +133,6 @@ mod tests { .build()?; let producer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) - .with_enclave(ENCLAVE) .disable_multicast_scouting() .with_connect_endpoints([router.endpoint.as_str()]) .with_mode("client") @@ -162,7 +163,7 @@ mod tests { endpoint .node .as_ref() - .is_some_and(|owner| owner.domain_id == DOMAIN_ID && owner.enclave == ENCLAVE) + .is_some_and(|owner| owner.domain_id == DOMAIN_ID) })); let parameter_events = observer @@ -173,7 +174,7 @@ mod tests { && endpoint .node .as_ref() - .is_some_and(|owner| owner.domain_id == DOMAIN_ID && owner.enclave == ENCLAVE) + .is_some_and(|owner| owner.domain_id == DOMAIN_ID) })); Ok(()) From fd873fca3bc5ee711d7715dc7fcaf60fc694849f Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 17:58:35 +0800 Subject: [PATCH 05/10] feat: read ROS_DOMAIN_ID as the default domain Fixes circle/hiroz#151. ZContextBuilder derived its domain_id default from usize::default(), always 0 regardless of environment -- ROS_DOMAIN_ID was never read anywhere in the crate. rclcpp/rclpy read it at init time and use it unless the caller overrides it in code; a plain hiroz ZContext gave the env var no effect at all, so the normal ROS 2 deployment story (set the variable, don't touch source) silently did nothing. default_domain_id() now reads ROS_DOMAIN_ID, falling back to 0 when unset or unparseable. .with_domain_id() called after default() still overrides it, same precedence as every other ROS 2 client library. --- crates/hiroz/src/context.rs | 92 ++++++++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-) diff --git a/crates/hiroz/src/context.rs b/crates/hiroz/src/context.rs index ba6d02f22..3d189f813 100644 --- a/crates/hiroz/src/context.rs +++ b/crates/hiroz/src/context.rs @@ -71,7 +71,6 @@ impl RemapRules { } } -#[derive(Default)] pub struct ZContextBuilder { domain_id: usize, namespace: String, @@ -86,6 +85,45 @@ pub struct ZContextBuilder { clock: Option, } +impl Default for ZContextBuilder { + fn default() -> Self { + Self { + domain_id: default_domain_id(), + namespace: String::default(), + enclave: String::default(), + zenoh_config: None, + config_file: None, + config_overrides: Vec::default(), + remap_rules: RemapRules::default(), + enable_logging: bool::default(), + shm_config: None, + keyexpr_format: hiroz_protocol::KeyExprFormat::default(), + clock: None, + } + } +} + +/// Domain ID to use when the builder is not given one explicitly. +/// +/// Matches `rclcpp`/`rclpy`: read `ROS_DOMAIN_ID` from the environment, so +/// the normal ROS 2 deployment story (set the env var, don't touch source) +/// works here too. `.with_domain_id()` called after `default()` still +/// overrides this, same precedence as every other ROS 2 client library. +fn default_domain_id() -> usize { + match std::env::var("ROS_DOMAIN_ID") { + Ok(val) => match val.parse::() { + Ok(id) => id, + Err(_) => { + warn!( + "[CTX] ROS_DOMAIN_ID={val:?} is not a valid non-negative integer, using domain 0" + ); + 0 + } + }, + Err(_) => 0, + } +} + impl ZContextBuilder { /// Set the ROS domain ID pub fn with_domain_id(mut self, domain_id: usize) -> Self { @@ -672,3 +710,55 @@ impl ZContext { &self.clock } } + +#[cfg(test)] +mod tests { + use super::*; + use serial_test::serial; + + #[test] + #[serial] + fn default_domain_id_falls_back_to_zero_when_unset() { + unsafe { + std::env::remove_var("ROS_DOMAIN_ID"); + } + assert_eq!(default_domain_id(), 0); + } + + #[test] + #[serial] + fn default_domain_id_reads_ros_domain_id() { + unsafe { + std::env::set_var("ROS_DOMAIN_ID", "42"); + } + assert_eq!(default_domain_id(), 42); + unsafe { + std::env::remove_var("ROS_DOMAIN_ID"); + } + } + + #[test] + #[serial] + fn default_domain_id_falls_back_to_zero_on_invalid_value() { + unsafe { + std::env::set_var("ROS_DOMAIN_ID", "not-a-number"); + } + assert_eq!(default_domain_id(), 0); + unsafe { + std::env::remove_var("ROS_DOMAIN_ID"); + } + } + + #[test] + #[serial] + fn with_domain_id_overrides_the_env_default() { + unsafe { + std::env::set_var("ROS_DOMAIN_ID", "42"); + } + let builder = ZContextBuilder::default().with_domain_id(7); + assert_eq!(builder.domain_id, 7); + unsafe { + std::env::remove_var("ROS_DOMAIN_ID"); + } + } +} From da6e368d89dd057d86afc24f3d2285a9a7adff07 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 18:12:35 +0800 Subject: [PATCH 06/10] test: cover keyexpr_format propagation to built-in services Addresses the non-blocking Crane finding on circle/hiroz#150: the domain-propagation test exercised domain_id but not keyexpr_format, even though ParameterServiceConfig/TypeDescriptionService::new_with_node thread both through identically. A regression that silently re-hardcoded KeyExprFormat::default() for either service would have passed unnoticed. Uses a pass-through custom KeyExprFormatter with its own admin space, so the built-in services are only discoverable at all if they actually used the context's format. --- crates/hiroz/tests/graph.rs | 126 +++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index 515ef33a6..c19f2a9e1 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -7,7 +7,7 @@ //! - Node discovery and information //! - Service availability checking -use std::time::Duration; +use std::{sync::Arc, time::Duration}; use hiroz::{ Builder, Result, @@ -15,8 +15,70 @@ use hiroz::{ entity::{EndpointKind, NodeKey}, }; use hiroz_msgs::{example_interfaces::srv::AddTwoInts, std_msgs::String as RosString}; +use hiroz_protocol::{ + EndpointEntity, Entity, KeyExprFormat, KeyExprFormatter, KeyExprFormatterAdapter, NodeEntity, + RmwZenohFormatter, + entity::{LivelinessKE, TopicKE}, + qos::QosProfile, +}; use zenoh::{Wait, config::WhatAmI}; +/// A pass-through `KeyExprFormatter` that delegates to `RmwZenohFormatter` for +/// everything except its own admin space, so a node using it is +/// distinguishable on the wire from a plain rmw_zenoh node without +/// reimplementing any encoding. +#[derive(Debug)] +struct TestKeyExprFormatter; + +impl TestKeyExprFormatter { + fn from_rmw_liveliness(key: LivelinessKE) -> hiroz::Result { + Ok(LivelinessKE::new( + key.as_str() + .replacen("@ros2_lv", Self::ADMIN_SPACE, 1) + .try_into()?, + )) + } +} + +impl KeyExprFormatter for TestKeyExprFormatter { + const ESCAPE_CHAR: char = '%'; + const ADMIN_SPACE: &'static str = "@hiroz_test_lv"; + + fn topic_key_expr(entity: &EndpointEntity) -> hiroz::Result { + let rmw = RmwZenohFormatter::topic_key_expr(entity)?; + Ok(TopicKE::new( + format!("hiroz_test/{}", rmw.as_str()).try_into()?, + )) + } + + fn liveliness_key_expr( + entity: &EndpointEntity, + zid: &zenoh::session::ZenohId, + ) -> hiroz::Result { + Self::from_rmw_liveliness(RmwZenohFormatter::liveliness_key_expr(entity, zid)?) + } + + fn node_liveliness_key_expr(entity: &NodeEntity) -> hiroz::Result { + Self::from_rmw_liveliness(RmwZenohFormatter::node_liveliness_key_expr(entity)?) + } + + fn parse_liveliness(key: &zenoh::key_expr::KeyExpr) -> hiroz::Result { + let rmw: zenoh::key_expr::KeyExpr<'static> = key + .as_str() + .replacen(Self::ADMIN_SPACE, "@ros2_lv", 1) + .try_into()?; + RmwZenohFormatter::parse_liveliness(&rmw) + } + + fn encode_qos(qos: &QosProfile, keyless: bool) -> String { + RmwZenohFormatter::encode_qos(qos, keyless) + } + + fn decode_qos(encoded: &str) -> hiroz::Result<(bool, QosProfile)> { + RmwZenohFormatter::decode_qos(encoded) + } +} + struct DomainTestRouter { endpoint: String, _session: zenoh::Session, @@ -180,6 +242,68 @@ mod tests { Ok(()) } + /// Built-in services must also inherit the context's `keyexpr_format`, + /// not the default rmw_zenoh format -- the other half of what + /// `ParameterServiceConfig`/`TypeDescriptionService::new_with_node` + /// thread through alongside `domain_id`. Uses a pass-through custom + /// formatter with its own admin space, so a node's built-in services are + /// only discoverable at all if they actually used it. + #[tokio::test(flavor = "multi_thread")] + async fn built_in_services_inherit_custom_keyexpr_format() -> Result<()> { + let router = DomainTestRouter::new(); + let format = KeyExprFormat::Custom(Arc::new( + KeyExprFormatterAdapter::::new(), + )); + let observer_ctx = ZContextBuilder::default() + .keyexpr_format(format.clone()) + .disable_multicast_scouting() + .with_connect_endpoints([router.endpoint.as_str()]) + .with_mode("client") + .build()?; + let observer = observer_ctx + .create_node("custom_format_observer") + .without_parameters() + .build()?; + let producer_ctx = ZContextBuilder::default() + .keyexpr_format(format) + .disable_multicast_scouting() + .with_connect_endpoints([router.endpoint.as_str()]) + .with_mode("client") + .build()?; + let _producer = producer_ctx + .create_node("custom_format_builtins") + .with_type_description_service() + .build()?; + let node_key: NodeKey = (String::new(), "custom_format_builtins".to_string()); + + let deadline = tokio::time::Instant::now() + Duration::from_secs(2); + let services = loop { + let services = observer + .graph() + .get_entities_by_node(EndpointKind::Service, node_key.clone()); + if services.len() >= 7 || tokio::time::Instant::now() >= deadline { + break services; + } + tokio::time::sleep(Duration::from_millis(10)).await; + }; + + assert_eq!( + services.len(), + 7, + "built-in services must use the context's custom liveliness format" + ); + assert!( + observer + .graph() + .get_entities_by_node(EndpointKind::Publisher, node_key) + .iter() + .any(|endpoint| endpoint.topic == "/parameter_events"), + "parameter-events publisher must use the context's custom liveliness format" + ); + + Ok(()) + } + /// Tests getting topic names and types from the graph #[tokio::test(flavor = "multi_thread")] async fn test_get_topic_names_and_types() -> Result<()> { From 79be528fcda4e9f1edf9e7d5d2fe99c4a94dec4c Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:19:23 +0800 Subject: [PATCH 07/10] fix: address Copilot review on GH#336, share test router helper Two real findings from Copilot's review of ZettaScaleLabs/hiroz#336: - ROS_DOMAIN_ID validity now gates ZContextBuilder::build() with a real error, matching rcl_get_default_domain_id/rcl_init aborting on an invalid domain, instead of silently falling back to 0 with a warning. Modeled as a DomainId enum (Value/Invalid) rather than a usize plus a side-channel error field, so "pending an invalid ROS_DOMAIN_ID" is a state the type carries rather than an invariant call sites (and with_domain_id()) have to remember to check/clear. - The domain-id tests no longer mutate the real ROS_DOMAIN_ID env var at all. DomainId::parse() takes the value as a plain argument, so parsing is a pure unit test with no process-global mutation, no #[serial], and no race against the many other tests elsewhere in this crate that build a ZContextBuilder::default() on their own thread -- eliminating the concurrency hazard Copilot flagged rather than mitigating it. Also: crates/hiroz/tests/graph.rs duplicated message_type_info_derive.rs's TestRouter almost verbatim. Moved to tests/common/mod.rs and both files now share it. --- crates/hiroz/src/context.rs | 153 +++++++++++------- crates/hiroz/tests/common/mod.rs | 48 ++++++ crates/hiroz/tests/graph.rs | 38 +---- .../hiroz/tests/message_type_info_derive.rs | 43 +---- 4 files changed, 149 insertions(+), 133 deletions(-) create mode 100644 crates/hiroz/tests/common/mod.rs diff --git a/crates/hiroz/src/context.rs b/crates/hiroz/src/context.rs index 3d189f813..d4ca400dc 100644 --- a/crates/hiroz/src/context.rs +++ b/crates/hiroz/src/context.rs @@ -72,7 +72,7 @@ impl RemapRules { } pub struct ZContextBuilder { - domain_id: usize, + domain_id: DomainId, namespace: String, enclave: String, zenoh_config: Option, @@ -85,10 +85,65 @@ pub struct ZContextBuilder { clock: Option, } +/// The builder's resolved (or pending) ROS domain id. +/// +/// `Invalid` only exists between `ZContextBuilder::default()` and +/// `.build()`/`.with_domain_id()` -- a live `ZContext` always carries a +/// concrete `usize`. Modeling "haven't resolved an invalid ROS_DOMAIN_ID +/// yet" as a variant, rather than a `usize` plus a side-channel error +/// field, makes it a state the type carries instead of an invariant call +/// sites have to remember to check (and that `with_domain_id()` has to +/// remember to clear). +#[derive(Debug, Clone, PartialEq, Eq)] +enum DomainId { + /// A concrete domain, either explicit or read from `ROS_DOMAIN_ID`. + Value(usize), + /// `ROS_DOMAIN_ID` was set but is not a valid non-negative integer. + /// `build()` rejects this unless `.with_domain_id()` overrides it + /// first. Falling back to domain 0 silently would put a node on the + /// wrong ROS graph after an operator typo; `rcl_get_default_domain_id` + /// treats this the same way, returning an error that aborts + /// `rcl_init` rather than defaulting. + Invalid(String), +} + +impl DomainId { + /// Matches `rclcpp`/`rclpy`: read `ROS_DOMAIN_ID` from the environment, + /// so the normal ROS 2 deployment story (set the env var, don't touch + /// source) works here too. `.with_domain_id()` called after + /// `default()` still overrides this, same precedence as every other + /// ROS 2 client library. + fn from_env() -> Self { + Self::parse(std::env::var("ROS_DOMAIN_ID").ok()) + } + + /// Pure parsing, taking the env var's value directly rather than + /// reading it -- so this is unit-testable without mutating (and + /// racing on) real process-global state. + fn parse(value: Option) -> Self { + match value { + Some(val) => match val.parse::() { + Ok(id) => Self::Value(id), + Err(_) => Self::Invalid(val), + }, + None => Self::Value(0), + } + } +} + +impl std::fmt::Display for DomainId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Value(id) => write!(f, "{id}"), + Self::Invalid(raw) => write!(f, ""), + } + } +} + impl Default for ZContextBuilder { fn default() -> Self { Self { - domain_id: default_domain_id(), + domain_id: DomainId::from_env(), namespace: String::default(), enclave: String::default(), zenoh_config: None, @@ -103,31 +158,11 @@ impl Default for ZContextBuilder { } } -/// Domain ID to use when the builder is not given one explicitly. -/// -/// Matches `rclcpp`/`rclpy`: read `ROS_DOMAIN_ID` from the environment, so -/// the normal ROS 2 deployment story (set the env var, don't touch source) -/// works here too. `.with_domain_id()` called after `default()` still -/// overrides this, same precedence as every other ROS 2 client library. -fn default_domain_id() -> usize { - match std::env::var("ROS_DOMAIN_ID") { - Ok(val) => match val.parse::() { - Ok(id) => id, - Err(_) => { - warn!( - "[CTX] ROS_DOMAIN_ID={val:?} is not a valid non-negative integer, using domain 0" - ); - 0 - } - }, - Err(_) => 0, - } -} - impl ZContextBuilder { - /// Set the ROS domain ID + /// Set the ROS domain ID, overriding `ROS_DOMAIN_ID` (and any error + /// parsing it) with an explicit value. pub fn with_domain_id(mut self, domain_id: usize) -> Self { - self.domain_id = domain_id; + self.domain_id = DomainId::Value(domain_id); self } @@ -514,6 +549,15 @@ impl Builder for ZContextBuilder { // 4. **NEW DEFAULT**: ROS session config (connects to router at tcp/localhost:7447) // This matches rmw_zenoh_cpp behavior + let DomainId::Value(_) = &self.domain_id else { + return Err(format!( + "{}: not a valid non-negative integer; set ROS_DOMAIN_ID to a \ + valid domain or call .with_domain_id() explicitly", + self.domain_id + ) + .into()); + }; + debug!( "[CTX] Building context: domain_id={}, has_config={}", self.domain_id, @@ -588,7 +632,9 @@ impl Builder for ZContextBuilder { } } - let domain_id = builder.domain_id; + let DomainId::Value(domain_id) = builder.domain_id else { + unreachable!("build() already rejected a non-Value domain_id above") + }; let graph = Arc::new(Graph::new( &session, domain_id, @@ -714,51 +760,38 @@ impl ZContext { #[cfg(test)] mod tests { use super::*; - use serial_test::serial; + + // `DomainId::parse` takes the env var's value as a plain argument + // rather than reading `ROS_DOMAIN_ID` itself, so these are pure unit + // tests: no process-global mutation, no #[serial], no race with the + // dozens of other tests elsewhere in this crate that build a + // `ZContextBuilder::default()` on their own thread. #[test] - #[serial] - fn default_domain_id_falls_back_to_zero_when_unset() { - unsafe { - std::env::remove_var("ROS_DOMAIN_ID"); - } - assert_eq!(default_domain_id(), 0); + fn parse_falls_back_to_zero_when_unset() { + assert_eq!(DomainId::parse(None), DomainId::Value(0)); } #[test] - #[serial] - fn default_domain_id_reads_ros_domain_id() { - unsafe { - std::env::set_var("ROS_DOMAIN_ID", "42"); - } - assert_eq!(default_domain_id(), 42); - unsafe { - std::env::remove_var("ROS_DOMAIN_ID"); - } + fn parse_reads_a_valid_value() { + assert_eq!(DomainId::parse(Some("42".to_string())), DomainId::Value(42)); } #[test] - #[serial] - fn default_domain_id_falls_back_to_zero_on_invalid_value() { - unsafe { - std::env::set_var("ROS_DOMAIN_ID", "not-a-number"); - } - assert_eq!(default_domain_id(), 0); - unsafe { - std::env::remove_var("ROS_DOMAIN_ID"); - } + fn parse_rejects_an_invalid_value() { + assert_eq!( + DomainId::parse(Some("not-a-number".to_string())), + DomainId::Invalid("not-a-number".to_string()) + ); } #[test] - #[serial] - fn with_domain_id_overrides_the_env_default() { - unsafe { - std::env::set_var("ROS_DOMAIN_ID", "42"); - } - let builder = ZContextBuilder::default().with_domain_id(7); - assert_eq!(builder.domain_id, 7); - unsafe { - std::env::remove_var("ROS_DOMAIN_ID"); + fn with_domain_id_overrides_an_invalid_parse() { + let builder = ZContextBuilder { + domain_id: DomainId::Invalid("garbage".to_string()), + ..Default::default() } + .with_domain_id(7); + assert_eq!(builder.domain_id, DomainId::Value(7)); } } diff --git a/crates/hiroz/tests/common/mod.rs b/crates/hiroz/tests/common/mod.rs new file mode 100644 index 000000000..43baaa865 --- /dev/null +++ b/crates/hiroz/tests/common/mod.rs @@ -0,0 +1,48 @@ +//! Shared integration-test helpers. +//! +//! `tests/*.rs` files are each compiled as a separate crate, so anything +//! reused across them belongs here (`mod common;` per Rust's own +//! `tests/common/mod.rs` convention -- naming it `common.rs` instead would +//! make cargo treat it as its own test binary). Add a helper here rather +//! than copying it into a new test file. + +use std::time::Duration; + +use zenoh::{Wait, config::WhatAmI}; + +/// A local zenoh router bound to an ephemeral port, for tests that need +/// real cross-session discovery rather than the default in-process/loopback +/// config. +pub struct TestRouter { + pub endpoint: String, + _session: zenoh::Session, +} + +impl TestRouter { + pub fn new() -> Self { + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind test port"); + let port = listener.local_addr().expect("test address").port(); + drop(listener); + + let endpoint = format!("tcp/127.0.0.1:{port}"); + let mut config = zenoh::Config::default(); + config.set_mode(Some(WhatAmI::Router)).unwrap(); + config + .insert_json5("listen/endpoints", &format!("[\"{endpoint}\"]")) + .unwrap(); + config + .insert_json5("scouting/multicast/enabled", "false") + .unwrap(); + let session = zenoh::open(config).wait().expect("open test router"); + std::thread::sleep(Duration::from_millis(300)); + + Self { + endpoint, + _session: session, + } + } + + pub fn endpoint(&self) -> &str { + &self.endpoint + } +} diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index c19f2a9e1..1c697f0b2 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -21,7 +21,9 @@ use hiroz_protocol::{ entity::{LivelinessKE, TopicKE}, qos::QosProfile, }; -use zenoh::{Wait, config::WhatAmI}; + +mod common; +use common::TestRouter; /// A pass-through `KeyExprFormatter` that delegates to `RmwZenohFormatter` for /// everything except its own admin space, so a node using it is @@ -79,36 +81,6 @@ impl KeyExprFormatter for TestKeyExprFormatter { } } -struct DomainTestRouter { - endpoint: String, - _session: zenoh::Session, -} - -impl DomainTestRouter { - fn new() -> Self { - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind test port"); - let port = listener.local_addr().expect("test address").port(); - drop(listener); - - let endpoint = format!("tcp/127.0.0.1:{port}"); - let mut config = zenoh::Config::default(); - config.set_mode(Some(WhatAmI::Router)).unwrap(); - config - .insert_json5("listen/endpoints", &format!("[\"{endpoint}\"]")) - .unwrap(); - config - .insert_json5("scouting/multicast/enabled", "false") - .unwrap(); - let session = zenoh::open(config).wait().expect("open test router"); - std::thread::sleep(Duration::from_millis(300)); - - Self { - endpoint, - _session: session, - } - } -} - /// Helper to create a test context and node async fn setup_test_node( node_name: &str, @@ -182,7 +154,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] async fn built_in_services_inherit_context_domain() -> Result<()> { const DOMAIN_ID: usize = 123; - let router = DomainTestRouter::new(); + let router = TestRouter::new(); let observer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) .disable_multicast_scouting() @@ -250,7 +222,7 @@ mod tests { /// only discoverable at all if they actually used it. #[tokio::test(flavor = "multi_thread")] async fn built_in_services_inherit_custom_keyexpr_format() -> Result<()> { - let router = DomainTestRouter::new(); + let router = TestRouter::new(); let format = KeyExprFormat::Custom(Arc::new( KeyExprFormatterAdapter::::new(), )); diff --git a/crates/hiroz/tests/message_type_info_derive.rs b/crates/hiroz/tests/message_type_info_derive.rs index f633260d9..c2777310d 100644 --- a/crates/hiroz/tests/message_type_info_derive.rs +++ b/crates/hiroz/tests/message_type_info_derive.rs @@ -6,7 +6,9 @@ use hiroz::{ dynamic::{FieldType, MessageSchemaTypeDescription}, }; use serde::{Deserialize, Serialize}; -use zenoh::{Wait, config::WhatAmI}; + +mod common; +use common::TestRouter; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, hiroz::MessageTypeInfo)] #[ros_msg(type_name = "custom_msgs/msg/Position2D")] @@ -29,45 +31,6 @@ impl hiroz::msg::ZMessage for RobotTelemetry { type Serdes = hiroz::msg::SerdeCdrSerdes; } -struct TestRouter { - endpoint: String, - _session: zenoh::Session, -} - -impl TestRouter { - fn new() -> Self { - let port = { - let listener = - std::net::TcpListener::bind("127.0.0.1:0").expect("failed to bind port 0"); - listener.local_addr().unwrap().port() - }; - - let endpoint = format!("tcp/127.0.0.1:{port}"); - let mut config = zenoh::Config::default(); - config.set_mode(Some(WhatAmI::Router)).unwrap(); - config - .insert_json5("listen/endpoints", &format!("[\"{endpoint}\"]")) - .unwrap(); - config - .insert_json5("scouting/multicast/enabled", "false") - .unwrap(); - - let session = zenoh::open(config) - .wait() - .expect("failed to open test router"); - std::thread::sleep(Duration::from_millis(300)); - - Self { - endpoint, - _session: session, - } - } - - fn endpoint(&self) -> &str { - &self.endpoint - } -} - fn create_context_with_router(router: &TestRouter) -> hiroz::Result { ZContextBuilder::default() .disable_multicast_scouting() From 862973b884d387965f0e750ca1293e7a92ae324c Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:21:03 +0800 Subject: [PATCH 08/10] test: assert build() actually rejects an invalid domain Directly exercises the behavior Copilot's review asked for -- the existing tests only covered DomainId::parse and with_domain_id, not that build() itself turns an unresolved DomainId::Invalid into an Err rather than silently constructing a ZContext on domain 0. --- crates/hiroz/src/context.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/hiroz/src/context.rs b/crates/hiroz/src/context.rs index d4ca400dc..ca9ae7fcb 100644 --- a/crates/hiroz/src/context.rs +++ b/crates/hiroz/src/context.rs @@ -794,4 +794,18 @@ mod tests { .with_domain_id(7); assert_eq!(builder.domain_id, DomainId::Value(7)); } + + /// The actual behavior Copilot's review asked for: an invalid domain + /// aborts `build()` with an error, matching `rcl_init`, rather than + /// silently producing a `ZContext` on domain 0. + #[test] + fn build_rejects_an_invalid_domain() { + let err = ZContextBuilder { + domain_id: DomainId::Invalid("garbage".to_string()), + ..Default::default() + } + .build() + .expect_err("an invalid ROS_DOMAIN_ID must not silently build"); + assert!(err.to_string().contains("garbage")); + } } From ae420688cada5027ce6de8120572d1a5327acbc3 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:27:34 +0800 Subject: [PATCH 09/10] test(hiroz): move domain_id tests to dedicated integration folder Copilot's two findings were fixed with private unit tests in context.rs's mod tests, which is the wrong place per the workspace's test-folder convention -- new tests belong in tests/, exercising the public API, not private internals. Added ZContext::domain_id() as the public observation point, moved all five tests to tests/domain_id.rs against ZContextBuilder's public surface, and deleted the inline mod tests block. Each tests/*.rs file is its own process, so #[serial] here only guards this file's own tests -- no cross-file or cross-binary race. --- crates/hiroz/src/context.rs | 60 +++------------------- crates/hiroz/tests/domain_id.rs | 90 +++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 53 deletions(-) create mode 100644 crates/hiroz/tests/domain_id.rs diff --git a/crates/hiroz/src/context.rs b/crates/hiroz/src/context.rs index ca9ae7fcb..cc94a6538 100644 --- a/crates/hiroz/src/context.rs +++ b/crates/hiroz/src/context.rs @@ -751,61 +751,15 @@ impl ZContext { &self.graph } + /// The resolved ROS domain id this context was built with -- either + /// explicit via `.with_domain_id()`, from `ROS_DOMAIN_ID`, or the + /// default of 0. + pub fn domain_id(&self) -> usize { + self.domain_id + } + /// Access the context clock used by nodes and runtime helpers. pub fn clock(&self) -> &ZClock { &self.clock } } - -#[cfg(test)] -mod tests { - use super::*; - - // `DomainId::parse` takes the env var's value as a plain argument - // rather than reading `ROS_DOMAIN_ID` itself, so these are pure unit - // tests: no process-global mutation, no #[serial], no race with the - // dozens of other tests elsewhere in this crate that build a - // `ZContextBuilder::default()` on their own thread. - - #[test] - fn parse_falls_back_to_zero_when_unset() { - assert_eq!(DomainId::parse(None), DomainId::Value(0)); - } - - #[test] - fn parse_reads_a_valid_value() { - assert_eq!(DomainId::parse(Some("42".to_string())), DomainId::Value(42)); - } - - #[test] - fn parse_rejects_an_invalid_value() { - assert_eq!( - DomainId::parse(Some("not-a-number".to_string())), - DomainId::Invalid("not-a-number".to_string()) - ); - } - - #[test] - fn with_domain_id_overrides_an_invalid_parse() { - let builder = ZContextBuilder { - domain_id: DomainId::Invalid("garbage".to_string()), - ..Default::default() - } - .with_domain_id(7); - assert_eq!(builder.domain_id, DomainId::Value(7)); - } - - /// The actual behavior Copilot's review asked for: an invalid domain - /// aborts `build()` with an error, matching `rcl_init`, rather than - /// silently producing a `ZContext` on domain 0. - #[test] - fn build_rejects_an_invalid_domain() { - let err = ZContextBuilder { - domain_id: DomainId::Invalid("garbage".to_string()), - ..Default::default() - } - .build() - .expect_err("an invalid ROS_DOMAIN_ID must not silently build"); - assert!(err.to_string().contains("garbage")); - } -} diff --git a/crates/hiroz/tests/domain_id.rs b/crates/hiroz/tests/domain_id.rs new file mode 100644 index 000000000..244417657 --- /dev/null +++ b/crates/hiroz/tests/domain_id.rs @@ -0,0 +1,90 @@ +//! `ROS_DOMAIN_ID` resolution, exercised only through `ZContextBuilder`'s +//! public API -- no reach into `context.rs`'s private `DomainId` type. +//! +//! Each `tests/*.rs` file compiles as its own process, so mutating the +//! process-global `ROS_DOMAIN_ID` here cannot race with the rest of the +//! crate's tests in other files or in the `--lib` binary. `#[serial]` only +//! has to guard against races between the handful of tests in *this* file. + +use hiroz::{Builder, context::ZContextBuilder}; +use serial_test::serial; + +/// Restores the previous `ROS_DOMAIN_ID` (or its absence) on drop, so one +/// test's env mutation can't leak into the next. +struct EnvVarGuard { + key: &'static str, + previous: Option, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &str) -> Self { + let previous = std::env::var(key).ok(); + // SAFETY: serialized by #[serial] -- no other thread reads/writes + // process env vars while a guard is live. + unsafe { std::env::set_var(key, value) }; + Self { key, previous } + } + + fn unset(key: &'static str) -> Self { + let previous = std::env::var(key).ok(); + // SAFETY: see above. + unsafe { std::env::remove_var(key) }; + Self { key, previous } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + // SAFETY: see above. + unsafe { + match &self.previous { + Some(val) => std::env::set_var(self.key, val), + None => std::env::remove_var(self.key), + } + } + } +} + +#[test] +#[serial] +fn unset_env_defaults_to_domain_zero() { + let _guard = EnvVarGuard::unset("ROS_DOMAIN_ID"); + let ctx = ZContextBuilder::default() + .build() + .expect("default domain must build"); + assert_eq!(ctx.domain_id(), 0); +} + +#[test] +#[serial] +fn valid_env_is_used() { + let _guard = EnvVarGuard::set("ROS_DOMAIN_ID", "42"); + let ctx = ZContextBuilder::default() + .build() + .expect("a valid ROS_DOMAIN_ID must build"); + assert_eq!(ctx.domain_id(), 42); +} + +/// The behavior Copilot's review asked for: an invalid `ROS_DOMAIN_ID` +/// aborts `build()` with an error naming the bad value, matching +/// `rcl_init`, rather than silently producing a `ZContext` on domain 0. +#[test] +#[serial] +fn invalid_env_is_rejected_by_build() { + let _guard = EnvVarGuard::set("ROS_DOMAIN_ID", "not-a-number"); + let err = ZContextBuilder::default() + .build() + .expect_err("an invalid ROS_DOMAIN_ID must not silently build"); + assert!(err.to_string().contains("not-a-number")); +} + +#[test] +#[serial] +fn with_domain_id_overrides_an_invalid_env_value() { + let _guard = EnvVarGuard::set("ROS_DOMAIN_ID", "garbage"); + let ctx = ZContextBuilder::default() + .with_domain_id(7) + .build() + .expect("explicit with_domain_id must override a bad env value"); + assert_eq!(ctx.domain_id(), 7); +} From 137e6a62fde9b8cce490177fa6d92816f8946c14 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:32:15 +0800 Subject: [PATCH 10/10] fix(hiroz): use TestRouter::endpoint() consistently in graph.rs CI clippy (--all-targets -D warnings) failed: graph.rs read the endpoint field directly instead of calling the endpoint() method, so within that test binary's own compilation the method was unused -- each tests/*.rs file is a separate crate, so dead-code is judged per binary, not per source file. --- crates/hiroz/tests/graph.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/hiroz/tests/graph.rs b/crates/hiroz/tests/graph.rs index 1c697f0b2..f16df2f1e 100644 --- a/crates/hiroz/tests/graph.rs +++ b/crates/hiroz/tests/graph.rs @@ -158,7 +158,7 @@ mod tests { let observer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) .disable_multicast_scouting() - .with_connect_endpoints([router.endpoint.as_str()]) + .with_connect_endpoints([router.endpoint()]) .with_mode("client") .build()?; let observer = observer_ctx @@ -168,7 +168,7 @@ mod tests { let producer_ctx = ZContextBuilder::default() .with_domain_id(DOMAIN_ID) .disable_multicast_scouting() - .with_connect_endpoints([router.endpoint.as_str()]) + .with_connect_endpoints([router.endpoint()]) .with_mode("client") .build()?; let _producer = producer_ctx @@ -229,7 +229,7 @@ mod tests { let observer_ctx = ZContextBuilder::default() .keyexpr_format(format.clone()) .disable_multicast_scouting() - .with_connect_endpoints([router.endpoint.as_str()]) + .with_connect_endpoints([router.endpoint()]) .with_mode("client") .build()?; let observer = observer_ctx @@ -239,7 +239,7 @@ mod tests { let producer_ctx = ZContextBuilder::default() .keyexpr_format(format) .disable_multicast_scouting() - .with_connect_endpoints([router.endpoint.as_str()]) + .with_connect_endpoints([router.endpoint()]) .with_mode("client") .build()?; let _producer = producer_ctx