tests: remove the network capability rather than relying on not using it

Merged
IC
2mo ago
+111-7

Six test rigs across main.rs and admin_surface.rs built a real ReqwestTransport.

None of them ever sent a request through it. Five construct the engine with an empty adapter list, so no refresh can dispatch; the sixth passes TtlFixtureAdapter, which ignores the transport it is handed.

But that is a property of the arguments at each call site, not of the type. Adding one real adapter to any rig — or a fixture that forwards rather than ignores — would silently turn a unit test into a live token exchange against a provider. Nothing would flag it. The test would still pass, and it would pass for a reason supplied by a remote service.

Why this is worth changing when nothing is currently broken

Two sibling repos found exactly this today, independently.

One suite had been issuing 30 real token-exchange requests per run against a vendor endpoint with fabricated credentials, from every dev machine and every CI run, for months. One of its tests passed only because the endpoint rejected them — the assertion was reading the provider's live response rather than the code under test. It went red the moment a network guard was added, which is how it was found.

The traffic is not the defect. A remote service supplying a test's precondition is.

This repo is one adapter argument away from the same shape, and the argument that protects it is not visible from any test's own text.

Empirical first, then structural

I checked this suite the empirical way before changing anything — ran it inside a network namespace with egress dropped:

unshare -rn sh -c 'ip link set lo up; cargo test --release --locked --offline --workspace'

It passed, with loopback up (the e2e arms need it) and external egress unreachable. So the current state is genuinely clean.

That result expires the moment someone edits a rig, and re-running it requires knowing to. NoHttp makes it structural instead: no adapter added later can reach outward, because the transport it would be handed has no outward. Returning RefreshError::Transport also names the cause at the point of use rather than leaving someone to diagnose a timeout.

Verified as a capability change, not a behaviour change

Patching both NoHttp arms to panic! leaves the suite green — 80 passed, nothing touches the transport at all. So this removes a capability that was never exercised rather than altering something that was.

The replacement asserted its five call sites were present before editing (a substitution that silently matches nothing is indistinguishable from one that worked), and the production wiring at main.rs:437 is untouched.

bash scripts/gate.sh exit 0, nine real-daemon e2e arms executing.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Replaces the real ReqwestTransport in all six test rigs with a NoHttp stub that returns RefreshError::Transport if a request is ever attempted.

The suites never actually made network calls, but that was enforced only by the arguments at each call site, not by the type. Adding one real adapter to any rig would have silently turned a unit test into a live token exchange. NoHttp makes the guarantee structural.

  • Verified the change is capability-only: patching NoHttp to panic! leaves all 80 tests green.
  • Production wiring at main.rs:437 is untouched.