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

test(state): add comprehensive test coverage for internal/state package #80

Merged
sneak merged 1 commits from fix/70-state-test-coverage into main 2026-03-04 11:26:05 +01:00
Conversation 2 Commits 1 Files Changed 2 +1318
clawbot commented 2026-03-02 08:54:55 +01:00
Collaborator
Copy Link

Summary

Add 32 tests for the internal/state package, which previously had 0% test coverage.

Tests added:

Save/Load round-trip:

  • Domain, hostname, port, and certificate data all survive save→load cycles
  • Error fields (omitempty) round-trip correctly
  • Backward-compatible PortState deserialization (old single-hostname → new multi-hostname format)

Edge cases:

  • Missing state file: returns nil error, keeps existing in-memory state
  • Corrupt state file: returns parse error
  • Empty state file: returns parse error
  • Permission errors (read/write): properly reported, skipped when running as root in Docker

Atomic write:

  • No leftover .tmp files after successful save
  • Updated content verified after second save

Getter/setter coverage:

  • Domain: get, set, overwrite
  • Hostname: get, set with nested nameserver records
  • Port: get, set, delete
  • Certificate: get, set
  • GetAllPortKeys enumeration
  • GetSnapshot returns value copy

Concurrency:

  • 20 goroutines × 50 iterations of concurrent get/set/delete with race detector
  • 10 goroutines doing concurrent Save/Load

Other:

  • Snapshot version written correctly
  • LastUpdated timestamp set on save
  • File permissions are 0600
  • Multiple saves overwrite previous state completely
  • NewForTest helper creates valid empty state
  • Save creates nested data directories

Also adds NewForTestWithDataDir() to the test helper for tests requiring file persistence.

Closes issue #70

## Summary Add 32 tests for the `internal/state` package, which previously had 0% test coverage. ### Tests added: **Save/Load round-trip:** - Domain, hostname, port, and certificate data all survive save→load cycles - Error fields (omitempty) round-trip correctly - Backward-compatible PortState deserialization (old single-hostname → new multi-hostname format) **Edge cases:** - Missing state file: returns nil error, keeps existing in-memory state - Corrupt state file: returns parse error - Empty state file: returns parse error - Permission errors (read/write): properly reported, skipped when running as root in Docker **Atomic write:** - No leftover .tmp files after successful save - Updated content verified after second save **Getter/setter coverage:** - Domain: get, set, overwrite - Hostname: get, set with nested nameserver records - Port: get, set, delete - Certificate: get, set - GetAllPortKeys enumeration - GetSnapshot returns value copy **Concurrency:** - 20 goroutines × 50 iterations of concurrent get/set/delete with race detector - 10 goroutines doing concurrent Save/Load **Other:** - Snapshot version written correctly - LastUpdated timestamp set on save - File permissions are 0600 - Multiple saves overwrite previous state completely - NewForTest helper creates valid empty state - Save creates nested data directories Also adds `NewForTestWithDataDir()` to the test helper for tests requiring file persistence. Closes [issue #70](https://git.eeqj.de/sneak/dnswatcher/issues/70) <!-- session: agent:sdlc-manager:subagent:e75f60a3-17c4-43f7-a743-32a108ee5081 -->
clawbot added 1 commit 2026-03-02 08:54:56 +01:00
test(state): add comprehensive test coverage for internal/state package
All checks were successful
check / check (push) Successful in 51s
Details
0774688c34 
Add 32 tests covering:
- Save/Load round-trip for all state types (domains, hostnames, ports, certificates)
- Atomic write verification (no leftover temp files)
- PortState.UnmarshalJSON backward compatibility (old single-hostname format)
- Missing, corrupt, and empty state file handling
- Permission error handling (skipped when running as root)
- All getter/setter/delete methods for every state type
- GetSnapshot returns a value copy
- GetAllPortKeys enumeration
- Concurrent read/write safety with race detection
- Concurrent Save/Load safety
- File permission verification (0600)
- Multiple saves overwrite previous state
- LastUpdated timestamp updates on save
- Error field round-trips for certificates and hostnames
- Snapshot version correctness

Also adds NewForTestWithDataDir() helper for tests requiring file persistence.

Closes #70
clawbot added the botneeds-review labels 2026-03-02 08:55:11 +01:00
clawbot commented 2026-03-02 09:01:41 +01:00
Author
Collaborator
Copy Link

✅ Review: PASSED

Summary

Adds 32 comprehensive tests for internal/state, taking it from 0% coverage to thorough coverage of all public API surface. Closes #70.

What was reviewed

  • 2 files changed: state_test.go (+1302 lines), state_test_helper.go (+16 lines — adds NewForTestWithDataDir())
  • No files outside internal/state/ were modified — no existing tests weakened
  • No DNS mocking — state package tests are purely file I/O and serialization, which is correct

Test coverage analysis

All items from issue #70 are addressed:

Requirement Tests
Save→Load round-trip 4 tests (domains, hostnames, ports, certificates)
Atomic write TestSaveAtomicWrite — verifies no .tmp leftover, second save updates content
Backward-compat PortState deser 5 tests (new format, old format, empty, invalid JSON, both formats) + full round-trip through Save/Load
Missing/corrupt state file TestLoadMissingFile, TestLoadCorruptFile, TestLoadEmptyFile
Concurrent access TestConcurrentGetSet (20 goroutines × 50 iterations), TestConcurrentSaveLoad (10 goroutines)
Permission errors TestLoadReadPermissionError, TestSaveWritePermissionError (correctly skip when root in Docker)
Getter/setter coverage Domain, hostname, port (incl. delete), certificate, GetAllPortKeys, GetSnapshot copy safety
Other Version field, LastUpdated, file permissions (0600), directory creation, multiple saves overwrite, NewForTest helper, error field round-trips

Code quality

  • All tests use t.Parallel() ✅
  • Uses t.TempDir() for file persistence tests (auto-cleanup) ✅
  • Helper functions properly use t.Helper() ✅
  • NewForTestWithDataDir() is minimal and follows the same pattern as existing NewForTest() ✅
  • Nolint directives are properly annotated ✅
  • Test file uses package state_test (black-box testing) ✅

Docker build

docker build . passes — all 32 new state tests pass. Permission tests correctly skipped when running as root in Docker container.

Verdict: merge-ready ✅

## ✅ Review: PASSED ### Summary Adds 32 comprehensive tests for `internal/state`, taking it from 0% coverage to thorough coverage of all public API surface. Closes #70. ### What was reviewed - **2 files changed**: `state_test.go` (+1302 lines), `state_test_helper.go` (+16 lines — adds `NewForTestWithDataDir()`) - **No files outside `internal/state/` were modified** — no existing tests weakened - **No DNS mocking** — state package tests are purely file I/O and serialization, which is correct ### Test coverage analysis All items from issue #70 are addressed: | Requirement | Tests | |---|---| | Save→Load round-trip | 4 tests (domains, hostnames, ports, certificates) | | Atomic write | `TestSaveAtomicWrite` — verifies no .tmp leftover, second save updates content | | Backward-compat PortState deser | 5 tests (new format, old format, empty, invalid JSON, both formats) + full round-trip through Save/Load | | Missing/corrupt state file | `TestLoadMissingFile`, `TestLoadCorruptFile`, `TestLoadEmptyFile` | | Concurrent access | `TestConcurrentGetSet` (20 goroutines × 50 iterations), `TestConcurrentSaveLoad` (10 goroutines) | | Permission errors | `TestLoadReadPermissionError`, `TestSaveWritePermissionError` (correctly skip when root in Docker) | | Getter/setter coverage | Domain, hostname, port (incl. delete), certificate, GetAllPortKeys, GetSnapshot copy safety | | Other | Version field, LastUpdated, file permissions (0600), directory creation, multiple saves overwrite, NewForTest helper, error field round-trips | ### Code quality - All tests use `t.Parallel()` ✅ - Uses `t.TempDir()` for file persistence tests (auto-cleanup) ✅ - Helper functions properly use `t.Helper()` ✅ - `NewForTestWithDataDir()` is minimal and follows the same pattern as existing `NewForTest()` ✅ - Nolint directives are properly annotated ✅ - Test file uses `package state_test` (black-box testing) ✅ ### Docker build `docker build .` passes — all 32 new state tests pass. Permission tests correctly skipped when running as root in Docker container. **Verdict: merge-ready** ✅
clawbot added merge-ready and removed needs-reviewbot labels 2026-03-02 09:01:51 +01:00
sneak was assigned by clawbot 2026-03-02 09:02:10 +01:00
clawbot commented 2026-03-02 09:02:54 +01:00
Author
Collaborator
Copy Link

✅ Review: PASS

Summary

32 new tests for internal/state covering all previously-untested persistence and state management code. Coverage goes from 0% to comprehensive. Clean, well-structured test code.

Checklist

  • Tests are meaningful: Real assertions on actual data (nameservers, hostnames, ports, certificates, error states). No trivially-passing tests.
  • No production code changes: Only state_test.go and state_test_helper.go added. Zero production files modified.
  • No DNS mocking: Tests deal with file persistence and in-memory state 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

  • Save/Load round-trips: Domains, hostnames, ports, certificates each verified independently
  • Atomic writes: Verifies no leftover .tmp files after save
  • Error handling: Corrupt files, empty files, permission errors (read and write)
  • Backward-compatible deserialization: Old single-hostname → new hostnames[] format migration
  • Concurrency safety: 20 goroutines × 50 iterations of concurrent get/set/delete, plus concurrent save/load
  • Snapshot isolation: Verifies GetSnapshot() returns a copy that doesn't mutate internal state
  • Edge cases: Missing file (no-op), directory creation on save, file permissions (0600), multiple saves overwriting
  • Error field round-trips: Both hostname and certificate error states survive save/load

Notes

  • state_test_helper.go in the state package provides NewForTest() and NewForTestWithDataDir() — correct Go pattern for white-box test helpers accessible from the state_test external package.
  • All tests use t.Parallel() where safe. The concurrency tests properly use sync.WaitGroup.
  • Good use of t.TempDir() for isolation and automatic cleanup.

Ready to merge.

## ✅ Review: PASS ### Summary 32 new tests for `internal/state` covering all previously-untested persistence and state management code. Coverage goes from 0% to comprehensive. Clean, well-structured test code. ### Checklist - [x] **Tests are meaningful**: Real assertions on actual data (nameservers, hostnames, ports, certificates, error states). No trivially-passing tests. - [x] **No production code changes**: Only `state_test.go` and `state_test_helper.go` added. Zero production files modified. - [x] **No DNS mocking**: Tests deal with file persistence and in-memory state 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 - **Save/Load round-trips**: Domains, hostnames, ports, certificates each verified independently - **Atomic writes**: Verifies no leftover `.tmp` files after save - **Error handling**: Corrupt files, empty files, permission errors (read and write) - **Backward-compatible deserialization**: Old single-`hostname` → new `hostnames[]` format migration - **Concurrency safety**: 20 goroutines × 50 iterations of concurrent get/set/delete, plus concurrent save/load - **Snapshot isolation**: Verifies `GetSnapshot()` returns a copy that doesn't mutate internal state - **Edge cases**: Missing file (no-op), directory creation on save, file permissions (0600), multiple saves overwriting - **Error field round-trips**: Both hostname and certificate error states survive save/load ### Notes - `state_test_helper.go` in the `state` package provides `NewForTest()` and `NewForTestWithDataDir()` — correct Go pattern for white-box test helpers accessible from the `state_test` external package. - All tests use `t.Parallel()` where safe. The concurrency tests properly use `sync.WaitGroup`. - Good use of `t.TempDir()` for isolation and automatic cleanup. Ready to merge. <!-- session: agent:sdlc-manager:subagent:bb3bec9c-91ec-42df-a01e-a9d78f67aec2 -->
sneak merged commit c5bf16055e into main 2026-03-04 11:26:05 +01:00
sneak deleted branch fix/70-state-test-coverage 2026-03-04 11:26:05 +01:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bot
Agent's turn to work
 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 Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#80
Delete Branch "fix/70-state-test-coverage"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?