test: close v0.2.0 review coverage gaps#17
Merged
Conversation
Address the remaining QA findings from the v0.2.0 release review
that were flagged as P0/P1 and deferred to a follow-up pass. No
production behavior change in this PR beyond a small, test-only
refactor in internal/app and a randSource injection point in
internal/httpapi/middleware.
Coverage added:
1. internal/logging/logging_test.go (new)
- TestParseLevel: table-driven, 10 cases covering every branch
of parseLevel including the defense-in-depth default that
config.Validate now renders unreachable via Load() but which
stays in the code for future refactor safety.
- TestNew_RespectsConfiguredLevel: 4 cases verifying the level
wired from Config is actually honored by the returned slog
logger (Enabled(Debug/Info/Warn/Error) per level).
2. internal/httpapi/middleware/clientip_test.go (new)
- TestClientIP: 11 cases covering both the X-Forwarded-For
path and the RemoteAddr fallback. IPv6 edge cases are the
primary motivation: XFF single/multi IPv6, RemoteAddr
bracketed IPv6, and bare IPv6 without port that falls
through to the raw r.RemoteAddr. The Fly.io production
smoke test already observed real-world IPv6 clients; these
unit tests now guard the invariant so a refactor does not
silently break rate_limit keying for IPv6 users.
3. internal/httpapi/middleware/requestid.go / requestid_test.go
- Tiny refactor: the crypto/rand.Reader call is indirected
through a package-level randSource io.Reader variable so a
test can swap it out. rand.Reader's declared type is
io.Reader, so type inference keeps the declaration minimal
(var randSource = rand.Reader).
- New TestNewRequestID_FallsBackToSentinelOnReadFailure swaps
randSource for an errorReader and verifies newRequestID
returns the documented "00000000000000000000000000"
sentinel instead of panicking or returning an empty string.
This closes a P1 from the review — the failure path was
previously uncovered because crypto/rand.Reader essentially
never fails on Linux.
4. internal/app/app.go / app_chain_test.go
- Refactor: extract newWithHandlers as an internal test seam.
New() now delegates to it with production usecases
(usecase.NewAnalyzer / usecase.NewPrompter). Middleware
chain, routing, and server setup are unchanged.
- Add stubAnalyzeUsecase and stubPromptUsecase implementing
the handler-side interfaces so tests can substitute fakes
without reaching pkg/prism.
- TestChain_AnalyzeEndpointSuccess: full POST /v1/analyze
through the real middleware chain with a canned prism.Result.
Verifies status 200, X-Request-Id header, and the expected
{"result": {pull_request, analysis}} envelope.
- TestChain_PromptEndpointSuccess: parallel test for
/v1/prompt with explicit mode/language in the request body
and a canned prompt string response.
- TestChain_ConcurrencyLimitRejectsBeyondCapacity: cap the
server at 1 in-flight request, hold the first in the stub
via a release channel, and verify the second request gets
503 service_unavailable via the concurrency_limit
middleware. This closes the P0 E2E gap from PR 6 which
shipped without this case.
Remaining deferred items from the review (to be picked up by
subsequent v0.2.0 cleanup PRs):
- Shared writePrismError helper to dedupe
writeAnalyzeUsecaseError / writePromptUsecaseError
- Map context.DeadlineExceeded to response.CodeTimeout in the
handler error mapping (currently surfaces as 500), plus an
E2E timeout integration test once the fix lands.
- README + apiVersion bump + v0.2.0 tag
Address the PR 17 review nit: the "UC" abbreviation in analyzeUC / promptUC was cute but inconsistent with Go convention (stdlib and most Go codebases spell out acronyms in parameter names unless they are well-known like URL/HTTP/ID). Use analyzeUsecase / promptUsecase for consistency with the handler package's AnalyzeUsecase / PromptUsecase interface names. No behavior change; the symbol is a package-private helper parameter so the rename is purely cosmetic.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 2/4 of the v0.2.0 cleanup pass. Closes every P0/P1 test coverage gap surfaced by the v0.2.0 release review, plus the small test-seam refactor needed to make `internal/app` E2E tests hit the real middleware chain with fake usecases.
No production behavior change beyond:
Everything else is new test code.
What's covered now
1. `internal/logging` (previously 0% coverage)
The `parseLevel` default branch is now unreachable via `Load()` (config.Validate rejects unknown levels since PR 16), but the test stays as defense-in-depth so a future refactor that bypasses `Load` still gets a sane default.
2. `internal/httpapi/middleware` IPv6 and sentinel coverage
The Fly.io production smoke test already observed real-world IPv6 clients (the `2400:4050:b701:9800:...` address in the smoke-test logs was my own IPv6). These unit tests now guard the XFF extraction path so a refactor cannot silently break IPv6 rate-limit keying.
3. `internal/app` chain integration coverage
Refactor: `app.New` → `newWithHandlers(cfg, logger, analyzeUC, promptUC)`. `New` is now a 1-line wrapper that passes in `usecase.NewAnalyzer()` and `usecase.NewPrompter()`. Tests call `newWithHandlers` directly with `stubAnalyzeUsecase` / `stubPromptUsecase` to exercise the real middleware chain with controllable handler responses.
Stubs: `stubAnalyzeUsecase` supports `block` and `entered` channels for concurrent scenario orchestration. The stub also honors `ctx.Done()` so future timeout tests work without changes.
This closes the P0 "concurrency_limit not E2E tested" gap that PR 6 deliberately deferred.
Not addressed in this PR (deferred to subsequent v0.2.0 cleanup)
Local checks
v0.2.0 cleanup context
Intended to be squash-merged.