diff --git a/crates/spectacular-macros/src/attr.rs b/crates/spectacular-macros/src/attr.rs index 1f5a7de..3a61b14 100644 --- a/crates/spectacular-macros/src/attr.rs +++ b/crates/spectacular-macros/src/attr.rs @@ -181,7 +181,6 @@ pub(crate) fn expand( let has_before = before_name.is_some(); let has_after = after_name.is_some(); let has_after_each = after_each_name.is_some(); - let test_count = test_fns.len(); // Extract params from before_each (ref params come from before context) let before_each_params = before_each_fn.map(extract_params).unwrap_or_default(); @@ -277,10 +276,36 @@ pub(crate) fn expand( None }; - let countdown_static = has_after.then(|| { + // The `#[after]` hook runs once at process exit (registered via + // `register_teardown`), guarded by this `Once` so it registers a single time + // no matter which — or how many — of the group's tests libtest selects. + // Counting tests at expansion time can't work: name filters, `--skip`, and + // `#[ignore]` all change the run set at runtime (issue #1). + let after_at_exit = has_after.then(|| { + let name = after_name.unwrap(); + let invocation = if after_params.is_empty() { + quote! { #name(); } + } else { + // In practice `#[after]` only takes `&T` references to the + // `#[before]` context, which lives in `__SPEC_BEFORE_CTX` for the + // whole program and is initialized by the first test that runs. + let args = after_params.iter().map(|_| quote! { __spec_after_ctx }); + quote! { + // If `#[before]` never produced context (e.g. no test ran, or + // `#[before]` itself panicked) there is nothing to tear down. + let ::std::option::Option::Some(__spec_after_ctx) = + __SPEC_BEFORE_CTX.get() + else { + return; + }; + #name(#(#args),*); + } + }; quote! { - static __SPEC_AFTER_REMAINING: ::std::sync::atomic::AtomicUsize = - ::std::sync::atomic::AtomicUsize::new(#test_count); + static __SPEC_AFTER_ONCE: ::std::sync::Once = ::std::sync::Once::new(); + fn __spec_after_at_exit() { + #invocation + } } }); @@ -306,6 +331,15 @@ pub(crate) fn expand( let mut pre = proc_macro2::TokenStream::new(); let mut post = proc_macro2::TokenStream::new(); + // --- Register group `after` teardown (runs once, at process exit) --- + if has_after { + pre.extend(quote! { + __SPEC_AFTER_ONCE.call_once(|| { + ::spectacular::__internal::register_teardown(__spec_after_at_exit); + }); + }); + } + // --- Suite before --- if has_suite { pre.extend(quote! { super::__spectacular_suite::before(); }); @@ -530,42 +564,10 @@ pub(crate) fn expand( post.extend(quote! { super::__spectacular_suite::after_each(); }); } - // --- after (countdown) --- - if let Some(name) = after_name { - let call_args: Vec = after_params - .iter() - .map(|p| { - if p.is_ref { - quote! { __before_ctx } - } else { - let pat = &p.pat; - quote! { #pat } - } - }) - .collect(); - - if call_args.is_empty() { - post.extend(quote! { - if __SPEC_AFTER_REMAINING - .fetch_sub(1, ::std::sync::atomic::Ordering::SeqCst) - == 1 - { - #name(); - } - }); - } else { - post.extend(quote! { - if __SPEC_AFTER_REMAINING - .fetch_sub(1, ::std::sync::atomic::Ordering::SeqCst) - == 1 - { - #name(#(#call_args),*); - } - }); - } - } + // NOTE: the group `after` hook is NOT run here. It runs once at + // process exit — see `after_at_exit` and the registration in `pre`. - let needs_catch = has_after || has_after_each || has_suite; + let needs_catch = has_after_each || has_suite; if test_needs_async { let rt = runtime.unwrap(); @@ -597,7 +599,7 @@ pub(crate) fn expand( #vis mod #mod_name { #(#cleaned_items)* #once_static - #countdown_static + #after_at_exit #(#test_fn_defs)* } }) diff --git a/crates/spectacular-macros/src/spec.rs b/crates/spectacular-macros/src/spec.rs index 50d8cc2..814e584 100644 --- a/crates/spectacular-macros/src/spec.rs +++ b/crates/spectacular-macros/src/spec.rs @@ -386,7 +386,6 @@ pub(crate) fn expand(input: proc_macro2::TokenStream) -> syn::Result syn::Result syn::Result syn::Result = after_params - .iter() - .map(|p| { - if p.is_ref { - quote! { __before_ctx } - } else { - let pat = &p.pat; - quote! { #pat } - } - }) - .collect(); - - if call_args.is_empty() { - post.extend(quote! { - if __SPEC_AFTER_REMAINING - .fetch_sub(1, ::std::sync::atomic::Ordering::SeqCst) - == 1 - { - __spec_after(); - } - }); - } else { - post.extend(quote! { - if __SPEC_AFTER_REMAINING - .fetch_sub(1, ::std::sync::atomic::Ordering::SeqCst) - == 1 - { - __spec_after(#(#call_args),*); - } - }); - } - } + // NOTE: the group `after` hook is NOT run here. It runs once at + // process exit — see `after_at_exit` and the registration in `pre`. - let needs_catch = has_after || has_after_each || has_suite; + let needs_catch = has_after_each || has_suite; if test_needs_async { let rt = runtime.unwrap(); @@ -778,7 +777,7 @@ pub(crate) fn expand(input: proc_macro2::TokenStream) -> syn::Result core::ffi::c_int; + } + + static TEARDOWNS: Mutex> = Mutex::new(Vec::new()); + static ATEXIT_REGISTERED: Once = Once::new(); + + extern "C" fn run_teardowns() { + let pending: Vec = { + let mut guard = TEARDOWNS.lock().unwrap_or_else(|e| e.into_inner()); + std::mem::take(&mut *guard) + }; + // Reverse registration order: the last group set up is torn down first. + // Each teardown is a normal Rust `fn`, so a panic (e.g. a failing + // assertion in `after`) unwinds safely into this `catch_unwind` rather + // than crossing the `extern "C"` boundary. + let mut any_panicked = false; + for teardown in pending.into_iter().rev() { + if catch_unwind(AssertUnwindSafe(teardown)).is_err() { + any_panicked = true; + } + } + if any_panicked { + eprintln!("spectacular: an `after` teardown hook panicked during shutdown; aborting"); + std::process::abort(); + } + } + + /// Register a once-per-group teardown to run at process exit. + /// + /// Generated code calls this — guarded by a per-group [`Once`] — the first + /// time one of the group's tests runs, so the group's `after` hook fires + /// exactly once regardless of how libtest filtered the run. + pub fn register_teardown(teardown: fn()) { + ATEXIT_REGISTERED.call_once(|| { + // SAFETY: `run_teardowns` is a valid `extern "C" fn()`; `atexit` is + // provided by the C runtime linked into every Rust binary. + unsafe { + atexit(run_teardowns); + } + }); + TEARDOWNS + .lock() + .unwrap_or_else(|e| e.into_inner()) + .push(teardown); + } } /// Convenience re-export of all spectacular macros. diff --git a/crates/spectacular/tests/integration.rs b/crates/spectacular/tests/integration.rs index eca89e5..496df7f 100644 --- a/crates/spectacular/tests/integration.rs +++ b/crates/spectacular/tests/integration.rs @@ -1050,3 +1050,101 @@ spec! { } } } + +// ===== Regression (issue #1): `#[after]` runs once at process exit, so it +// fires even when libtest runs only a *subset* of a group's tests — via a name +// filter, `--skip`, `#[ignore]`, or `#[cfg]`. A compile-time count could never +// know which tests the harness would select at runtime. +// +// This is verified out-of-process: we re-exec this very test binary with a +// filter that selects a single test of the group, then confirm the group's +// `#[after]` still ran at exit by checking a marker file it writes. + +const AFTER_MARKER_ENV: &str = "SPECTACULAR_AFTER_MARKER"; + +#[test_suite] +mod at_exit_group { + use super::*; + + #[before] + pub fn init() -> String { + "ctx-value".to_string() + } + + // Runs once at process exit. Writes the `#[before]` context to the marker + // file named by `AFTER_MARKER_ENV`, but only in the spawned child (the + // parent's own run leaves the env unset, so this is a no-op there). + #[after] + pub fn teardown(ctx: &String) { + if let Ok(path) = std::env::var(AFTER_MARKER_ENV) { + std::fs::write(path, ctx).expect("failed to write after marker"); + } + } + + #[test] + pub fn first(ctx: &String) { + assert_eq!(ctx, "ctx-value"); + } + + #[test] + pub fn second(ctx: &String) { + assert_eq!(ctx, "ctx-value"); + } + + // Never runs in the child: filtered/ignored/cfg-excluded. `#[after]` must + // still fire regardless. + #[test] + #[ignore] + pub fn ignored() { + unreachable!("ignored test must never run"); + } + + #[test] + #[cfg(any())] + pub fn cfg_excluded() { + unreachable!("cfg-excluded test is never compiled"); + } +} + +#[test] +fn after_runs_at_process_exit_even_when_filtered() { + // Guard: a child (marker env set) must never recurse. The child's filter + // wouldn't select this test anyway, but be explicit. + if std::env::var(AFTER_MARKER_ENV).is_ok() { + return; + } + + let marker = + std::env::temp_dir().join(format!("spectacular_after_marker_{}", std::process::id())); + let _ = std::fs::remove_file(&marker); + + let exe = std::env::current_exe().expect("resolve current test binary"); + let output = std::process::Command::new(exe) + // Run exactly one test of the group; `second` is filtered out, `ignored` + // is skipped, `cfg_excluded` isn't compiled — a strict subset executes. + .args(["--exact", "at_exit_group::first"]) + .env(AFTER_MARKER_ENV, &marker) + .output() + .expect("spawn child test process"); + + assert!( + output.status.success(), + "child run failed: status={:?}\nstdout:\n{}\nstderr:\n{}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + + let contents = std::fs::read_to_string(&marker).unwrap_or_else(|e| { + panic!( + "`#[after]` did not run at process exit (marker missing: {e}); \ + issue #1: teardown skipped when only a subset of tests runs" + ) + }); + let _ = std::fs::remove_file(&marker); + + assert_eq!( + contents, "ctx-value", + "`#[after]` ran but received the wrong `#[before]` context at exit" + ); +} diff --git a/docs/src/content/docs/guides/attribute-style.md b/docs/src/content/docs/guides/attribute-style.md index c13fb3e..ceb7246 100644 --- a/docs/src/content/docs/guides/attribute-style.md +++ b/docs/src/content/docs/guides/attribute-style.md @@ -70,7 +70,7 @@ mod with_hooks { #[after] fn cleanup() { - // runs once after the last test + // runs once at process exit, after the selected tests finish } #[test] diff --git a/docs/src/content/docs/guides/hooks.md b/docs/src/content/docs/guides/hooks.md index f26b8b0..be54b3f 100644 --- a/docs/src/content/docs/guides/hooks.md +++ b/docs/src/content/docs/guides/hooks.md @@ -34,7 +34,7 @@ spec! { ### `after` -- once-per-group teardown -Runs exactly once after the last test in the group completes. Uses an atomic countdown internally -- when the last test decrements the counter to zero, the `after` hook fires. +Runs exactly once at process exit, after every test the harness selected has finished (as long as at least one test in the group ran). Registering teardown to run at exit -- rather than counting tests -- keeps `after` correct even when you run a subset of tests: name filters (`cargo test some_name`), `--skip`, and `#[ignore]`/`#[cfg]` all change which tests run at runtime, and a compile-time count cannot know that set. The one thing it cannot survive is the process being killed (`SIGKILL`) or aborting -- no in-process hook can. ```rust use spectacular::spec; diff --git a/docs/src/content/docs/guides/suite-hooks.md b/docs/src/content/docs/guides/suite-hooks.md index 8f64c8b..7b2f643 100644 --- a/docs/src/content/docs/guides/suite-hooks.md +++ b/docs/src/content/docs/guides/suite-hooks.md @@ -87,7 +87,7 @@ suite::before (Once -- first test in binary triggers it) TEST group::after_each suite::after_each - group::after (countdown -- last test in group triggers it) + group::after (once -- at process exit, if any test in the group ran) ``` Key details: @@ -96,7 +96,7 @@ Key details: - **Group before** runs at most once per group, also guarded by `Once` - **Suite before_each** runs before group's `before_each`, for every test - **After hooks** run in reverse order (innermost first) -- **Group after** uses an atomic countdown -- the last test in the group triggers it +- **Group after** runs once at process exit, so it fires even when only a subset of the group's tests is selected (name filter, `--skip`, `#[ignore]`) ## Mixing Opted-in and Standalone Groups diff --git a/docs/src/content/docs/reference/execution-order.md b/docs/src/content/docs/reference/execution-order.md index 6e86307..58928fb 100644 --- a/docs/src/content/docs/reference/execution-order.md +++ b/docs/src/content/docs/reference/execution-order.md @@ -17,7 +17,7 @@ suite::before (Once -- first test in binary) TEST BODY group::after_each (every test) suite::after_each (every test) - group::after (countdown -- last test in group) + group::after (once -- at process exit, if any test in the group ran) ``` ## Group-Only Order @@ -29,7 +29,7 @@ group::before (Once -- first test in group) group::before_each (every test) TEST BODY group::after_each (every test) -group::after (countdown -- last test in group) +group::after (once -- at process exit, if any test in the group ran) ``` ## No Hooks @@ -50,10 +50,10 @@ TEST BODY ### `after` (group only) -- Uses `AtomicUsize` countdown initialized to the number of tests in the group -- Each test decrements the counter after running its body and after-each hooks -- When the counter hits zero, the `after` hook fires -- Thread-safe: exactly one test triggers it +- Registered (once, guarded by `std::sync::Once`) the first time any of the group's tests runs +- Runs at process exit via a C `atexit` handler, after every selected test has finished +- Fires exactly once, and only if at least one test in the group ran — robust to name filters, `--skip`, and `#[ignore]`/`#[cfg]` +- Does **not** run if the process is killed (`SIGKILL`) or aborts ### `before_each` / `after_each` @@ -113,13 +113,13 @@ let result = catch_unwind_future(async { // 4. after_each — receives &T + owned U teardown(shared, ctx).await; -// 5. after (countdown) — receives &T -if counter.fetch_sub(1, SeqCst) == 1 { - cleanup(shared); -} - -// 6. re-raise if test panicked +// 5. re-raise if test panicked if let Err(e) = result { std::panic::resume_unwind(e); } + +// `after` is NOT run inline. The first test to run registers it once, and it +// executes at process exit — reading `&T` back from __SPEC_BEFORE_CTX: +// __SPEC_AFTER_ONCE.call_once(|| register_teardown(run_after_at_exit)); +// // at exit: cleanup(__SPEC_BEFORE_CTX.get().unwrap()); ```