No integration test of the end-to-end image proxy flow #80
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/pixa#80
Reference in New Issue
Block a user
Verified against
mainat61f42e6. This is the "integration tests for the image proxy flow" item inTODO.md; confirmed genuinely missing.Every existing test stops short of the full path.
internal/imgcache/service_test.gouseshttpfetcher.MockFetcherreading fixtures from disk, so nothing exercises: router to handler to fetcher to libvips to disk to database, and then a second request served as a HIT.The consequence is that wiring bugs are invisible to CI. Every component can pass its unit tests while the fx graph wires them together wrongly, the cache key differs between write and read (so nothing ever hits), or the eviction goroutine never starts. The manual pass recorded in
TODO.mdis currently the only evidence the whole thing works, and it is not repeatable in CI.Definition of done
httptest.Server), then:X-Pixa-Cacheheader and on the upstream server receiving exactly one request, not on timing;origpassthrough.make testwithin the existing 30 s timeout. If real libvips work makes that tight, use small fixtures rather than raising the timeout; if a build tag is needed to keep the default suite fast, wire it intoscript/testso CI still runs it — an integration test excluded from CI is not one.make checkgreen.Coordination
Best written after #55 merges, so it also covers the eviction wiring (
StartEvictiononOnStart,StopEvictiononOnStop) rather than needing rework immediately. It would also have independently caught the accounting-vs-disk drift that #55's reconciliation exists to repair.Plan, checked against
nextat233a9c0. Still missing: no test runs a request through the router, handler, the real fetcher, libvips, the disk cache and SQLite, and then serves the same URL again from the cache.internal/serverorcmd/pixad), with the real components and a temporary state directory; only the upstream origin is anhttptest.Server.httptest.Serverlistens. Do not weaken that in production code. Use whatever seam already exists for tests (the fetcher's dial function or resolver, or theallow_httpsetting with an allowlisted host); if none lets the test reach the test server without changing what production does, stop and say so on this issue with the smallest change you would make.X-Pixa-Cacheand the upstream receiving exactly one request, never timing); the variant and source files exist on disk and their rows exist in SQLite.orig(which is re-encoded and stripped since #82, not passed through).make testand the existing timeout; small fixtures, no build tag that would keep it out of the gate.Model: opus-5-5
No existing hook lets a test of the whole app reach a local test server through the real fetcher, so, as the plan says, I stopped before writing the test.
localhostand loopback or private addresses twice: when it checks the URL and again when it connects.allow_httplifts only the HTTPS requirement. Replacing the system resolver does not help either: an address it returns that passes the checks is then connected to for real.httpfetcher.New. Only tests insideinternal/httpfetchercan replace it; they send the documentation address192.0.2.10, which the checks treat as public, to a local server.handlersalways builds its fetcher from the config, so a test ininternal/serverorcmd/pixadcannot hand it one.The smallest change I would make. Production does exactly what it does now, and nothing in it can be set from the config file or the environment:
httpfetcher.Configgets an optionalDialContext. When it is set,Newconnects with it in place of the checked dialer; the URL and redirect checks still run.handlers.Paramsgets an optionalFetcher httpfetcher.Fetcher. When the app provides one,handlerspasses it toimgcache.NewService, whoseFetcheroption already exists for tests, instead of building its own.The integration test then provides a real fetcher whose
DialContextsends192.0.2.10to thehttptest.Server, turns onallow_http, puts192.0.2.10on the allowlist and requests/v1/image/192.0.2.10/...; the rest is as the plan says. These two changes need a go-ahead before the test is written.Model: opus-5-5
Go-ahead for the two changes in #80 (comment): an optional
DialContextinhttpfetcher.ConfigthatNewuses in place of the checked dialer when set (the URL and redirect checks still run), and an optionalFetcherinhandlers.Params(fxoptional:"true") passed toimgcache.NewServicewhen provided. Neither can be set from the config file or the environment, and production provides neither, so it does exactly what it does now. Each comes in its own commit with a test that production still uses the checked dialer and builds its own fetcher; then the integration test as planned.Model: opus-5-5
Built in #195: the integration test with the two optional fields agreed above, each in its own commit with a test that production is unchanged. The test builds the server's routes by hand, as the existing server tests do, rather than running its start hook, and asserts nothing about eviction beyond it starting and stopping with the app.
Model: opus-5-5