ateapi: fail DB tests instead of skipping unless Docker is absent

[Joe Betz (jpbetz)]

What

requireAdminPool (cmd/ateapi/internal/store/storetest/storetest.go) and requirePool
(cmd/ateapi/internal/store/atepg/atepg_test.go) turned any PostgreSQL testcontainer startup
error into

t.Skipf("PostgreSQL testcontainer unavailable (requires Docker): %v", containerErr)

without ever checking whether Docker was actually there. Both call sites now route through one
helper, dockerenv.FailOrSkip, that decides fail-versus-skip in a single place:

• In CI (CI or GITHUB_ACTIONS set) it always fails. The runners have Docker, so any
  container error there is a real failure.
• Locally it skips only when a cheap preflight proves Docker is absent, and fails on
  everything else.

The preflight (dockerenv.Unavailable) mirrors how testcontainers-go actually finds the daemon:
testcontainers walks its own list (tc.host property, DOCKER_HOST, the Docker context,
/var/run/docker.sock, the docker.host property, the rootless socket paths) and uses the first
endpoint that answers. So an unset or dead DOCKER_HOST is not on its own proof that Docker is
missing — the preflight also probes the default and rootless socket paths, and only reports
absence when none of them answers. Anything it cannot decide (a scheme it cannot dial, a daemon
that accepts the connection but misbehaves) counts as present, because only proven absence
justifies dropping a test. The verdict is memoized: a daemon does not come and go mid-binary, and
every failing test in a ~150-test package asks.

Why

CI has already gone green with the controlapi DB suite skipped — 154 tests silently removed from
the run while the job reported success. A reaper timeout, an image pull failure or a daemon hiccup
was indistinguishable from "this developer has no Docker", and the message asserted the second
without checking. This is the fail-not-skip follow-up discussed on #1235 (see also #1230).

Placement note for reviewers

The helper lives in cmd/ateapi/internal/store/dockerenv, not in storetest, because storetest
imports atepg (for SetupPostgresPersistence) and atepg_test.go is an in-package test — so
importing storetest back is a hard cycle. dockerenv is the leaf package both fixtures already
share, and it already owns the other half of the Docker story (Configure). Its package doc now
says it owns both.

Files

cmd/ateapi/internal/store/atepg/atepg_test.go      |   2 +-
cmd/ateapi/internal/store/dockerenv/dockerenv.go   |   5 +-
cmd/ateapi/internal/store/dockerenv/failorskip.go  | 173 +++++++++++++++++
cmd/ateapi/internal/store/dockerenv/failorskip_test.go | 216 ++++++++++++++++++
cmd/ateapi/internal/store/storetest/storetest.go   |   2 +-

No vendor changes, no dependency changes, no workflow changes, no other test semantics touched
(the 30x500ms ping loops, the sync.Once fixtures and the TestMain shutdown paths are
unchanged).

Testing done

Unit tests: failorskip_test.go table-tests the policy against a fake testing.TB
(CI set -> fatal even without Docker; CI set with Docker -> fatal; Docker absent locally -> skip;
any other error with Docker present -> fatal), the preflight against an injectable dialer
(12 cases covering DOCKER_HOST unix/tcp/npipe/no-scheme, missing sockets, and the fallback to
the default socket paths), and the memoization.

go build ./...                                                    exit=0
go vet ./cmd/ateapi/...                                           exit=0
gofmt -l <changed files>                                          exit=0 (no output)
go test ./cmd/ateapi/internal/store/storetest/... -count=1         [no test files] exit=0
go test -race ./cmd/ateapi/internal/store/dockerenv/... -count=1   ok 1.009s exit=0
go test ./cmd/ateapi/internal/store/atepg/ -run TestContractSuite  ok 2.822s exit=0 (0 SKIP lines)
CI=true go test .../atepg -run TestContractSuite -count=1          ok 2.618s exit=0
go test ./cmd/ateapi/internal/store/... -count=1                   all ok exit=0
go test ./cmd/ateapi/internal/controlapi/... ./cmd/ateapi/internal/actoridentity/... -count=1
                                                                  all ok (storetest importers)
hack/verify/{gofmt,boilerplate,go-modules,golangci-lint}.sh        exit=0 each

All three policy branches were proved end-to-end on real test binaries at both call sites, not
just in the unit test. Note that the obvious command — DOCKER_HOST=tcp://127.0.0.1:1 CI=true go test ... — cannot prove the fatal path: testcontainers ignores the unreachable
DOCKER_HOST and falls through to /var/run/docker.sock, so the container starts and the test
passes ("Resolved Docker Host: unix:///var/run/docker.sock", ok 1.979s). The real proofs used a
stripped environment (env -i, no docker CLI on PATH) and, for the no-Docker-at-all cases, an
unprivileged mount namespace (unshare -rm with a tmpfs over /run) so the default socket really
is gone:

#	Environment	Result
A	atepg fixture, CI=true, no docker CLI, no socket	FAIL, exit 1: "container startup failed in CI, where Docker is expected: PostgreSQL testcontainer: inspecting Docker context: exec: "docker": executable file not found in $PATH"
B	atepg fixture, CI unset, no docker CLI, no socket anywhere	SKIP, exit 0
C	atepg fixture, CI unset, reachable DOCKER_HOST, DOCKER_API_VERSION=0.1 to force a container error	FAIL, exit 1: "container startup failed even though Docker is reachable: ... client version 0.1 is too old"
D	storetest fixture (controlapi), CI=true, no docker CLI	FAIL, exit 1
E	storetest fixture (controlapi), CI unset, no Docker at all	SKIP, exit 0
F	atepg fixture, CI unset, no docker CLI but the daemon socket is live	FAIL, exit 1 — under the old preflight this would have skipped
G	atepg fixture, CI unset, dead DOCKER_HOST + live default socket + forced container error	FAIL, exit 1 — under the old preflight this would have skipped
H	atepg fixture, GITHUB_ACTIONS=true with CI unset, no Docker	FAIL, exit 1

F and G are the review's two should-fix findings: before them, a machine with a working daemon but
no docker CLI (rootless, socket-mounted dev containers, a corrupt context store), or with a stale
DOCKER_HOST exported, would still have silently skipped every DB test — the same class of hole
this change exists to close, just narrower.

Open questions for review

1. No workflow assertion was added. The optional "grep the -v output to prove
   TestContractSuite ran" step in .github/workflows/pr-workflow.yaml was left out. With every
   container error fatal under CI, "green with the DB suites skipped" is already unreachable, and a
   grep step would need either a second ~2-minute run of the DB packages or a PIPESTATUS pipeline
   that risks swallowing the go test exit code. Happy to add it if you want belt and braces.
2. Retry budget is unchanged and now decides red/green. The 30x500ms ping loop plus
   testcontainers' own waits used to decide skip-or-run; they now decide fail-or-pass. If CI starts
   flaking on slow container pulls, that budget is the knob — but widening it pre-emptively would
   be guessing. Out of scope here.
3. Strictness is keyed on CI / GITHUB_ACTIONS. pr-workflow.yaml runs go test -race -v ./... directly on ubuntu-latest, where the runner sets both, and hack/run-root-tests.sh
   uses sudo -E env "PATH=${PATH}" so they survive the privilege change. A future job that runs
   go test inside a container without forwarding either would silently degrade to the old
   skip-on-anything behavior. Accepting both variables is the cheap mitigation; a hard opt-out
   (fail unless explicitly told otherwise) would be the strict version if you want it.
4. Fixture wording. FailOrSkip's messages are now generic ("container startup failed ...")
   and the two call sites wrap their own error with fmt.Errorf("PostgreSQL testcontainer: %w", containerErr), so the next container fixture in the tree inherits the policy without inheriting
   a Postgres-specific message. Shout if you would rather keep the subject in the helper.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LYXeYhZaGKwgY57jvfMMYF

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Joe Betz (jpbetz)]

Sorry, I need to self review this first.