internal/state ships test-only constructors in the production build #111

New Issue

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

clawbot commented

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

internal/state/state_test_helper.go is a normal, non-test Go file in package state. It exports two constructors that exist purely for tests:

func NewForTest() *State { ... }              // config: &config.Config{DataDir: ""}
func NewForTestWithDataDir(dataDir string) *State { ... }

Because the filename does not end in _test.go, the Go toolchain compiles it into the shipped dnswatcher binary. Test scaffolding becomes part of the production package's public API — anything importing internal/state can call NewForTest() and get a *State that bypasses the fx lifecycle entirely.

NewForTest() in particular hardcodes DataDir: "", which makes StatePath() resolve to /state.json. Any caller that reaches Save() on such an instance tries to write to the filesystem root.

This repo already knows the right pattern

Three sibling packages already solve exactly this problem the idiomatic way, with a file named export_test.go:

internal/config/export_test.go
internal/handlers/export_test.go
internal/notify/export_test.go

A _test.go file is compiled only during testing, yet its exported identifiers are still visible to the external package state_test. So the helpers keep working, and they stop shipping. internal/state is the lone holdout.

Definition of done

internal/state/state_test_helper.go is renamed to internal/state/export_test.go (use git mv so history follows), with no change to the helpers' behaviour or signatures.
internal/state/state_test.go (external package state_test) still compiles and passes unchanged — it uses both helpers today, at state_test.go:90,99,127 and eight NewForTest() call sites around :635-1268.
Verify the helpers are genuinely gone from the production build. A clean make build must succeed, and NewForTest must not appear in the built binary's symbols.
While here, consider whether NewForTest()'s DataDir: "" default should instead be a required parameter, so no caller can accidentally construct a State that writes to /. If you change it, update the call sites; if you decide to leave it, say why in the PR description. Either answer is acceptable — an unexamined one is not.
make check green; TODO.md updated in the same commit.

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

Sequencing

Land this after PR #97 merges. #97 rewrites internal/watcher/watcher_test.go and switches it from state.NewForTest() to state.NewForTestWithDataDir(cfg.DataDir) — which is what stops the watcher suite from spraying failed to save state ... open /state.json.tmp: permission denied errors through every make check run. Starting this before #97 lands will conflict and will make it look like the write-to-root problem is unfixed. Do not touch PR #97.

`internal/state/state_test_helper.go` is a **normal, non-test Go file** in `package state`. It exports two constructors that exist purely for tests: ```go func NewForTest() *State { ... } // config: &config.Config{DataDir: ""} func NewForTestWithDataDir(dataDir string) *State { ... } ``` Because the filename does not end in `_test.go`, the Go toolchain compiles it into the shipped `dnswatcher` binary. Test scaffolding becomes part of the production package's public API — anything importing `internal/state` can call `NewForTest()` and get a `*State` that bypasses the fx lifecycle entirely. `NewForTest()` in particular hardcodes `DataDir: ""`, which makes `StatePath()` resolve to `/state.json`. Any caller that reaches `Save()` on such an instance tries to write to the filesystem root. ## This repo already knows the right pattern Three sibling packages already solve exactly this problem the idiomatic way, with a file named `export_test.go`: - `internal/config/export_test.go` - `internal/handlers/export_test.go` - `internal/notify/export_test.go` A `_test.go` file is compiled only during testing, yet its exported identifiers are still visible to the external `package state_test`. So the helpers keep working, and they stop shipping. `internal/state` is the lone holdout. ## Definition of done 1. `internal/state/state_test_helper.go` is renamed to `internal/state/export_test.go` (use `git mv` so history follows), with no change to the helpers' behaviour or signatures. 2. `internal/state/state_test.go` (external `package state_test`) still compiles and passes unchanged — it uses both helpers today, at `state_test.go:90,99,127` and eight `NewForTest()` call sites around `:635-1268`. 3. Verify the helpers are genuinely gone from the production build. A clean `make build` must succeed, and `NewForTest` must not appear in the built binary's symbols. 4. While here, consider whether `NewForTest()`'s `DataDir: ""` default should instead be a required parameter, so no caller can accidentally construct a `State` that writes to `/`. If you change it, update the call sites; if you decide to leave it, say why in the PR description. Either answer is acceptable — an unexamined one is not. 5. `make check` green; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Sequencing **Land this after PR #97 merges.** #97 rewrites `internal/watcher/watcher_test.go` and switches it from `state.NewForTest()` to `state.NewForTestWithDataDir(cfg.DataDir)` — which is what stops the watcher suite from spraying `failed to save state ... open /state.json.tmp: permission denied` errors through every `make check` run. Starting this before #97 lands will conflict and will make it look like the write-to-root problem is unfixed. Do not touch PR #97.

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

clawbot referenced this issue

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

1.0/mvp #66

clawbot referenced this issue

2026-08-09 03:55:06 +02:00

feat: add security response headers middleware (closes #98) #112

clawbot referenced this issue

2026-08-09 07:35:34 +02:00

1.0/mvp #66

clawbot referenced this issue

2026-08-09 07:52:15 +02:00