Trailing commas, leading commas, consecutive commas, whitespace, tabs
Debug mode
DNSWATCHER_DEBUG=true enables debug logging
Target classification
Domains vs hostnames correctly separated via PSL
StatePath
Path construction with various DataDir values
Empty appname
Falls back to "dnswatcher" config file name
Coverage: 23% → 92.5%
Notes
Tests use viper.Reset() for isolation since Viper has global state
Non-parallel tests use t.Setenv() for automatic env var cleanup
Uses testify assert/require consistent with other test files in the repo
No production code changes
## Summary
Add comprehensive tests for the `internal/config` package, covering the main configuration loading path that was previously untested.
Closes [issue #72](https://git.eeqj.de/sneak/dnswatcher/issues/72)
## What Changed
Added three new test files:
- **`config_test.go`** — 16 tests covering `New()`, `StatePath()`, and the full config loading pipeline
- **`parsecsv_test.go`** — 10 test cases for `parseCSV()` edge cases
- **`export_test.go`** — standard Go export bridge for testing unexported `parseCSV`
## Test Coverage
| Area | Tests |
|------|-------|
| Default values | All 14 config fields verified against documented defaults |
| Environment overrides | All env vars tested including `PORT` (unprefixed) |
| Invalid duration fallback | `DNSWATCHER_DNS_INTERVAL=banana` falls back to 1h |
| Invalid TLS interval | `DNSWATCHER_TLS_INTERVAL=notaduration` falls back to 12h |
| No targets error | Empty/missing `DNSWATCHER_TARGETS` returns `ErrNoTargets` |
| Invalid targets | Public suffix (`co.uk`) rejected with error |
| CSV parsing | Trailing commas, leading commas, consecutive commas, whitespace, tabs |
| Debug mode | `DNSWATCHER_DEBUG=true` enables debug logging |
| Target classification | Domains vs hostnames correctly separated via PSL |
| StatePath | Path construction with various `DataDir` values |
| Empty appname | Falls back to "dnswatcher" config file name |
**Coverage: 23% → 92.5%**
## Notes
- Tests use `viper.Reset()` for isolation since Viper has global state
- Non-parallel tests use `t.Setenv()` for automatic env var cleanup
- Uses testify `assert`/`require` consistent with other test files in the repo
- No production code changes
<!-- session: agent:sdlc-manager:subagent:d7fe6cf2-4746-4793-a738-9df8f5f5f0c6 -->
Add tests covering the main configuration loading path which was
previously untested (only ClassifyTargets and PSL logic had tests).
New test coverage:
- New() constructor with default values, env overrides, and error cases
- buildConfig() interval parsing with fallback to defaults on invalid input
- setupViper() env prefix binding and default value registration
- parseCSV() edge cases: whitespace, trailing/leading commas, empty segments
- configureDebugLogging() debug mode activation
- StatePath() path construction with various DataDir values
- ErrNoTargets sentinel error for missing/empty targets
- PORT env without DNSWATCHER_ prefix (compatibility)
- Target classification into domains vs hostnames via PSL
- Invalid targets (public suffixes) rejected with clear error
Coverage for internal/config increased from 23% to 92.5%.
25+ new tests for internal/config covering config loading, environment overrides, edge cases, and the parseCSV helper. Coverage goes from 23% to ~92.5%. Clean, well-structured test code.
Checklist
Tests are meaningful: Real assertions on actual config field values, error conditions, and edge cases. No trivially-passing tests.
No production code changes: Only config_test.go, export_test.go, and parsecsv_test.go added. Zero production files modified.
No DNS mocking: Tests deal with config parsing and environment variables only. No DNS client interaction.
No weakened existing tests: No pre-existing test files modified.
No CI/linter config changes: Dockerfile, Makefile, .golangci.yml unchanged.
CI passes: docker build . succeeds (all checks pass including make check).
Branch is up to date with main: No rebase needed.
Test Coverage Highlights
Default values: Verifies all 13+ config fields have correct defaults when only DNSWATCHER_TARGETS is set
Environment overrides: Every config field verified with custom env var values (Port, Debug, DataDir, DNSInterval, TLSInterval, TLSExpiryWarning, SlackWebhook, MattermostWebhook, NtfyTopic, SentryDSN, MaintenanceMode, MetricsUsername, MetricsPassword)
StatePath: Multiple data directory variants verified
PORT env: Non-prefixed PORT env var works (not DNSWATCHER_PORT)
Notes
Config tests correctly avoid t.Parallel() due to viper global state — documented with a comment. Each test resets viper for isolation.
export_test.go exports parseCSV for external package testing — correct Go pattern.
Good use of t.Setenv() for env var isolation (automatically restored after each test).
Uses testify/assert and testify/require consistently, matching the project's existing test style.
Ready to merge.
## ✅ Review: PASS
### Summary
25+ new tests for `internal/config` covering config loading, environment overrides, edge cases, and the `parseCSV` helper. Coverage goes from 23% to ~92.5%. Clean, well-structured test code.
### Checklist
- [x] **Tests are meaningful**: Real assertions on actual config field values, error conditions, and edge cases. No trivially-passing tests.
- [x] **No production code changes**: Only `config_test.go`, `export_test.go`, and `parsecsv_test.go` added. Zero production files modified.
- [x] **No DNS mocking**: Tests deal with config parsing and environment variables only. No DNS client interaction.
- [x] **No weakened existing tests**: No pre-existing test files modified.
- [x] **No CI/linter config changes**: Dockerfile, Makefile, `.golangci.yml` unchanged.
- [x] **CI passes**: `docker build .` succeeds (all checks pass including `make check`).
- [x] **Branch is up to date with main**: No rebase needed.
### Test Coverage Highlights
- **Default values**: Verifies all 13+ config fields have correct defaults when only `DNSWATCHER_TARGETS` is set
- **Environment overrides**: Every config field verified with custom env var values (Port, Debug, DataDir, DNSInterval, TLSInterval, TLSExpiryWarning, SlackWebhook, MattermostWebhook, NtfyTopic, SentryDSN, MaintenanceMode, MetricsUsername, MetricsPassword)
- **Error handling**: Missing targets (`ErrNoTargets`), empty CSV segments, public suffix rejection
- **Duration parsing edge cases**: Invalid `DNSWATCHER_DNS_INTERVAL` and `DNSWATCHER_TLS_INTERVAL` fall back to defaults
- **Target classification**: Apex domains vs hostnames correctly separated
- **`parseCSV` edge cases**: Empty string, whitespace, trailing/leading commas, consecutive commas, tabs — 10 test cases
- **`StatePath`**: Multiple data directory variants verified
- **PORT env**: Non-prefixed `PORT` env var works (not `DNSWATCHER_PORT`)
### Notes
- Config tests correctly avoid `t.Parallel()` due to viper global state — documented with a comment. Each test resets viper for isolation.
- `export_test.go` exports `parseCSV` for external package testing — correct Go pattern.
- Good use of `t.Setenv()` for env var isolation (automatically restored after each test).
- Uses `testify/assert` and `testify/require` consistently, matching the project's existing test style.
Ready to merge.
<!-- session: agent:sdlc-manager:subagent:bb3bec9c-91ec-42df-a01e-a9d78f67aec2 -->
Excellent test coverage for the internal/config package. Takes coverage from 23% to 92.5% with clean, well-structured tests that exercise the full config loading pipeline.
What Was Reviewed
config_test.go (260 lines) — 16 tests covering New(), StatePath(), and the full config pipeline
parsecsv_test.go (44 lines) — 10 test cases for parseCSV() edge cases
export_test.go (6 lines) — standard Go export bridge for parseCSV
Checklist
Defaults verified: All 14 config fields checked against documented defaults
Env overrides: All env vars tested including unprefixed PORT
Empty appname: Falls back to dnswatcher config file name
No DNS mocking — config tests don't involve DNS (as expected)
No production code changes — 310 additions, 0 deletions, test files only
No weakening of existing tests — classify_test.go untouched
docker build . passes (no-cache, all tests green, lint clean)
Viper isolation: viper.Reset() before each test, t.Setenv() for env cleanup
Consistent style: Uses testify/assert + testify/require, matches existing test patterns
Parallel where safe: TestStatePath and TestParseCSV use t.Parallel(), Viper-dependent tests correctly sequential
Notes
Clean design decision to use export_test.go bridge pattern rather than making parseCSV public
Good coverage of the Viper global state footgun — each test properly resets state
Test names are descriptive and follow Go conventions
No issues found. Ready to merge.
## ✅ Review: PASS
### Summary
Excellent test coverage for the `internal/config` package. Takes coverage from 23% to 92.5% with clean, well-structured tests that exercise the full config loading pipeline.
### What Was Reviewed
- **`config_test.go`** (260 lines) — 16 tests covering `New()`, `StatePath()`, and the full config pipeline
- **`parsecsv_test.go`** (44 lines) — 10 test cases for `parseCSV()` edge cases
- **`export_test.go`** (6 lines) — standard Go export bridge for `parseCSV`
### Checklist
- [x] **Defaults verified**: All 14 config fields checked against documented defaults
- [x] **Env overrides**: All env vars tested including unprefixed `PORT`
- [x] **Invalid duration fallback**: `banana` → 1h, `notaduration` → 12h, both invalid simultaneously
- [x] **No targets error**: Empty string and whitespace-only CSV both return `ErrNoTargets`
- [x] **Invalid targets**: Public suffix (`co.uk`) correctly rejected
- [x] **CSV edge cases**: Trailing/leading/consecutive commas, whitespace, tabs, all-empty segments
- [x] **Debug mode**: Enables debug logging
- [x] **Target classification**: Domains vs hostnames correctly separated via PSL
- [x] **StatePath**: 4 sub-cases (default, absolute, nested, empty)
- [x] **Empty appname**: Falls back to `dnswatcher` config file name
- [x] **No DNS mocking** — config tests don't involve DNS (as expected)
- [x] **No production code changes** — 310 additions, 0 deletions, test files only
- [x] **No weakening of existing tests** — `classify_test.go` untouched
- [x] **`docker build .` passes** (no-cache, all tests green, lint clean)
- [x] **Viper isolation**: `viper.Reset()` before each test, `t.Setenv()` for env cleanup
- [x] **Consistent style**: Uses `testify/assert` + `testify/require`, matches existing test patterns
- [x] **Parallel where safe**: `TestStatePath` and `TestParseCSV` use `t.Parallel()`, Viper-dependent tests correctly sequential
### Notes
- Clean design decision to use `export_test.go` bridge pattern rather than making `parseCSV` public
- Good coverage of the Viper global state footgun — each test properly resets state
- Test names are descriptive and follow Go conventions
No issues found. Ready to merge.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Add comprehensive tests for the
internal/configpackage, covering the main configuration loading path that was previously untested.Closes issue #72
What Changed
Added three new test files:
config_test.go— 16 tests coveringNew(),StatePath(), and the full config loading pipelineparsecsv_test.go— 10 test cases forparseCSV()edge casesexport_test.go— standard Go export bridge for testing unexportedparseCSVTest Coverage
PORT(unprefixed)DNSWATCHER_DNS_INTERVAL=bananafalls back to 1hDNSWATCHER_TLS_INTERVAL=notadurationfalls back to 12hDNSWATCHER_TARGETSreturnsErrNoTargetsco.uk) rejected with errorDNSWATCHER_DEBUG=trueenables debug loggingDataDirvaluesCoverage: 23% → 92.5%
Notes
viper.Reset()for isolation since Viper has global statet.Setenv()for automatic env var cleanupassert/requireconsistent with other test files in the repo✅ Review: PASS
Summary
25+ new tests for
internal/configcovering config loading, environment overrides, edge cases, and theparseCSVhelper. Coverage goes from 23% to ~92.5%. Clean, well-structured test code.Checklist
config_test.go,export_test.go, andparsecsv_test.goadded. Zero production files modified..golangci.ymlunchanged.docker build .succeeds (all checks pass includingmake check).Test Coverage Highlights
DNSWATCHER_TARGETSis setErrNoTargets), empty CSV segments, public suffix rejectionDNSWATCHER_DNS_INTERVALandDNSWATCHER_TLS_INTERVALfall back to defaultsparseCSVedge cases: Empty string, whitespace, trailing/leading commas, consecutive commas, tabs — 10 test casesStatePath: Multiple data directory variants verifiedPORTenv var works (notDNSWATCHER_PORT)Notes
t.Parallel()due to viper global state — documented with a comment. Each test resets viper for isolation.export_test.goexportsparseCSVfor external package testing — correct Go pattern.t.Setenv()for env var isolation (automatically restored after each test).testify/assertandtestify/requireconsistently, matching the project's existing test style.Ready to merge.
✅ Review: PASS
Summary
Excellent test coverage for the
internal/configpackage. Takes coverage from 23% to 92.5% with clean, well-structured tests that exercise the full config loading pipeline.What Was Reviewed
config_test.go(260 lines) — 16 tests coveringNew(),StatePath(), and the full config pipelineparsecsv_test.go(44 lines) — 10 test cases forparseCSV()edge casesexport_test.go(6 lines) — standard Go export bridge forparseCSVChecklist
PORTbanana→ 1h,notaduration→ 12h, both invalid simultaneouslyErrNoTargetsco.uk) correctly rejecteddnswatcherconfig file nameclassify_test.gountoucheddocker build .passes (no-cache, all tests green, lint clean)viper.Reset()before each test,t.Setenv()for env cleanuptestify/assert+testify/require, matches existing test patternsTestStatePathandTestParseCSVuset.Parallel(), Viper-dependent tests correctly sequentialNotes
export_test.gobridge pattern rather than makingparseCSVpublicNo issues found. Ready to merge.