Add coverage-focused tests across utility and HTTP packages

[GodPuffin]

Summary

• add coverage-focused tests for bootstrap arg parsing, session helpers, middleware, random helpers, view rendering, and router setup
• add tests for auth page handlers, profile auth guards, and lightweight plans/payments helper behavior
• cover record-to-view-model mapping and redirect helpers without requiring a full PocketBase integration harness

Verification

• go test ./...
• golangci-lint run --timeout=5m0s
• go test ./... -coverprofile=coverage

Coverage Notes

• aggregate statement coverage is now 16.4%
• new high-coverage areas include router (87.9%), view (85.7%), sessionutil (100%), random (92.3%), bootstrap (56.7%), middleware (60.0%), and improved auth/profile handler coverage

[GodPuffin]

@claude pls review

[claude]

Claude finished @GodPuffin's task in 1m 41s —— View job

---

PR Review: Add coverage-focused tests across utility and HTTP packages

• Gather context and understand the request
• Read CLAUDE.md for repo-specific guidelines (none found)
• Review all 13 test files
• Post findings

---

Overall this is a solid set of coverage tests. The structure is clean, error messages are descriptive, and HTTP handler tests make proper use of httptest. A few issues worth addressing:

---

Issues

1. Template cache not cleaned up on test failure — view_test.go

TestRenderPageUsesSessionDefaults, TestRenderPagePreservesProvidedValues, and TestRenderPageReturnsTemplateErrorForMissingPage each call resetTemplateCache() at the top but not via t.Cleanup. If a test panics or fails before completing, the cache is left dirty and can corrupt subsequent tests in the same run.

// current
func TestRenderPageUsesSessionDefaults(t *testing.T) {
    resetTemplateCache()
    ...
}

// better
func TestRenderPageUsesSessionDefaults(t *testing.T) {
    t.Cleanup(resetTemplateCache)
    resetTemplateCache()
    ...
}

Fix this →

---

2. secureString panics with empty charset — untested — random_test.go

secureString calls big.NewInt(int64(len(charset))) where len(charset) could be 0. Passing max=0 to crand.Int causes a panic. TestSecureStringSupportsZeroLength already exists as a good model; a complementary test for the empty-charset case would make the contract explicit and prevent silent regressions.

Fix this →

---

3. Duplicate sessionOrRedirect tests across packages

src/internal/http/handlers/plans/session_test.go and src/internal/http/handlers/payments/session_test.go are byte-for-byte identical. This mirrors duplicated production code — both packages define their own sessionOrRedirect. Not a test bug, but worth flagging as a signal that this helper could be lifted to sessionutil to eliminate both copies.

---

4. router_test.go passes nil as app — router_test.go:14

Setup(nil, e)

This works today because route registration doesn't dereference the app, but it's fragile. If a handler is ever registered inline with a closure that captures the app, or if a middleware is wired during Setup, this test will panic. Consider using a *pocketbase.PocketBase{} zero-value instead if it doesn't require initialization.

---

Minor observations

• bootstrap/app_test.go — --version flag behavior is not covered. It's analogous to --help (tested at line 38) and is a trivial case to add for completeness.

• random_test.go — Tests check length and charset but not uniqueness. Adding a two-call distinctness check (e.g., asserting two GenerateJoinCode calls differ) would catch a hypothetical constant-return regression. Low priority but cheap to add.

• payments/helpers_test.go — TestParseForMonth subtests don't call t.Parallel() inside t.Run. Not incorrect, just a consistency note given the rest of the file uses t.Parallel().

• view_test.go:110 — The div template func test asserts div(9, 2) == 4.5 and div(9, 0) == 0 but doesn't test non-numeric interface inputs. If the template can receive a string, that path is unexercised.

---

Summary

The test suite is well-written and achieves the stated coverage goals. The two issues most worth fixing before merge are the t.Cleanup gap in view_test.go (risk of test pollution) and documenting/fixing the secureString("") panic path. Everything else is advisory.
|

[GodPuffin]

@claude, made some updates what do you think now?

[claude]

Claude finished @GodPuffin's task in 51s —— View job

---

Re-review: Add coverage-focused tests across utility and HTTP packages

• Gather context
• Read updated files (view_test.go, random_test.go, random.go, router_test.go)
• Compare against previous review findings
• Post updated review

---

All four issues from the previous review have been resolved:

Issue 1 — t.Cleanup(resetTemplateCache) in view_test.go ✅
All four tests that call resetTemplateCache() at the top now also register it as a cleanup handler. Cache state is guaranteed to be reset even on panic or early failure.

Issue 2 — secureString empty charset guard ✅
random.go now returns an explicit error when charset == "" (lines 37–39), and TestSecureStringRejectsEmptyCharset in random_test.go (line 70) verifies that contract. No more silent panic.

Issue 3 — Duplicate sessionOrRedirect tests ✅ (acknowledged, advisory)
Still duplicated across plans and payments, but that mirrors the production code. Worth a future refactor to sessionutil, not a blocker.

Issue 4 — router_test.go passing nil as app ✅
Now uses &pocketbase.PocketBase{} (line 15), which is a safe zero-value and won't panic if a handler starts dereferencing the app during route registration.

---

The minor observations from before still apply (low priority):

• bootstrap/app_test.go — --version flag not covered
• random_test.go — no two-call distinctness assertion
• payments/helpers_test.go subtests missing t.Parallel() inside t.Run
• view_test.go — div func not tested with non-numeric inputs

None of these are blockers. The test suite looks good to merge.