internal/globals, internal/healthcheck, and internal/logger have no tests at all #110

New Issue

clawbot · 2026-08-09T03:41:03+02:00

clawbot commented

2026-08-09 03:41:03 +02:00

REPO_POLICIES.md: "All repos with software must have tests that run via the platform-standard test framework... If no meaningful tests exist yet, add the most minimal test possible... There is no excuse for make test to be a no-op."

Five packages currently have zero test files. Two of them (internal/middleware, internal/server) will gain coverage from the HTTP hardening work in #98, #99, #100, and #101, each of which mandates tests in those packages. That leaves three genuinely uncovered:

Package	Files	Test file
`internal/globals`	`globals.go`	none
`internal/healthcheck`	`healthcheck.go`	none
`internal/logger`	`logger.go`	none

internal/healthcheck is the one that actually matters. It backs two live HTTP routes (/health and /.well-known/healthcheck, internal/server/routes.go:41-47) and is the endpoint an orchestrator uses to decide whether this process is alive. It is untested.

Definition of done

internal/healthcheck has real behavioural tests, not a smoke test. Cover: the response's JSON shape and field names; that Status is "ok"; that the maintenanceMode field reflects Config.MaintenanceMode in both states (healthcheck.go:73); and that the version string surfaced from internal/globals appears in the payload (healthcheck.go:38,72).
internal/globals has a test covering SetVersion and the version read-back. It is a tiny package — a small, honest test is the right size. Note the package-level global carries an intentional //nolint:gochecknoglobals; do not remove or relocate it to make testing easier.
internal/logger has at least a construction test asserting a usable *slog.Logger is returned and that the debug/non-debug configurations differ as intended. Do not assert on exact log line formatting — that is brittle. If TTY detection is not testable without contortions, test what is reachable and leave the rest alone rather than restructuring production code to chase coverage.
Tests follow the conventions already established in this repo: external package foo_test, table-driven where there is more than one case, t.Parallel() where safe, and an export_test.go shim if unexported access is genuinely needed (internal/config, internal/handlers, and internal/notify already do this).
No test writes to the filesystem outside t.TempDir(), and none makes a network call.
make check green and make test wall time stays well under the 20-second policy ceiling. TODO.md updated in the same commit.

The finishing commit's title must end with (closes #N) referencing this issue.

Scope discipline

This is a test-only change. Do not refactor production code to make it more testable; if something is genuinely untestable without a refactor, note it in the PR description and leave it. Chasing a coverage percentage is explicitly not the goal — the goal is that these three packages stop being completely unexercised.

Sequencing

Land this after #98-#101, or at least be aware they add the internal/middleware and internal/server tests. Do not duplicate their work here.

`REPO_POLICIES.md`: "All repos with software must have tests that run via the platform-standard test framework... If no meaningful tests exist yet, add the most minimal test possible... There is no excuse for `make test` to be a no-op." Five packages currently have **zero** test files. Two of them (`internal/middleware`, `internal/server`) will gain coverage from the HTTP hardening work in #98, #99, #100, and #101, each of which mandates tests in those packages. That leaves three genuinely uncovered: | Package | Files | Test file | |---|---|---| | `internal/globals` | `globals.go` | none | | `internal/healthcheck` | `healthcheck.go` | none | | `internal/logger` | `logger.go` | none | `internal/healthcheck` is the one that actually matters. It backs two live HTTP routes (`/health` and `/.well-known/healthcheck`, `internal/server/routes.go:41-47`) and is the endpoint an orchestrator uses to decide whether this process is alive. It is untested. ## Definition of done 1. `internal/healthcheck` has real behavioural tests, not a smoke test. Cover: the response's JSON shape and field names; that `Status` is `"ok"`; that the `maintenanceMode` field reflects `Config.MaintenanceMode` in both states (`healthcheck.go:73`); and that the version string surfaced from `internal/globals` appears in the payload (`healthcheck.go:38,72`). 2. `internal/globals` has a test covering `SetVersion` and the version read-back. It is a tiny package — a small, honest test is the right size. Note the package-level global carries an intentional `//nolint:gochecknoglobals`; do not remove or relocate it to make testing easier. 3. `internal/logger` has at least a construction test asserting a usable `*slog.Logger` is returned and that the debug/non-debug configurations differ as intended. Do not assert on exact log line formatting — that is brittle. If TTY detection is not testable without contortions, test what is reachable and leave the rest alone rather than restructuring production code to chase coverage. 4. Tests follow the conventions already established in this repo: external `package foo_test`, table-driven where there is more than one case, `t.Parallel()` where safe, and an `export_test.go` shim if unexported access is genuinely needed (`internal/config`, `internal/handlers`, and `internal/notify` already do this). 5. No test writes to the filesystem outside `t.TempDir()`, and none makes a network call. 6. `make check` green and `make test` wall time stays well under the 20-second policy ceiling. `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Scope discipline This is a **test-only** change. Do not refactor production code to make it more testable; if something is genuinely untestable without a refactor, note it in the PR description and leave it. Chasing a coverage percentage is explicitly not the goal — the goal is that these three packages stop being completely unexercised. ## Sequencing Land this **after** #98-#101, or at least be aware they add the `internal/middleware` and `internal/server` tests. Do not duplicate their work here.

clawbot added this to the 1.0 milestone 2026-08-09 03:41:03 +02:00

clawbot referenced this issue

2026-08-09 03:42:01 +02:00

1.0/mvp #66

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/dnswatcher#110