From 465a537a9f53f3b441d86362fa6d12e70c68c4bb Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 1 Oct 2026 17:59:27 +0000 Subject: [PATCH] tests: move the test-only constructors into export_test.go (closes #111) state.NewForTest, state.NewForTestWithDataDir and watcher.NewForTest were compiled into and exported by the production packages internal/state and internal/watcher. NewForTestWithDataDir and watcher.NewForTest now live in their package's export_test.go. Tests in other packages cannot see those files, so the watcher and middleware tests build their State with state.New and a temporary data directory; the watcher tests no longer try to save to /state.json. state.NewForTest, whose State saved to /, is deleted: the state tests that used it pass t.TempDir() to NewForTestWithDataDir, and the test of the helper itself is gone. Model: opus-5-5 --- TODO.md | 2 ++ internal/middleware/middleware_test.go | 10 +++++- internal/state/export_test.go | 23 ++++++++++++++ internal/state/state_test.go | 43 +++++--------------------- internal/state/state_test_helper.go | 38 ----------------------- internal/watcher/export_test.go | 25 +++++++++++++++ internal/watcher/watcher.go | 22 ------------- internal/watcher/watcher_test.go | 23 +++++++++++++- 8 files changed, 88 insertions(+), 98 deletions(-) create mode 100644 internal/state/export_test.go delete mode 100644 internal/state/state_test_helper.go diff --git a/TODO.md b/TODO.md index 5e7cad9..a7c1ce8 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-10-01: `internal/state` and `internal/watcher` no longer export test-only + constructors: two moved to `export_test.go`, one is deleted (closes #111). - 2026-10-01: notify shutdown tests use one timing constant per meaning, name the bound they check, and require the drain's debug line (closes #116). - 2026-09-29: the live-DNS test package is renamed `internal/livednstest` and diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index 598f476..da51423 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -276,10 +276,18 @@ func newTestHandlers(t *testing.T) *handlers.Handlers { t.Fatalf("notify.New: %v", err) } + st, err := state.New(fxtest.NewLifecycle(t), state.Params{ + Logger: log, + Config: &config.Config{DataDir: t.TempDir()}, + }) + if err != nil { + t.Fatalf("state.New: %v", err) + } + hnd, err := handlers.New(nil, handlers.Params{ Logger: log, Globals: glob, - State: state.NewForTest(), + State: st, Notify: notifier, }) if err != nil { diff --git a/internal/state/export_test.go b/internal/state/export_test.go new file mode 100644 index 0000000..8defb0c --- /dev/null +++ b/internal/state/export_test.go @@ -0,0 +1,23 @@ +package state + +import ( + "log/slog" + + "sneak.berlin/go/dnswatcher/internal/config" +) + +// NewForTestWithDataDir creates an empty State that saves to dataDir, +// without the fx lifecycle. +func NewForTestWithDataDir(dataDir string) *State { + return &State{ + log: slog.Default(), + snapshot: &Snapshot{ + Version: stateVersion, + Domains: make(map[string]*DomainState), + Hostnames: make(map[string]*HostnameState), + Ports: make(map[string]*PortState), + Certificates: make(map[string]*CertificateState), + }, + config: &config.Config{DataDir: dataDir}, + } +} diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 3fbca93..fabc142 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -739,7 +739,7 @@ func TestPortStateUnmarshalJSON_BothFormats(t *testing.T) { func TestGetSnapshot_ReturnsCopy(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) populateState(t, s) @@ -761,7 +761,7 @@ func TestGetSnapshot_ReturnsCopy(t *testing.T) { func TestDomainState_GetSet(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) // Get on missing key returns false. _, ok := s.GetDomainState("nonexistent.com") @@ -812,7 +812,7 @@ func TestDomainState_GetSet(t *testing.T) { func TestHostnameState_GetSet(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) _, ok := s.GetHostnameState("missing.example.com") if ok { @@ -857,7 +857,7 @@ func TestHostnameState_GetSet(t *testing.T) { func TestPortState_GetSetDelete(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) _, ok := s.GetPortState("1.2.3.4:80") if ok { @@ -895,7 +895,7 @@ func TestPortState_GetSetDelete(t *testing.T) { func TestGetAllPortKeys(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) keys := s.GetAllPortKeys() if len(keys) != 0 { @@ -937,7 +937,7 @@ func TestGetAllPortKeys(t *testing.T) { func TestCertificateState_GetSet(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) _, ok := s.GetCertificateState("1.2.3.4:443:www.example.com") if ok { @@ -1158,7 +1158,7 @@ func TestLoadPreservesExistingStateOnMissingFile(t *testing.T) { func TestConcurrentGetSet(t *testing.T) { t.Parallel() - s := state.NewForTest() + s := state.NewForTestWithDataDir(t.TempDir()) const goroutines = 20 @@ -1368,35 +1368,6 @@ func TestMultipleSavesOverwrite(t *testing.T) { } } -// TestNewForTest verifies the test helper creates a valid empty state. -func TestNewForTest(t *testing.T) { - t.Parallel() - - s := state.NewForTest() - - snap := s.GetSnapshot() - - if snap.Version != 1 { - t.Errorf("version: got %d, want 1", snap.Version) - } - - if snap.Domains == nil { - t.Error("Domains map should be initialized") - } - - if snap.Hostnames == nil { - t.Error("Hostnames map should be initialized") - } - - if snap.Ports == nil { - t.Error("Ports map should be initialized") - } - - if snap.Certificates == nil { - t.Error("Certificates map should be initialized") - } -} - // TestSaveFilePermissions verifies the saved file has restricted permissions. func TestSaveFilePermissions(t *testing.T) { t.Parallel() diff --git a/internal/state/state_test_helper.go b/internal/state/state_test_helper.go deleted file mode 100644 index 7e9aac7..0000000 --- a/internal/state/state_test_helper.go +++ /dev/null @@ -1,38 +0,0 @@ -package state - -import ( - "log/slog" - - "sneak.berlin/go/dnswatcher/internal/config" -) - -// NewForTest creates a State for unit testing with no persistence. -func NewForTest() *State { - return &State{ - log: slog.Default(), - snapshot: &Snapshot{ - Version: stateVersion, - Domains: make(map[string]*DomainState), - Hostnames: make(map[string]*HostnameState), - Ports: make(map[string]*PortState), - Certificates: make(map[string]*CertificateState), - }, - config: &config.Config{DataDir: ""}, - } -} - -// NewForTestWithDataDir creates a State backed by the given directory -// for tests that need file persistence. -func NewForTestWithDataDir(dataDir string) *State { - return &State{ - log: slog.Default(), - snapshot: &Snapshot{ - Version: stateVersion, - Domains: make(map[string]*DomainState), - Hostnames: make(map[string]*HostnameState), - Ports: make(map[string]*PortState), - Certificates: make(map[string]*CertificateState), - }, - config: &config.Config{DataDir: dataDir}, - } -} diff --git a/internal/watcher/export_test.go b/internal/watcher/export_test.go index 04a36d5..c647a6e 100644 --- a/internal/watcher/export_test.go +++ b/internal/watcher/export_test.go @@ -2,10 +2,35 @@ package watcher import ( "context" + "log/slog" + "time" + "sneak.berlin/go/dnswatcher/internal/config" "sneak.berlin/go/dnswatcher/internal/state" ) +// NewForTest creates a Watcher without fx for unit testing. +func NewForTest( + cfg *config.Config, + st *state.State, + res DNSResolver, + pc PortChecker, + tc TLSChecker, + n Notifier, +) *Watcher { + return &Watcher{ + log: slog.Default(), + config: cfg, + state: st, + resolver: res, + portCheck: pc, + tlsCheck: tc, + notify: n, + firstRun: true, + expiryNotified: make(map[string]time.Time), + } +} + // NewlyDisagreeingPairs exports newlyDisagreeingPairs for testing. func NewlyDisagreeingPairs( prev *state.HostnameState, diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 1af5709..c3d59f0 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -102,28 +102,6 @@ func New( return w, nil } -// NewForTest creates a Watcher without fx for unit testing. -func NewForTest( - cfg *config.Config, - st *state.State, - res DNSResolver, - pc PortChecker, - tc TLSChecker, - n Notifier, -) *Watcher { - return &Watcher{ - log: slog.Default(), - config: cfg, - state: st, - resolver: res, - portCheck: pc, - tlsCheck: tc, - notify: n, - firstRun: true, - expiryNotified: make(map[string]time.Time), - } -} - // Run starts the monitoring loop with periodic scheduling. func (w *Watcher) Run(ctx context.Context) { w.log.Info( diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 8ff670e..0620f8a 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -9,8 +9,12 @@ import ( "testing" "time" + "go.uber.org/fx/fxtest" + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" "sneak.berlin/go/dnswatcher/internal/livednstest" + "sneak.berlin/go/dnswatcher/internal/logger" "sneak.berlin/go/dnswatcher/internal/portcheck" "sneak.berlin/go/dnswatcher/internal/resolver" "sneak.berlin/go/dnswatcher/internal/state" @@ -148,7 +152,24 @@ func newTestWatcher( config: cfg, } - deps.state = state.NewForTest() + g, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log, err := logger.New(nil, logger.Params{Globals: g}) + if err != nil { + t.Fatalf("logger.New: %v", err) + } + + // The watcher saves state after every check, into cfg.DataDir. + deps.state, err = state.New(fxtest.NewLifecycle(t), state.Params{ + Logger: log, + Config: cfg, + }) + if err != nil { + t.Fatalf("state.New: %v", err) + } w := watcher.NewForTest( deps.config,