Skip to content

capture_logs lets a future test assert on an empty capture; make it unrepresentable #97

Description

@plusky

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

  • capture_logs returns a newtype with no Deref to str and no
    public contains; the text is reachable only through a path that
    has already established the capture is non-empty.
  • The vacuous form — a negative assertion over a possibly-empty
    capture — no longer compiles anywhere, including the http_auth.rs
    I12 tests, which migrate off raw logs.contains(...).
  • The failure messages from test(server): capture logs through one process-wide subscriber #96 survive, still distinguishing
    "captured nothing" from "captured something that lacked the
    needle".
  • Secondary: a Drop guard clears the slot when f panics, so the
    debug_assert! on entry stops being the only line of defence.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestsecurityGuard, key custody, or disclosure surface

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions