[TASK] timpani-o: Improve unit test coverage (#74)

timpani_rust/timpani-o/src/fault/mod.rs

@@ -221,4 +221,41 @@ mod tests {
         FaultClient::connect_lazy("http://localhost:59999".to_string())
             .expect("valid URI should not fail");
     }
+
+    // ── New tests for coverage ────────────────────────────────────────────────
+
+    #[test]
+    fn fault_client_connect_lazy_invalid_uri_returns_error() {
+        // A URI without a scheme causes Endpoint::from_shared to return Err.
+        let result = FaultClient::connect_lazy("not-a-valid-uri !!".to_string());
+        assert!(result.is_err(), "invalid URI must return Err");
+    }
+
+    #[test]
+    fn fault_notification_all_fields_are_stored() {
+        let n = FaultNotification {
+            workload_id: "my_workload".into(),
+            node_id: "node_x".into(),
+            task_name: "safety_task".into(),
+            fault_type: FaultType::Dmiss,
+        };
+        assert_eq!(n.workload_id, "my_workload");
+        assert_eq!(n.node_id, "node_x");
+        assert_eq!(n.task_name, "safety_task");
+    }
+
+    #[tokio::test]
+    async fn mock_notifier_records_node_id_correctly() {
+        let notifier = MockFaultNotifier::arc();
+        notifier
+            .notify_fault(FaultNotification {
+                workload_id: "w".into(),
+                node_id: "expected_node".into(),
+                task_name: "t".into(),
+                fault_type: FaultType::Dmiss,
+            })
+            .await
+            .unwrap();
+        assert_eq!(notifier.calls.lock().unwrap()[0].node_id, "expected_node");
+    }
 }

timpani_rust/timpani-o/src/hyperperiod/mod.rs

@@ -478,4 +478,68 @@ mod tests {
         let info = mgr.calculate_hyperperiod("w1", &tasks).unwrap();
         assert_eq!(info.unique_periods, vec![1_000, 2_000, 5_000]);
     }
+
+    // ── New tests for coverage ────────────────────────────────────────────────
+
+    // Display impl — all three HyperperiodError arms (lines 60–72)
+
+    #[test]
+    fn hyperperiod_error_no_valid_periods_display() {
+        let s = HyperperiodError::NoValidPeriods.to_string();
+        assert!(s.contains("non-zero"), "got: {s}");
+    }
+
+    #[test]
+    fn hyperperiod_error_overflow_display() {
+        let s = HyperperiodError::Overflow { a: 100, b: 200 }.to_string();
+        assert!(s.contains("100") && s.contains("200"), "got: {s}");
+    }
+
+    #[test]
+    fn hyperperiod_error_too_large_display() {
+        let s = HyperperiodError::TooLarge {
+            value_us: 7_000_000,
+            limit_us: 5_000_000,
+        }
+        .to_string();
+        // Should contain the numeric values
+        assert!(s.contains("7000000") || s.contains("7.0"), "got: {s}");
+        assert!(s.contains("5000000") || s.contains("5.0"), "got: {s}");
+    }
+
+    // Default trait impl (lines 268–269)
+
+    #[test]
+    fn hyperperiod_manager_default_equals_new() {
+        let mgr: HyperperiodManager = Default::default();
+        assert!(mgr.get("anything").is_none());
+        assert!(!mgr.has("anything"));
+        assert_eq!(mgr.all().len(), 0);
+    }
+
+    // clear_all on an already-empty map — exercises the `if !is_empty` false branch
+
+    #[test]
+    fn clear_all_on_empty_map_is_noop() {
+        let mut mgr = HyperperiodManager::new();
+        mgr.clear_all(); // must not panic
+        assert_eq!(mgr.all().len(), 0);
+    }
+
+    // calculate_hyperperiod with 3 distinct periods — iterates the debug loop
+    // more than once, covering the loop body that was previously missed
+
+    #[test]
+    fn hyperperiod_multiple_unique_periods_debug_loop() {
+        let tasks = vec![
+            make_task("w1", 1_000),
+            make_task("w1", 3_000),
+            make_task("w1", 7_000),
+        ];
+        let mut mgr = HyperperiodManager::new();
+        let info = mgr.calculate_hyperperiod("w1", &tasks).unwrap();
+        assert_eq!(info.unique_periods.len(), 3);
+        // LCM(1000, 3000, 7000) = 21000
+        assert_eq!(info.hyperperiod_us, 21_000);
+    }
 }

timpani_rust/timpani-o/src/main.rs

@@ -70,6 +70,16 @@ struct Cli {
     node_config: Option<PathBuf>,
 }
 
+// ── Helpers ──────────────────────────────────────────────────────────────────
+
+/// Build the full URI used to reach Pullpiri's FaultService.
+///
+/// Extracted into a free function so it can be unit-tested without spinning
+/// up an async runtime.
+fn build_pullpiri_addr(host: &str, port: u16) -> String {
+    format!("http://{}:{}", host, port)
+}
+
 // ── Entry point ───────────────────────────────────────────────────────────────
 
 #[tokio::main]
@@ -139,7 +149,7 @@ async fn main() {
     let workload_store = new_workload_store();
 
     // ── Fault client (lazy — connects to Pullpiri on first RPC call) ──────────
-    let pullpiri_addr = format!("http://{}:{}", cli.fault_host, cli.fault_port);
+    let pullpiri_addr = build_pullpiri_addr(&cli.fault_host, cli.fault_port);
     let fault_notifier = match FaultClient::connect_lazy(pullpiri_addr.clone()) {
         Ok(n) => n,
         Err(e) => {
@@ -254,3 +264,83 @@ async fn main() {
         }
     }
 }
+
+// ── Unit tests ────────────────────────────────────────────────────────────────
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use clap::Parser;
+
+    // ── build_pullpiri_addr ───────────────────────────────────────────────────
+
+    #[test]
+    fn build_pullpiri_addr_formats_correctly() {
+        assert_eq!(
+            build_pullpiri_addr("localhost", 50053),
+            "http://localhost:50053"
+        );
+    }
+
+    #[test]
+    fn build_pullpiri_addr_custom_host_and_port() {
+        assert_eq!(
+            build_pullpiri_addr("10.0.0.5", 9090),
+            "http://10.0.0.5:9090"
+        );
+    }
+
+    // ── CLI argument parsing via try_parse_from ───────────────────────────────
+    // Uses clap's try_parse_from so we can parse a &[&str] in a unit test
+    // without touching the process argv.
+
+    #[test]
+    fn cli_defaults_are_sane() {
+        let cli = Cli::try_parse_from(["timpani-o"]).unwrap();
+        assert_eq!(cli.sinfo_port, 50052);
+        assert_eq!(cli.fault_host, "localhost");
+        assert_eq!(cli.fault_port, 50053);
+        assert_eq!(cli.node_port, 50054);
+        assert!(!cli.notify_fault);
+        assert!(cli.node_config.is_none());
+        assert_eq!(
+            cli.sync_timeout_secs,
+            timpani_o::grpc::node_service::DEFAULT_SYNC_TIMEOUT_SECS
+        );
+    }
+
+    #[test]
+    fn cli_short_flags_are_parsed() {
+        let cli = Cli::try_parse_from([
+            "timpani-o",
+            "-s",
+            "9001",
+            "-f",
+            "10.0.0.1",
+            "-p",
+            "9002",
+            "-d",
+            "9003",
+        ])
+        .unwrap();
+        assert_eq!(cli.sinfo_port, 9001);
+        assert_eq!(cli.fault_host, "10.0.0.1");
+        assert_eq!(cli.fault_port, 9002);
+        assert_eq!(cli.node_port, 9003);
+    }
+
+    #[test]
+    fn cli_nodeconfig_flag_sets_path() {
+        let cli = Cli::try_parse_from(["timpani-o", "--nodeconfig", "/tmp/nodes.yaml"]).unwrap();
+        assert_eq!(
+            cli.node_config.unwrap().to_str().unwrap(),
+            "/tmp/nodes.yaml"
+        );
+    }
+
+    #[test]
+    fn cli_notifyfault_flag_enables_feature() {
+        let cli = Cli::try_parse_from(["timpani-o", "-n"]).unwrap();
+        assert!(cli.notify_fault);
+    }
+}