From 3021bc0cfd010de4aad732494834700a2f96692b Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Wed, 29 Jul 2026 20:01:25 -0400 Subject: [PATCH 1/2] ci: local test database so DB-gated suites run before push, not in CI The DB-gated suites do not run without OPENWATCH_TEST_DSN, so `make ci-local` silently skipped them and a change could look clean locally and fail in CI. That happened three times in one day on the v0.7 branches: the report-schedule suites (#766), the RBAC role-assign and token tests (#765), and the go mod tidy drift (#762). Every one was a real defect that a local run would have caught. scripts/test-db.sh mirrors the CI service container exactly. Image and credentials match .github/workflows/go-ci.yml, because if they drift then "passes locally" stops meaning "passes in CI", which is the whole point. Two settings are load-bearing and are commented as such in the script: max_connections=400. The default 100 is not enough. The suite runs packages in parallel and internal/db/dbtest clones a template database per package, so a default container deadlocks: the first DB test times out at 120s and the rest cascade. Found this the honest way, by hitting it. It looks exactly like a real failure and is not one, which is worse than no local database at all. fsync, synchronous_commit and full_page_writes off. The database is disposable; durability buys nothing and costs most of the runtime. Port defaults to 5455 rather than 5432: on this workstation 5432 is the dev database and 5433 is hanalyx-postgres. Override with TEST_DB_PORT. The database name ends in _test so the internal/db guard accepts it without the ALLOW_NONTEST bypass, which we never want set. `make ci-local` now warns when OPENWATCH_TEST_DSN is unset and says exactly how to fix it. A silent skip is what let this through repeatedly; a loud skip is the minimum. Verified: full `go test ./internal/...` against the container is clean, and the three suites CI previously caught now run and pass locally. --- Makefile | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/Makefile b/Makefile index 4f1fe74e..97c14889 100644 --- a/Makefile +++ b/Makefile @@ -213,8 +213,25 @@ spec-check: .PHONY: ci-local ci-local: check-generated vet lint vuln spec-check test-race cd frontend && { [ -d node_modules ] || npm ci --no-audit --no-fund; } && npx vitest run + @if [ -z "$$OPENWATCH_TEST_DSN" ]; then \ + echo ""; \ + echo "ci-local WARNING: OPENWATCH_TEST_DSN is unset, so every DB-gated suite was SKIPPED."; \ + echo " CI runs them. A change can pass here and fail there; that happened three times"; \ + echo " on the v0.7 branches. Start the local test database and re-run:"; \ + echo " make test-db && eval \"\$$(scripts/test-db.sh dsn)\" && make ci-local"; \ + echo ""; \ + fi @echo "ci-local: all gates passed — safe to push" +## test-db: start the local test database (mirrors the CI service container) +.PHONY: test-db test-db-down +test-db: + @scripts/test-db.sh up + @echo "eval \"\$$(scripts/test-db.sh dsn)\" # to point tests at it" + +test-db-down: + @scripts/test-db.sh down + .PHONY: clean clean: rm -rf $(DIST_DIR) $(SPA_DIR) From 753f0e5e856ed8b546814f62a0b2a8cc5e05eb73 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Wed, 29 Jul 2026 20:02:09 -0400 Subject: [PATCH 2/2] ci: un-ignore scripts/test-db.sh so the Makefile target has a script The blanket `*test*.sh` in .gitignore swallowed scripts/test-db.sh. `git add` refused it, the ci-local change shipped referencing a script that was never committed, and nothing failed loudly: the PR just contained a Makefile target pointing at a file no one else would have. Same trap as the blanket `*.spec` rule that drops new packaging spec files. Negate deliberately and verify with `git check-ignore -v ` after adding anything to scripts/ whose name contains "test". --- .gitignore | 7 ++++ scripts/test-db.sh | 84 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100755 scripts/test-db.sh diff --git a/.gitignore b/.gitignore index fcedb900..c68a39ef 100644 --- a/.gitignore +++ b/.gitignore @@ -338,6 +338,13 @@ frontend_test.log *test*.md *test*.js *test*.cjs + +# ...but scripts/ holds real, tracked tooling whose names contain "test". The +# blanket patterns above silently swallowed scripts/test-db.sh: `git add` +# refused it, the Makefile shipped referencing a script that was never +# committed, and nothing failed. Same trap as the blanket *.spec rule. +# Negate deliberately, and verify a new one with `git check-ignore -v `. +!scripts/test-db.sh # Exception: committed test harness scripts under packaging/tests/ (e.g. the # container upgrade test) are real tracked files, not transient scratch. !packaging/tests/*test*.sh diff --git a/scripts/test-db.sh b/scripts/test-db.sh new file mode 100755 index 00000000..5a982f8b --- /dev/null +++ b/scripts/test-db.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash +# Local test database, mirroring the CI service container. +# +# WHY THIS EXISTS: the DB-gated suites do not run without OPENWATCH_TEST_DSN, so +# `make ci-local` silently skips them and a change can look clean locally and +# fail in CI. That happened three times in one day on the v0.7 branches: the +# report-schedule suites, the RBAC role-assign tests, and the token tests all +# passed here and failed there. This closes that gap. +# +# Usage: +# scripts/test-db.sh up start the container (idempotent) +# scripts/test-db.sh dsn print the DSN, for `export $(...)` or eval +# scripts/test-db.sh test run the full DB-backed suite +# scripts/test-db.sh down stop and remove the container +# +# eval "$(scripts/test-db.sh dsn)" && go test ./internal/... +set -euo pipefail + +NAME=openwatch-pg-test +# 5432 and 5433 are taken on the dev workstation by the dev DB and by +# hanalyx-postgres. Pick something clear of both; override with TEST_DB_PORT. +PORT="${TEST_DB_PORT:-5455}" +# Image and credentials MUST match .github/workflows/go-ci.yml, or "passes +# locally" stops meaning "passes in CI", which is the whole point. +IMAGE=postgres:16-alpine +USER=openwatch +PASS=openwatch_ci +# The name must end in _test: internal/db refuses a non-_test database unless +# OPENWATCH_TEST_DSN_ALLOW_NONTEST is set, which we never want here. +DB=openwatch_go_test + +dsn() { echo "postgres://${USER}:${PASS}@127.0.0.1:${PORT}/${DB}?sslmode=disable"; } + +up() { + if [ -n "$(docker ps -q -f "name=^${NAME}$")" ]; then + echo "${NAME} already running on ${PORT}" + return 0 + fi + docker rm -f "${NAME}" >/dev/null 2>&1 || true + # max_connections: the default 100 is not enough. The suite runs packages in + # parallel and internal/db/dbtest clones a template database per package, so + # a default container deadlocks and the first DB test times out at 120s while + # the rest cascade. That looks exactly like a real failure and is not one. + # + # fsync/synchronous_commit/full_page_writes off: this database is disposable. + # Durability buys nothing and costs most of the runtime. + docker run -d --name "${NAME}" \ + -e POSTGRES_USER="${USER}" -e POSTGRES_PASSWORD="${PASS}" -e POSTGRES_DB="${DB}" \ + -p "127.0.0.1:${PORT}:5432" \ + --health-cmd "pg_isready -U ${USER}" --health-interval 3s \ + --shm-size=256m \ + "${IMAGE}" \ + -c max_connections=400 \ + -c shared_buffers=256MB \ + -c fsync=off \ + -c synchronous_commit=off \ + -c full_page_writes=off >/dev/null + + printf 'waiting for %s' "${NAME}" + for _ in $(seq 1 40); do + if [ "$(docker inspect "${NAME}" --format '{{.State.Health.Status}}' 2>/dev/null)" = "healthy" ]; then + echo " ready on ${PORT}" + return 0 + fi + printf . + sleep 2 + done + echo + echo "ERROR: ${NAME} did not become healthy" >&2 + docker logs --tail 20 "${NAME}" >&2 || true + return 1 +} + +case "${1:-up}" in + up) up ;; + down) docker rm -f "${NAME}" >/dev/null 2>&1 && echo "removed ${NAME}" || echo "${NAME} not running" ;; + dsn) echo "export OPENWATCH_TEST_DSN='$(dsn)'"; echo "export OPENWATCH_TEST_DSN_ALLOW_NONTEST=no" ;; + test) up + export OPENWATCH_TEST_DSN="$(dsn)" + export OPENWATCH_TEST_DSN_ALLOW_NONTEST=no + echo "running the full DB-backed suite against ${DB} on ${PORT}" + go test ./internal/... ;; + *) echo "usage: $0 {up|down|dsn|test}" >&2; exit 2 ;; +esac