Summary
capture_logs returns a bare String, so nothing forces a caller to
establish the capture is non-empty before asserting on it. A future test
that asserts only an absence —
let (_, logs) = capture_logs(|| something());
assert!(!logs.contains("secret"));
— passes against an empty capture, proving nothing. That is exactly the
failure mode #92 was about, and #96 (the fix for #92) closed it only at
the call sites that existed at the time, by convention rather than by construction.
Why the convention is not enough
The assert_captured / assert_logged helpers added in #96 do the right
thing, but using them is optional. Nothing in the type system, and no
lint, stops the next test from reaching for logs.contains(...)
directly. Negative assertions are precisely the ones where the omission
is invisible: a positive assertion over an empty capture fails loudly, a
negative one goes green.
Since #105 this is no longer hypothetical: the http_auth.rs tests
(the_startup_mode_line_never_carries_token_material,
refusal_logging_is_bounded_and_names_no_material) assert their I12
negatives through raw logs.contains(...), guarded only by a
hand-written positive-first assertion and a comment explaining why it
must come first. The convention held — but as a thing a reviewer had to
check, which is the mode this issue exists to retire.
This matters more than a normal test-hygiene nit because the negative
assertions in question are the I12 ones — "the log must never carry
key material". A vacuous I12 assertion looks identical to a passing one.
Suggested direction
Have capture_logs return a newtype (say Captured) rather than
String, with no Deref to str and no public contains. The only way
to reach the text is through something that has already checked
non-emptiness — assert_logged(&captured, needle), or an
as_str(&self) that panics on empty. Then a test cannot express the
vacuous form; the mistake stops being a thing reviewers must catch.
Keep the failure messages #96 introduced, which distinguish "captured
nothing" from "captured something that lacked the needle".
Secondary, smaller
If f panics inside capture_logs, the thread's capture slot stays
Some. #96's debug_assert! on entry means the next capture on that
thread now fails loudly rather than silently inheriting the residue, so
this is mostly closed — libtest also gives each test its own thread. A
Drop guard that clears the slot would close it fully and would remove
the reliance on that reasoning holding for every future harness
configuration.
Scope note
Test-infrastructure only; no production code involved. Found during the
adversarial review of #96 (the fix for #92), deliberately left out of
that PR to keep it to
one logical change.
Acceptance criteria
Summary
capture_logsreturns a bareString, so nothing forces a caller toestablish the capture is non-empty before asserting on it. A future test
that asserts only an absence —
— passes against an empty capture, proving nothing. That is exactly the
failure mode #92 was about, and #96 (the fix for #92) closed it only at
the call sites that existed at the time, by convention rather than by construction.
Why the convention is not enough
The
assert_captured/assert_loggedhelpers added in #96 do the rightthing, but using them is optional. Nothing in the type system, and no
lint, stops the next test from reaching for
logs.contains(...)directly. Negative assertions are precisely the ones where the omission
is invisible: a positive assertion over an empty capture fails loudly, a
negative one goes green.
Since #105 this is no longer hypothetical: the
http_auth.rstests(
the_startup_mode_line_never_carries_token_material,refusal_logging_is_bounded_and_names_no_material) assert their I12negatives through raw
logs.contains(...), guarded only by ahand-written positive-first assertion and a comment explaining why it
must come first. The convention held — but as a thing a reviewer had to
check, which is the mode this issue exists to retire.
This matters more than a normal test-hygiene nit because the negative
assertions in question are the I12 ones — "the log must never carry
key material". A vacuous I12 assertion looks identical to a passing one.
Suggested direction
Have
capture_logsreturn a newtype (sayCaptured) rather thanString, with noDereftostrand no publiccontains. The only wayto reach the text is through something that has already checked
non-emptiness —
assert_logged(&captured, needle), or anas_str(&self)that panics on empty. Then a test cannot express thevacuous form; the mistake stops being a thing reviewers must catch.
Keep the failure messages #96 introduced, which distinguish "captured
nothing" from "captured something that lacked the needle".
Secondary, smaller
If
fpanics insidecapture_logs, the thread's capture slot staysSome. #96'sdebug_assert!on entry means the next capture on thatthread now fails loudly rather than silently inheriting the residue, so
this is mostly closed — libtest also gives each test its own thread. A
Dropguard that clears the slot would close it fully and would removethe reliance on that reasoning holding for every future harness
configuration.
Scope note
Test-infrastructure only; no production code involved. Found during the
adversarial review of #96 (the fix for #92), deliberately left out of
that PR to keep it to
one logical change.
Acceptance criteria
capture_logsreturns a newtype with noDereftostrand nopublic
contains; the text is reachable only through a path thathas already established the capture is non-empty.
capture — no longer compiles anywhere, including the
http_auth.rsI12 tests, which migrate off raw
logs.contains(...)."captured nothing" from "captured something that lacked the
needle".
Dropguard clears the slot whenfpanics, so thedebug_assert!on entry stops being the only line of defence.