From e0cc369d6034b8f873941c211ecab888c9e89fe9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 1 Aug 2025 12:44:40 +0100 Subject: [PATCH 1/4] Add IR generation BDD scenarios --- tests/features/ir_generation.feature | 43 ++++++++++++++++++++++++++++ tests/steps/ir_steps.rs | 9 +++++- tests/steps/manifest_steps.rs | 16 +++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 tests/features/ir_generation.feature diff --git a/tests/features/ir_generation.feature b/tests/features/ir_generation.feature new file mode 100644 index 00000000..7b731453 --- /dev/null +++ b/tests/features/ir_generation.feature @@ -0,0 +1,43 @@ +Feature: Intermediate Representation (IR) Generation + As a developer, + I want to compile a manifest into a valid build graph, + So that I can detect configuration errors before execution. + + Scenario: A new, empty BuildGraph has no content + Given a new BuildGraph is created + When its contents are checked + Then the graph has 0 actions + And the graph has 0 targets + And the graph has 0 default targets + + Scenario: Compiling a valid manifest with one rule and one target + Given the manifest file "tests/data/rules.yml" is compiled to IR + When the graph contents are checked + Then the graph has 1 actions + And the graph has 1 targets + + Scenario: Identical rules are deduplicated during IR generation + Given the manifest file "tests/data/duplicate_rules.yml" is compiled to IR + When the graph contents are checked + Then the graph has 1 actions + And the graph has 2 targets + + Scenario: IR generation fails if a target references a rule that does not exist + Given the manifest file "tests/data/missing_rule.yml" is compiled to IR + When the generation result is checked + Then IR generation fails + + Scenario: IR generation fails if a target specifies multiple rules + Given the manifest file "tests/data/multiple_rules_per_target.yml" is compiled to IR + When the generation result is checked + Then IR generation fails + + Scenario: IR generation fails if multiple targets produce the same output file + Given the manifest file "tests/data/duplicate_outputs.yml" is compiled to IR + When the generation result is checked + Then IR generation fails + + Scenario: IR generation fails if there is a circular dependency between targets + Given the manifest file "tests/data/circular.yml" is compiled to IR + When the generation result is checked + Then IR generation fails diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index 53533182..f1700996 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -1,9 +1,10 @@ //! Step definitions for `BuildGraph` scenarios. use crate::CliWorld; -use cucumber::{then, when}; +use cucumber::{given, then, when}; use netsuke::ir::BuildGraph; +#[given("a new BuildGraph is created")] #[when("a new BuildGraph is created")] fn create_graph(world: &mut CliWorld) { world.build_graph = Some(BuildGraph::default()); @@ -31,6 +32,7 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )] +#[given(expr = "the manifest file {string} is compiled to IR")] #[when(expr = "the manifest file {string} is compiled to IR")] fn compile_manifest(world: &mut CliWorld, path: String) { match netsuke::manifest::from_path(&path) @@ -47,6 +49,11 @@ fn compile_manifest(world: &mut CliWorld, path: String) { } } +#[when("its contents are checked")] +fn graph_checked(world: &mut CliWorld) { + assert!(world.build_graph.is_some() || world.manifest_error.is_some()); +} + #[then("IR generation fails")] fn ir_generation_fails(world: &mut CliWorld) { assert!( diff --git a/tests/steps/manifest_steps.rs b/tests/steps/manifest_steps.rs index 686b090f..1be26d76 100644 --- a/tests/steps/manifest_steps.rs +++ b/tests/steps/manifest_steps.rs @@ -34,6 +34,20 @@ fn assert_parsed(world: &CliWorld) { "manifest should have been parsed" ); } + +fn assert_graph(world: &CliWorld) { + assert!( + world.build_graph.is_some(), + "build graph should have been generated", + ); +} + +fn assert_generation(world: &CliWorld) { + assert!( + world.build_graph.is_some() || world.manifest_error.is_some(), + "IR generation should have been attempted", + ); +} #[given(expr = "the manifest file {string} is parsed")] fn given_parse_manifest(world: &mut CliWorld, path: String) { parse_manifest_inner(world, &path); @@ -49,6 +63,8 @@ fn when_item_checked(world: &mut CliWorld, item: String) { match item.as_str() { "parsing result" => assert_parsed(world), "manifest" | "version" | "flags" | "rules" => assert_manifest(world), + "graph contents" => assert_graph(world), + "generation result" => assert_generation(world), unexpected => panic!("Unexpected item checked: '{unexpected}'"), } } From 77fd73c1e9170bff3e22f98e987ecdc7666b38be Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 1 Aug 2025 14:08:33 +0100 Subject: [PATCH 2/4] Refactor IR steps --- tests/steps/ir_steps.rs | 27 +++++++++++++++++++++++++-- tests/steps/manifest_steps.rs | 17 +---------------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index f1700996..60c5e556 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -4,8 +4,21 @@ use crate::CliWorld; use cucumber::{given, then, when}; use netsuke::ir::BuildGraph; +fn assert_graph(world: &CliWorld) { + assert!( + world.build_graph.is_some(), + "build graph should have been generated", + ); +} + +fn assert_generation_attempted(world: &CliWorld) { + assert!( + world.build_graph.is_some() || world.manifest_error.is_some(), + "IR generation should have been attempted", + ); +} + #[given("a new BuildGraph is created")] -#[when("a new BuildGraph is created")] fn create_graph(world: &mut CliWorld) { world.build_graph = Some(BuildGraph::default()); } @@ -51,7 +64,17 @@ fn compile_manifest(world: &mut CliWorld, path: String) { #[when("its contents are checked")] fn graph_checked(world: &mut CliWorld) { - assert!(world.build_graph.is_some() || world.manifest_error.is_some()); + assert_graph(world); +} + +#[when("the graph contents are checked")] +fn graph_contents_checked(world: &mut CliWorld) { + assert_graph(world); +} + +#[when("the generation result is checked")] +fn generation_result_checked(world: &mut CliWorld) { + assert_generation_attempted(world); } #[then("IR generation fails")] diff --git a/tests/steps/manifest_steps.rs b/tests/steps/manifest_steps.rs index 1be26d76..1494ebd2 100644 --- a/tests/steps/manifest_steps.rs +++ b/tests/steps/manifest_steps.rs @@ -35,19 +35,6 @@ fn assert_parsed(world: &CliWorld) { ); } -fn assert_graph(world: &CliWorld) { - assert!( - world.build_graph.is_some(), - "build graph should have been generated", - ); -} - -fn assert_generation(world: &CliWorld) { - assert!( - world.build_graph.is_some() || world.manifest_error.is_some(), - "IR generation should have been attempted", - ); -} #[given(expr = "the manifest file {string} is parsed")] fn given_parse_manifest(world: &mut CliWorld, path: String) { parse_manifest_inner(world, &path); @@ -58,13 +45,11 @@ fn parse_manifest(world: &mut CliWorld, path: String) { parse_manifest_inner(world, &path); } -#[when(regex = r"^the (?P[a-z ]+) (?:is|are) checked$")] +#[when(regex = r"^the (?Pparsing result|manifest|version|flags|rules) (?:is|are) checked$")] fn when_item_checked(world: &mut CliWorld, item: String) { match item.as_str() { "parsing result" => assert_parsed(world), "manifest" | "version" | "flags" | "rules" => assert_manifest(world), - "graph contents" => assert_graph(world), - "generation result" => assert_generation(world), unexpected => panic!("Unexpected item checked: '{unexpected}'"), } } From 346fcabe204576e7de8fbdfe5a59d87960f762cf Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 1 Aug 2025 23:20:06 +0100 Subject: [PATCH 3/4] Refine IR generation checks --- src/ir.rs | 24 ++++++++++++------------ tests/steps/ir_steps.rs | 9 +++++---- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/ir.rs b/src/ir.rs index ca6bba19..cf5fbbf6 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -322,10 +322,10 @@ fn find_cycle(targets: &HashMap) -> Option> { stack.push(node.clone()); - if let Some(edge) = targets.get(node) - && let Some(cycle) = visit_dependencies(targets, &edge.inputs, stack, states) - { - return Some(cycle); + if let Some(edge) = targets.get(node) { + if let Some(cycle) = visit_dependencies(targets, &edge.inputs, stack, states) { + return Some(cycle); + } } stack.pop(); @@ -340,10 +340,10 @@ fn find_cycle(targets: &HashMap) -> Option> { states: &mut HashMap, ) -> Option> { for dep in deps { - if targets.contains_key(dep) - && let Some(cycle) = visit(targets, dep, stack, states) - { - return Some(cycle); + if targets.contains_key(dep) { + if let Some(cycle) = visit(targets, dep, stack, states) { + return Some(cycle); + } } } None @@ -353,10 +353,10 @@ fn find_cycle(targets: &HashMap) -> Option> { let mut stack = Vec::new(); for node in targets.keys() { - if !states.contains_key(node) - && let Some(cycle) = visit(targets, node, &mut stack, &mut states) - { - return Some(cycle); + if !states.contains_key(node) { + if let Some(cycle) = visit(targets, node, &mut stack, &mut states) { + return Some(cycle); + } } } None diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index 60c5e556..a74d3ce9 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -12,10 +12,11 @@ fn assert_graph(world: &CliWorld) { } fn assert_generation_attempted(world: &CliWorld) { - assert!( - world.build_graph.is_some() || world.manifest_error.is_some(), - "IR generation should have been attempted", - ); + match (world.build_graph.is_some(), world.manifest_error.is_some()) { + (true, false) | (false, true) => (), + (true, true) => panic!("unexpected: graph and error present"), + (false, false) => panic!("IR generation not attempted"), + } } #[given("a new BuildGraph is created")] From 484ec3b94480602a768604ace66082b31cdfa7d7 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 1 Aug 2025 23:47:01 +0100 Subject: [PATCH 4/4] Refine cycle detection conditionals --- src/ir.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/ir.rs b/src/ir.rs index cf5fbbf6..9fb84c35 100644 --- a/src/ir.rs +++ b/src/ir.rs @@ -323,7 +323,8 @@ fn find_cycle(targets: &HashMap) -> Option> { stack.push(node.clone()); if let Some(edge) = targets.get(node) { - if let Some(cycle) = visit_dependencies(targets, &edge.inputs, stack, states) { + let deps_result = visit_dependencies(targets, &edge.inputs, stack, states); + if let Some(cycle) = deps_result { return Some(cycle); } } @@ -341,7 +342,8 @@ fn find_cycle(targets: &HashMap) -> Option> { ) -> Option> { for dep in deps { if targets.contains_key(dep) { - if let Some(cycle) = visit(targets, dep, stack, states) { + let visit_result = visit(targets, dep, stack, states); + if let Some(cycle) = visit_result { return Some(cycle); } } @@ -353,10 +355,11 @@ fn find_cycle(targets: &HashMap) -> Option> { let mut stack = Vec::new(); for node in targets.keys() { - if !states.contains_key(node) { - if let Some(cycle) = visit(targets, node, &mut stack, &mut states) { - return Some(cycle); - } + if states.contains_key(node) { + continue; + } + if let Some(cycle) = visit(targets, node, &mut stack, &mut states) { + return Some(cycle); } } None