From fe24d7dd96f01d86cef22f387c82ceaa16af46e4 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 9 Aug 2026 01:36:09 +0300 Subject: [PATCH] ci: run the test suites against a real postgres MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not one of this repository's 28 test files had ever run on the builder: the manifest went from cache_restore straight to abuild, and options="!check" meant abuild did not run them either. Every apk published so far was built from code CI compiled and never executed. A postgres task brings up a database in the VM and exports DOLTSRHT_TEST_PG; a test task runs make vet and make test. Measured on this tree, db/ has 10 tests and 9 of them gate on that DSN, so without it the whole persistence layer is skipped and the build is green regardless; no other package needs it. Hence the guard: an empty DSN fails the task loudly rather than skipping every database suite. That guard is what makes !check an honest claim about where the suites ran instead of a licence to ship untested code, and the APKBUILD now says so. make vet and make test rather than bare go commands, because the Makefile is where -tags gms_pure_go and CGO_ENABLED=0 are named — without the tag either command pulls go-icu-regex in and wants ICU headers the builder lacks. remoteapi/integration_test.go prefers DOLTSRHT_TEST_PG and only falls back to docker run when it is empty, so it never reaches for a daemon the builder does not have. It is moot either way: the file is behind //go:build integration and go test ./... does not compile it, as with spike. No gofmt gate: web/beads.go and four test files are gofmt-dirty on master, so one would be red on arrival. --- .build.yml | 38 +++++++++++++++++++++++++ APKBUILD | 9 +++++- docs/ci.md | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 123 insertions(+), 6 deletions(-) diff --git a/.build.yml b/.build.yml index 670f98283e3ef480c32acc50c886fb5d8d067dc8..422e9ebfc2337fd4e8cd2f0caa173f93ac8005ed 100644 --- a/.build.yml +++ b/.build.yml @@ -15,6 +15,9 @@ packages: - rclone - sassc - minify + # For the database suites, not for the package — see docs/ci.md#packages. + - postgresql + - postgresql-client secrets: # File secret `apk-ci-s3`, installed at ~/.apk-ci.env, containing # APK_CI_S3_ACCESS_KEY / APK_CI_S3_SECRET_KEY for the Garage `repo` bucket. @@ -127,6 +130,41 @@ tasks: # Neither line above may rewrite go.mod or go.sum — a dirty tree here is a # "-dirty" apk, so check rather than trust. docs/ci.md#cache_restore. git status --porcelain + - postgres: | + # A real Postgres in the VM. Without it db/ skips 9 of its 10 tests and + # the build goes green over the whole persistence layer — which is where + # this repository had stood since it was written. Every flag below is + # load-bearing: docs/ci.md#postgres. + sudo install -d -o postgres -g postgres /run/postgresql /var/lib/postgresql/data + sudo -u postgres initdb -D /var/lib/postgresql/data + sudo -u postgres pg_ctl -D /var/lib/postgresql/data -l /tmp/pg.log -w start \ + -o "-k /run/postgresql -h 127.0.0.1 \ + -c fsync=off -c full_page_writes=off -c synchronous_commit=off" + sudo -u postgres createuser -s "$(id -un)" + sudo -u postgres createdb -O "$(id -un)" doltsrht_test + echo "export DOLTSRHT_TEST_PG='postgresql://$(id -un)@127.0.0.1/doltsrht_test?sslmode=disable'" \ + >> ~/.buildenv + - test: | + cd "$REPO" + # An empty DSN would skip every database suite and leave the build green + # over untested code — and options="!check" in the APKBUILD says this task + # is where the suites run, so a silent skip here is a package built on a + # promise nothing kept. It also catches a reordering of the two tasks. + # docs/ci.md#test. + if [ -z "$DOLTSRHT_TEST_PG" ]; then + echo "DOLTSRHT_TEST_PG is unset: the postgres task did not export it," >&2 + echo "so every database suite would skip and this build would lie." >&2 + exit 1 + fi + # `make vet` and `make test` rather than bare go commands: the Makefile is + # where -tags gms_pure_go and CGO_ENABLED=0 are named, and a second copy + # of those here is a second copy to forget. docs/ci.md#test. + make vet + make test + # Printed, not gated, for the same reason as cache_restore's: this task + # runs before abuild, so anything the suites left in the checkout is a + # "-dirty" apk, and this is where it would show. docs/ci.md#test. + git status --porcelain - build: | cd "$REPO" # -d: makedepends are already installed via `packages:` above. diff --git a/APKBUILD b/APKBUILD index 1977c6487a965801e76b346f3ca68c8380845cc2..63a55f2d32c4d56e3467fc6644e3efb22bdbdc21 100644 --- a/APKBUILD +++ b/APKBUILD @@ -24,7 +24,14 @@ pkgdesc="Dolt database hosting for a sourcehut instance" url="https://sourcecraft.dev/bigbes/sr-ht-dolt" arch="x86_64" license="MIT" -# !check — tests want a live Postgres and a chunk store +# !check — the suites are run by the `test` task of .build.yml, against the +# Postgres that manifest brings up, and they run BEFORE this +# package is built. Letting abuild run them again would repeat the +# work with DOLTSRHT_TEST_PG unset, i.e. with 9 of db/'s 10 tests +# skipping. What makes this option honest rather than a licence to +# ship untested code is that task's DSN guard: an empty +# DOLTSRHT_TEST_PG fails it loudly instead of skipping the +# database suites and going green. # !tracedeps — CGO_ENABLED=0 with -tags gms_pure_go, so the binaries are # static: no ICU, no gozstd, nothing to trace options="!check !tracedeps" diff --git a/docs/ci.md b/docs/ci.md index af2c8c4316cd1c58dfef17715b95019c795fb889..bec5c17603beb6fabf2dd31ca895e6f88ef64525 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -6,8 +6,9 @@ submitted at all — the failure is at submission time and reads like nothing in particular. Rationale therefore lives here, and the manifest carries pointers. The pipeline is one linear job on `alpine/edge`: install the cache helper, -assemble the shared SCSS, decide a version, restore caches, package with -`abuild`, publish the apk to `repo.bigb.es/alpine/v3.22/bigbes`, save caches. +assemble the shared SCSS, decide a version, restore caches, start a Postgres, +test, package with `abuild`, publish the apk to +`repo.bigb.es/alpine/v3.22/bigbes`, save caches. It is triggered by a push to the **sourcehut** side. A push to sourcecraft cannot reach builds.sr.ht; the gitsync mirror is what puts the commit on @@ -19,7 +20,11 @@ the critical path for the package repository, not merely an offsite copy. `sassc` and `minify` build the stylesheet; `curl` installs `cacher`; `rclone` publishes the apk; `abuild`, `go` and `git` are the build itself. -There is no `postgresql` here and no compiler toolchain beyond `go`: this +`postgresql` and `postgresql-client` are for the test suite, not for the +package. The apk declares no runtime dependency on Postgres — the daemon talks +to whatever `connection-string` names, which in production is another host. + +There is no compiler toolchain beyond `go`: this package is built `CGO_ENABLED=0` with `-tags gms_pure_go` (see the APKBUILD and the Makefile), which is what keeps `libicu` and `gozstd` off the builder entirely. A default cgo build of this tree fails on missing ICU headers and @@ -189,6 +194,71 @@ largest in the family, so the gap here is the widest; it is not zero anywhere. The same failure was seen for real on a sibling: bench build #359 failed its version gate with `M go.sum` as the only thing in the way. +## postgres + +A real Postgres in the VM, initialised from scratch each build. + +Until this task existed, **not one of this repository's 28 test files had ever +run on the builder** — the manifest went straight from `cache_restore` to +`abuild`, and `options="!check"` in the `APKBUILD` meant abuild did not run them +either. Every apk this pipeline has published was built from code CI had +compiled and never executed. + +What the database needs to be there for, measured on this tree: `db/` has 10 +tests and 9 of them gate themselves on `DOLTSRHT_TEST_PG` (`db/db_test.go`'s +`newTestStore`), so without a DSN the entire persistence layer — repository and +key CRUD, the access-control queries, the schema — is skipped and the build is +green regardless. The other packages do not need it and do not skip without it. + +The suites do not share state: each creates a scratch schema +`doltsrht_test_`, applies `schema.sql` into it, routes its pool there +with lib/pq's `options=-c search_path=…`, and drops it afterwards. So one +database serves the whole suite, and no `CREATE DATABASE` privilege is needed +past the one `createdb` above. + +`fsync=off`, `full_page_writes=off` and `synchronous_commit=off` are safe here +and only here: the database lives for the length of one build and its durability +guarantees protect nothing. `-k /run/postgresql` is why the task creates that +directory — Alpine's package does not — and `createuser -s "$(id -un)"` is what +lets the build user connect without a password, which is what keeps the DSN in +`~/.buildenv` free of a credential. + +### `remoteapi/integration_test.go` does not want Docker + +That file's `startPostgres` prefers `DOLTSRHT_TEST_PG` and falls back to +`docker run postgres:16-alpine` only when the variable is empty, so with the DSN +exported it never looks for a Docker daemon the builder does not have. + +It is moot in any case: the file is behind `//go:build integration`, so `go test +./...` does not even compile it. Running it would additionally need the `dolt` +CLI, which it skips on. `storage/spike_test.go` is behind `//go:build spike` for +the same kind of reason. Neither tag is set here, deliberately. + +## test + +`make vet` and `make test`, not bare `go` commands: the Makefile is where +`-tags gms_pure_go` and `CGO_ENABLED=0` are named, and a second copy of those +two here is a second copy to forget. The tags are not optional — a `go vet` or +`go test` without `gms_pure_go` pulls `go-icu-regex` in and wants ICU headers +the builder has never had. + +The guard on an empty `DOLTSRHT_TEST_PG` exists because the failure it prevents +is silent. If the `postgres` task did not export the DSN — or if someone +reorders the two tasks — every database suite skips with a friendly message, +`go test` exits 0, and the build is green over untested code. An explicit +refusal is the difference between a broken pipeline and a lying one. It is also +what makes `options="!check"` in the `APKBUILD` an honest claim rather than a +licence: that option says "the suites ran in CI", and this guard is the only +thing that keeps it true. + +There is no `gofmt` gate, unlike the tokens sibling. `web/beads.go` and four +test files are gofmt-dirty on master, so a gate would be red on arrival; fixing +them is a separate change and not one to smuggle into a CI wave. + +The task runs **before** `build`, so the tree it leaves behind is the tree +`abuild` packages; the closing `git status --porcelain` is where anything the +suites wrote into the checkout would show. + ## build `REPODEST=$HOME/packages abuild -d` builds and stages the apk. @@ -288,5 +358,7 @@ subpackage ships changes bytes on the next build even where nothing else did. name changes every commit, which `artifacts:` cannot express (it has no globbing). - **No matrix.** One architecture, one image. -- **No `gofmt` gate.** `web/beads.go` and four test files are gofmt-dirty on - master; a gate would be red on arrival. `go vet` runs instead. +- **No `gofmt` gate.** See [test](#test). +- **No `integration` or `spike` build tag.** See + [`remoteapi/integration_test.go` does not want + Docker](#remoteapiintegration_testgo-does-not-want-docker).