ci: test the backend on every PR - #306
Open
FrameAutomata wants to merge 4 commits into
Open
Conversation
No workflow ran the backend suite. cli.yml and cli-contract.yml are the only ones invoking `go test`, and both run with working-directory: cli -- the backend was exercised only indirectly, through the contract harness booting it in-process on in-memory SQLite. The PostgreSQL + ClickHouse build, which is what the released image ships, was verified only when someone remembered to run scripts/test-backend-pgch.sh on a machine with Docker. Three jobs, one per supported build-tag combination: dual SQLite, DuckDB telemetry, and PostgreSQL + ClickHouse. The pgch job uses service containers on the same images docker-compose.yml pins. Verified locally before this landed, against rootless podman running the same two images: all 84 ClickHouse and 141 PostgreSQL migrations apply with failures surfaced rather than swallowed, and the tagged suite is green -- 80 tests run, 80 pass, 0 skipped. Worth stating because setupTestDB skips when TEST_CLICKHOUSE_SERVER is unset and a skipped package still reports ok, so a naive run looks green either way. Details worth knowing: - Test scope is a list, not ./... : app/chdb/batch_retry_test.go is `//go:build telemetry_ch` and app/retention/log_cap_duckdb_test.go is `//go:build telemetry_duckdb`, so both ran in no job under the scope this was first modelled on. ./... is not usable for duckdb -- three packages fail under that tag today, which is a separate problem. - cli/** is in the path filter: backend/go.mod replaces the local cli module and app/services/mcpmount imports it, so a cli-only change can break the backend build. cli.yml already lists backend/** for the same reason; this wires up the other half. - CGO_ENABLED is pinned per job to match each Dockerfile (0 for sqlite and pgch, 1 for duckdb) so CI links what ships. The race step has to override it back to 1, since the detector requires cgo. - Go caches are keyed per tag set. setup-go keys only on go.sum, which is identical across all three jobs, so they would race for a single key and the winner's cache would then freeze -- leaving the duckdb job to refetch ~131MB of libduckdb on every run. - The runner is cross-compiled for the platforms release-traceway.yml publishes; proc_windows.go is `//go:build windows` and compiles nowhere else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pgch job died in "Initialize containers" before running a single step: the clickhouse service never reported healthy, and after ~2 minutes of probing the runner aborted the job. Setting CLICKHOUSE_DB makes the image's entrypoint take its first-boot init path. With neither CLICKHOUSE_USER nor CLICKHOUSE_PASSWORD set that path also disables network access for the default user, and on a GitHub runner the container never becomes healthy afterwards. Reverting to the explicit CREATE DATABASE step restores the configuration that was verified end-to-end before the env var replaced it. Worth recording why this was not caught: CLICKHOUSE_DB was verified in isolation -- start the container, confirm the database exists -- but not in the shape CI actually uses, where a health check gates whether the container is usable at all. It also does not reproduce locally under podman, where the same container recovers and answers the identical health check with exit 0, so the comment now warns against re-simplifying it. Re-verified against the current main (post #296/#297/#300) with rootless podman: create-database step OK, 84/84 ClickHouse and 141/141 PostgreSQL migrations applied with failures surfaced, and 87 tests run / 87 pass / 0 skipped across ./app/repositories/... and ./app/chdb/... Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The container never reported healthy, so the job died in "Initialize containers" before a single step ran, in both attempts. Two independent causes, one of them latent behind the other: The entrypoint logs "neither CLICKHOUSE_USER nor CLICKHOUSE_PASSWORD is set, disabling network access for user 'default'" and overwrites default-user.xml with <networks>::1, 127.0.0.1</networks>. Every connection this job makes -- the migration curls and the Go client alike -- arrives from the runner over the published port, so ClickHouse sees the bridge gateway address and rejects it. CLICKHOUSE_SKIP_USER_SETUP=1 keeps the stock users.xml, whose default user accepts ::/0. This would have failed the job at the first query even with a healthy container. The previous commit blamed CLICKHOUSE_DB for the unhealthy container, but the message above appears with or without it -- the run that dropped it failed identically. Both runs also prove the server itself came up: with CLICKHOUSE_DB set, the entrypoint's own readiness loop cleared and it logged "create database 'traceway_test'". So drop the healthcheck rather than iterate on it blind: a --health-cmd that never passes leaves no diagnostic beyond the word "unhealthy" plus four lines of stdout, because ClickHouse logs to files. Waiting from the runner instead checks what the job actually needs -- reachability over the published port, not from inside the container -- and puts any failure in a step that can print the server's real log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every test in this suite reaches live ClickHouse and PostgreSQL through setupTestDB, which calls t.Skip when TEST_CLICKHOUSE_SERVER is missing -- and a package whose every test skipped still prints "ok". Dropping one env var from the Test step would leave the job green while asserting nothing, which is the one failure mode a test workflow must not have. The 80/80 count was verified by hand before the workflow was opened; CI had no way to check it. The run is now emitted as -json, replayed through jq so the log still reads like ordinary go test output, and tallied: any skipped test, or a run that executes no test at all, fails the job. Scoped to pgch deliberately. It is the only job whose suite is gated on env vars -- the sqlite and duckdb harnesses open their databases in-process, so there is no equivalent silent-skip path, and ./... under those tags does contain tests that skip for legitimate reasons. pipefail is set explicitly because the default shell is `bash -e`, which takes the exit status of the last command in a pipeline -- without it, jq succeeding would mask a failing test run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The gap
No workflow runs the backend test suite.
cli.ymlandcli-contract.ymlare the only ones invokinggo test, and both run withworking-directory: cli. Backend changes triggercli.ymlvia its path filter, but that job tests the CLI — the backend is exercised only indirectly, through the contract harness booting it in-process on in-memory SQLite.The
transactional_pg telemetry_chbuild — what the released image ships — was verified only when someone remembered to runscripts/test-backend-pgch.shon a machine with Docker.Three jobs, one per supported build-tag combination
sqlitego test -race ./...+ cross-compile the runnerduckdb-tags telemetry_duckdb, in-memory, no servicespgchdocker-compose.ymlpinsVerified before opening this
Run locally against rootless podman with the same two images:
That last detail matters.
setupTestDBskips whenTEST_CLICKHOUSE_SERVERis unset, and a skipped package still reportsok— so a naive run looks green either way. I checked the skip count explicitly: 80=== RUN, 80--- PASS.So the ClickHouse production build is currently healthy, which is why
pgchblocks rather than starting advisory: a red run later means a real regression, not pre-existing rot.Decisions worth reviewing
Test scope is a list, not
./....app/chdb/batch_retry_test.gois//go:build telemetry_chandapp/retention/log_cap_duckdb_test.gois//go:build telemetry_duckdb— under the scope this was first modelled on (scripts/test-pgch-entrypoint.sh), both ran in no job at all: the untagged job compiles them out, the tagged jobs never reach their packages../...isn't usable for duckdb — three packages fail under that tag today, which is a separate problem.cli/**is in the path filter.backend/go.modhasreplace github.com/tracewayapp/traceway/cli => ../cliandapp/services/mcpmountimportscli/pkg/{client,mcpserver}, so a cli-only change can break the backend build.cli.ymlalready listsbackend/**; this wires up the other half of a symmetry the repo had already chosen.CGO_ENABLEDis pinned per job (0for sqlite/pgch,1for duckdb) to match each Dockerfile, so CI links what ships. The race step overrides back to1— the detector requires cgo, so release parity and race detection want opposite answers, and they're split rather than one being silently dropped.Go caches are keyed per tag set.
setup-gokeys only ongo.sum, which is identical across all three jobs — they'd race for a single key, and the winner's cache would then freeze on exact hits, leaving the duckdb job to refetch ~131MB of libduckdb every run.The runner is cross-compiled for the platforms
release-traceway.ymlpublishes.app/synthetics/browserexec/proc_windows.gois//go:build windowsand compiles in no other job, so editing itsproc_unix.gosibling would break the release build with every PR check still green.Migrations drop the
|| true.scripts/test-pgch-entrypoint.shsuffixes every migration with it, which silently yields a half-built schema. Both loops here fail loudly (--fail-with-body,ON_ERROR_STOP=1) — and I confirmed nothing actually fails, so the suppression protects against nothing.Follow-ups deliberately not in this PR
migrations.Run(), so it builds its schema by a path production never uses. Fixing it needs a migrate-only entrypoint — Go code, not YAML.scripts/test-pgch-entrypoint.shnow disagrees with CI on what a broken migration means. Dropping its|| trueand addingON_ERROR_STOP=1is a two-line change that makes "passes locally" mean the same as "passes in CI".Merge order
Independent of #301/#302/#303/#305 and safe to merge any time. Note it will run against those open PRs as soon as it lands.
🤖 Generated with Claude Code