[WIP] Fix test gaps in compile/common.rs and compile/onees.rs (#163)

* Initial plan

* test: add missing unit tests for compile/common.rs, compile/onees.rs, and fuzzy_schedule.rs

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/cc9ca027-17fc-4675-abd6-a6e5ff2e8b2c

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

Author: Copilot

src/compile/common.rs

@@ -1786,4 +1786,155 @@ mod tests {
         let args = generate_enabled_tools_args(&fm);
         assert!(args.is_empty(), "memory-only safe-outputs should produce no args (all tools available)");
     }
+
+    // ─── replace_with_indent ─────────────────────────────────────────────────
+
+    #[test]
+    fn test_replace_with_indent_multiline_replacement() {
+        let template = "steps:\n    {{ my_marker }}\n";
+        let replacement = "- bash: echo hello\n  displayName: Hello";
+        let result = replace_with_indent(template, "{{ my_marker }}", replacement);
+        // The 4-space indent on the placeholder line is inherited by continuation lines
+        assert_eq!(result, "steps:\n    - bash: echo hello\n      displayName: Hello\n");
+    }
+
+    #[test]
+    fn test_replace_with_indent_not_at_line_start_no_indent() {
+        // When the placeholder is not at the start of a line (preceded by non-whitespace),
+        // no extra indentation is added to continuation lines.
+        let template = "prefix {{ marker }} suffix";
+        let result = replace_with_indent(template, "{{ marker }}", "VALUE");
+        assert_eq!(result, "prefix VALUE suffix");
+    }
+
+    #[test]
+    fn test_replace_with_indent_single_line_replacement_preserves_trailing_newline() {
+        let template = "    {{ placeholder }}\n";
+        let result = replace_with_indent(template, "{{ placeholder }}", "value");
+        assert_eq!(result, "    value\n");
+    }
+
+    #[test]
+    fn test_replace_with_indent_replacement_ending_with_newline() {
+        let template = "    {{ placeholder }}\n";
+        let result = replace_with_indent(template, "{{ placeholder }}", "line1\nline2\n");
+        // The trailing \n in the replacement should be preserved
+        assert!(result.contains("line1"));
+        assert!(result.contains("line2"));
+        assert!(result.ends_with('\n'));
+    }
+
+    // ─── format_step_yaml / format_step_yaml_indented ────────────────────────
+
+    #[test]
+    fn test_format_step_yaml_single_line() {
+        let result = format_step_yaml("bash: echo hi");
+        assert_eq!(result, "  - bash: echo hi");
+    }
+
+    #[test]
+    fn test_format_step_yaml_multiline() {
+        let result = format_step_yaml("bash: |\n  echo hi\n  echo bye");
+        let lines: Vec<&str> = result.lines().collect();
+        assert_eq!(lines[0], "  - bash: |");
+        // Continuation lines get 8 spaces prepended (existing indent is preserved)
+        assert_eq!(lines[1], "          echo hi");
+        assert_eq!(lines[2], "          echo bye");
+    }
+
+    #[test]
+    fn test_format_step_yaml_strips_yaml_document_separator() {
+        let result = format_step_yaml("--- bash: echo hi");
+        assert_eq!(result, "  - bash: echo hi");
+    }
+
+    #[test]
+    fn test_format_step_yaml_indented_custom_base() {
+        let result = format_step_yaml_indented("bash: echo hi", 6);
+        assert_eq!(result, "      - bash: echo hi");
+    }
+
+    #[test]
+    fn test_format_step_yaml_indented_zero_base() {
+        let result = format_step_yaml_indented("bash: echo hi", 0);
+        assert_eq!(result, "- bash: echo hi");
+    }
+
+    // ─── generate_acquire_ado_token ──────────────────────────────────────────
+
+    #[test]
+    fn test_generate_acquire_ado_token_with_sc() {
+        let result = generate_acquire_ado_token(Some("my-arm-sc"), "SC_READ_TOKEN");
+        assert!(result.contains("AzureCLI@2"), "Should use AzureCLI@2 task");
+        assert!(
+            result.contains("azureSubscription: 'my-arm-sc'"),
+            "Should embed service connection name"
+        );
+        assert!(
+            result.contains("variable=SC_READ_TOKEN;issecret=true"),
+            "Should set correct pipeline variable as secret"
+        );
+        assert!(
+            result.contains("az account get-access-token"),
+            "Should call az CLI to get access token"
+        );
+    }
+
+    #[test]
+    fn test_generate_acquire_ado_token_none_returns_empty() {
+        let result = generate_acquire_ado_token(None, "SC_READ_TOKEN");
+        assert!(result.is_empty(), "None service connection should return empty string");
+    }
+
+    #[test]
+    fn test_generate_acquire_ado_token_write_token_variable() {
+        let result = generate_acquire_ado_token(Some("write-sc"), "SC_WRITE_TOKEN");
+        assert!(result.contains("variable=SC_WRITE_TOKEN;issecret=true"));
+        assert!(!result.contains("SC_READ_TOKEN"));
+    }
+
+    // ─── generate_copilot_ado_env / generate_executor_ado_env ────────────────
+
+    #[test]
+    fn test_generate_copilot_ado_env_with_connection() {
+        let result = generate_copilot_ado_env(Some("my-sc"));
+        assert!(
+            result.contains("AZURE_DEVOPS_EXT_PAT: $(SC_READ_TOKEN)"),
+            "Should set AZURE_DEVOPS_EXT_PAT to SC_READ_TOKEN"
+        );
+        assert!(
+            result.contains("SYSTEM_ACCESSTOKEN: $(SC_READ_TOKEN)"),
+            "Should set SYSTEM_ACCESSTOKEN to SC_READ_TOKEN"
+        );
+    }
+
+    #[test]
+    fn test_generate_copilot_ado_env_none_empty() {
+        assert!(
+            generate_copilot_ado_env(None).is_empty(),
+            "None service connection should produce empty env block"
+        );
+    }
+
+    #[test]
+    fn test_generate_executor_ado_env_with_connection() {
+        let result = generate_executor_ado_env(Some("my-sc"));
+        assert!(
+            result.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)"),
+            "Executor should use SC_WRITE_TOKEN"
+        );
+        // Must NOT expose the read token in the executor env
+        assert!(
+            !result.contains("SC_READ_TOKEN"),
+            "Executor env must not contain SC_READ_TOKEN"
+        );
+    }
+
+    #[test]
+    fn test_generate_executor_ado_env_none_empty() {
+        assert!(
+            generate_executor_ado_env(None).is_empty(),
+            "None service connection should produce empty env block"
+        );
+    }
 }

src/compile/onees.rs

@@ -388,3 +388,160 @@ fn generate_teardown_job(teardown_steps: &[serde_yaml::Value], agent_name: &str)
         steps_yaml.join("\n    ")
     )
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use super::super::types::McpOptions;
+
+    // ─── generate_agent_context_root ─────────────────────────────────────────
+
+    #[test]
+    fn test_generate_agent_context_root_repo() {
+        assert_eq!(
+            generate_agent_context_root("repo"),
+            "$(Build.Repository.Name)"
+        );
+    }
+
+    #[test]
+    fn test_generate_agent_context_root_root() {
+        assert_eq!(generate_agent_context_root("root"), ".");
+    }
+
+    #[test]
+    fn test_generate_agent_context_root_unknown_defaults_to_dot() {
+        // Any unrecognised workspace value should fall through to "."
+        assert_eq!(generate_agent_context_root("something-else"), ".");
+    }
+
+    // ─── generate_mcp_configuration ──────────────────────────────────────────
+
+    #[test]
+    fn test_generate_mcp_configuration_empty_returns_braces() {
+        let mcps = HashMap::new();
+        let result = generate_mcp_configuration(&mcps);
+        assert_eq!(result, "{}");
+    }
+
+    #[test]
+    fn test_generate_mcp_configuration_skips_custom_mcp_with_command() {
+        let mut mcps = HashMap::new();
+        mcps.insert(
+            "my-tool".to_string(),
+            McpConfig::WithOptions(McpOptions {
+                command: Some("node".to_string()),
+                ..Default::default()
+            }),
+        );
+        let result = generate_mcp_configuration(&mcps);
+        // Custom MCPs with `command:` are not supported in 1ES — must be excluded
+        assert!(
+            !result.contains("my-tool"),
+            "Custom MCP with command should be excluded in 1ES target"
+        );
+        assert_eq!(result, "{}", "Only custom MCPs → empty config");
+    }
+
+    #[test]
+    fn test_generate_mcp_configuration_service_connection_mcp() {
+        let mut mcps = HashMap::new();
+        mcps.insert(
+            "my-mcp".to_string(),
+            McpConfig::WithOptions(McpOptions {
+                service_connection: Some("mcp-my-mcp-sc".to_string()),
+                ..Default::default()
+            }),
+        );
+        let result = generate_mcp_configuration(&mcps);
+        assert!(result.contains("my-mcp"), "Service-connection MCP should appear in output");
+        assert!(
+            result.contains("serviceConnection: mcp-my-mcp-sc"),
+            "Should reference the explicit service connection"
+        );
+    }
+
+    #[test]
+    fn test_generate_mcp_configuration_default_service_connection_naming() {
+        // When no explicit service_connection is set, a default name is generated.
+        let mut mcps = HashMap::new();
+        mcps.insert("my-tool".to_string(), McpConfig::Enabled(true));
+        let result = generate_mcp_configuration(&mcps);
+        assert!(result.contains("my-tool"));
+        assert!(result.contains("serviceConnection: mcp-my-tool-service-connection"));
+    }
+
+    #[test]
+    fn test_generate_mcp_configuration_disabled_mcp_excluded() {
+        let mut mcps = HashMap::new();
+        mcps.insert("disabled-mcp".to_string(), McpConfig::Enabled(false));
+        let result = generate_mcp_configuration(&mcps);
+        assert!(!result.contains("disabled-mcp"), "Disabled MCP should not appear in output");
+        assert_eq!(result, "{}");
+    }
+
+    // ─── generate_inline_steps ────────────────────────────────────────────────
+
+    #[test]
+    fn test_generate_inline_steps_empty() {
+        let result = generate_inline_steps(&[]);
+        assert!(result.is_empty(), "Empty steps list should return empty string");
+    }
+
+    #[test]
+    fn test_generate_inline_steps_single_step() {
+        let step: serde_yaml::Value =
+            serde_yaml::from_str("bash: echo hello").expect("valid yaml");
+        let result = generate_inline_steps(&[step]);
+        assert!(result.contains("bash"), "Step YAML should contain the bash key");
+        assert!(result.contains("echo hello"), "Step YAML should contain the command");
+    }
+
+    // ─── generate_setup_job ──────────────────────────────────────────────────
+
+    #[test]
+    fn test_generate_setup_job_empty_steps() {
+        let result = generate_setup_job(&[], "My Agent");
+        assert!(result.is_empty(), "Empty setup steps should return empty string");
+    }
+
+    #[test]
+    fn test_generate_setup_job_with_steps() {
+        let step: serde_yaml::Value =
+            serde_yaml::from_str("bash: echo setup").expect("valid yaml");
+        let result = generate_setup_job(&[step], "My Agent");
+        assert!(result.contains("SetupJob"), "Should define a SetupJob");
+        assert!(
+            result.contains("My Agent - Setup"),
+            "Should include agent name in display name"
+        );
+        assert!(result.contains("checkout: self"), "Should include self checkout");
+        assert!(result.contains("echo setup"), "Should include the step content");
+    }
+
+    // ─── generate_teardown_job ───────────────────────────────────────────────
+
+    #[test]
+    fn test_generate_teardown_job_empty_steps() {
+        let result = generate_teardown_job(&[], "My Agent");
+        assert!(result.is_empty(), "Empty teardown steps should return empty string");
+    }
+
+    #[test]
+    fn test_generate_teardown_job_with_steps() {
+        let step: serde_yaml::Value =
+            serde_yaml::from_str("bash: echo teardown").expect("valid yaml");
+        let result = generate_teardown_job(&[step], "My Agent");
+        assert!(result.contains("TeardownJob"), "Should define a TeardownJob");
+        assert!(
+            result.contains("My Agent - Teardown"),
+            "Should include agent name in display name"
+        );
+        assert!(
+            result.contains("ProcessSafeOutputs"),
+            "Should depend on ProcessSafeOutputs"
+        );
+        assert!(result.contains("checkout: self"), "Should include self checkout");
+        assert!(result.contains("echo teardown"), "Should include the step content");
+    }
+}

src/fuzzy_schedule.rs

@@ -1036,6 +1036,38 @@ mod tests {
         assert!(err.to_string().contains("at least 5 minutes"));
     }
 
+    // ─── invalid hour interval error path ────────────────────────────────────
+
+    #[test]
+    fn test_parse_invalid_hour_interval_5h() {
+        let err = parse_fuzzy_schedule("every 5h").unwrap_err();
+        assert!(
+            err.to_string().contains("Valid intervals"),
+            "Error for 5h should mention valid intervals: {}",
+            err
+        );
+    }
+
+    #[test]
+    fn test_parse_invalid_hour_interval_7h() {
+        let err = parse_fuzzy_schedule("every 7h").unwrap_err();
+        assert!(
+            err.to_string().contains("not recommended"),
+            "Error for 7h should say 'not recommended': {}",
+            err
+        );
+    }
+
+    #[test]
+    fn test_parse_zero_hour_interval() {
+        let err = parse_fuzzy_schedule("every 0h").unwrap_err();
+        assert!(
+            err.to_string().contains("greater than 0"),
+            "Error for 0h should mention interval must be greater than 0: {}",
+            err
+        );
+    }
+
     #[test]
     fn test_backward_compatibility() {
         // Test that simple "hourly" and "daily" still work