From 7f07ed5c85e2c0775aa28ac9bbb0ed7b1d457917 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 12 Dec 2025 03:12:19 +0000 Subject: [PATCH 1/8] feat(runner): add `graph` subcommand to invoke ninja graph tool Implemented the `graph` command, which generates the Ninja manifest into a temporary file and runs `ninja -t graph` to output the build dependency graph in DOT format. Updated CLI help and documentation to reflect this new command. Added comprehensive tests to verify the behavior of the graph subcommand under various conditions. This provides a useful introspection tool for users to visualize and debug build dependencies using Ninja's graph functionality. Co-authored-by: terragon-labs[bot] --- docs/netsuke-design.md | 13 ++++---- docs/roadmap.md | 2 +- docs/users-guide.md | 6 ++-- src/runner/mod.rs | 31 +++++++++++++++--- tests/features/graph.feature | 28 ++++++++++++++++ tests/runner_tests.rs | 63 ++++++++++++++++++++++++++++++++++++ tests/steps/process_steps.rs | 38 ++++++++++++++++++++++ 7 files changed, 166 insertions(+), 15 deletions(-) create mode 100644 tests/features/graph.feature diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 475e5a13..b2dd6ad3 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1997,13 +1997,12 @@ the targets listed in the `defaults` section of the manifest are built. directory. It will invoke the Ninja backend with the appropriate flags, such as `ninja -t clean`, to remove the outputs of the build rules. -- `Netsuke graph`: This command is an introspection and debugging tool. It will - run the Netsuke pipeline up to Stage 4 (IR Generation) and then invoke Ninja - with the graph tool, `ninja -t graph`. This outputs the complete build - dependency graph in the DOT language. The result can be piped through - `dot -Tsvg` or displayed via `netsuke graph --html` using an embedded - Dagre.js viewer. Visualizing the graph is invaluable for understanding and - debugging complex projects. +- `Netsuke graph`: This command is an introspection and debugging tool. It + runs the Netsuke pipeline through Ninja synthesis (Stage 6) to produce a + temporary `build.ninja`, then invokes Ninja with the graph tool, + `ninja -t graph`, which outputs the complete build dependency graph in the + DOT language. The result can be piped through Graphviz tools such as + `dot -Tsvg`. An optional `--html` renderer is planned for a later milestone. - `Netsuke manifest FILE`: This command performs the pipeline up to Ninja synthesis and writes the resulting Ninja file to `FILE` without invoking diff --git a/docs/roadmap.md b/docs/roadmap.md index eac2d53c..6d4b6d3a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -181,7 +181,7 @@ library, and CLI ergonomics. - [x] Implement the `clean` subcommand by invoking `ninja -t clean`. - - [ ] Implement the graph subcommand by invoking ninja -t graph to output + - [x] Implement the graph subcommand by invoking ninja -t graph to output a DOT representation of the dependency graph. - [ ] Refine all CLI output for clarity, ensuring help messages are diff --git a/docs/users-guide.md b/docs/users-guide.md index 0da3388e..fdbe3c8e 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -497,9 +497,9 @@ netsuke [OPTIONS] [COMMAND] [TARGETS...] rules/targets to be properly configured for cleaning in Ninja (often via `phony` targets). -- `graph`: Generates the build dependency graph and outputs it in DOT format - (suitable for Graphviz). Future versions may support other formats like - `--html`. +- `graph`: Generates the build dependency graph by running `ninja -t graph` on + the generated `build.ninja`, outputting DOT to stdout (suitable for + Graphviz). Future versions may support other formats like `--html`. ### Exit Codes diff --git a/src/runner/mod.rs b/src/runner/mod.rs index f084a9c9..c6752c15 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -95,10 +95,7 @@ pub fn run(cli: &Cli) -> Result<()> { Ok(()) } Commands::Clean => handle_clean(cli), - Commands::Graph => { - info!(target: "netsuke::subcommand", subcommand = "graph", "Subcommand requested"); - Ok(()) - } + Commands::Graph => handle_graph(cli), } } @@ -182,6 +179,32 @@ fn handle_clean(cli: &Cli) -> Result<()> { Ok(()) } +/// Display build dependency graph by invoking `ninja -t graph`. +/// +/// Generates the Ninja manifest to a temporary file, then invokes Ninja's graph +/// tool to emit a DOT representation to stdout. +/// +/// # Errors +/// +/// Returns an error if manifest generation or Ninja execution fails. +fn handle_graph(cli: &Cli) -> Result<()> { + info!(target: "netsuke::subcommand", subcommand = "graph", "Subcommand requested"); + let ninja = generate_ninja(cli)?; + + let tmp = process::create_temp_ninja_file(&ninja)?; + let build_path = tmp.path(); + + let program = process::resolve_ninja_program(); + run_ninja_tool(program.as_path(), cli, build_path, "graph").with_context(|| { + format!( + "running {} -t graph with build file {}", + program.display(), + build_path.display() + ) + })?; + Ok(()) +} + /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. /// /// # Errors diff --git a/tests/features/graph.feature b/tests/features/graph.feature new file mode 100644 index 00000000..285bf839 --- /dev/null +++ b/tests/features/graph.feature @@ -0,0 +1,28 @@ +Feature: Graph subcommand execution + + Scenario: Graph invokes ninja with tool flag + Given a fake ninja executable that expects the graph tool + And the CLI is parsed with "graph" + And the CLI uses the temporary directory + When the graph process is run + Then the command should succeed + + Scenario: Graph fails when ninja fails + Given a fake ninja executable that exits with 1 + And the CLI is parsed with "graph" + And the CLI uses the temporary directory + When the graph process is run + Then the command should fail with error "ninja exited" + + Scenario: Graph respects jobs flag + Given a fake ninja executable that expects graph with 4 jobs + And the CLI is parsed with "-j 4 graph" + And the CLI uses the temporary directory + When the graph process is run + Then the command should succeed + + Scenario: Graph fails when ninja is missing + Given no ninja executable is available + And the CLI is parsed with "graph" + When the graph process is run + Then the command should fail with error "No such file or directory" diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 39b04a02..431845f9 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -377,3 +377,66 @@ fn run_clean_fails_with_invalid_manifest() -> Result<()> { ); Ok(()) } + +// --- Graph subcommand tests --- + +/// Fixture: point `NINJA_ENV` at a fake `ninja` that expects `-t graph`. +/// +/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) +#[fixture] +fn ninja_expecting_graph() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { + let (ninja_dir, ninja_path) = check_ninja::fake_ninja_expect_tool(ToolName::new("graph"))?; + let env = SystemEnv::new(); + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, ninja_path, guard)) +} + +#[cfg(unix)] +#[rstest] +fn run_graph_subcommand_succeeds() -> Result<()> { + let (_ninja_dir, _ninja_path, _guard) = ninja_expecting_graph()?; + let (temp, manifest_path) = create_test_manifest()?; + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + command: Some(Commands::Graph), + ..Cli::default() + }; + + run(&cli).context("expected graph subcommand to succeed")?; + + ensure!( + !temp.path().join("build.ninja").exists(), + "graph subcommand should not leave build.ninja in project directory" + ); + Ok(()) +} + +#[cfg(unix)] +#[rstest] +fn run_graph_fails_with_failing_ninja() -> Result<()> { + assert_ninja_failure_propagates(Some(Commands::Graph)) +} + +#[cfg(unix)] +#[rstest] +fn run_graph_fails_with_invalid_manifest() -> Result<()> { + let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/invalid_version.yml", &manifest_path) + .with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?; + let cli = Cli { + file: manifest_path.clone(), + command: Some(Commands::Graph), + ..Cli::default() + }; + + let Err(err) = run(&cli) else { + bail!("expected graph to fail for invalid manifest"); + }; + ensure!( + err.to_string().contains("loading manifest at"), + "error should mention manifest loading, got: {err}" + ); + Ok(()) +} diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index 5a1e584c..5bf8aff1 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -81,6 +81,25 @@ fn fake_ninja_expects_clean_with_jobs(world: &mut CliWorld, jobs: u32) -> Result install_test_ninja(&env, world, dir, path) } +/// Creates a fake ninja executable that expects the `-t graph` tool invocation. +#[cfg(unix)] +#[given("a fake ninja executable that expects the graph tool")] +fn fake_ninja_expects_graph(world: &mut CliWorld) -> Result<()> { + let (dir, path) = check_ninja::fake_ninja_expect_tool(ToolName::new("graph"))?; + let env = env::mocked_path_env(); + install_test_ninja(&env, world, dir, path) +} + +/// Creates a fake ninja executable that expects `-t graph` and `-j `. +#[cfg(unix)] +#[given(expr = "a fake ninja executable that expects graph with {int} jobs")] +fn fake_ninja_expects_graph_with_jobs(world: &mut CliWorld, jobs: u32) -> Result<()> { + let (dir, path) = + check_ninja::fake_ninja_expect_tool_with_jobs(ToolName::new("graph"), Some(jobs), None)?; + let env = env::mocked_path_env(); + install_test_ninja(&env, world, dir, path) +} + /// Sets up a scenario where no ninja executable is available. /// /// This step creates a temporary directory and records the path to a @@ -219,6 +238,25 @@ fn run_clean(world: &mut CliWorld) -> Result<()> { Ok(()) } +/// Executes the graph subcommand and captures the result in the test world. +/// +/// This step runs the full `runner::run` function with the Graph command, +/// ensuring the manifest exists first and updating the world's `run_status` +/// and `run_error` fields based on the execution outcome. +#[cfg(unix)] +#[when("the graph process is run")] +fn run_graph(world: &mut CliWorld) -> Result<()> { + prepare_cli_with_absolute_file(world)?; + let cli = world + .cli + .as_ref() + .context("CLI configuration has not been initialised")?; + // Use alternate formatting to capture the full anyhow error chain. + let result = runner::run(cli).map_err(|e| format!("{e:#}")); + record_result(world, result); + Ok(()) +} + /// Asserts that the command succeeds. #[then("the command should succeed")] fn command_should_succeed(world: &mut CliWorld) -> Result<()> { From fbfe8558ed83c58eac24671d400facf4512d2db3 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 12 Dec 2025 12:37:29 +0000 Subject: [PATCH 2/8] refactor(runner): unify Ninja tool subcommand handling and improve tests Refactored the Ninja tool invocations by introducing handle_ninja_tool to handle generic Ninja tools via temporary build files. Replaced specialized handle_clean and handle_graph implementations to use this unified handler. Updated tests to use common helpers for validating subcommand success and failure cases without leaving temporary build files. Refactored Cucumber test steps to consolidate subcommand runs through a shared function. This improves code reuse, clarity, and maintainability. Co-authored-by: terragon-labs[bot] --- src/runner/mod.rs | 44 ++++++------ tests/runner_tests.rs | 125 ++++++++++++++++------------------- tests/steps/process_steps.rs | 29 ++++---- 3 files changed, 92 insertions(+), 106 deletions(-) diff --git a/src/runner/mod.rs b/src/runner/mod.rs index c6752c15..73b90e99 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -153,32 +153,46 @@ fn handle_build(cli: &Cli, args: &BuildArgs) -> Result<()> { Ok(()) } -/// Remove build artefacts by invoking `ninja -t clean`. +/// Execute a Ninja tool (e.g., `ninja -t clean`) using a temporary build file. /// -/// Generates the Ninja manifest to a temporary file, then invokes Ninja's clean -/// tool to remove all outputs defined by the build graph. +/// Generates the Ninja manifest to a temporary file, then invokes Ninja with +/// `-t ` while preserving the CLI settings (working directory and job +/// count). /// /// # Errors /// /// Returns an error if manifest generation or Ninja execution fails. -fn handle_clean(cli: &Cli) -> Result<()> { - info!(target: "netsuke::subcommand", subcommand = "clean", "Subcommand requested"); +fn handle_ninja_tool(cli: &Cli, tool: &str) -> Result<()> { + info!(target: "netsuke::subcommand", subcommand = tool, "Subcommand requested"); let ninja = generate_ninja(cli)?; let tmp = process::create_temp_ninja_file(&ninja)?; let build_path = tmp.path(); let program = process::resolve_ninja_program(); - run_ninja_tool(program.as_path(), cli, build_path, "clean").with_context(|| { + run_ninja_tool(program.as_path(), cli, build_path, tool).with_context(|| { format!( - "running {} -t clean with build file {}", + "running {} -t {} with build file {}", program.display(), + tool, build_path.display() ) })?; Ok(()) } +/// Remove build artefacts by invoking `ninja -t clean`. +/// +/// Generates the Ninja manifest to a temporary file, then invokes Ninja's clean +/// tool to remove all outputs defined by the build graph. +/// +/// # Errors +/// +/// Returns an error if manifest generation or Ninja execution fails. +fn handle_clean(cli: &Cli) -> Result<()> { + handle_ninja_tool(cli, "clean") +} + /// Display build dependency graph by invoking `ninja -t graph`. /// /// Generates the Ninja manifest to a temporary file, then invokes Ninja's graph @@ -188,21 +202,7 @@ fn handle_clean(cli: &Cli) -> Result<()> { /// /// Returns an error if manifest generation or Ninja execution fails. fn handle_graph(cli: &Cli) -> Result<()> { - info!(target: "netsuke::subcommand", subcommand = "graph", "Subcommand requested"); - let ninja = generate_ninja(cli)?; - - let tmp = process::create_temp_ninja_file(&ninja)?; - let build_path = tmp.path(); - - let program = process::resolve_ninja_program(); - run_ninja_tool(program.as_path(), cli, build_path, "graph").with_context(|| { - format!( - "running {} -t graph with build file {}", - program.display(), - build_path.display() - ) - })?; - Ok(()) + handle_ninja_tool(cli, "graph") } /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 431845f9..cf86eb39 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -132,6 +132,59 @@ where Ok(()) } +fn assert_subcommand_succeeds_without_persisting_file( + fixture: Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>, + command: Commands, +) -> Result<()> { + let name = match command { + Commands::Clean => "clean", + Commands::Graph => "graph", + other => bail!("unsupported subcommand for test helper: {other:?}"), + }; + let (_ninja_dir, _ninja_path, _guard) = fixture?; + let (temp, manifest_path) = create_test_manifest()?; + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + command: Some(command), + ..Cli::default() + }; + + run(&cli).with_context(|| format!("expected {name} subcommand to succeed"))?; + + ensure!( + !temp.path().join("build.ninja").exists(), + "{name} subcommand should not leave build.ninja in project directory" + ); + Ok(()) +} + +fn assert_subcommand_fails_with_invalid_manifest(command: Commands) -> Result<()> { + let name = match command { + Commands::Clean => "clean", + Commands::Graph => "graph", + other => bail!("unsupported subcommand for test helper: {other:?}"), + }; + let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/invalid_version.yml", &manifest_path) + .with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?; + let cli = Cli { + file: manifest_path.clone(), + command: Some(command), + ..Cli::default() + }; + + let Err(err) = run(&cli) else { + bail!("expected {name} to fail for invalid manifest"); + }; + ensure!( + err.to_string().contains("loading manifest at"), + "error should mention manifest loading, got: {err}" + ); + Ok(()) +} + #[rstest] fn run_ninja_not_found() -> Result<()> { assert_binary_not_found(|| { @@ -318,22 +371,7 @@ fn ninja_expecting_clean() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard) #[cfg(unix)] #[rstest] fn run_clean_subcommand_succeeds() -> Result<()> { - let (_ninja_dir, _ninja_path, _guard) = ninja_expecting_clean()?; - let (temp, manifest_path) = create_test_manifest()?; - let cli = Cli { - file: manifest_path.clone(), - directory: Some(temp.path().to_path_buf()), - command: Some(Commands::Clean), - ..Cli::default() - }; - - run(&cli).context("expected clean subcommand to succeed")?; - - ensure!( - !temp.path().join("build.ninja").exists(), - "clean subcommand should not leave build.ninja in project directory" - ); - Ok(()) + assert_subcommand_succeeds_without_persisting_file(ninja_expecting_clean(), Commands::Clean) } #[cfg(unix)] @@ -358,24 +396,7 @@ fn run_ninja_tool_not_found() -> Result<()> { #[cfg(unix)] #[rstest] fn run_clean_fails_with_invalid_manifest() -> Result<()> { - let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/invalid_version.yml", &manifest_path) - .with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?; - let cli = Cli { - file: manifest_path.clone(), - command: Some(Commands::Clean), - ..Cli::default() - }; - - let Err(err) = run(&cli) else { - bail!("expected clean to fail for invalid manifest"); - }; - ensure!( - err.to_string().contains("loading manifest at"), - "error should mention manifest loading, got: {err}" - ); - Ok(()) + assert_subcommand_fails_with_invalid_manifest(Commands::Clean) } // --- Graph subcommand tests --- @@ -394,22 +415,7 @@ fn ninja_expecting_graph() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard) #[cfg(unix)] #[rstest] fn run_graph_subcommand_succeeds() -> Result<()> { - let (_ninja_dir, _ninja_path, _guard) = ninja_expecting_graph()?; - let (temp, manifest_path) = create_test_manifest()?; - let cli = Cli { - file: manifest_path.clone(), - directory: Some(temp.path().to_path_buf()), - command: Some(Commands::Graph), - ..Cli::default() - }; - - run(&cli).context("expected graph subcommand to succeed")?; - - ensure!( - !temp.path().join("build.ninja").exists(), - "graph subcommand should not leave build.ninja in project directory" - ); - Ok(()) + assert_subcommand_succeeds_without_persisting_file(ninja_expecting_graph(), Commands::Graph) } #[cfg(unix)] @@ -421,22 +427,5 @@ fn run_graph_fails_with_failing_ninja() -> Result<()> { #[cfg(unix)] #[rstest] fn run_graph_fails_with_invalid_manifest() -> Result<()> { - let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/invalid_version.yml", &manifest_path) - .with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?; - let cli = Cli { - file: manifest_path.clone(), - command: Some(Commands::Graph), - ..Cli::default() - }; - - let Err(err) = run(&cli) else { - bail!("expected graph to fail for invalid manifest"); - }; - ensure!( - err.to_string().contains("loading manifest at"), - "error should mention manifest loading, got: {err}" - ); - Ok(()) + assert_subcommand_fails_with_invalid_manifest(Commands::Graph) } diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index 5bf8aff1..fb43ec93 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -219,14 +219,8 @@ fn run(world: &mut CliWorld) -> Result<()> { Ok(()) } -/// Executes the clean subcommand and captures the result in the test world. -/// -/// This step runs the full `runner::run` function with the Clean command, -/// ensuring the manifest exists first and updating the world's `run_status` -/// and `run_error` fields based on the execution outcome. #[cfg(unix)] -#[when("the clean process is run")] -fn run_clean(world: &mut CliWorld) -> Result<()> { +fn run_subcommand(world: &mut CliWorld) -> Result<()> { prepare_cli_with_absolute_file(world)?; let cli = world .cli @@ -238,6 +232,17 @@ fn run_clean(world: &mut CliWorld) -> Result<()> { Ok(()) } +/// Executes the clean subcommand and captures the result in the test world. +/// +/// This step runs the full `runner::run` function with the Clean command, +/// ensuring the manifest exists first and updating the world's `run_status` +/// and `run_error` fields based on the execution outcome. +#[cfg(unix)] +#[when("the clean process is run")] +fn run_clean(world: &mut CliWorld) -> Result<()> { + run_subcommand(world) +} + /// Executes the graph subcommand and captures the result in the test world. /// /// This step runs the full `runner::run` function with the Graph command, @@ -246,15 +251,7 @@ fn run_clean(world: &mut CliWorld) -> Result<()> { #[cfg(unix)] #[when("the graph process is run")] fn run_graph(world: &mut CliWorld) -> Result<()> { - prepare_cli_with_absolute_file(world)?; - let cli = world - .cli - .as_ref() - .context("CLI configuration has not been initialised")?; - // Use alternate formatting to capture the full anyhow error chain. - let result = runner::run(cli).map_err(|e| format!("{e:#}")); - record_result(world, result); - Ok(()) + run_subcommand(world) } /// Asserts that the command succeeds. From 9507355bb3ce2dd93125b816a9e26d429900c6f4 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 13 Dec 2025 20:59:40 +0000 Subject: [PATCH 3/8] refactor(tests): move feature tests to features_unix and add @unix tag Renamed tests/features/clean.feature, graph.feature, and ninja_process.feature to tests/features_unix/clean.feature, graph.feature, and ninja_process.feature respectively. Added @unix tag to each renamed feature file to specify Unix platform. Co-authored-by: terragon-labs[bot] --- docs/roadmap.md | 4 ++-- tests/{features => features_unix}/clean.feature | 1 + tests/{features => features_unix}/graph.feature | 1 + tests/{features => features_unix}/ninja_process.feature | 1 + 4 files changed, 5 insertions(+), 2 deletions(-) rename tests/{features => features_unix}/clean.feature (99%) rename tests/{features => features_unix}/graph.feature (99%) rename tests/{features => features_unix}/ninja_process.feature (99%) diff --git a/docs/roadmap.md b/docs/roadmap.md index 6d4b6d3a..d0ba6185 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -181,8 +181,8 @@ library, and CLI ergonomics. - [x] Implement the `clean` subcommand by invoking `ninja -t clean`. - - [x] Implement the graph subcommand by invoking ninja -t graph to output - a DOT representation of the dependency graph. + - [x] Implement the graph subcommand by invoking `ninja -t graph` to output a + DOT representation of the dependency graph. - [ ] Refine all CLI output for clarity, ensuring help messages are descriptive and command feedback is intuitive. diff --git a/tests/features/clean.feature b/tests/features_unix/clean.feature similarity index 99% rename from tests/features/clean.feature rename to tests/features_unix/clean.feature index 77f79521..79a448dd 100644 --- a/tests/features/clean.feature +++ b/tests/features_unix/clean.feature @@ -1,3 +1,4 @@ +@unix Feature: Clean subcommand execution Scenario: Clean invokes ninja with tool flag diff --git a/tests/features/graph.feature b/tests/features_unix/graph.feature similarity index 99% rename from tests/features/graph.feature rename to tests/features_unix/graph.feature index 285bf839..735d582a 100644 --- a/tests/features/graph.feature +++ b/tests/features_unix/graph.feature @@ -1,3 +1,4 @@ +@unix Feature: Graph subcommand execution Scenario: Graph invokes ninja with tool flag diff --git a/tests/features/ninja_process.feature b/tests/features_unix/ninja_process.feature similarity index 99% rename from tests/features/ninja_process.feature rename to tests/features_unix/ninja_process.feature index 4d4785f6..da591f90 100644 --- a/tests/features/ninja_process.feature +++ b/tests/features_unix/ninja_process.feature @@ -1,3 +1,4 @@ +@unix Feature: Ninja process execution Scenario: Ninja succeeds From 9fc4879e9839391d0e6831e020f15f7fb78119f6 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 13 Dec 2025 22:03:41 +0000 Subject: [PATCH 4/8] test: make cucumber ninja resolution deterministic - Document YAML 1.2 manifest constraints in the user guide. - Ensure Cucumber process steps override NINJA_ENV so missing-ninja scenarios are deterministic. - Collapse clean/graph subcommand tests into an rstest table to reduce duplication. --- docs/users-guide.md | 4 +++ test_support/src/env.rs | 6 ++++ test_support/src/env_lock.rs | 7 ++++ tests/cucumber.rs | 9 ++++- tests/runner_tests.rs | 65 ++++++++++++++++++------------------ tests/steps/process_steps.rs | 3 ++ 6 files changed, 61 insertions(+), 33 deletions(-) diff --git a/docs/users-guide.md b/docs/users-guide.md index fdbe3c8e..b53c27fb 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -65,6 +65,10 @@ guiding you. The `Netsukefile` is a YAML file describing your build process. +Netsuke targets YAML 1.2 and forbids duplicate keys in manifests. If the same +mapping key appears more than once (even if a YAML parser would normally accept +it with “last key wins” behaviour), Netsuke treats this as an error. + ### Top-Level Structure ```yaml diff --git a/test_support/src/env.rs b/test_support/src/env.rs index 7d6cae14..1168a031 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -223,6 +223,12 @@ pub struct NinjaEnvGuard { _lock: EnvLock, } +impl std::fmt::Debug for NinjaEnvGuard { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("NinjaEnvGuard").finish_non_exhaustive() + } +} + /// Override the `NINJA_ENV` variable with `path`, returning a guard that resets it. /// /// In Rust 2024 `std::env::set_var` is `unsafe` because it mutates process-global diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs index 3dcadae6..93c73780 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -4,6 +4,7 @@ //! synchronised, preventing interference between concurrently running tests. use std::sync::{Mutex, MutexGuard}; +use std::{fmt, fmt::Formatter}; static ENV_LOCK: Mutex<()> = Mutex::new(()); @@ -12,6 +13,12 @@ pub struct EnvLock { _guard: MutexGuard<'static, ()>, } +impl fmt::Debug for EnvLock { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.debug_struct("EnvLock").finish_non_exhaustive() + } +} + impl EnvLock { /// Acquire the global lock serialising environment mutations. pub fn acquire() -> Self { diff --git a/tests/cucumber.rs b/tests/cucumber.rs index 994bfa77..56f3f6cf 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -7,7 +7,11 @@ use netsuke::stdlib::{NetworkPolicy, StdlibState}; #[cfg(unix)] use std::os::unix::fs::FileTypeExt; use std::{collections::HashMap, ffi::OsString, net::TcpListener}; -use test_support::{PathGuard, env::restore_many, http}; +use test_support::{ + PathGuard, + env::{NinjaEnvGuard, restore_many}, + http, +}; /// Shared state for Cucumber scenarios. #[derive(Debug, Default, World)] @@ -37,6 +41,8 @@ pub struct CliWorld { pub temp: Option, /// Guard that restores `PATH` after each scenario. pub path_guard: Option, + /// Guard that overrides `NINJA_ENV` for deterministic Ninja resolution. + pub ninja_env_guard: Option, /// Root directory for stdlib scenarios. pub stdlib_root: Option, /// Captured output from the last stdlib render. @@ -181,6 +187,7 @@ fn block_device_exists() -> bool { impl Drop for CliWorld { fn drop(&mut self) { self.shutdown_http_server(); + self.ninja_env_guard.take(); self.restore_environment(); self.stdlib_text = None; } diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index cf86eb39..3d6093ff 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -135,12 +135,8 @@ where fn assert_subcommand_succeeds_without_persisting_file( fixture: Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>, command: Commands, + name: &'static str, ) -> Result<()> { - let name = match command { - Commands::Clean => "clean", - Commands::Graph => "graph", - other => bail!("unsupported subcommand for test helper: {other:?}"), - }; let (_ninja_dir, _ninja_path, _guard) = fixture?; let (temp, manifest_path) = create_test_manifest()?; let cli = Cli { @@ -150,7 +146,9 @@ fn assert_subcommand_succeeds_without_persisting_file( ..Cli::default() }; - run(&cli).with_context(|| format!("expected {name} subcommand to succeed"))?; + run(&cli) + .with_context(|| format!("running subcommand {:?}", cli.command)) + .with_context(|| format!("expected {name} subcommand to succeed"))?; ensure!( !temp.path().join("build.ninja").exists(), @@ -159,12 +157,10 @@ fn assert_subcommand_succeeds_without_persisting_file( Ok(()) } -fn assert_subcommand_fails_with_invalid_manifest(command: Commands) -> Result<()> { - let name = match command { - Commands::Clean => "clean", - Commands::Graph => "graph", - other => bail!("unsupported subcommand for test helper: {other:?}"), - }; +fn assert_subcommand_fails_with_invalid_manifest( + command: Commands, + name: &'static str, +) -> Result<()> { let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; let manifest_path = temp.path().join("Netsukefile"); std::fs::copy("tests/data/invalid_version.yml", &manifest_path) @@ -185,6 +181,8 @@ fn assert_subcommand_fails_with_invalid_manifest(command: Commands) -> Result<() Ok(()) } +type NinjaToolFixture = fn() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>; + #[rstest] fn run_ninja_not_found() -> Result<()> { assert_binary_not_found(|| { @@ -368,12 +366,6 @@ fn ninja_expecting_clean() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard) Ok((ninja_dir, ninja_path, guard)) } -#[cfg(unix)] -#[rstest] -fn run_clean_subcommand_succeeds() -> Result<()> { - assert_subcommand_succeeds_without_persisting_file(ninja_expecting_clean(), Commands::Clean) -} - #[cfg(unix)] #[rstest] fn run_clean_fails_with_failing_ninja() -> Result<()> { @@ -393,12 +385,6 @@ fn run_ninja_tool_not_found() -> Result<()> { }) } -#[cfg(unix)] -#[rstest] -fn run_clean_fails_with_invalid_manifest() -> Result<()> { - assert_subcommand_fails_with_invalid_manifest(Commands::Clean) -} - // --- Graph subcommand tests --- /// Fixture: point `NINJA_ENV` at a fake `ninja` that expects `-t graph`. @@ -412,12 +398,6 @@ fn ninja_expecting_graph() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard) Ok((ninja_dir, ninja_path, guard)) } -#[cfg(unix)] -#[rstest] -fn run_graph_subcommand_succeeds() -> Result<()> { - assert_subcommand_succeeds_without_persisting_file(ninja_expecting_graph(), Commands::Graph) -} - #[cfg(unix)] #[rstest] fn run_graph_fails_with_failing_ninja() -> Result<()> { @@ -426,6 +406,27 @@ fn run_graph_fails_with_failing_ninja() -> Result<()> { #[cfg(unix)] #[rstest] -fn run_graph_fails_with_invalid_manifest() -> Result<()> { - assert_subcommand_fails_with_invalid_manifest(Commands::Graph) +#[case( + Some(ninja_expecting_clean as NinjaToolFixture), + Commands::Clean, + "clean" +)] +#[case(None, Commands::Clean, "clean")] +#[case( + Some(ninja_expecting_graph as NinjaToolFixture), + Commands::Graph, + "graph" +)] +#[case(None, Commands::Graph, "graph")] +fn run_tool_subcommand_table_cases( + #[case] fixture: Option, + #[case] command: Commands, + #[case] name: &'static str, +) -> Result<()> { + match fixture { + Some(factory) => { + assert_subcommand_succeeds_without_persisting_file(factory(), command, name) + } + None => assert_subcommand_fails_with_invalid_manifest(command, name), + } } diff --git a/tests/steps/process_steps.rs b/tests/steps/process_steps.rs index fb43ec93..45f14f38 100644 --- a/tests/steps/process_steps.rs +++ b/tests/steps/process_steps.rs @@ -40,6 +40,9 @@ fn install_test_ninja( let guard = env::prepend_dir_to_path(env, dir.path())?; world.path_guard = Some(guard); world.ninja = Some(ninja_path.to_string_lossy().into_owned()); + world.ninja_env_guard.take(); + let system_env = env::SystemEnv::new(); + world.ninja_env_guard = Some(env::override_ninja_env(&system_env, &ninja_path)); world.temp = Some(dir); Ok(()) } From d6bfb30bf0804f94ade9e3aa2ddeb1741154a03a Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 15 Dec 2025 07:29:00 +0000 Subject: [PATCH 5/8] test: split tool subcommand runner tests Move clean/graph tool-subcommand coverage into its own integration test module to keep test files under the 400-line limit. Also assert invalid-manifest failures via the error chain for robustness. --- tests/runner_tests.rs | 118 +---------------- tests/runner_tool_subcommands_tests.rs | 170 +++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 117 deletions(-) create mode 100644 tests/runner_tool_subcommands_tests.rs diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 3d6093ff..bb535f8d 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -6,7 +6,7 @@ use netsuke::runner::{BuildTargets, run, run_ninja, run_ninja_tool}; use rstest::{fixture, rstest}; use std::path::{Path, PathBuf}; use test_support::{ - check_ninja::{self, ToolName}, + check_ninja, env::{NinjaEnvGuard, SystemEnv, override_ninja_env, prepend_dir_to_path}, fake_ninja, }; @@ -132,57 +132,6 @@ where Ok(()) } -fn assert_subcommand_succeeds_without_persisting_file( - fixture: Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>, - command: Commands, - name: &'static str, -) -> Result<()> { - let (_ninja_dir, _ninja_path, _guard) = fixture?; - let (temp, manifest_path) = create_test_manifest()?; - let cli = Cli { - file: manifest_path.clone(), - directory: Some(temp.path().to_path_buf()), - command: Some(command), - ..Cli::default() - }; - - run(&cli) - .with_context(|| format!("running subcommand {:?}", cli.command)) - .with_context(|| format!("expected {name} subcommand to succeed"))?; - - ensure!( - !temp.path().join("build.ninja").exists(), - "{name} subcommand should not leave build.ninja in project directory" - ); - Ok(()) -} - -fn assert_subcommand_fails_with_invalid_manifest( - command: Commands, - name: &'static str, -) -> Result<()> { - let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/invalid_version.yml", &manifest_path) - .with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?; - let cli = Cli { - file: manifest_path.clone(), - command: Some(command), - ..Cli::default() - }; - - let Err(err) = run(&cli) else { - bail!("expected {name} to fail for invalid manifest"); - }; - ensure!( - err.to_string().contains("loading manifest at"), - "error should mention manifest loading, got: {err}" - ); - Ok(()) -} - -type NinjaToolFixture = fn() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>; - #[rstest] fn run_ninja_not_found() -> Result<()> { assert_binary_not_found(|| { @@ -353,25 +302,6 @@ fn run_fails_with_failing_ninja_env() -> Result<()> { assert_ninja_failure_propagates(None) } -// --- Clean subcommand tests --- - -/// Fixture: point `NINJA_ENV` at a fake `ninja` that expects `-t clean`. -/// -/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) -#[fixture] -fn ninja_expecting_clean() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { - let (ninja_dir, ninja_path) = check_ninja::fake_ninja_expect_tool(ToolName::new("clean"))?; - let env = SystemEnv::new(); - let guard = override_ninja_env(&env, ninja_path.as_path()); - Ok((ninja_dir, ninja_path, guard)) -} - -#[cfg(unix)] -#[rstest] -fn run_clean_fails_with_failing_ninja() -> Result<()> { - assert_ninja_failure_propagates(Some(Commands::Clean)) -} - #[rstest] fn run_ninja_tool_not_found() -> Result<()> { assert_binary_not_found(|| { @@ -384,49 +314,3 @@ fn run_ninja_tool_not_found() -> Result<()> { ) }) } - -// --- Graph subcommand tests --- - -/// Fixture: point `NINJA_ENV` at a fake `ninja` that expects `-t graph`. -/// -/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) -#[fixture] -fn ninja_expecting_graph() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { - let (ninja_dir, ninja_path) = check_ninja::fake_ninja_expect_tool(ToolName::new("graph"))?; - let env = SystemEnv::new(); - let guard = override_ninja_env(&env, ninja_path.as_path()); - Ok((ninja_dir, ninja_path, guard)) -} - -#[cfg(unix)] -#[rstest] -fn run_graph_fails_with_failing_ninja() -> Result<()> { - assert_ninja_failure_propagates(Some(Commands::Graph)) -} - -#[cfg(unix)] -#[rstest] -#[case( - Some(ninja_expecting_clean as NinjaToolFixture), - Commands::Clean, - "clean" -)] -#[case(None, Commands::Clean, "clean")] -#[case( - Some(ninja_expecting_graph as NinjaToolFixture), - Commands::Graph, - "graph" -)] -#[case(None, Commands::Graph, "graph")] -fn run_tool_subcommand_table_cases( - #[case] fixture: Option, - #[case] command: Commands, - #[case] name: &'static str, -) -> Result<()> { - match fixture { - Some(factory) => { - assert_subcommand_succeeds_without_persisting_file(factory(), command, name) - } - None => assert_subcommand_fails_with_invalid_manifest(command, name), - } -} diff --git a/tests/runner_tool_subcommands_tests.rs b/tests/runner_tool_subcommands_tests.rs new file mode 100644 index 00000000..01c48f9b --- /dev/null +++ b/tests/runner_tool_subcommands_tests.rs @@ -0,0 +1,170 @@ +//! Integration tests for Netsuke tool subcommands. +//! +//! Covers the `clean` and `graph` subcommands which invoke Ninja tools via +//! `ninja -t `. + +use anyhow::{Context, Result, bail, ensure}; +use netsuke::cli::{Cli, Commands}; +use netsuke::runner::run; +use rstest::{fixture, rstest}; +use std::path::PathBuf; +use test_support::{ + check_ninja::{self, ToolName}, + env::{NinjaEnvGuard, SystemEnv, override_ninja_env}, + fake_ninja, +}; + +/// Create a temporary project with a Netsukefile from `minimal.yml`. +fn create_test_manifest() -> Result<(tempfile::TempDir, PathBuf)> { + let temp = tempfile::tempdir().context("create temp dir for test manifest")?; + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/minimal.yml", &manifest_path) + .with_context(|| format!("copy minimal.yml to {}", manifest_path.display()))?; + Ok((temp, manifest_path)) +} + +/// Fixture: point `NINJA_ENV` at a fake `ninja` with a configurable exit code. +/// +/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) +#[fixture] +fn ninja_with_exit_code( + #[default(0u8)] exit_code: u8, +) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { + let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; + let env = SystemEnv::new(); + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, ninja_path, guard)) +} + +/// Helper: test that a command fails when ninja exits with non-zero status. +fn assert_ninja_failure_propagates(command: Commands) -> Result<()> { + let (_ninja_dir, _ninja_path, _guard) = ninja_with_exit_code(7)?; + let (temp, manifest_path) = create_test_manifest()?; + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + command: Some(command), + ..Cli::default() + }; + + let Err(err) = run(&cli) else { + bail!("expected run to fail when ninja exits non-zero"); + }; + let messages: Vec = err.chain().map(ToString::to_string).collect(); + ensure!( + messages.iter().any(|m| m.contains("ninja exited")), + "error should report ninja exit status, got: {messages:?}" + ); + Ok(()) +} + +fn assert_subcommand_succeeds_without_persisting_file( + fixture: Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>, + command: Commands, + name: &'static str, +) -> Result<()> { + let (_ninja_dir, _ninja_path, _guard) = fixture?; + let (temp, manifest_path) = create_test_manifest()?; + let cli = Cli { + file: manifest_path.clone(), + directory: Some(temp.path().to_path_buf()), + command: Some(command), + ..Cli::default() + }; + + run(&cli).with_context(|| format!("expected {name} subcommand to succeed"))?; + + ensure!( + !temp.path().join("build.ninja").exists(), + "{name} subcommand should not leave build.ninja in project directory" + ); + Ok(()) +} + +fn assert_subcommand_fails_with_invalid_manifest( + command: Commands, + name: &'static str, +) -> Result<()> { + let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/invalid_version.yml", &manifest_path) + .with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?; + let cli = Cli { + file: manifest_path.clone(), + command: Some(command), + ..Cli::default() + }; + + let Err(err) = run(&cli) else { + bail!("expected {name} to fail for invalid manifest"); + }; + let messages: Vec = err.chain().map(ToString::to_string).collect(); + ensure!( + messages.iter().any(|m| m.contains("loading manifest at")), + "error should mention manifest loading, got: {messages:?}" + ); + Ok(()) +} + +type NinjaToolFixture = fn() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)>; + +/// Fixture: point `NINJA_ENV` at a fake `ninja` that expects `-t clean`. +/// +/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) +#[fixture] +fn ninja_expecting_clean() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { + let (ninja_dir, ninja_path) = check_ninja::fake_ninja_expect_tool(ToolName::new("clean"))?; + let env = SystemEnv::new(); + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, ninja_path, guard)) +} + +/// Fixture: point `NINJA_ENV` at a fake `ninja` that expects `-t graph`. +/// +/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) +#[fixture] +fn ninja_expecting_graph() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { + let (ninja_dir, ninja_path) = check_ninja::fake_ninja_expect_tool(ToolName::new("graph"))?; + let env = SystemEnv::new(); + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, ninja_path, guard)) +} + +#[cfg(unix)] +#[rstest] +fn run_clean_fails_with_failing_ninja() -> Result<()> { + assert_ninja_failure_propagates(Commands::Clean) +} + +#[cfg(unix)] +#[rstest] +fn run_graph_fails_with_failing_ninja() -> Result<()> { + assert_ninja_failure_propagates(Commands::Graph) +} + +#[cfg(unix)] +#[rstest] +#[case( + Some(ninja_expecting_clean as NinjaToolFixture), + Commands::Clean, + "clean" +)] +#[case(None, Commands::Clean, "clean")] +#[case( + Some(ninja_expecting_graph as NinjaToolFixture), + Commands::Graph, + "graph" +)] +#[case(None, Commands::Graph, "graph")] +fn run_tool_subcommand_table_cases( + #[case] fixture: Option, + #[case] command: Commands, + #[case] name: &'static str, +) -> Result<()> { + match fixture { + Some(factory) => { + assert_subcommand_succeeds_without_persisting_file(factory(), command, name) + } + None => assert_subcommand_fails_with_invalid_manifest(command, name), + } +} From 89ce60b474f1ef61c33258aa9db9d9a4b0c3e610 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 16 Dec 2025 09:38:31 +0000 Subject: [PATCH 6/8] test: share runner fixtures Introduce a shared tests/common module for integration tests. - Deduplicate create_test_manifest and ninja_with_exit_code. - Keep rstest fixture discovery via local re-exports where needed. --- tests/common/mod.rs | 35 ++++++++++++++++++++++++++ tests/runner_tests.rs | 34 +++++++++---------------- tests/runner_tool_subcommands_tests.rs | 21 ++++------------ 3 files changed, 52 insertions(+), 38 deletions(-) create mode 100644 tests/common/mod.rs diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 00000000..a6d70c18 --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,35 @@ +//! Shared helpers for integration tests. +//! +//! Integration tests under `tests/` compile as independent crates. This module +//! is included via `mod common;` in individual test files to share fixtures and +//! helpers while keeping test modules small and avoiding duplication. + +use anyhow::{Context, Result}; +use rstest::fixture; +use std::path::PathBuf; +use test_support::{ + env::{NinjaEnvGuard, SystemEnv, override_ninja_env}, + fake_ninja, +}; + +/// Create a temporary project with a Netsukefile from `minimal.yml`. +pub fn create_test_manifest() -> Result<(tempfile::TempDir, PathBuf)> { + let temp = tempfile::tempdir().context("create temp dir for test manifest")?; + let manifest_path = temp.path().join("Netsukefile"); + std::fs::copy("tests/data/minimal.yml", &manifest_path) + .with_context(|| format!("copy minimal.yml to {}", manifest_path.display()))?; + Ok((temp, manifest_path)) +} + +/// Fixture: point `NINJA_ENV` at a fake `ninja` with a configurable exit code. +/// +/// Returns: (tempdir holding ninja, `NINJA_ENV` guard) +#[fixture] +pub fn ninja_with_exit_code( + #[default(0u8)] exit_code: u8, +) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { + let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; + let env = SystemEnv::new(); + let guard = override_ninja_env(&env, ninja_path.as_path()); + Ok((ninja_dir, ninja_path, guard)) +} diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index bb535f8d..adf163dd 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -11,6 +11,18 @@ use test_support::{ fake_ninja, }; +mod common; +use common::create_test_manifest; + +// Re-export `common::ninja_with_exit_code` as a local fixture so rstest can +// discover it in this integration test crate. +#[fixture] +fn ninja_with_exit_code( + #[default(0u8)] exit_code: u8, +) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { + common::ninja_with_exit_code(exit_code) +} + /// Fixture: point `NINJA_ENV` at a fake `ninja` that validates `-f` files. /// /// Using `NINJA_ENV` avoids mutating `PATH`, letting tests run in parallel @@ -25,19 +37,6 @@ fn ninja_in_env() -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { Ok((ninja_dir, ninja_path, guard)) } -/// Fixture: point `NINJA_ENV` at a fake `ninja` with a configurable exit code. -/// -/// Returns: (tempdir holding ninja, `NINJA_ENV` guard) -#[fixture] -fn ninja_with_exit_code( - #[default(0u8)] exit_code: u8, -) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { - let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; - let env = SystemEnv::new(); - let guard = override_ninja_env(&env, ninja_path.as_path()); - Ok((ninja_dir, ninja_path, guard)) -} - /// Shared setup for tests that rely on `NINJA_ENV`. /// /// Returns the fake ninja directory, temp project directory, constructed CLI, @@ -59,15 +58,6 @@ fn setup_ninja_env_test() -> Result<( Ok((ninja_dir, ninja_path, temp, cli, guard)) } -/// Create a temporary project with a Netsukefile from `minimal.yml`. -fn create_test_manifest() -> Result<(tempfile::TempDir, PathBuf)> { - let temp = tempfile::tempdir().context("create temp dir for test manifest")?; - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/minimal.yml", &manifest_path) - .with_context(|| format!("copy minimal.yml to {}", manifest_path.display()))?; - Ok((temp, manifest_path)) -} - #[test] fn run_exits_with_manifest_error_on_invalid_version() -> Result<()> { let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?; diff --git a/tests/runner_tool_subcommands_tests.rs b/tests/runner_tool_subcommands_tests.rs index 01c48f9b..750ed33c 100644 --- a/tests/runner_tool_subcommands_tests.rs +++ b/tests/runner_tool_subcommands_tests.rs @@ -11,29 +11,18 @@ use std::path::PathBuf; use test_support::{ check_ninja::{self, ToolName}, env::{NinjaEnvGuard, SystemEnv, override_ninja_env}, - fake_ninja, }; -/// Create a temporary project with a Netsukefile from `minimal.yml`. -fn create_test_manifest() -> Result<(tempfile::TempDir, PathBuf)> { - let temp = tempfile::tempdir().context("create temp dir for test manifest")?; - let manifest_path = temp.path().join("Netsukefile"); - std::fs::copy("tests/data/minimal.yml", &manifest_path) - .with_context(|| format!("copy minimal.yml to {}", manifest_path.display()))?; - Ok((temp, manifest_path)) -} +mod common; +use common::create_test_manifest; -/// Fixture: point `NINJA_ENV` at a fake `ninja` with a configurable exit code. -/// -/// Returns: (tempdir holding ninja, path to ninja, `NINJA_ENV` guard) +// Re-export `common::ninja_with_exit_code` as a local fixture so rstest can +// discover it in this integration test crate. #[fixture] fn ninja_with_exit_code( #[default(0u8)] exit_code: u8, ) -> Result<(tempfile::TempDir, PathBuf, NinjaEnvGuard)> { - let (ninja_dir, ninja_path) = fake_ninja(exit_code)?; - let env = SystemEnv::new(); - let guard = override_ninja_env(&env, ninja_path.as_path()); - Ok((ninja_dir, ninja_path, guard)) + common::ninja_with_exit_code(exit_code) } /// Helper: test that a command fails when ninja exits with non-zero status. From b7c95f07925922c94044a5768d3e509edd979885 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 17 Dec 2025 01:16:36 +0000 Subject: [PATCH 7/8] test(runner): refactor not found tests to use helper function Extract a helper function `assert_runner_not_found` to consolidate repeated code that asserts runner functions fail with `NotFound` errors when the ninja binary is missing. Updated existing tests `run_ninja_not_found` and `run_ninja_tool_not_found` to use this helper, improving code reuse and clarity. Co-authored-by: terragon-labs[bot] --- tests/runner_tests.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index adf163dd..067cf000 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -122,14 +122,24 @@ where Ok(()) } -#[rstest] -fn run_ninja_not_found() -> Result<()> { +/// Helper: assert that a runner function fails with `NotFound` when ninja binary is missing. +fn assert_runner_not_found(runner_call: F) -> Result<()> +where + F: FnOnce(&Cli) -> std::io::Result<()>, +{ assert_binary_not_found(|| { let cli = Cli::default(); + runner_call(&cli) + }) +} + +#[rstest] +fn run_ninja_not_found() -> Result<()> { + assert_runner_not_found(|cli| { let targets = BuildTargets::default(); run_ninja( Path::new("does-not-exist"), - &cli, + cli, Path::new("build.ninja"), &targets, ) @@ -294,11 +304,10 @@ fn run_fails_with_failing_ninja_env() -> Result<()> { #[rstest] fn run_ninja_tool_not_found() -> Result<()> { - assert_binary_not_found(|| { - let cli = Cli::default(); + assert_runner_not_found(|cli| { run_ninja_tool( Path::new("does-not-exist"), - &cli, + cli, Path::new("build.ninja"), "clean", ) From bfe336a7bd1f8be1bef448330a92a3db94bc7b51 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 19 Dec 2025 00:03:54 +0000 Subject: [PATCH 8/8] test(runner_tool_subcommands): improve ninja exit failure test coverage and error messages - Enhance test to verify error messages include the specific ninja tool invoked (e.g., clean, graph) when ninja exits with a failure status. - Check presence of build file context in error outputs. - Add doc comments to fixture re-exports for clarity. Co-authored-by: terragon-labs[bot] --- tests/runner_tests.rs | 8 ++++++-- tests/runner_tool_subcommands_tests.rs | 25 +++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 067cf000..f01ecd5b 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -14,8 +14,12 @@ use test_support::{ mod common; use common::create_test_manifest; -// Re-export `common::ninja_with_exit_code` as a local fixture so rstest can -// discover it in this integration test crate. +/// Fixture: provide a fake `ninja` binary with a configurable exit code. +/// +/// This is a re-export of `common::ninja_with_exit_code` so `rstest` can +/// discover it in this integration test crate. +/// +/// Returns: (`tempfile::TempDir`, path to the ninja binary, `NinjaEnvGuard`) #[fixture] fn ninja_with_exit_code( #[default(0u8)] exit_code: u8, diff --git a/tests/runner_tool_subcommands_tests.rs b/tests/runner_tool_subcommands_tests.rs index 750ed33c..f462b1db 100644 --- a/tests/runner_tool_subcommands_tests.rs +++ b/tests/runner_tool_subcommands_tests.rs @@ -16,8 +16,12 @@ use test_support::{ mod common; use common::create_test_manifest; -// Re-export `common::ninja_with_exit_code` as a local fixture so rstest can -// discover it in this integration test crate. +/// Fixture: provide a fake `ninja` binary with a configurable exit code. +/// +/// This is a re-export of `common::ninja_with_exit_code` so `rstest` can +/// discover it in this integration test crate. +/// +/// Returns: (`tempfile::TempDir`, path to the ninja binary, `NinjaEnvGuard`) #[fixture] fn ninja_with_exit_code( #[default(0u8)] exit_code: u8, @@ -29,6 +33,11 @@ fn ninja_with_exit_code( fn assert_ninja_failure_propagates(command: Commands) -> Result<()> { let (_ninja_dir, _ninja_path, _guard) = ninja_with_exit_code(7)?; let (temp, manifest_path) = create_test_manifest()?; + let expected_tool = match &command { + Commands::Clean => "clean", + Commands::Graph => "graph", + other => bail!("unsupported command for this helper: {other:?}"), + }; let cli = Cli { file: manifest_path.clone(), directory: Some(temp.path().to_path_buf()), @@ -44,6 +53,18 @@ fn assert_ninja_failure_propagates(command: Commands) -> Result<()> { messages.iter().any(|m| m.contains("ninja exited")), "error should report ninja exit status, got: {messages:?}" ); + ensure!( + messages + .iter() + .any(|m| m.contains(&format!("-t {expected_tool}"))), + "error should mention running ninja tool {expected_tool}, got: {messages:?}" + ); + ensure!( + messages + .iter() + .any(|m| m.contains("with build file") && m.contains(".ninja")), + "error should include build file context, got: {messages:?}" + ); Ok(()) }