diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index a83983c4..48dedec1 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1181,17 +1181,17 @@ The command construction will follow this pattern: 1. A new `Command` is created via `Command::new("ninja")`. Netsuke will assume `ninja` is available in the system's `PATH`. -1. Arguments passed to Netsuke's own CLI will be translated and forwarded to +2. Arguments passed to Netsuke's own CLI will be translated and forwarded to Ninja. For example, a `Netsuke build my_target` command would result in `Command::new("ninja").arg("my_target")`. Flags like `-j` for parallelism will also be passed through.[^8] -1. The working directory for the Ninja process will be set using +3. The working directory for the Ninja process will be set using `.current_dir()`. When the user supplies a `-C` flag, Netsuke canonicalises the path and applies it via `current_dir` rather than forwarding the flag to Ninja. -1. Standard I/O streams (`stdin`, `stdout`, `stderr`) will be configured using +4. Standard I/O streams (`stdin`, `stdout`, `stderr`) will be configured using `.stdout(Stdio::piped())` and `.stderr(Stdio::piped())`.[^24] This allows Netsuke to capture the real-time output from Ninja, which can then be streamed to the user's console, potentially with additional formatting or @@ -1279,27 +1279,32 @@ three fundamental questions: 1. **What** went wrong? A concise summary of the failure (e.g., "YAML parsing failed," "Build configuration is invalid"). -1. **Where** did it go wrong? Precise location information, including the file, +2. **Where** did it go wrong? Precise location information, including the file, line number, and column where applicable (e.g., "in `Netsukefile` at line 15, column 3"). -1. **Why** did it go wrong, and what can be done about it? The underlying cause +3. **Why** did it go wrong, and what can be done about it? The underlying cause of the error and a concrete suggestion for how to fix it (e.g., "Cause: Found a tab character, which is not allowed. Hint: Use spaces for indentation instead."). ### 7.2 Crate Selection and Strategy: `anyhow`, `thiserror`, and `miette` -To implement this philosophy, Netsuke adopts a hybrid error handling strategy -using the `anyhow`, `thiserror`, and `miette` crates. This is a common and -highly effective pattern in the Rust ecosystem for creating robust applications -and libraries.[^27] `miette` renders user-facing diagnostics, computing spans -directly from parser locations. +Netsuke uses a two-tier error architecture: -- `thiserror`: This crate will be used *within* Netsuke's internal library - modules (e.g., `parser`, `ir`, `ninja_gen`) to define specific, structured - error types. The `#[derive(Error)]` macro reduces boilerplate and allows for - the creation of rich, semantic errors.[^29] +1. `anyhow` captures internal context as errors propagate through the + application. +2. `miette` renders user-facing diagnostics and **is not optional**. All + surface errors must implement `miette::Diagnostic` so the CLI can present + spans, annotated source, and helpful suggestions. + +This hybrid strategy is common in the Rust ecosystem and provides both rich +context and polished user output.[^27] + +- `thiserror`: This crate is used *within* Netsuke's internal library modules + (e.g., `parser`, `ir`, `ninja_gen`) to define specific, structured error + types. The `#[derive(Error)]` macro reduces boilerplate and allows for the + creation of rich, semantic errors.[^29] Rust @@ -1327,17 +1332,64 @@ pub enum IrGenError { ActionSerialisation(#[from] serde_json::Error), } ``` -- `anyhow`: This crate will be used in the main application logic (`main.rs`) - and at the boundaries between modules. `anyhow::Result` serves as a - convenient, dynamic error type that can wrap any underlying error that - implements `std::error::Error`.[^30] The primary tools used will be the - - `?` operator for clean error propagation and the `.context()` and - `.with_context()` methods for adding high-level, human-readable context to - errors as they bubble up the call stack.[^31] +- `anyhow`: Used in the main application logic (`main.rs`) and at the + boundaries between modules. `anyhow::Result` wraps any error implementing + `std::error::Error`.[^30] The `?` operator provides clean propagation, while + `.context()` and `.with_context()` attach high-level explanations as errors + bubble up.[^31] - `miette`: Presents human-friendly diagnostics, highlighting exact error - locations with computed spans. + locations with computed spans. Every diagnostic must retain `miette`'s + `Diagnostic` implementation as it travels through `anyhow`. + +#### Canonical pattern: `YamlDiagnostic` + +`YamlDiagnostic` is the reference implementation of a Netsuke diagnostic. It +wraps `yaml-rust` errors with annotated source, spans, and optional help text: + +```rust +#[derive(Debug, Error, Diagnostic)] +#[error("{message}")] +#[diagnostic(code(netsuke::yaml::parse))] +pub struct YamlDiagnostic { + #[source_code] + src: NamedSource, + #[label("parse error here")] + span: Option, + #[help] + help: Option, + #[source] + source: YamlError, + message: String, +} + +#[derive(Debug, Error, Diagnostic)] +pub enum ManifestError { + #[error("manifest parse error")] + #[diagnostic(code(netsuke::manifest::parse))] + Parse { + #[source] + #[diagnostic_source] + source: Box, + }, +} +``` + +`ManifestError::Parse` boxes the diagnostic to preserve the rich error so +`miette` can show the offending YAML snippet. All new user-facing errors with +source context must follow this model. + +Common use cases requiring `miette` diagnostics include: + +- YAML parsing errors. +- Jinja template rendering failures with line numbers and context. +- Any scenario where highlighting spans or providing structured help benefits + the user. + +Although `src/diagnostics.rs` is currently unused, it contains prototypes for +`miette` patterns and remains a valuable reference. Future diagnostics should +mirror the `YamlDiagnostic` approach by implementing `Diagnostic`, providing a +`NamedSource`, a `SourceSpan`, and actionable help text. ### 7.3 Error Handling Flow @@ -1346,23 +1398,28 @@ enrichment: 1. A specific, low-level error occurs within a module. For instance, the IR generator detects a missing rule and creates an `IrGenError::RuleNotFound`. + Likewise, the Ninja generator returns `NinjaGenError::MissingAction` when a + build edge references an undefined action, preventing panics during file + generation. -1. The function where the error occurred returns +2. The function where the error occurred returns `Err(IrGenError::RuleNotFound {... }.into())`. The `.into()` call converts the specific `thiserror` enum variant into a generic `anyhow::Error` object, preserving the original error as its source. -1. A higher-level function in the call stack, which called the failing function, +3. A higher-level function in the call stack, which called the failing function, receives this `Err` value. It uses the `.with_context()` method to wrap the error with more application-level context. For example: `ir::from_manifest(ast)` `.with_context(|| "Failed to build the internal build graph from the manifest")?` . -1. This process of propagation and contextualization repeats as the error - bubbles up towards `main`. +4. This process of propagation and contextualisation repeats as the error + bubbles up towards `main`. Use `anyhow::Context` to add detail, but never + convert a `miette::Diagnostic` into a plain `anyhow::Error`—doing so would + discard spans and help text. -1. Finally, the `main` function receives the `Err` result. It prints the entire +5. Finally, the `main` function receives the `Err` result. It prints the entire error chain provided by `anyhow`, which displays the highest-level context first, followed by a list of underlying "Caused by:" messages. This provides the user with a rich, layered explanation of the failure, from the general @@ -1530,15 +1587,15 @@ goal. 1. Implement the initial `clap` CLI structure for the `build` command. - 1. Implement the YAML parser using `serde_yml` and the AST data structures + 2. Implement the YAML parser using `serde_yml` and the AST data structures (`ast.rs`). - 1. Implement the AST-to-IR transformation logic, including basic validation + 3. Implement the AST-to-IR transformation logic, including basic validation like checking for rule existence. - 1. Implement the IR-to-Ninja file generator (`ninja_gen.rs`). + 4. Implement the IR-to-Ninja file generator (`ninja_gen.rs`). - 1. Implement the `std::process::Command` logic to invoke `ninja`. + 5. Implement the `std::process::Command` logic to invoke `ninja`. - **Success Criterion:** Netsuke can successfully take a `Netsukefile` file *without any Jinja syntax* and compile it to a `build.ninja` file, then @@ -1554,13 +1611,13 @@ goal. 1. Integrate the `minijinja` crate into the build pipeline. - 1. Implement the two-pass parsing mechanism: first render the manifest with + 2. Implement the two-pass parsing mechanism: first render the manifest with `minijinja`, then parse the result with `serde_yml`. - 1. Populate the initial Jinja context with the global `vars` from the + 3. Populate the initial Jinja context with the global `vars` from the manifest. - 1. Implement basic Jinja control flow (`{% if... %}`, `{% for... %}`) and + 4. Implement basic Jinja control flow (`{% if... %}`, `{% for... %}`) and variable substitution. - **Success Criterion:** Netsuke can successfully build a manifest that uses @@ -1577,15 +1634,15 @@ goal. 1. Implement the full suite of custom Jinja functions (`glob`, `env`, etc.) and filters (`shell_escape`). - 1. Mandate the use of `shell-quote` for all command variable substitutions. + 2. Mandate the use of `shell-quote` for all command variable substitutions. - 1. Refactor the error handling to fully adopt the `anyhow`/`thiserror` + 3. Refactor the error handling to fully adopt the `anyhow`/`thiserror` strategy, ensuring all user-facing errors are contextual and actionable as specified in Section 7. - 1. Implement the `clean` and `graph` subcommands. + 4. Implement the `clean` and `graph` subcommands. - 1. Refine the CLI output for clarity and readability. + 5. Refine the CLI output for clarity and readability. - **Success Criterion:** Netsuke is a feature-complete, secure, and user-friendly build tool that meets all the initial design goals. @@ -1652,7 +1709,8 @@ projects. ### **Works cited** [^1]: Ninja, a small build system with a focus on speed. Accessed on 12 July - 2025. + + 1. [^2]: "Ninja (build system)." Wikipedia. Accessed on 12 July 2025. diff --git a/docs/roadmap.md b/docs/roadmap.md index 510c240b..6dbbf41e 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -138,18 +138,20 @@ library, and CLI ergonomics. - [x] After interpolation, validate the final command string is parsable using the shlex crate. -- [ ] **Actionable Error Reporting:** +- [x] **Actionable Error Reporting:** - - [ ] Adopt the `anyhow` and `thiserror` error handling strategy. + - [x] Adopt the `anyhow` and `thiserror` error handling strategy. - - [ ] Use thiserror to define specific, structured error types within library + - [x] Use thiserror to define specific, structured error types within library modules (e.g., IrGenError::RuleNotFound, IrGenError::CircularDependency). - - [ ] Use anyhow in the application logic to add human-readable context to + - [x] Use anyhow in the application logic to add human-readable context to errors as they propagate (e.g., using .with_context()). + - [x] Use `miette` to render diagnostics with source spans and helpful + messages. - - [ ] Refactor all error-producing code to provide the clear, contextual, and + - [x] Refactor all error-producing code to provide the clear, contextual, and actionable error messages specified in the design document. - [ ] **Template Standard Library:** diff --git a/docs/snapshot-testing-in-netsuke-using-insta.md b/docs/snapshot-testing-in-netsuke-using-insta.md index c13ae619..d120466f 100644 --- a/docs/snapshot-testing-in-netsuke-using-insta.md +++ b/docs/snapshot-testing-in-netsuke-using-insta.md @@ -176,8 +176,11 @@ fn simple_manifest_ninja_snapshot() { let build_graph = BuildGraph::from_manifest(&manifest).expect("IR generation succeeded"); // Generate Ninja file content from the IR - let ninja_file = ninja_gen::generate_ninja(&build_graph) - .expect("Ninja file generation succeeded"); + // `generate` returns `Result`; handle errors + let ninja_file = match ninja_gen::generate(&build_graph) { + Ok(ninja) => ninja, + Err(e) => panic!("Ninja file generation failed: {e}"), + }; // The output is a multi-line Ninja build script (as a String) // Ensure the output is deterministic @@ -190,15 +193,20 @@ fn simple_manifest_ninja_snapshot() { } ``` +The match explicitly handles the `Result` from `generate` so any formatting or +missing action errors surface during tests. Production code should propagate +the error and report it with `miette` rather than panicking. + Key points for Ninja snapshot tests: - Use a known manifest input and first derive the IR. An IR can also be constructed directly for tests, but using the manifest→IR pipeline ensures realistic coverage. -- Call the Ninja generation function (e.g. `ninja_gen::generate_ninja`), which - produces the Ninja file contents as a `String`. This function traverses the - IR and outputs rules and build statements in Ninja syntax. +- Call the Ninja generation function (`ninja_gen::generate`), which + yields a `Result`. This function traverses the IR and + outputs rules and build statements in Ninja syntax, returning an error if any + build edge references an undefined action. - As with IR, **determinism is crucial**. The Ninja output should list rules, targets, and dependencies in a consistent order. For example, if the IR does diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 3421e86b..d0ce0178 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -29,6 +29,10 @@ use std::fmt::Display; /// File::open(path).diag("open file") /// } /// ``` +#[expect( + dead_code, + reason = "temporarily retained during anyhow migration; remove when no call sites remain" +)] pub(crate) trait ResultExt { /// Attach a static context message to any error. /// diff --git a/src/manifest.rs b/src/manifest.rs index 6fd99766..bc2ba5cf 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -7,11 +7,9 @@ //! filesystem patterns during template evaluation. Both helpers fail fast when //! inputs are missing or patterns are invalid. -use crate::{ - ast::{NetsukeManifest, Recipe, StringOrList, Target, Vars}, - diagnostics::ResultExt, -}; -use miette::{Diagnostic, NamedSource, Report, Result, SourceSpan}; +use crate::ast::{NetsukeManifest, Recipe, StringOrList, Target, Vars}; +use anyhow::{Context, Result}; +use miette::{Diagnostic, NamedSource, SourceSpan}; use minijinja::{Environment, Error, ErrorKind, UndefinedBehavior, context, value::Value}; use serde_yml::{Error as YamlError, Location}; use serde_yml::{Mapping as YamlMapping, Value as YamlValue}; @@ -38,7 +36,7 @@ fn to_span(src: &str, loc: Location) -> SourceSpan { } }; let len = end.saturating_sub(start); - #[allow(clippy::useless_conversion, reason = "future-proof span length type")] + #[expect(clippy::useless_conversion, reason = "future-proof span length type")] SourceSpan::new(start.into(), len.into()) } @@ -86,11 +84,15 @@ pub enum ManifestError { Parse { #[source] #[diagnostic_source] - source: YamlDiagnostic, + source: Box, }, } -fn map_yaml_error(err: YamlError, src: &str, name: &str) -> YamlDiagnostic { +fn map_yaml_error( + err: YamlError, + src: &str, + name: &str, +) -> Box { let loc = err.location(); let (line, col, span) = loc.map_or((1, 1, None), |l| { (l.line(), l.column(), Some(to_span(src, l))) @@ -103,13 +105,13 @@ fn map_yaml_error(err: YamlError, src: &str, name: &str) -> YamlDiagnostic { message.push_str(h); } - YamlDiagnostic { + Box::new(YamlDiagnostic { src: NamedSource::new(name, src.to_string()), span, help: hint, source: err, message, - } + }) } /// Resolve the value of an environment variable for the `env()` Jinja helper. @@ -530,8 +532,9 @@ fn glob_paths(pattern: &str) -> std::result::Result, Error> { /// /// Returns an error if YAML parsing or Jinja evaluation fails. fn from_str_named(yaml: &str, name: &str) -> Result { - let mut doc: YamlValue = - serde_yml::from_str(yaml).map_err(|e| Report::new(map_yaml_error(e, yaml, name)))?; + let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| ManifestError::Parse { + source: map_yaml_error(e, yaml, name), + })?; let mut jinja = Environment::new(); jinja.set_undefined_behavior(UndefinedBehavior::Strict); @@ -543,7 +546,7 @@ fn from_str_named(yaml: &str, name: &str) -> Result { for (k, v) in vars { let key = k .as_str() - .ok_or_else(|| miette::miette!("non-string key in 'vars' mapping: {k:?}"))? + .ok_or_else(|| anyhow::anyhow!("non-string key in vars mapping: {k:?}"))? .to_string(); jinja.add_global(key, Value::from_serialize(v)); } @@ -551,11 +554,10 @@ fn from_str_named(yaml: &str, name: &str) -> Result { expand_foreach(&mut doc, &jinja)?; - let manifest: NetsukeManifest = serde_yml::from_value(doc).map_err(|e| { - Report::new(ManifestError::Parse { + let manifest: NetsukeManifest = + serde_yml::from_value(doc).map_err(|e| ManifestError::Parse { source: map_yaml_error(e, yaml, name), - }) - })?; + })?; render_manifest(manifest, &jinja) } @@ -618,7 +620,7 @@ fn parse_foreach_values(expr_val: &YamlValue, env: &Environment) -> Result R None => YamlMapping::new(), Some(YamlValue::Mapping(m)) => m, Some(other) => { - return Err(miette::miette!( + return Err(anyhow::anyhow!( "target.vars must be a mapping, got: {other:?}" )); } }; vars.insert( YamlValue::String("item".into()), - serde_yml::to_value(item).diag("serialise item")?, + serde_yml::to_value(item).context("serialise item")?, ); vars.insert( YamlValue::String("index".into()), @@ -664,14 +666,14 @@ fn inject_iteration_vars(map: &mut YamlMapping, item: &Value, index: usize) -> R fn as_str<'a>(value: &'a YamlValue, field: &str) -> Result<&'a str> { value .as_str() - .ok_or_else(|| miette::miette!("{field} must be a string expression")) + .ok_or_else(|| anyhow::anyhow!("{field} must be a string expression")) } fn eval_expression(env: &Environment, name: &str, expr: &str, ctx: Value) -> Result { env.compile_expression(expr) - .diag_with(|| format!("{name} expression parse error"))? + .with_context(|| format!("{name} expression parse error"))? .eval(ctx) - .diag_with(|| format!("{name} evaluation error")) + .with_context(|| format!("{name} evaluation error")) } /// Render a Jinja template and label any error with the given context. @@ -681,7 +683,7 @@ fn render_str_with( ctx: &impl serde::Serialize, what: impl FnOnce() -> String, ) -> Result { - env.render_str(tpl, ctx).diag_with(what) + env.render_str(tpl, ctx).with_context(what) } /// Render all templated strings in the manifest. @@ -769,7 +771,7 @@ fn render_string_or_list(value: &mut StringOrList, env: &Environment, ctx: &Vars pub fn from_path(path: impl AsRef) -> Result { let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref) - .diag_with(|| format!("failed to read {}", path_ref.display()))?; + .with_context(|| format!("failed to read {}", path_ref.display()))?; from_str_named(&data, &path_ref.display().to_string()) } diff --git a/src/ninja_gen.rs b/src/ninja_gen.rs index 906824b0..e2ed7615 100644 --- a/src/ninja_gen.rs +++ b/src/ninja_gen.rs @@ -10,6 +10,15 @@ use itertools::Itertools; use std::collections::HashSet; use std::fmt::{self, Display, Formatter, Write}; use std::path::PathBuf; +use thiserror::Error; + +#[derive(Debug, Error)] +pub enum NinjaGenError { + #[error("action '{id}' referenced by build edge was not found")] + MissingAction { id: String }, + #[error("failed to format Ninja output")] + Format(#[from] fmt::Error), +} macro_rules! write_kv { ($f:expr, $key:expr, $opt:expr) => { @@ -29,18 +38,69 @@ macro_rules! write_flag { /// Generate a Ninja build file as a string. /// -/// # Panics +/// # Examples +/// ``` +/// use crate::ast::Recipe; +/// use crate::ir::{Action, BuildEdge, BuildGraph}; +/// use std::path::PathBuf; +/// let mut graph = BuildGraph::default(); +/// graph.actions.insert("a".into(), Action { +/// recipe: Recipe::Command { command: "true".into() }, +/// description: None, depfile: None, deps_format: None, +/// pool: None, restat: false +/// }); +/// graph.targets.insert(PathBuf::from("out"), BuildEdge { +/// action_id: "a".into(), inputs: Vec::new(), +/// explicit_outputs: vec![PathBuf::from("out")], +/// implicit_outputs: Vec::new(), order_only_deps: Vec::new(), +/// phony: false, always: false +/// }); +/// let text = crate::ninja_gen::generate(&graph).expect("generate ninja"); +/// assert!(text.contains("rule a")); +/// ``` /// -/// Panics if a build edge references an unknown action or if writing to the -/// output string fails (which is unexpected under normal conditions). -#[must_use] -pub fn generate(graph: &BuildGraph) -> String { +/// # Errors +/// +/// Returns [`NinjaGenError`] if a build edge references an unknown action or +/// writing to the output fails. +pub fn generate(graph: &BuildGraph) -> Result { let mut out = String::new(); + generate_into(graph, &mut out)?; + Ok(out) +} +/// Write a Ninja build file to the provided writer. +/// +/// # Examples +/// ``` +/// use crate::ast::Recipe; +/// use crate::ir::{Action, BuildEdge, BuildGraph}; +/// use std::path::PathBuf; +/// let mut graph = BuildGraph::default(); +/// graph.actions.insert("a".into(), Action { +/// recipe: Recipe::Command { command: "true".into() }, +/// description: None, depfile: None, deps_format: None, +/// pool: None, restat: false +/// }); +/// graph.targets.insert(PathBuf::from("out"), BuildEdge { +/// action_id: "a".into(), inputs: Vec::new(), +/// explicit_outputs: vec![PathBuf::from("out")], +/// implicit_outputs: Vec::new(), order_only_deps: Vec::new(), +/// phony: false, always: false +/// }); +/// let mut out = String::new(); +/// crate::ninja_gen::generate_into(&graph, &mut out).expect("format ninja"); +/// assert!(out.contains("build out: a")); +/// ``` +/// +/// # Errors +/// +/// Returns [`NinjaGenError`] if a build edge references an unknown action or writing to the output fails. +pub fn generate_into(graph: &BuildGraph, out: &mut W) -> Result<(), NinjaGenError> { let mut actions: Vec<_> = graph.actions.iter().collect(); actions.sort_by_key(|(id, _)| *id); for (id, action) in actions { - write!(out, "{}", NamedAction { id, action }).expect("write Ninja rule"); + write!(out, "{}", NamedAction { id, action })?; } let mut edges: Vec<_> = graph.targets.values().collect(); @@ -51,7 +111,13 @@ pub fn generate(graph: &BuildGraph) -> String { if !seen.insert(key.clone()) { continue; } - let action = graph.actions.get(&edge.action_id).expect("action"); + let action = + graph + .actions + .get(&edge.action_id) + .ok_or_else(|| NinjaGenError::MissingAction { + id: edge.action_id.clone(), + })?; write!( out, "{}", @@ -59,17 +125,16 @@ pub fn generate(graph: &BuildGraph) -> String { edge, action_restat: action.restat, } - ) - .expect("write Ninja edge"); + )?; } if !graph.default_targets.is_empty() { let mut defs = graph.default_targets.clone(); defs.sort(); - writeln!(out, "default {}", join(&defs)).expect("write defaults"); + writeln!(out, "default {}", join(&defs))?; } - out + Ok(()) } /// Convert a slice of paths into a space-separated string. @@ -195,7 +260,7 @@ mod tests { graph.targets.insert(PathBuf::from("out"), edge); graph.default_targets.push(PathBuf::from("out")); - let ninja = generate(&graph); + let ninja = generate(&graph).expect("generate ninja"); let expected = concat!( "rule a\n", " command = echo hi\n\n", diff --git a/src/runner.rs b/src/runner.rs index 09148250..187cb14c 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -5,16 +5,12 @@ //! subprocess, streaming its output back to the user. use crate::cli::{BuildArgs, Cli, Commands}; -use crate::diagnostics::ResultExt; use crate::{ir::BuildGraph, manifest, ninja_gen}; -use miette::{Context, Result}; +use anyhow::{Context, Result}; use serde_json; use std::borrow::Cow; use std::fs; -use std::io::{self, BufRead, BufReader, Write}; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; -use std::thread; use tempfile::{Builder, NamedTempFile}; use tracing::{debug, info}; @@ -23,31 +19,11 @@ pub const NINJA_PROGRAM: &str = "ninja"; /// Environment variable override for the Ninja executable. pub use ninja_env::NINJA_ENV; -// Public helpers for doctests only. This exposes internal helpers as a stable -// testing surface without exporting them in release builds. +mod process; #[cfg(doctest)] #[doc(hidden)] -pub mod doc { - pub use super::CommandArg; - - // Public wrappers to expose crate-private helpers to doctests. - #[must_use] - pub fn contains_sensitive_keyword(arg: &CommandArg) -> bool { - super::contains_sensitive_keyword(arg) - } - #[must_use] - pub fn is_sensitive_arg(arg: &CommandArg) -> bool { - super::is_sensitive_arg(arg) - } - #[must_use] - pub fn redact_argument(arg: &CommandArg) -> CommandArg { - super::redact_argument(arg) - } - #[must_use] - pub fn redact_sensitive_args(args: &[CommandArg]) -> Vec { - super::redact_sensitive_args(args) - } -} +pub use process::doc; +pub use process::run_ninja; #[derive(Debug, Clone)] pub struct NinjaContent(String); @@ -66,19 +42,6 @@ impl NinjaContent { } } -#[derive(Debug, Clone)] -pub struct CommandArg(String); -impl CommandArg { - #[must_use] - pub fn new(arg: String) -> Self { - Self(arg) - } - #[must_use] - pub fn as_str(&self) -> &str { - &self.0 - } -} - /// Target list passed through to Ninja. /// An empty slice means “use the defaults” emitted by IR generation /// (default targets). @@ -175,7 +138,13 @@ fn handle_build(cli: &Cli, args: &BuildArgs) -> Result<()> { } let program = resolve_ninja_program(); - run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).diag("run ninja")?; + run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).with_context(|| { + format!( + "running {} with build file {}", + program.display(), + build_path.display() + ) + })?; drop(tmp_file); Ok(()) } @@ -197,7 +166,7 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { .prefix("netsuke.") .suffix(".ninja") .tempfile() - .diag("create temp file")?; + .context("create temp file")?; write_ninja_file(tmp.path(), content)?; Ok(tmp) } @@ -218,10 +187,10 @@ fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { // do not attempt to create the current directory on some platforms. if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { fs::create_dir_all(parent) - .diag_with(|| format!("failed to create parent directory {}", parent.display()))?; + .with_context(|| format!("failed to create parent directory {}", parent.display()))?; } fs::write(path, content.as_str()) - .diag_with(|| format!("failed to write Ninja file to {}", path.display()))?; + .with_context(|| format!("failed to write Ninja file to {}", path.display()))?; info!("Generated Ninja file at {}", path.display()); Ok(()) } @@ -250,10 +219,13 @@ fn generate_ninja(cli: &Cli) -> Result { let manifest_path = resolve_manifest_path(cli); let manifest = manifest::from_path(&manifest_path) .with_context(|| format!("loading manifest at {}", manifest_path.display()))?; - let ast_json = serde_json::to_string_pretty(&manifest).diag("serialising manifest")?; - debug!("AST:\n{ast_json}"); - let graph = BuildGraph::from_manifest(&manifest).diag("building graph")?; - Ok(NinjaContent::new(ninja_gen::generate(&graph))) + if tracing::enabled!(tracing::Level::DEBUG) { + let ast_json = serde_json::to_string_pretty(&manifest).context("serialising manifest")?; + debug!("AST:\n{ast_json}"); + } + let graph = BuildGraph::from_manifest(&manifest).context("building graph")?; + let ninja = ninja_gen::generate(&graph).context("generating Ninja file")?; + Ok(NinjaContent::new(ninja)) } /// Determine the manifest path respecting the CLI's directory option. @@ -277,157 +249,3 @@ fn resolve_manifest_path(cli: &Cli) -> std::path::PathBuf { fn resolve_ninja_program() -> PathBuf { std::env::var_os(NINJA_ENV).map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from) } - -/// Check if `arg` contains a sensitive keyword. -/// -/// # Examples -/// ``` -/// # use netsuke::runner::doc::{CommandArg, contains_sensitive_keyword}; -/// assert!(contains_sensitive_keyword(&CommandArg::new("token=abc".into()))); -/// assert!(!contains_sensitive_keyword(&CommandArg::new("path=/tmp".into()))); -/// ``` -pub(crate) fn contains_sensitive_keyword(arg: &CommandArg) -> bool { - let lower = arg.as_str().to_lowercase(); - lower.contains("password") || lower.contains("token") || lower.contains("secret") -} - -/// Determine whether the argument should be redacted. -/// -/// # Examples -/// ``` -/// # use netsuke::runner::doc::{CommandArg, is_sensitive_arg}; -/// assert!(is_sensitive_arg(&CommandArg::new("password=123".into()))); -/// assert!(!is_sensitive_arg(&CommandArg::new("file=readme".into()))); -/// ``` -pub(crate) fn is_sensitive_arg(arg: &CommandArg) -> bool { - contains_sensitive_keyword(arg) -} - -/// Redact sensitive information in a single argument. -/// -/// Sensitive values are replaced with `***REDACTED***`, preserving keys. -/// -/// # Examples -/// ``` -/// # use netsuke::runner::doc::{CommandArg, redact_argument}; -/// let arg = CommandArg::new("token=abc".into()); -/// assert_eq!(redact_argument(&arg).as_str(), "token=***REDACTED***"); -/// let arg = CommandArg::new("path=/tmp".into()); -/// assert_eq!(redact_argument(&arg).as_str(), "path=/tmp"); -/// ``` -pub(crate) fn redact_argument(arg: &CommandArg) -> CommandArg { - if is_sensitive_arg(arg) { - let redacted = arg.as_str().split_once('=').map_or_else( - || "***REDACTED***".to_string(), - |(key, _)| format!("{key}=***REDACTED***"), - ); - CommandArg::new(redacted) - } else { - arg.clone() - } -} - -/// Redact sensitive information from all `args`. -/// -/// # Examples -/// ``` -/// # use netsuke::runner::doc::{CommandArg, redact_sensitive_args}; -/// let args = vec![ -/// CommandArg::new("ninja".into()), -/// CommandArg::new("token=abc".into()), -/// ]; -/// let redacted = redact_sensitive_args(&args); -/// assert_eq!(redacted[1].as_str(), "token=***REDACTED***"); -/// ``` -pub(crate) fn redact_sensitive_args(args: &[CommandArg]) -> Vec { - args.iter().map(redact_argument).collect() -} - -/// Invoke the Ninja executable with the provided CLI settings. -/// -/// The function forwards the job count and working directory to Ninja, -/// specifies the temporary build file, and streams its standard output and -/// error back to the user. -/// -/// # Errors -/// -/// Returns an [`io::Error`] if the Ninja process fails to spawn or reports a -/// non-zero exit status. -/// -/// # Panics -/// -/// Panics if the child's output streams cannot be captured. -pub fn run_ninja( - program: &Path, - cli: &Cli, - build_file: &Path, - targets: &BuildTargets<'_>, -) -> io::Result<()> { - let mut cmd = Command::new(program); - if let Some(dir) = &cli.directory { - // Resolve and canonicalise the directory so Ninja receives a stable - // absolute path. Using only `current_dir` avoids combining it with - // Ninja's own `-C` flag which would otherwise double-apply the - // directory and break relative paths. - let dir = fs::canonicalize(dir)?; - cmd.current_dir(dir); - } - if let Some(jobs) = cli.jobs { - cmd.arg("-j").arg(jobs.to_string()); - } - // Canonicalise the build file path so Ninja resolves it correctly from the - // working directory. Fall back to the original on failure so Ninja can - // surface a meaningful error. - let build_file_path = build_file - .canonicalize() - .unwrap_or_else(|_| build_file.to_path_buf()); - cmd.arg("-f").arg(&build_file_path); - cmd.args(targets.as_slice()); - cmd.stdout(Stdio::piped()); - cmd.stderr(Stdio::piped()); - - let program = cmd.get_program().to_string_lossy().into_owned(); - let args: Vec = cmd - .get_args() - .map(|a| CommandArg::new(a.to_string_lossy().into_owned())) - .collect(); - let redacted_args = redact_sensitive_args(&args); - let arg_strings: Vec<&str> = redacted_args.iter().map(CommandArg::as_str).collect(); - info!("Running command: {} {}", program, arg_strings.join(" ")); - - let mut child = cmd.spawn()?; - let stdout = child.stdout.take().expect("child stdout"); - let stderr = child.stderr.take().expect("child stderr"); - - let out_handle = thread::spawn(move || { - let reader = BufReader::new(stdout); - let mut handle = io::stdout(); - for line in reader.lines().map_while(Result::ok) { - let _ = writeln!(handle, "{line}"); - } - }); - let err_handle = thread::spawn(move || { - let reader = BufReader::new(stderr); - let mut handle = io::stderr(); - for line in reader.lines().map_while(Result::ok) { - let _ = writeln!(handle, "{line}"); - } - }); - - let status = child.wait()?; - let _ = out_handle.join(); - let _ = err_handle.join(); - - if status.success() { - Ok(()) - } else { - #[expect( - clippy::io_other_error, - reason = "use explicit error kind for compatibility with older Rust" - )] - Err(io::Error::new( - io::ErrorKind::Other, - format!("ninja exited with {status}"), - )) - } -} diff --git a/src/runner/process.rs b/src/runner/process.rs new file mode 100644 index 00000000..b4a6f187 --- /dev/null +++ b/src/runner/process.rs @@ -0,0 +1,196 @@ +use super::BuildTargets; +use crate::cli::Cli; +use std::{ + fs, + io::{self, BufRead, BufReader, Write}, + path::Path, + process::{Command, Stdio}, + thread, +}; +use tracing::info; + +#[derive(Debug, Clone)] +pub struct CommandArg(String); +impl CommandArg { + #[must_use] + pub fn new(arg: String) -> Self { + Self(arg) + } + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +// Public helpers for doctests only. This exposes internal helpers as a stable +// testing surface without exporting them in release builds. +#[cfg(doctest)] +#[doc(hidden)] +pub mod doc { + pub use super::CommandArg; + + #[must_use] + pub fn contains_sensitive_keyword(arg: &CommandArg) -> bool { + super::contains_sensitive_keyword(arg) + } + #[must_use] + pub fn is_sensitive_arg(arg: &CommandArg) -> bool { + super::is_sensitive_arg(arg) + } + #[must_use] + pub fn redact_argument(arg: &CommandArg) -> CommandArg { + super::redact_argument(arg) + } + #[must_use] + pub fn redact_sensitive_args(args: &[CommandArg]) -> Vec { + super::redact_sensitive_args(args) + } +} + +/// Check if `arg` contains a sensitive keyword. +/// +/// # Examples +/// ``` +/// # use netsuke::runner::doc::{CommandArg, contains_sensitive_keyword}; +/// assert!(contains_sensitive_keyword(&CommandArg::new("token=abc".into()))); +/// assert!(!contains_sensitive_keyword(&CommandArg::new("path=/tmp".into()))); +/// ``` +pub(crate) fn contains_sensitive_keyword(arg: &CommandArg) -> bool { + let lower = arg.as_str().to_lowercase(); + lower.contains("password") || lower.contains("token") || lower.contains("secret") +} + +/// Determine whether the argument should be redacted. +/// Determine whether the argument should be redacted. +/// +/// # Examples +/// ``` +/// # use netsuke::runner::doc::{CommandArg, is_sensitive_arg}; +/// assert!(is_sensitive_arg(&CommandArg::new("password=123".into()))); +/// assert!(!is_sensitive_arg(&CommandArg::new("file=readme".into()))); +/// ``` +pub(crate) fn is_sensitive_arg(arg: &CommandArg) -> bool { + contains_sensitive_keyword(arg) +} + +/// Redact sensitive information in a single argument. +/// +/// Sensitive values are replaced with `***REDACTED***`, preserving keys. +/// +/// # Examples +/// ``` +/// # use netsuke::runner::doc::{CommandArg, redact_argument}; +/// let arg = CommandArg::new("token=abc".into()); +/// assert_eq!(redact_argument(&arg).as_str(), "token=***REDACTED***"); +/// let arg = CommandArg::new("path=/tmp".into()); +/// assert_eq!(redact_argument(&arg).as_str(), "path=/tmp"); +/// ``` +pub(crate) fn redact_argument(arg: &CommandArg) -> CommandArg { + if is_sensitive_arg(arg) { + let redacted = arg.as_str().split_once('=').map_or_else( + || "***REDACTED***".to_string(), + |(key, _)| format!("{key}=***REDACTED***"), + ); + CommandArg::new(redacted) + } else { + arg.clone() + } +} + +/// Redact sensitive information from all `args`. +/// +/// # Examples +/// ``` +/// # use netsuke::runner::doc::{CommandArg, redact_sensitive_args}; +/// let args = vec![ +/// CommandArg::new("ninja".into()), +/// CommandArg::new("token=abc".into()), +/// ]; +/// let redacted = redact_sensitive_args(&args); +/// assert_eq!(redacted[1].as_str(), "token=***REDACTED***"); +/// ``` +pub(crate) fn redact_sensitive_args(args: &[CommandArg]) -> Vec { + args.iter().map(redact_argument).collect() +} + +/// Invoke the Ninja executable with the provided CLI settings. +/// +/// The function forwards the job count and working directory to Ninja, +/// specifies the temporary build file, and streams its standard output and +/// error back to the user. +/// +/// # Errors +/// +/// Returns an [`io::Error`] if the Ninja process fails to spawn or reports a +/// non-zero exit status. +/// +/// # Panics +/// +/// Panics if the child's output streams cannot be captured. +pub fn run_ninja( + program: &Path, + cli: &Cli, + build_file: &Path, + targets: &BuildTargets<'_>, +) -> io::Result<()> { + let mut cmd = Command::new(program); + if let Some(dir) = &cli.directory { + let dir = fs::canonicalize(dir)?; + cmd.current_dir(dir); + } + if let Some(jobs) = cli.jobs { + cmd.arg("-j").arg(jobs.to_string()); + } + let build_file_path = build_file + .canonicalize() + .unwrap_or_else(|_| build_file.to_path_buf()); + cmd.arg("-f").arg(&build_file_path); + cmd.args(targets.as_slice()); + cmd.stdout(Stdio::piped()); + cmd.stderr(Stdio::piped()); + + let program = cmd.get_program().to_string_lossy().into_owned(); + let args: Vec = cmd + .get_args() + .map(|a| CommandArg::new(a.to_string_lossy().into_owned())) + .collect(); + let redacted_args = redact_sensitive_args(&args); + let arg_strings: Vec<&str> = redacted_args.iter().map(CommandArg::as_str).collect(); + info!("Running command: {} {}", program, arg_strings.join(" ")); + + let mut child = cmd.spawn()?; + let stdout = child.stdout.take().expect("child stdout"); + let stderr = child.stderr.take().expect("child stderr"); + + let out_handle = thread::spawn(move || { + let reader = BufReader::new(stdout); + let mut handle = io::stdout(); + for line in reader.lines().map_while(Result::ok) { + let _ = writeln!(handle, "{line}"); + } + }); + let err_handle = thread::spawn(move || { + let reader = BufReader::new(stderr); + let mut handle = io::stderr(); + for line in reader.lines().map_while(Result::ok) { + let _ = writeln!(handle, "{line}"); + } + }); + + let status = child.wait()?; + let _ = out_handle.join(); + let _ = err_handle.join(); + + if status.success() { + Ok(()) + } else { + #[expect( + clippy::io_other_error, + reason = "use explicit error kind for compatibility with older Rust" + )] + Err(io::Error::new( + io::ErrorKind::Other, + format!("ninja exited with {status}"), + )) + } +} diff --git a/tests/ast_tests.rs b/tests/ast_tests.rs index 132c20ac..19ea6844 100644 --- a/tests/ast_tests.rs +++ b/tests/ast_tests.rs @@ -1,6 +1,6 @@ //! Unit tests for Netsuke manifest AST deserialisation. -use miette::Result; +use anyhow::Result; use netsuke::{ast::*, manifest}; use rstest::rstest; use semver::Version; @@ -291,7 +291,7 @@ fn phony_and_always_flags() { true, true )] -fn actions_behavior( +fn actions_behaviour( #[case] yaml: &str, #[case] expected_phony: bool, #[case] expected_always: bool, diff --git a/tests/cucumber.rs b/tests/cucumber.rs index 422225ac..5267767d 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -14,6 +14,10 @@ pub struct CliWorld { pub build_graph: Option, /// Generated Ninja file content. pub ninja: Option, + /// Error message from Ninja generation. + pub ninja_error: Option, + /// Identifier of the action removed for negative tests. + pub removed_action_id: Option, /// Status of the last process execution (true for success, false for /// failure). pub run_status: Option, diff --git a/tests/features/ninja.feature b/tests/features/ninja.feature index e4d1dd39..d714fd90 100644 --- a/tests/features/ninja.feature +++ b/tests/features/ninja.feature @@ -22,3 +22,9 @@ Feature: Ninja file generation And the ninja file is generated Then shlex splitting command 3 yields "printf, %s, -in file, >, o'utfile" + Scenario: Missing action is reported + When the manifest file "tests/data/rules.yml" is compiled to IR + And an action is removed from the graph + And the ninja file is generated + Then ninja generation fails mentioning the removed action id + diff --git a/tests/ninja_gen_tests.rs b/tests/ninja_gen_tests.rs index 2ddd5dd8..58e6d7ec 100644 --- a/tests/ninja_gen_tests.rs +++ b/tests/ninja_gen_tests.rs @@ -8,7 +8,7 @@ use insta::{Settings, assert_snapshot}; use netsuke::ast::Recipe; use netsuke::ir::{Action, BuildEdge, BuildGraph}; -use netsuke::ninja_gen::generate; +use netsuke::ninja_gen::{NinjaGenError, generate, generate_into}; use rstest::{fixture, rstest}; use std::{fs, path::PathBuf, process::Command}; use tempfile::{TempDir, tempdir}; @@ -131,14 +131,14 @@ fn generate_ninja_scenarios( graph.actions.insert(edge.action_id.clone(), action); graph.targets.insert(target_path, edge); - let ninja = generate(&graph); + let ninja = generate(&graph).expect("generate ninja"); assert_eq!(ninja, expected); } #[rstest] fn generate_empty_graph() { let graph = BuildGraph::default(); - let ninja = generate(&graph); + let ninja = generate(&graph).expect("generate ninja"); assert!(ninja.is_empty()); } @@ -172,7 +172,7 @@ fn generate_multiline_script_snapshot() { ); graph.default_targets.push(PathBuf::from("out")); - let ninja = generate(&graph); + let ninja = generate(&graph).expect("generate ninja"); let mut settings = Settings::new(); settings.set_snapshot_path(concat!( env!("CARGO_MANIFEST_DIR"), @@ -299,7 +299,7 @@ fn ninja_integration_tests( graph.targets.insert(output.clone(), edge); graph.default_targets.push(output); - let ninja = generate(&graph); + let ninja = generate(&graph).expect("generate ninja"); fs::write(dir.path().join("build.ninja"), &ninja).expect("write ninja"); let status = Command::new("ninja") .args(&ninja_args) @@ -321,3 +321,59 @@ fn ninja_integration_tests( } } } + +#[rstest] +fn errors_when_action_missing() { + let mut graph = BuildGraph::default(); + let edge = BuildEdge { + action_id: "missing".into(), + inputs: Vec::new(), + explicit_outputs: vec![PathBuf::from("out")], + implicit_outputs: Vec::new(), + order_only_deps: Vec::new(), + phony: false, + always: false, + }; + graph.targets.insert(PathBuf::from("out"), edge); + let err = generate(&graph).expect_err("missing action"); + assert!(matches!(err, NinjaGenError::MissingAction { id } if id == "missing")); +} + +#[rstest] +fn generate_format_error() { + use std::fmt::{self, Write}; + + struct FailWriter; + impl Write for FailWriter { + fn write_str(&mut self, _: &str) -> fmt::Result { + Err(fmt::Error) + } + } + + let action = Action { + recipe: Recipe::Command { + command: "true".into(), + }, + description: None, + depfile: None, + deps_format: None, + pool: None, + restat: false, + }; + let edge = BuildEdge { + action_id: "a".into(), + inputs: Vec::new(), + explicit_outputs: vec![PathBuf::from("out")], + implicit_outputs: Vec::new(), + order_only_deps: Vec::new(), + phony: false, + always: false, + }; + let mut graph = BuildGraph::default(); + graph.actions.insert("a".into(), action); + graph.targets.insert(PathBuf::from("out"), edge); + + let mut writer = FailWriter; + let err = generate_into(&graph, &mut writer).expect_err("format error"); + assert!(matches!(err, NinjaGenError::Format(_))); +} diff --git a/tests/ninja_snapshot_tests.rs b/tests/ninja_snapshot_tests.rs index 10b3d60e..c80537d4 100644 --- a/tests/ninja_snapshot_tests.rs +++ b/tests/ninja_snapshot_tests.rs @@ -40,7 +40,7 @@ fn touch_manifest_ninja_validation() { let manifest = manifest::from_str(manifest_yaml).expect("parse manifest"); let ir = BuildGraph::from_manifest(&manifest).expect("ir generation"); - let ninja_content = ninja_gen::generate(&ir); + let ninja_content = ninja_gen::generate(&ir).expect("generate ninja"); let mut settings = Settings::new(); settings.set_snapshot_path(concat!( diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index ceac21b9..7dcf6425 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -1,8 +1,8 @@ //! Step definitions for `BuildGraph` scenarios. use crate::CliWorld; +use anyhow::Context; use cucumber::{given, then, when}; -use miette::{Context, IntoDiagnostic}; use netsuke::ir::BuildGraph; fn assert_graph(world: &CliWorld) { @@ -51,11 +51,7 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { #[when(expr = "the manifest file {string} is compiled to IR")] fn compile_manifest(world: &mut CliWorld, path: String) { match netsuke::manifest::from_path(&path) - .and_then(|m| { - BuildGraph::from_manifest(&m) - .into_diagnostic() - .wrap_err("building IR from manifest") - }) + .and_then(|m| BuildGraph::from_manifest(&m).context("building IR from manifest")) .with_context(|| format!("IR generation failed for {path}")) { Ok(graph) => { @@ -88,6 +84,16 @@ fn generation_result_checked(world: &mut CliWorld) { fn ir_generation_fails(world: &mut CliWorld) { assert!( world.manifest_error.is_some(), - "expected IR generation error" + "expected IR generation error", ); } + +#[when("an action is removed from the graph")] +fn remove_action(world: &mut CliWorld) { + let graph = world.build_graph.as_mut().expect("graph"); + let first_action = graph.targets.values().next().map(|e| e.action_id.clone()); + if let Some(id) = first_action { + graph.actions.remove(&id); + world.removed_action_id = Some(id); + } +} diff --git a/tests/steps/ninja_steps.rs b/tests/steps/ninja_steps.rs index 6800fc4c..412af163 100644 --- a/tests/steps/ninja_steps.rs +++ b/tests/steps/ninja_steps.rs @@ -10,14 +10,23 @@ fn generate_ninja(world: &mut CliWorld) { .build_graph .as_ref() .expect("build graph should be available"); - world.ninja = Some(ninja_gen::generate(graph)); + match ninja_gen::generate(graph) { + Ok(n) => { + world.ninja = Some(n); + world.ninja_error = None; + } + Err(e) => { + world.ninja = None; + world.ninja_error = Some(e.to_string()); + } + } } -#[allow( +#[then(expr = "the ninja file contains {string}")] +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )] -#[then(expr = "the ninja file contains {string}")] fn ninja_contains(world: &mut CliWorld, text: String) { let ninja = world .ninja @@ -26,11 +35,11 @@ fn ninja_contains(world: &mut CliWorld, text: String) { assert!(ninja.contains(&text)); } -#[allow( +#[then(expr = "shlex splitting command {int} yields {string}")] +#[expect( clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments" )] -#[then(expr = "shlex splitting command {int} yields {string}")] fn ninja_command_tokens(world: &mut CliWorld, index: usize, expected: String) { let ninja = world .ninja @@ -50,11 +59,33 @@ fn ninja_command_tokens(world: &mut CliWorld, index: usize, expected: String) { assert_eq!(words, expected); } -#[allow( - clippy::needless_pass_by_value, - reason = "Cucumber requires owned String arguments" -)] #[then(expr = "shlex splitting the command yields {string}")] fn ninja_first_command_tokens(world: &mut CliWorld, expected: String) { ninja_command_tokens(world, 2, expected); } + +#[then(expr = "ninja generation fails with {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber requires owned String arguments" +)] +fn ninja_generation_fails(world: &mut CliWorld, text: String) { + let err = world + .ninja_error + .as_ref() + .expect("ninja error should be available"); + assert!(err.contains(&text)); +} + +#[then("ninja generation fails mentioning the removed action id")] +fn ninja_generation_fails_with_removed_action_id(world: &mut CliWorld) { + let err = world + .ninja_error + .as_ref() + .expect("ninja error should be available"); + let id = world + .removed_action_id + .as_ref() + .expect("removed action id should be available"); + assert!(err.contains(id)); +} diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs index d783a834..4f1ad259 100644 --- a/tests/yaml_error_tests.rs +++ b/tests/yaml_error_tests.rs @@ -3,7 +3,6 @@ //! These tests ensure diagnostics include line numbers and optional hints, and //! that rendering is stable across terminals. -use miette::GraphicalReportHandler; use netsuke::manifest; use rstest::rstest; use strip_ansi_escapes::strip; @@ -69,11 +68,12 @@ fn normalise_report(report: &str) -> String { )] fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) { let err = manifest::from_str(yaml).expect_err("parse should fail"); - let mut msg = String::new(); - GraphicalReportHandler::new() - .render_report(&mut msg, err.as_ref()) - .expect("render yaml error"); - let msg = normalise_report(&msg); + let msg = normalise_report( + &err.chain() + .map(ToString::to_string) + .collect::>() + .join("\n"), + ); for needle in needles { assert!(msg.contains(needle), "missing: {needle}\nmessage: {msg}"); }