ITADN

Codify catch_unwind(AssertUnwindSafe(...)) testing pattern as testing::assert_panics

#171Opensandersaares 创建于 2026-05-25
S
sandersaarescommented
# Codify the "assert a closure panics" pattern as `testing::assert_panics` ## Summary Several test sites across the workspace use the same multi-line incantation to assert that a particular closure call panics — without using `#[should_panic]`, because the test needs to continue running afterwards (e.g. to verify that the data structure is still in a valid state, that subsequent calls still work, or to make additional assertions). The recurring shape is: ```rust let panic_result = catch_unwind(AssertUnwindSafe(|| { // ... call expected to panic ... })); assert!(panic_result.is_err()); ``` This pattern is verbose, repeats `AssertUnwindSafe` boilerplate at every site, and forces every consumer to remember to import `std::panic::catch_unwind`, `std::panic::AssertUnwindSafe`, and to combine them correctly. A small helper in the `testing` package would centralize the idiom and shave 5 lines down to 1 at every call site. ## Proposed API ```rust // In packages/testing/src/lib.rs /// Asserts that the closure panics. /// /// Captures the panic payload (and silences the default panic hook for the /// duration) so the surrounding test can keep running and make further /// assertions about state after the panic. /// /// Prefer `#[should_panic]` when the panic is the only thing the test /// verifies — this helper is for the case where the test must also assert on /// post-panic state. #[track_caller] pub fn assert_panics<F, R>(f: F) where F: FnOnce() -> R, { let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); assert!(result.is_err(), "closure did not panic as expected"); } ``` A few open design questions worth resolving before implementation: 1. **Silence the panic hook?** By default the panic message and backtrace still print to stderr even when caught. Should `assert_panics` install a no-op hook for the duration of the call to keep test output clean? This makes the helper non-`Send` / non-thread-safe (the panic hook is global), so it would need a documented caveat or a separate variant. 2. **Return the payload?** Some sites might want to inspect the payload (e.g. `await_rethrows_panic` in `vicinal::join_handle` captures it to forward along a channel). A `pub fn catch_panic<F, R>(f: F) -> Box<dyn Any + Send>` variant could cover that. Strawman: ship `assert_panics` first, and add `catch_panic` later only if real demand materializes. 3. **`#[track_caller]`** so a failing assertion points at the test, not at the helper. ## Existing usage to migrate Direct "expect this to panic, then keep going" sites (≈7) that would migrate to `assert_panics`: - `packages/nm_otel/src/state.rs:432, 462` — `EventState::histogram_deltas` bucket-count-mismatch and fewer-buckets panic tests - `packages/region_local/src/region_local.rs:691, 715` — `callback_panic_during_*` tests - `packages/region_cached/src/region_cached.rs:789, 813` — `callback_panic_during_*` tests - `packages/infinity_pool/src/opaque/slab.rs:859` — drop-panic isolation test Adjacent sites that use `catch_unwind(AssertUnwindSafe(...))` for *other* legitimate reasons (capture a payload to forward, do bookkeeping then `resume_unwind`, etc.) — these are **not** candidates for `assert_panics` and should remain on raw `catch_unwind`: - `packages/vicinal/src/join_handle.rs:111` — captures a payload to feed into a channel - `packages/region_local/src/region_local.rs:768`, `packages/region_cached/src/region_cached.rs:872` — propagate the `Result<R, _>` so the orchestration thread can decide what to do with it - All `packages/events_once/**`, `packages/vicinal/src/{task,pool}.rs`, `packages/infinity_pool/src/**/pool_managed.rs`, `packages/infinity_pool/src/opaque/slab.rs:{459,1757}` — production code catching panics across abstraction boundaries ## Non-goals - Do **not** check the panic message. AGENTS.md ("Do not check for specific panic or error messages") is the existing rule and this helper should not encourage relaxing it. Canary substrings and pass-through verification are the documented exceptions; `assert_panics` is for neither. - Do **not** replace `#[should_panic]` at sites where it is the right tool. The `#[should_panic]` attribute is more idiomatic and has zero runtime cost; `assert_panics` is specifically for tests that need to continue asserting after the panic. ## Acceptance criteria - [ ] `testing::assert_panics` exists with documentation and an inline example. - [ ] All ≈7 candidate sites listed above are migrated. - [ ] AGENTS.md gets a one-line nudge under "Testing for panics and errors" pointing at the helper for the "keep going after the panic" use case. - [ ] Mutation testing on the migrated tests still catches the mutations that the existing assertions catch (the helper should not weaken coverage).
0 条评论