Extend expand_foreach test coverage (#296)

src/manifest/expand_test_cases/action_condition_cases.rs

@@ -44,6 +44,7 @@ fn expand_foreach_applies_action_when_expression() -> Result<()> {
     let actions = actions(&doc)?;
     anyhow::ensure!(actions.len() == 2, "expected filtered actions");
     anyhow::ensure!(indexes(actions, "action")? == vec![1, 2], "wrong indexes");
+    ensure_foreach_removed(actions, "filtered action")?;
     Ok(())
 }
 

src/manifest/expand_test_cases/condition_cases.rs

@@ -1,10 +1,14 @@
-//! Conditional expansion cases for manifest entries; action-only cases live
-//! in `action_condition_cases`.
+//! Conditional expansion cases; action-only cases live in `action_condition_cases`.
 
 use super::*;
 use anyhow::{Context, Result};
 use minijinja::Environment;
-use rstest::rstest;
+use rstest::{fixture, rstest};
+
+#[fixture]
+fn environment() -> Environment<'static> {
+    Environment::new()
+}
 
 #[rstest]
 #[case::targets("targets")]
@@ -156,6 +160,7 @@ fn expand_foreach_expands_sequence_values() -> Result<()> {
     expand_foreach(&mut doc, &env)?;
     let targets = targets(&doc)?;
     anyhow::ensure!(targets.len() == 2, "expected two targets");
+    ensure_foreach_removed(targets, "target")?;
     for (idx, target) in targets.iter().enumerate() {
         let map = target.as_object().context("target map")?;
         let vars = map
@@ -194,41 +199,132 @@ fn expand_foreach_applies_when_expression() -> Result<()> {
     expand_foreach(&mut doc, &env)?;
     let targets = targets(&doc)?;
     anyhow::ensure!(targets.len() == 2, "expected filtered targets");
-    let indexes = indexes(targets, "target")?;
+    anyhow::ensure!(indexes(targets, "target")? == vec![1, 2], "wrong indexes");
+    ensure_foreach_removed(targets, "filtered target")?;
+    Ok(())
+}
+
+#[rstest]
+fn expand_foreach_empty_foreach_produces_no_entries(
+    environment: Environment<'static>,
+) -> Result<()> {
+    let mut doc: ManifestValue = serde_saphyr::from_str(
+        "targets:
+  - name: literal
+    foreach: []
+    command: echo hi",
+    )?;
+    expand_foreach(&mut doc, &environment)?;
+    let targets = targets(&doc)?;
+    anyhow::ensure!(targets.is_empty(), "empty foreach must produce no targets");
+    Ok(())
+}
+
+#[rstest]
+fn expand_foreach_non_object_entry_is_passed_through(
+    environment: Environment<'static>,
+) -> Result<()> {
+    let mut doc: ManifestValue = serde_saphyr::from_str(
+        "targets:
+  - just-a-string
+  - name: real
+    foreach:
+      - expanded
+    command: echo hi",
+    )?;
+    expand_foreach(&mut doc, &environment)?;
+    let targets = targets(&doc)?;
+    anyhow::ensure!(targets.len() == 2, "expected both entries to survive");
+    anyhow::ensure!(
+        targets.first().and_then(ManifestValue::as_str) == Some("just-a-string"),
+        "bare string entry should pass through unexpanded: {:?}",
+        targets.first()
+    );
+    let second_target = targets
+        .get(1)
+        .and_then(ManifestValue::as_object)
+        .context("second target object")?;
     anyhow::ensure!(
-        indexes == vec![1, 2],
-        "unexpected filtered indexes: {:?}",
-        indexes
+        second_target.get("name").and_then(ManifestValue::as_str) == Some("real"),
+        "second target should remain object named real: {second_target:?}"
+    );
+    anyhow::ensure!(
+        !second_target.contains_key("foreach"),
+        "expanded target should no longer contain foreach: {second_target:?}"
+    );
+    let vars = second_target
+        .get("vars")
+        .and_then(ManifestValue::as_object)
+        .context("second target vars")?;
+    anyhow::ensure!(
+        vars.get("item").and_then(ManifestValue::as_str) == Some("expanded"),
+        "second target should retain the iteration item: {vars:?}"
+    );
+    anyhow::ensure!(
+        vars.get("index").and_then(ManifestValue::as_u64) == Some(0),
+        "second target should retain the iteration index: {vars:?}"
     );
     Ok(())
 }
 
-#[test]
-fn expand_foreach_preserves_object_key_order() -> Result<()> {
-    let env = Environment::new();
-    let yaml = r"targets:
+#[rstest]
+fn expand_foreach_iteration_vars_do_not_get_overwritten_by_entry_vars(
+    environment: Environment<'static>,
+) -> Result<()> {
+    let mut doc: ManifestValue = serde_saphyr::from_str(
+        "targets:
   - name: literal
-    vars:
-      existing: keep
     foreach:
-      - 1
-      - 2
-    when: 'true'
-    after: done
-";
-    let mut doc: ManifestValue = serde_saphyr::from_str(yaml)?;
-    expand_foreach(&mut doc, &env)?;
+      - from-iteration
+    vars:
+      item: from-entry
+      other: untouched",
+    )?;
+    expand_foreach(&mut doc, &environment)?;
     let targets = targets(&doc)?;
-    anyhow::ensure!(targets.len() == 2, "expected expanded targets");
-    for target in targets {
-        let map = target.as_object().context("target object")?;
-        let keys: Vec<&str> = map.keys().map(String::as_str).collect();
-        anyhow::ensure!(
-            keys == ["name", "vars", "after"],
-            "key order should remain stable: {:?}",
-            keys
-        );
-    }
+    anyhow::ensure!(targets.len() == 1, "expected one expanded target");
+    let vars = targets
+        .first()
+        .and_then(ManifestValue::as_object)
+        .and_then(|map| map.get("vars"))
+        .and_then(ManifestValue::as_object)
+        .context("vars map")?;
+    // Iteration vars override colliding entry vars while unrelated vars survive.
+    anyhow::ensure!(
+        vars.get("item").and_then(ManifestValue::as_str) == Some("from-iteration"),
+        "iteration item should override the entry's own item var: {vars:?}"
+    );
+    anyhow::ensure!(
+        vars.get("other").and_then(ManifestValue::as_str) == Some("untouched"),
+        "unrelated entry vars should survive expansion: {vars:?}"
+    );
+    Ok(())
+}
+
+#[test]
+fn expand_foreach_jinja_filter_in_name() -> Result<()> {
+    // Name rendering happens in `render_manifest`, so drive the full parsing pipeline.
+    let manifest = crate::manifest::from_str(
+        "netsuke_version: \"1.0.0\"
+targets:
+  - name: '{{ item | upper }}'
+    foreach:
+      - alpha
+      - beta
+    command: echo hi",
+    )?;
+    let names: Vec<&str> = manifest
+        .targets
+        .iter()
+        .map(|t| match &t.name {
+            crate::ast::StringOrList::String(s) => Ok(s.as_str()),
+            other => Err(anyhow::anyhow!("expected string name, got {other:?}")),
+        })
+        .collect::<Result<_>>()?;
+    anyhow::ensure!(
+        names == ["ALPHA", "BETA"],
+        "expected uppercased names from Jinja filter: {names:?}"
+    );
     Ok(())
 }
 

src/manifest/expand_test_cases/foreach_property_cases.rs

@@ -0,0 +1,150 @@
+//! Generated invariants for manifest foreach expansion.
+//!
+//! The fixed cases pin specific regressions, while these properties vary
+//! sequence values, filtering, variable collisions, and source key order.
+
+use super::*;
+use minijinja::Environment;
+use proptest::prelude::*;
+
+proptest! {
+    /// Expansion preserves a leading non-object entry and expands each value.
+    #[test]
+    fn foreach_expands_generated_sequence_values(values in proptest::collection::vec(-100_i16..100, 1..8)) {
+        let env = Environment::new();
+        let yaml = format!(
+            "targets:\n  - bare-string\n  - name: generated\n    foreach: {values:?}\n    command: echo {{{{ item }}}}"
+        );
+        let mut doc: ManifestValue = serde_saphyr::from_str(&yaml)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        expand_foreach(&mut doc, &env)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let expanded = targets(&doc).map_err(|error| TestCaseError::fail(error.to_string()))?;
+
+        prop_assert_eq!(expanded.len(), values.len() + 1);
+        prop_assert_eq!(expanded.first().and_then(ManifestValue::as_str), Some("bare-string"));
+        for (index, (entry, value)) in expanded.iter().skip(1).zip(&values).enumerate() {
+            let map = entry.as_object().ok_or_else(|| {
+                TestCaseError::fail(format!("expanded entry {index} should be an object: {entry:?}"))
+            })?;
+            prop_assert!(!map.contains_key("foreach"));
+            prop_assert_eq!(map.get("name").and_then(ManifestValue::as_str), Some("generated"));
+            let vars = map.get("vars").and_then(ManifestValue::as_object).ok_or_else(|| {
+                TestCaseError::fail(format!("expanded entry {index} should contain vars: {map:?}"))
+            })?;
+            prop_assert_eq!(vars.get("item").and_then(ManifestValue::as_i64), Some(i64::from(*value)));
+            prop_assert_eq!(vars.get("index").and_then(ManifestValue::as_u64), Some(index as u64));
+        }
+    }
+
+    /// Iteration values override colliding entry vars while other vars survive.
+    #[test]
+    fn foreach_generated_iteration_values_override_entry_vars(
+        values in proptest::collection::vec("[a-z]{1,6}", 1..8),
+        entry_item in "[a-z]{1,6}",
+        other_value in "[a-z]{1,6}",
+    ) {
+        let env = Environment::new();
+        let foreach = serde_json::to_string(&values)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let entry_item_json = serde_json::to_string(&entry_item)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let other = serde_json::to_string(&other_value)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let yaml = format!(
+            "targets:\n  - name: variables\n    foreach: {foreach}\n    vars:\n      item: {entry_item_json}\n      other: {other}"
+        );
+        let mut doc: ManifestValue = serde_saphyr::from_str(&yaml)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        expand_foreach(&mut doc, &env)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let expanded = targets(&doc).map_err(|error| TestCaseError::fail(error.to_string()))?;
+
+        prop_assert_eq!(expanded.len(), values.len());
+        for (index, (entry, value)) in expanded.iter().zip(&values).enumerate() {
+            let map = entry.as_object().ok_or_else(|| {
+                TestCaseError::fail(format!("expanded entry {index} should be an object: {entry:?}"))
+            })?;
+            let vars = map.get("vars").and_then(ManifestValue::as_object).ok_or_else(|| {
+                TestCaseError::fail(format!("expanded entry {index} should contain vars: {map:?}"))
+            })?;
+            prop_assert_eq!(vars.get("item").and_then(ManifestValue::as_str), Some(value.as_str()));
+            prop_assert_eq!(vars.get("other").and_then(ManifestValue::as_str), Some(other_value.as_str()));
+            prop_assert_eq!(vars.get("index").and_then(ManifestValue::as_u64), Some(index as u64));
+        }
+    }
+
+    /// Filtering preserves original indexes for every generated input sequence.
+    #[test]
+    fn foreach_filters_generated_sequence_values(
+        values in proptest::collection::vec(-20_i16..21, 0..8),
+        threshold in -20_i16..21,
+    ) {
+        let env = Environment::new();
+        let yaml = format!(
+            "targets:\n  - name: filtered\n    foreach: {values:?}\n    when: 'item > {threshold}'"
+        );
+        let mut doc: ManifestValue = serde_saphyr::from_str(&yaml)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        expand_foreach(&mut doc, &env)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let expanded = targets(&doc).map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let expected: Vec<_> = values
+            .iter()
+            .enumerate()
+            .filter(|(_, value)| **value > threshold)
+            .collect();
+
+        prop_assert_eq!(expanded.len(), expected.len());
+        for (entry, (index, value)) in expanded.iter().zip(expected) {
+            let map = entry.as_object().ok_or_else(|| {
+                TestCaseError::fail(format!("filtered entry should be an object: {entry:?}"))
+            })?;
+            prop_assert!(!map.contains_key("foreach"));
+            let vars = map.get("vars").and_then(ManifestValue::as_object).ok_or_else(|| {
+                TestCaseError::fail(format!("filtered entry should contain vars: {map:?}"))
+            })?;
+            prop_assert_eq!(vars.get("item").and_then(ManifestValue::as_i64), Some(i64::from(*value)));
+            prop_assert_eq!(vars.get("index").and_then(ManifestValue::as_u64), Some(index as u64));
+        }
+    }
+
+    /// Expansion removes `foreach` without reordering user-specified map keys.
+    #[test]
+    fn foreach_preserves_generated_source_key_order(order in prop_oneof![
+        Just(vec!["name", "vars", "after"]),
+        Just(vec!["name", "after", "vars"]),
+        Just(vec!["vars", "name", "after"]),
+        Just(vec!["vars", "after", "name"]),
+        Just(vec!["after", "name", "vars"]),
+        Just(vec!["after", "vars", "name"]),
+    ]) {
+        let env = Environment::new();
+        let mut yaml = String::from("targets:\n");
+        for (index, key) in order.iter().enumerate() {
+            yaml.push_str(if index == 0 { "  - " } else { "    " });
+            match *key {
+                "name" => yaml.push_str("name: ordered\n"),
+                "vars" => yaml.push_str("vars:\n      static: keep\n"),
+                "after" => yaml.push_str("after: done\n"),
+                _ => return Err(TestCaseError::fail("property strategy produced an unknown key".to_owned())),
+            }
+        }
+        yaml.push_str("    foreach: [value]");
+        let mut doc: ManifestValue = serde_saphyr::from_str(&yaml)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        expand_foreach(&mut doc, &env)
+            .map_err(|error| TestCaseError::fail(error.to_string()))?;
+        let expanded = targets(&doc).map_err(|error| TestCaseError::fail(error.to_string()))?;
+
+        prop_assert_eq!(expanded.len(), 1);
+        let entry = expanded.first().ok_or_else(|| {
+            TestCaseError::fail("expected one expanded target after length check".to_owned())
+        })?;
+        let map = entry.as_object().ok_or_else(|| {
+            TestCaseError::fail(format!("expanded target should be an object: {entry:?}"))
+        })?;
+        let keys: Vec<_> = map.keys().map(String::as_str).collect();
+        prop_assert_eq!(keys, order);
+    }
+}

src/manifest/expand_test_cases/structure_cases.rs

@@ -0,0 +1,34 @@
+//! Structural preservation cases for manifest foreach expansion.
+
+use super::*;
+use anyhow::{Context, Result};
+use minijinja::Environment;
+
+#[test]
+fn expand_foreach_preserves_object_key_order() -> Result<()> {
+    let env = Environment::new();
+    let yaml = r"targets:
+  - name: literal
+    vars:
+      existing: keep
+    foreach:
+      - 1
+      - 2
+    when: 'true'
+    after: done
+";
+    let mut doc: ManifestValue = serde_saphyr::from_str(yaml)?;
+    expand_foreach(&mut doc, &env)?;
+    let targets = targets(&doc)?;
+    anyhow::ensure!(targets.len() == 2, "expected expanded targets");
+    for target in targets {
+        let map = target.as_object().context("target object")?;
+        let keys: Vec<&str> = map.keys().map(String::as_str).collect();
+        anyhow::ensure!(
+            keys == ["name", "vars", "after"],
+            "key order should remain stable: {:?}",
+            keys
+        );
+    }
+    Ok(())
+}