Repository navigation
Run the ingestion tests against real Postgres and Redis - #1
Merged
Merged
Conversation
The db and cache suites were written against real infrastructure and skip themselves when none is reachable. The ingestion CI job ran neither container, so every one of those tests skipped on every run: db.go sat at 14% covered and redis.go at 31%, not because the behaviour was untested but because the tests never executed. The job stayed green the whole time, which is the part that actually needed fixing. Both containers now run in the job. The schema comes from Django's migrations rather than a copy of the DDL kept in this service, because Django owns those migrations and a second copy would drift the first time a model changed. A final step fails the job if any suite reports skipping for an unreachable dependency - silently testing less than you think is exactly the failure this had. I also split main.go. Everything with a decision in it - the retry loops, the request logger, the env fallbacks, and the route table, which is now a newRouter function with its own tests - moved to bootstrap.go. main() is left as wiring that could only be covered by starting the process and killing it, so it is excluded from coverage with that reasoning recorded in codecov.yml. The route tests assert more than that the paths exist: each one proves the route reaches the handler it claims to, by checking a response only that handler can produce. A route bound to the wrong handler compiles fine and fails only in production.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Running the db suite for the first time caught this immediately. Go's time.Now() carries nanoseconds and Postgres timestamptz stores microseconds, so the received_at returned from InsertHeartbeat disagreed with the value actually written to the row in its last three digits. A client comparing the timestamp in a heartbeat response against a later read of the same job would see two different times for one event. Truncating before the insert makes the returned value the stored value.
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.
PipelineOps sits at 79.97% against its own 80% target, and
codecov/patchis currently failing onmain. The cause turned out not to be missing tests.The db and cache suites have been skipping on every CI run. Both are written against real infrastructure and skip themselves when none is reachable — correct behaviour locally, silent failure in CI. The
ingestion-servicejob ran no Postgres and no Redis, so every one of those tests skipped while the job reported success:Those aren't untested behaviours.
TestFindJobByNameOrID,TestInsertHeartbeat,TestPingand the cache round-trip tests were all already written and thorough. They just never ran.What changed
main.gosplit. Everything with a decision in it — the retry loops, the request logger, the env fallbacks, and the route table, now extracted asnewRouter— moved tobootstrap.goand is tested.main()is left as pure wiring and excluded from coverage, with the reasoning recorded incodecov.ymlrather than left as an unexplained gap.On the new route tests: they assert the route table, then prove each route reaches the handler it claims to by checking a response only that handler can produce — a 503 with
"db":"down"for/healthz, a 400 from the binding on/v1/heartbeat, the resolved job id echoed back for the:jobparameter. A route bound to the wrong handler compiles perfectly and fails only when a real client calls it. The route-count assertion is deliberate too: an unintended route is as much a bug as a missing one.Verified locally:
go build,go vet,golangci-lint run(v2.12.2, the same version CI pins) all clean, and every function inbootstrap.gois at 100%. The db and cache suites still skip on my machine — the local Postgres belongs to another project — so CI is the real verification for those, which is the point of this change.