testing.go helper files (non-_test.go) are compiled into the production binary. This is a standard Go pattern for cross-package test helpers and is harmless since they're in internal/ packages. No action needed.
All tests use t.Parallel() for speed.
Assertion messages are descriptive throughout.
Labeling merge-ready and assigning to @sneak for final review.
## Code Review: PASS ✅
**Reviewed:** [PR #32](https://git.eeqj.de/sneak/webhooker/pulls/32) — test coverage improvements for [issue #28](https://git.eeqj.de/sneak/webhooker/issues/28)
### Verification Results
| Check | Result |
|-------|--------|
| Only test files modified | ✅ 5 new files: 3 `_test.go` + 2 `testing.go` helpers, zero production code changes |
| No linter/CI/Makefile changes | ✅ No changes to Makefile, Dockerfile, go.mod, go.sum, or config |
| Existing test assertions unchanged | ✅ No modifications to `engine_test.go` or `circuit_breaker_test.go` |
| Tests exercise real behavior | ✅ See quality assessment below |
| No mocking the thing under test | ✅ Test doubles are only for dependencies (httptest servers, in-memory SQLite) |
| `docker build .` passes | ✅ Build + `make check` (format, lint, tests) all pass |
### Test Quality Assessment
**`internal/delivery/engine_integration_test.go`** (17 tests, 70 assertions)
- Real SQLite databases with `AutoMigrate` for proper schema
- Real HTTP servers via `httptest` verifying actual request content (Content-Type headers, body content)
- Verifies delivery status transitions (pending → delivered, pending → failed)
- Tests large body DB fetch path (nil body → fetched from event table)
- Tests retry logic including skip-if-not-retrying guard
- Tests worker lifecycle (start/stop) with `require.Eventually` for async verification
- Tests recovery of pending and retrying deliveries
- Tests edge cases: unknown target type → failed, invalid config → failed, timeout → failed
**`internal/middleware/middleware_test.go`** (16 tests, 36 assertions)
- Tests logging middleware captures status codes and passes through to next handler
- Tests CORS in dev mode (allows `*`) vs prod mode (no CORS headers)
- Tests `RequireAuth` with real session round-trips (cookie create → set user → re-read)
- Tests `MetricsAuth` with valid/invalid/missing basic auth credentials
- Tests `ipFromHostPort` helper with table-driven subtests
**`internal/session/session_test.go`** (16 tests, 71 assertions)
- Full cookie round-trip persistence tests (create session → save → new request with cookie → verify data)
- Tests `SetUser`/`GetUserID`/`GetUsername` with actual value verification
- Tests `IsAuthenticated` across all states (new, authenticated, cleared, wrong-type value)
- Tests `Destroy` sets MaxAge=-1 and clears all user keys
- Tests edge cases: overwrite user, destroy-then-save deletes cookie
### Notes
- `testing.go` helper files (non-`_test.go`) are compiled into the production binary. This is a standard Go pattern for cross-package test helpers and is harmless since they're in `internal/` packages. No action needed.
- All tests use `t.Parallel()` for speed.
- Assertion messages are descriptive throughout.
Labeling `merge-ready` and assigning to @sneak for final review.
<!-- session: agent:sdlc-manager:subagent:586beb5d-2acf-4a18-b1a8-c82487ed94fd -->
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 test coverage for three previously-untested packages, addressing issue #28.
Coverage Improvements
internal/deliveryinternal/middlewareinternal/sessionWhat's Tested
delivery (37% → 75%)
processNewTaskwith inline and large (DB-fetched) bodiesprocessRetryTasksuccess, skip non-retrying, large body fetchprocessDeliveryunknown target type handlingrecoverPendingDeliveries,recoverWebhookDeliveries,recoverInFlightNotifybatchingmiddleware (0% → 70%)
LoggingResponseWriterdelegationRequireAuthredirect for unauthenticated, pass-through for authenticatedMetricsAuthbasic auth validationipFromHostPorthelpersession (0% → 52%)
Get/Saveround-trip with real cookie storeSetUser,GetUserID,GetUsername,IsAuthenticatedClearUserremoves all keysDestroyinvalidates session (MaxAge -1)Test Helpers Added
database.NewTestDatabase/NewTestWebhookDBManager— cross-package test helpers for delivery integration testssession.NewForTest— creates session manager without fx lifecycle for middleware testsNotes
httptest, SQLite in-memory, and real cookie stores — no external network callsdocker build .passes (lint + test + build)closes #28
PR created for issue #28. Coverage improvements:
internal/deliveryinternal/middlewareinternal/sessiondocker build .passes (lint + test + build). All tests complete in ~3.5s.Code Review: PASS ✅
Reviewed: PR #32 — test coverage improvements for issue #28
Verification Results
_test.go+ 2testing.gohelpers, zero production code changesengine_test.goorcircuit_breaker_test.godocker build .passesmake check(format, lint, tests) all passTest Quality Assessment
internal/delivery/engine_integration_test.go(17 tests, 70 assertions)AutoMigratefor proper schemahttptestverifying actual request content (Content-Type headers, body content)require.Eventuallyfor async verificationinternal/middleware/middleware_test.go(16 tests, 36 assertions)*) vs prod mode (no CORS headers)RequireAuthwith real session round-trips (cookie create → set user → re-read)MetricsAuthwith valid/invalid/missing basic auth credentialsipFromHostPorthelper with table-driven subtestsinternal/session/session_test.go(16 tests, 71 assertions)SetUser/GetUserID/GetUsernamewith actual value verificationIsAuthenticatedacross all states (new, authenticated, cleared, wrong-type value)Destroysets MaxAge=-1 and clears all user keysNotes
testing.gohelper files (non-_test.go) are compiled into the production binary. This is a standard Go pattern for cross-package test helpers and is harmless since they're ininternal/packages. No action needed.t.Parallel()for speed.Labeling
merge-readyand assigning to @sneak for final review.