diff --git a/src/manifest/expand_test_cases/action_condition_cases.rs b/src/manifest/expand_test_cases/action_condition_cases.rs index e851ae816..6a811b14a 100644 --- a/src/manifest/expand_test_cases/action_condition_cases.rs +++ b/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(()) } diff --git a/src/manifest/expand_test_cases/condition_cases.rs b/src/manifest/expand_test_cases/condition_cases.rs index 2d826f032..051caf597 100644 --- a/src/manifest/expand_test_cases/condition_cases.rs +++ b/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::>()?; + anyhow::ensure!( + names == ["ALPHA", "BETA"], + "expected uppercased names from Jinja filter: {names:?}" + ); Ok(()) } diff --git a/src/manifest/expand_test_cases/foreach_property_cases.rs b/src/manifest/expand_test_cases/foreach_property_cases.rs new file mode 100644 index 000000000..73ba90a6d --- /dev/null +++ b/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); + } +} diff --git a/src/manifest/expand_test_cases/structure_cases.rs b/src/manifest/expand_test_cases/structure_cases.rs new file mode 100644 index 000000000..164d76009 --- /dev/null +++ b/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(()) +} diff --git a/src/manifest/expand_tests.rs b/src/manifest/expand_tests.rs index edacb57ad..016d58495 100644 --- a/src/manifest/expand_tests.rs +++ b/src/manifest/expand_tests.rs @@ -10,6 +10,12 @@ mod action_condition_cases; #[path = "expand_test_cases/condition_cases.rs"] mod condition_cases; +#[path = "expand_test_cases/foreach_property_cases.rs"] +mod foreach_property_cases; + +#[path = "expand_test_cases/structure_cases.rs"] +mod structure_cases; + #[path = "expand_test_cases/property_cases.rs"] mod property_cases; #[path = "expand_test_cases/target_command_available_cases.rs"] @@ -29,6 +35,18 @@ pub(super) fn actions(doc: &ManifestValue) -> Result<&[ManifestValue]> { .context("actions sequence missing") } +pub(super) fn ensure_foreach_removed(entries: &[ManifestValue], section: &str) -> Result<()> { + for entry in entries { + let map = entry + .as_object() + .with_context(|| format!("{section} entry map"))?; + anyhow::ensure!( + !map.contains_key("foreach"), + "foreach should be removed after {section} expansion" + ); + } + Ok(()) +} pub(super) fn section_entries<'a>( doc: &'a ManifestValue, section: &str,