diff --git a/RELEASE.md b/RELEASE.md new file mode 100644 index 0000000..be4fabf --- /dev/null +++ b/RELEASE.md @@ -0,0 +1,4 @@ +RELEASE_TYPE: patch + +This release should significantly improve the format and quality of output +printing for failing test cases. diff --git a/hegel-macros/Cargo.toml b/hegel-macros/Cargo.toml index 964bfa1..c128cde 100644 --- a/hegel-macros/Cargo.toml +++ b/hegel-macros/Cargo.toml @@ -16,4 +16,4 @@ proc-macro = true [dependencies] proc-macro2 = "1.0" quote = "1.0" -syn = { version = "2.0", features = ["full"] } +syn = { version = "2.0", features = ["full", "visit-mut"] } diff --git a/hegel-macros/src/hegel_test.rs b/hegel-macros/src/hegel_test.rs index ed4ed9f..6cb2bb6 100644 --- a/hegel-macros/src/hegel_test.rs +++ b/hegel-macros/src/hegel_test.rs @@ -1,7 +1,10 @@ +use std::collections::HashMap; + use proc_macro2::TokenStream; use quote::quote; use syn::parse::{Parse, ParseStream}; -use syn::{Expr, FnArg, Ident, ItemFn, Token}; +use syn::visit_mut::VisitMut; +use syn::{Expr, FnArg, Ident, ItemFn, Pat, Token}; /// A single named argument in a `#[hegel::test(...)]` expression. struct SettingArg { @@ -59,6 +62,120 @@ impl Parse for TestArgs { } } +/// Extract a simple identifier from a pattern, handling type annotations. +fn extract_ident_from_pat(pat: &Pat) -> Option { + match pat { + Pat::Ident(pat_ident) => Some(pat_ident.ident.to_string()), + Pat::Type(pat_type) => extract_ident_from_pat(&pat_type.pat), + _ => None, + } +} + +/// Check if a `let` binding is of the form `let = .draw()`. +fn is_tc_draw_binding(node: &syn::Local, tc_ident: &str) -> Option { + let var_name = extract_ident_from_pat(&node.pat)?; + + let init = node.init.as_ref()?; + let method_call = match &*init.expr { + Expr::MethodCall(mc) => mc, + _ => return None, + }; + + if method_call.method != "draw" || method_call.args.len() != 1 { + return None; + } + + let is_tc = match &*method_call.receiver { + Expr::Path(path) => path.path.is_ident(tc_ident), + _ => false, + }; + if !is_tc { + return None; + } + + Some(var_name) +} + +/// Pass 1: Collect all draw variable names and determine per-name repeatable flags. +/// +/// If any use of a name appears in a repeatable context (nested block, closure), +/// ALL uses of that name become repeatable. This ensures the runtime never sees +/// inconsistent repeatable flags for the same name. +struct DrawNameCollector { + tc_ident: String, + repeatable_depth: usize, + name_flags: HashMap, +} + +impl VisitMut for DrawNameCollector { + fn visit_block_mut(&mut self, node: &mut syn::Block) { + self.repeatable_depth += 1; + syn::visit_mut::visit_block_mut(self, node); + self.repeatable_depth -= 1; + } + + fn visit_expr_closure_mut(&mut self, node: &mut syn::ExprClosure) { + self.repeatable_depth += 1; + syn::visit_mut::visit_expr_closure_mut(self, node); + self.repeatable_depth -= 1; + } + + fn visit_item_fn_mut(&mut self, _node: &mut syn::ItemFn) {} + + fn visit_local_mut(&mut self, node: &mut syn::Local) { + syn::visit_mut::visit_local_mut(self, node); + + if let Some(var_name) = is_tc_draw_binding(node, &self.tc_ident) { + let repeatable = self.repeatable_depth > 0; + let entry = self.name_flags.entry(var_name).or_insert(false); + if repeatable { + *entry = true; + } + } + } +} + +/// Pass 2: Rewrite `let x = tc.draw(gen)` to `let x = tc.draw_named(gen, "x", repeatable)`. +/// +/// Uses the pre-computed name_flags from DrawNameCollector so that every use of +/// a given name gets the same repeatable flag. +struct DrawRewriter { + tc_ident: String, + name_flags: HashMap, +} + +impl VisitMut for DrawRewriter { + fn visit_item_fn_mut(&mut self, _node: &mut syn::ItemFn) {} + + fn visit_local_mut(&mut self, node: &mut syn::Local) { + syn::visit_mut::visit_local_mut(self, node); + + let var_name = match is_tc_draw_binding(node, &self.tc_ident) { + Some(name) => name, + None => return, + }; + + let repeatable = self.name_flags.get(&var_name).copied().unwrap_or(false); + + let init = node.init.as_mut().unwrap(); + let method_call = match &mut *init.expr { + Expr::MethodCall(mc) => mc, + _ => unreachable!(), + }; + + let span = method_call.method.span(); + method_call.method = Ident::new("draw_named", span); + method_call.args.push(Expr::Lit(syn::ExprLit { + attrs: vec![], + lit: syn::Lit::Str(syn::LitStr::new(&var_name, span)), + })); + method_call.args.push(Expr::Lit(syn::ExprLit { + attrs: vec![], + lit: syn::Lit::Bool(syn::LitBool::new(repeatable, span)), + })); + } +} + pub fn expand_test(attr: proc_macro2::TokenStream, item: proc_macro2::TokenStream) -> TokenStream { let test_args: TestArgs = if attr.is_empty() { TestArgs { @@ -110,7 +227,40 @@ pub fn expand_test(attr: proc_macro2::TokenStream, item: proc_macro2::TokenStrea } } - let body = &func.block; + // Rewrite `let x = tc.draw(gen)` -> `let x = tc.draw_named(gen, "x", repeatable)` + // + // Two-pass approach: + // 1. Collect all draw variable names and determine per-name repeatable flags. + // If any use of a name is in a nested block/closure, all uses are repeatable. + // 2. Rewrite draws using the computed flags. + // + // We visit the function body's statements directly (not the block itself) so that + // the outermost block doesn't count as a nesting level. + let body = { + let mut body = (*func.block).clone(); + if let Some(tc_name) = extract_ident_from_pat(param_pat) { + // Pass 1: collect names + let mut collector = DrawNameCollector { + tc_ident: tc_name.clone(), + repeatable_depth: 0, + name_flags: HashMap::new(), + }; + for stmt in &mut body.stmts { + collector.visit_stmt_mut(stmt); + } + + // Pass 2: rewrite + let mut rewriter = DrawRewriter { + tc_ident: tc_name, + name_flags: collector.name_flags, + }; + for stmt in &mut body.stmts { + rewriter.visit_stmt_mut(stmt); + } + } + body + }; + let test_name = func.sig.ident.to_string(); let settings_args_chain: Vec = test_args diff --git a/src/test_case.rs b/src/test_case.rs index 4f229da..2400be4 100644 --- a/src/test_case.rs +++ b/src/test_case.rs @@ -4,6 +4,7 @@ use crate::protocol::{Channel, Connection, SERVER_CRASHED_MESSAGE}; use crate::runner::Verbosity; use ciborium::Value; use std::cell::RefCell; +use std::collections::{HashMap, HashSet}; use std::rc::Rc; use std::sync::{Arc, LazyLock}; @@ -63,12 +64,14 @@ pub(crate) struct TestCaseGlobalData { verbosity: Verbosity, is_last_run: bool, test_aborted: bool, + named_draw_counts: HashMap, + named_draw_repeatable: HashMap, + allocated_display_names: HashSet, } #[derive(Clone)] pub(crate) struct TestCaseLocalData { span_depth: usize, - draw_count: usize, indent: usize, on_draw: Rc, } @@ -129,10 +132,12 @@ impl TestCase { verbosity, is_last_run, test_aborted: false, + named_draw_counts: HashMap::new(), + named_draw_repeatable: HashMap::new(), + allocated_display_names: HashSet::new(), })), local: RefCell::new(TestCaseLocalData { span_depth: 0, - draw_count: 0, indent: 0, on_draw, }), @@ -152,10 +157,37 @@ impl TestCase { /// let s: String = tc.draw(gs::text()); /// } /// ``` + /// + /// Note: when run inside a `#[hegel::test]`, `draw()` will typically be + /// rewritten to `draw_named()` with an appropriate variable name + /// in order to give better test output. pub fn draw(&self, generator: impl Generator) -> T { + self.draw_named(generator, "draw", true) + } + + /// Draw a value from a generator with a specific name for output. + /// + /// When `repeatable` is true, a counter suffix is appended (e.g. `x_1`, `x_2`). + /// When `repeatable` is false, reusing the same name panics. + /// + /// Using the same name with different values of `repeatable` is an error. + /// + /// On the final replay of a failing test case, this prints: + /// - `let name = value;` (when not repeatable) + /// - `let name_N = value;` (when repeatable) + /// + /// Note: although this is public API and you are welcome to use it, + /// it's not really intended for direct use. It is the target that + /// `#[hegel::test]` rewrites `draw()` calls to where appropriate. + pub fn draw_named( + &self, + generator: impl Generator, + name: &str, + repeatable: bool, + ) -> T { let value = generator.do_draw(self); if self.local.borrow().span_depth == 0 { - self.record_draw(&value); + self.record_named_draw(&value, name, repeatable); } value } @@ -215,22 +247,70 @@ impl TestCase { global: self.global.clone(), local: RefCell::new(TestCaseLocalData { span_depth: 0, - draw_count: 0, indent: local.indent + extra_indent, on_draw: local.on_draw.clone(), }), } } - fn record_draw(&self, value: &T) { - let mut local = self.local.borrow_mut(); - local.draw_count += 1; - let count = local.draw_count; + fn record_named_draw(&self, value: &T, name: &str, repeatable: bool) { + let mut global = self.global.borrow_mut(); + + match global.named_draw_repeatable.get(name) { + Some(&prev) if prev != repeatable => { + panic!( + "draw_named: name {:?} used with inconsistent repeatable flag (was {}, now {}). \ + If you have not called draw_named deliberately yourself, this is likely a bug in \ + hegel. Please file a bug report at https://github.com/hegeldev/hegel-rust/issues", + name, prev, repeatable + ); + } + _ => { + global + .named_draw_repeatable + .insert(name.to_string(), repeatable); + } + } + + let count = global + .named_draw_counts + .entry(name.to_string()) + .or_insert(0); + *count += 1; + let current_count = *count; + + if !repeatable && current_count > 1 { + panic!( + "draw_named: name {:?} used more than once but repeatable is false. \ + If you have not called draw_named deliberately yourself, this is likely a bug in \ + hegel. Please file a bug report at https://github.com/hegeldev/hegel-rust/issues", + name + ); + } + + let display_name = if repeatable { + let mut candidate = current_count; + loop { + let name = format!("{}_{}", name, candidate); + if global.allocated_display_names.insert(name.clone()) { + break name; + } + candidate += 1; + } + } else { + let name = name.to_string(); + global.allocated_display_names.insert(name.clone()); + name + }; + drop(global); + + let local = self.local.borrow(); let indent = local.indent; + (local.on_draw)(&format!( - "{:indent$}Draw {}: {:?}", + "{:indent$}let {} = {:?};", "", - count, + display_name, value, indent = indent )); diff --git a/tests/test_draw_named.rs b/tests/test_draw_named.rs new file mode 100644 index 0000000..d751ea5 --- /dev/null +++ b/tests/test_draw_named.rs @@ -0,0 +1,342 @@ +mod common; + +use common::project::TempRustProject; +use common::utils::{assert_matches_regex, expect_panic}; +use hegel::TestCase; +use hegel::generators; + +// ============================================================ +// draw_named runtime behavior +// ============================================================ + +#[test] +fn test_draw_named_non_repeatable_reuse_panics() { + expect_panic( + || { + hegel::Hegel::new(|tc: hegel::TestCase| { + let _a = tc.draw_named(generators::booleans(), "x", false); + let _b = tc.draw_named(generators::booleans(), "x", false); + }) + .settings(hegel::Settings::new().test_cases(1)) + .run(); + }, + r#"draw_named.*"x".*more than once"#, + ); +} + +#[hegel::test(test_cases = 1)] +fn test_draw_named_repeatable_reuse_ok(tc: TestCase) { + let _a = tc.draw_named(generators::booleans(), "x", true); + let _b = tc.draw_named(generators::booleans(), "x", true); +} + +#[hegel::test(test_cases = 1)] +fn test_draw_named_repeatable_skips_taken_name(tc: TestCase) { + tc.draw_named(generators::booleans(), "x_1", false); + tc.draw_named(generators::booleans(), "x", true); + tc.draw_named(generators::booleans(), "x", true); +} + +#[hegel::test(test_cases = 1)] +fn test_draw_named_different_names_ok(tc: TestCase) { + let _a = tc.draw_named(generators::booleans(), "x", false); + let _b = tc.draw_named(generators::booleans(), "y", false); +} + +#[test] +fn test_draw_named_mixed_repeatable_panics() { + expect_panic( + || { + hegel::Hegel::new(|tc: hegel::TestCase| { + let _a = tc.draw_named(generators::booleans(), "x", false); + let _b = tc.draw_named(generators::booleans(), "x", true); + }) + .settings(hegel::Settings::new().test_cases(1)) + .run(); + }, + r#"draw_named.*inconsistent.*repeatable"#, + ); +} + +#[test] +fn test_draw_named_mixed_repeatable_reverse_panics() { + expect_panic( + || { + hegel::Hegel::new(|tc: hegel::TestCase| { + let _a = tc.draw_named(generators::booleans(), "x", true); + let _b = tc.draw_named(generators::booleans(), "x", false); + }) + .settings(hegel::Settings::new().test_cases(1)) + .run(); + }, + r#"draw_named.*inconsistent.*repeatable"#, + ); +} + +// ============================================================ +// draw_named output format (via TempRustProject) +// ============================================================ + +#[test] +fn test_draw_named_non_repeatable_output_format() { + let code = r#" +fn main() { + hegel::hegel(|tc| { + let _x = tc.draw_named(hegel::generators::integers::(), "my_var", false); + panic!("intentional"); + }); +} +"#; + let output = TempRustProject::new() + .main_file(code) + .expect_failure("intentional") + .cargo_run(&[]); + + assert_matches_regex(&output.stderr, r"let my_var = -?\d+;"); + assert!( + !output.stderr.contains("my_var_"), + "Non-repeatable should not have suffix. Actual: {}", + output.stderr + ); +} + +#[test] +fn test_draw_named_repeatable_output_format() { + let code = r#" +fn main() { + hegel::hegel(|tc| { + let _a = tc.draw_named(hegel::generators::integers::(), "val", true); + let _b = tc.draw_named(hegel::generators::integers::(), "val", true); + panic!("intentional"); + }); +} +"#; + let output = TempRustProject::new() + .main_file(code) + .expect_failure("intentional") + .cargo_run(&[]); + + assert_matches_regex(&output.stderr, r"let val_1 = -?\d+;"); + assert_matches_regex(&output.stderr, r"let val_2 = -?\d+;"); +} + +// ============================================================ +// Macro rewriting: succeeding tests (inline) +// ============================================================ + +#[hegel::test(test_cases = 1)] +fn test_macro_unique_names_at_top_level(tc: TestCase) { + let x = tc.draw(generators::booleans()); + let y = tc.draw(generators::booleans()); + let _ = (x, y); +} + +#[hegel::test(test_cases = 1)] +fn test_macro_for_loop_is_repeatable(tc: TestCase) { + for _ in 0..3 { + let val = tc.draw(generators::booleans()); + let _ = val; + } +} + +#[hegel::test(test_cases = 1)] +fn test_macro_while_loop_is_repeatable(tc: TestCase) { + let mut i = 0; + while i < 3 { + let val = tc.draw(generators::booleans()); + let _ = val; + i += 1; + } +} + +#[hegel::test(test_cases = 1)] +fn test_macro_loop_is_repeatable(tc: TestCase) { + let mut i = 0; + loop { + let val = tc.draw(generators::booleans()); + let _ = val; + i += 1; + if i >= 3 { + break; + } + } +} + +#[hegel::test(test_cases = 1)] +fn test_macro_closure_is_repeatable(tc: TestCase) { + #[allow(clippy::let_and_return)] + let f = || { + let val = tc.draw(generators::booleans()); + val + }; + let _a = f(); + let _b = f(); +} + +#[hegel::test(test_cases = 1)] +fn test_macro_non_assignment_draw_not_rewritten(tc: TestCase) { + // draw calls not in `let x = tc.draw(...)` form stay as draw(), + // which delegates to draw_named("draw", true) — repeatable, so no panic. + let _ = vec![ + tc.draw(generators::booleans()), + tc.draw(generators::booleans()), + ]; +} + +#[hegel::test(test_cases = 1)] +fn test_macro_type_annotated_draw(tc: TestCase) { + let x: bool = tc.draw(generators::booleans()); + let y: bool = tc.draw(generators::booleans()); + let _ = (x, y); +} + +#[hegel::test(test_cases = 1)] +fn test_macro_draw_in_if_is_repeatable(tc: TestCase) { + // Draw inside an if block is repeatable (block scope allows variable shadowing). + // Using unique names here, so this trivially succeeds. + if true { + let a = tc.draw(generators::booleans()); + let _ = a; + } + let b = tc.draw(generators::booleans()); + let _ = b; +} + +#[hegel::test(test_cases = 1)] +fn test_macro_variable_shadowing_in_block(tc: TestCase) { + // Same variable name at top level and inside a block should work, + // because the block-nested draw is repeatable (shadowing is expected). + let x = tc.draw(generators::booleans()); + let _ = x; + { + let x = tc.draw(generators::booleans()); + let _ = x; + } +} + +#[hegel::test(test_cases = 1)] +fn test_macro_shadowing_in_if_block(tc: TestCase) { + let x = tc.draw(generators::booleans()); + let _ = x; + if true { + let x = tc.draw(generators::booleans()); + let _ = x; + } +} + +// ============================================================ +// Macro rewriting: failing tests (via TempRustProject) +// ============================================================ + +#[test] +fn test_macro_top_level_same_name_panics() { + let code = r#" +#[hegel::test(test_cases = 1)] +fn test_dup(tc: hegel::TestCase) { + let x = tc.draw(hegel::generators::booleans()); + let x = tc.draw(hegel::generators::booleans()); + let _ = x; +} +"#; + TempRustProject::new() + .test_file("test_dup.rs", code) + .expect_failure(r#"draw_named.*"x".*more than once"#) + .cargo_test(&["--test", "test_dup"]); +} + +#[test] +fn test_macro_if_block_same_name_ok() { + // Draw inside if block is repeatable due to potential shadowing, + // so reusing the same name across the if body and outside is fine. + let code = r#" +#[hegel::test(test_cases = 1)] +fn test_if_dup(tc: hegel::TestCase) { + if true { + let x = tc.draw(hegel::generators::booleans()); + let _ = x; + } + let x = tc.draw(hegel::generators::booleans()); + let _ = x; +} +"#; + TempRustProject::new() + .test_file("test_if_dup.rs", code) + .cargo_test(&["--test", "test_if_dup"]); +} + +// ============================================================ +// Macro rewriting: output format (via TempRustProject) +// ============================================================ + +#[test] +fn test_macro_output_uses_variable_name() { + let code = r#" +fn main() { + hegel::hegel(|tc| { + // Simulate what #[hegel::test] would produce for: + // let my_number: i32 = tc.draw(generators::integers()); + let my_number: i32 = tc.draw_named(hegel::generators::integers(), "my_number", false); + panic!("fail: {}", my_number); + }); +} +"#; + let output = TempRustProject::new() + .main_file(code) + .expect_failure("fail") + .cargo_run(&[]); + + assert_matches_regex(&output.stderr, r"let my_number = -?\d+;"); +} + +#[test] +fn test_macro_loop_output_has_counter() { + let code = r#" +fn main() { + hegel::hegel(|tc| { + // Simulate what #[hegel::test] would produce for a loop: + // for _ in 0..2 { let val: i32 = tc.draw(generators::integers()); } + for _ in 0..2 { + let val: i32 = tc.draw_named(hegel::generators::integers(), "val", true); + let _ = val; + } + panic!("fail"); + }); +} +"#; + let output = TempRustProject::new() + .main_file(code) + .expect_failure("fail") + .cargo_run(&[]); + + assert_matches_regex(&output.stderr, r"let val_1 = -?\d+;"); + assert_matches_regex(&output.stderr, r"let val_2 = -?\d+;"); +} + +#[test] +fn test_repeatable_draw_skips_already_allocated_names() { + let code = r#" +fn main() { + hegel::hegel(|tc| { + // Simulate what the macro would produce for: + // let x_1 = tc.draw(gen); + // for _ in 0..2 { let x = tc.draw(gen); } + // The repeatable "x" draws must skip "x_1" which is already taken. + let _x_1 = tc.draw_named(hegel::generators::integers::(), "x_1", false); + for _ in 0..2 { + let _x = tc.draw_named(hegel::generators::integers::(), "x", true); + } + panic!("fail"); + }); +} +"#; + let output = TempRustProject::new() + .main_file(code) + .expect_failure("fail") + .cargo_run(&[]); + + // "x_1" is used by the non-repeatable draw + assert_matches_regex(&output.stderr, r"let x_1 = -?\d+;"); + // The repeatable draws should skip x_1 and use x_2, x_3 + assert_matches_regex(&output.stderr, r"let x_2 = -?\d+;"); + assert_matches_regex(&output.stderr, r"let x_3 = -?\d+;"); +} diff --git a/tests/test_output.rs b/tests/test_output.rs index cc4d5e1..911b6e9 100644 --- a/tests/test_output.rs +++ b/tests/test_output.rs @@ -22,14 +22,14 @@ fn test_failing_test_output() { .cargo_run(&[]); // For example: - // Draw 1: 0 + // let draw_1 = 0; // thread 'main' (1) panicked at src/main.rs:7:9: // intentional failure: 0 // note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace assert_matches_regex( &output.stderr, concat!( - r"Draw 1: -?\d+\n", + r"let draw_1 = -?\d+;\n", r"thread '.*' \(\d+\) panicked at src/main\.rs:\d+:\d+:\n", r"intentional failure: -?\d+", ), @@ -48,7 +48,7 @@ fn test_failing_test_output_with_backtrace() { // macOS stable (the exact conditions aren't fully understood). Accept both. let closure_name = r"(?:\{closure#0\}|\{\{closure\}\})"; // For example: - // Draw 1: 0 + // let draw_1 = 0; // thread 'main' (1) panicked at src/main.rs:7:9: // intentional failure: 0 // stack backtrace: @@ -66,7 +66,7 @@ fn test_failing_test_output_with_backtrace() { &format!( concat!( r"(?s)", - r"Draw 1: -?\d+\n", + r"let draw_1 = -?\d+;\n", r"thread 'main' \(\d+\) panicked at src/main\.rs:\d+:\d+:\n", r"intentional failure: -?\d+\n", r"stack backtrace:\n", @@ -103,7 +103,7 @@ fn test_failing_test_output_with_full_backtrace() { &format!( concat!( r"(?s)", - r"Draw 1: -?\d+\n", + r"let draw_1 = -?\d+;\n", r"thread 'main' \(\d+\) panicked at src/main\.rs:\d+:\d+:\n", r"intentional failure: -?\d+\n", r"stack backtrace:\n",