sneak/dnswatcher
1
0
Fork 0
Code Issues 3 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 03:41:03 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 03:41:03 +02:00
Collaborator
Copy Link
Copy Source

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.

`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 commented 2026-09-21 09:57:27 +02:00
Author
Collaborator
Copy Link
Copy Source

Opened #154 (base next).

Test-only. Added external (package foo_test) tests for the three
uncovered packages:

  • internal/globals: setters and read-back through New, plus that
    New returns an independent snapshot.
  • internal/healthcheck: Check returns Status "ok", the exact
    documented JSON field set, an RFC3339Nano timestamp, maintenanceMode
    mirroring Config.MaintenanceMode in both states, and the version and
    appname from internal/globals.
  • internal/logger: New yields a usable logger; debug is off at the
    default level and EnableDebugLogging turns it on.

No production code changed. make check is green. The TTY-vs-JSON
handler branch in the logger is not asserted, since testing it would
require restructuring production code, which this issue rules out.

Model: opus-4-8

Opened https://git.eeqj.de/sneak/dnswatcher/pulls/154 (base `next`). Test-only. Added external (`package foo_test`) tests for the three uncovered packages: - `internal/globals`: setters and read-back through `New`, plus that `New` returns an independent snapshot. - `internal/healthcheck`: `Check` returns `Status` `"ok"`, the exact documented JSON field set, an RFC3339Nano timestamp, `maintenanceMode` mirroring `Config.MaintenanceMode` in both states, and the version and appname from `internal/globals`. - `internal/logger`: `New` yields a usable logger; debug is off at the default level and `EnableDebugLogging` turns it on. No production code changed. `make check` is green. The TTY-vs-JSON handler branch in the logger is not asserted, since testing it would require restructuring production code, which this issue rules out. Model: opus-4-8
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No labels
Milestone
No items
No Milestone 1.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#110