From 800a0fe6a4d59aadf5eefc10d917a82c2e87c0fb Mon Sep 17 00:00:00 2001 From: Factory Date: Mon, 31 Aug 2026 15:41:18 +0000 Subject: [PATCH] chore(dev): local test stack matches CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo test --workspace` could not pass on a fresh local checkout, for two independent reasons — and both failed in a way that read as a broken branch rather than a missing container. Valkey was simply absent. `oversla-sh`'s integration tests panic by design when `VALKEY_URL` is unreachable, so 14 tests failed on every local run; CI has had a `valkey` service all along. `make local-db` now starts it next to Postgres, `bin/worktree-env.sh` allocates a per-worktree port, and both `VALKEY_URL` and `REDIS_URL` land in `.env.local` — the second name so the resolver cache and rate limiter exercise their Redis backend locally the way they do in CI, instead of quietly falling back to the in-memory store. Postgres was the subtler one. With Valkey up, the api suite still failed by the hundred — tests that pass one at a time. Each holds two pools against a database it clones from a template, and the stock `max_connections = 100` runs out long before the suite does. CI raises the same knob to 500 in its "Tune Postgres" step; the dev compose Postgres now sets it too. The rest is making the trap unreachable rather than merely documented: `make test` prefers nextest (what CI runs, and per-process isolation the env-var tests want) and otherwise caps threads at 4, and `make test` / `make check` preflight both services with a hint instead of a wall of connection errors. The shortener stack moves to its own `SHORTENER_VALKEY_HOST_PORT` so it can run alongside the test store. Verified locally: 2721 passed, 13 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RZvFwDS6Vm7xb17dExGLfm --- .env.example | 5 +- CLAUDE.md | 1 + Makefile | 40 +++++++++++--- bin/worktree-env.sh | 10 +++- crates/oversla-sh/tests/integration.rs | 5 +- docker/docker-compose.dev.yml | 21 ++++++++ docker/docker-compose.shortener.yml | 6 ++- scripts/check-test-services.sh | 74 ++++++++++++++++++++++++++ 8 files changed, 149 insertions(+), 13 deletions(-) create mode 100755 scripts/check-test-services.sh diff --git a/.env.example b/.env.example index 922e831a..2c40253f 100644 --- a/.env.example +++ b/.env.example @@ -51,7 +51,10 @@ RESOLVE_CACHE_MAX_ENTRIES=10000 # Optional Valkey/Redis. Backs rate-limit counters and, when set, the resolver # cache above. Unset means both fall back to process-local in-memory stores, # which is correct for a single replica. -# REDIS_URL=redis://localhost:6379 +# `make local-db` starts one on 6380 (a worktree gets its own port, written to +# .env.local along with REDIS_URL and VALKEY_URL) — that is also the Valkey the +# oversla-sh integration tests talk to. +# REDIS_URL=redis://localhost:6380 # Service-template variables (D44). Every `OVERSLASH_TEMPLATE_VAR_` here # supplies `${}` to a service template in services/ — nothing else in the # environment is reachable from a template, which is what makes it safe to let diff --git a/CLAUDE.md b/CLAUDE.md index ee6aba78..d87f2757 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,7 @@ Overslash is a standalone, multi-tenant actions and authentication gateway for A - **Split integration tests by provider.** Provider-specific tests (OAuth flows, service actions) go in their own file under `crates/overslash-api/tests/` (e.g., `oauth_x.rs`, `google_calendar.rs`). Shared helpers live in `tests/common/mod.rs`. The main `integration.rs` keeps core/generic tests only. - **Register every new test file in `tests/api.rs`.** Those files are modules of one test binary, not targets of their own (`autotests = false` in `crates/overslash-api/Cargo.toml`) — 100+ per-file binaries meant 100+ link steps and dominated CI. Add a `mod ;` line to `tests/api.rs` or the file is silently never compiled or run. Inside the file, use `use crate::common;` (not `mod common;`). +- **Backing services come from `make local-db`.** It starts Postgres *and* Valkey, mirroring the `services:` block of the CI test job. `oversla-sh`'s integration tests panic when `VALKEY_URL` is unreachable, and the API's resolver cache / rate limiter only exercise their Redis backend when `REDIS_URL` is set. In a worktree both URLs are written to `.env.local` (which `make` re-exports), so `make test` needs no manual wiring; `make test` / `make check` preflight the services and fail with a hint instead of a wall of connection errors. - **Use `--test-threads=4`** (or similar) when running the full suite locally to avoid Postgres connection pool exhaustion. - **PR screenshots use the scenarios library.** `dashboard/tests/scenarios/` is the canonical way to capture dashboard screenshots for PRs. Boot the real stack with `make e2e-up`, then run a `dashboard/scripts/screenshot-*.mjs` script — each one signs in via `/auth/dev/token` and seeds fixtures by hitting the real API, so the resulting PNGs reflect what the dashboard actually renders, not a hand-rolled JSON fake. Do not add new route-interception screenshot scripts; extend the scenarios library instead. See [dashboard/tests/scenarios/README.md](dashboard/tests/scenarios/README.md). diff --git a/Makefile b/Makefile index 5236a009..6296f353 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: local local-db local-down dev dev-api dev-dashboard down net test check line-count check-decisions diff-stats allocate-decision fmt clippy migrate new-migration schema sqlx-prepare check-sqlx mock-target install-hooks \ +.PHONY: local local-db local-down dev dev-api dev-dashboard down net test require-services check line-count check-decisions diff-stats allocate-decision fmt clippy migrate new-migration schema sqlx-prepare check-sqlx mock-target install-hooks \ tofu-init tofu-fmt tofu-validate tofu-plan tofu-apply tofu-destroy \ infra-shutdown infra-resume worktree-clean \ dashboard-static web-build web build install \ @@ -60,9 +60,14 @@ local dev: net # Stop the full local dev stack (alias of `down`). local-down: down -# Start local infra only (postgres) — used by e2e-up.sh and worktree isolation. +# Start the local backing services the test suite needs (postgres + valkey) — +# used by e2e-up.sh and worktree isolation. Mirrors the `services:` block of +# the CI test job: `cargo test --workspace` needs both, since `oversla-sh`'s +# integration tests panic when VALKEY_URL is unreachable. In a worktree the +# ports (and VALKEY_URL / REDIS_URL) come from .env.local; in the main repo +# they are the compose defaults, 55432 and 6380. local-db: - @$(WT_ENV); $(COMPOSE) $$PROJ_FLAG -f docker/docker-compose.dev.yml up -d postgres + @$(WT_ENV); $(COMPOSE) $$PROJ_FLAG -f docker/docker-compose.dev.yml up -d postgres valkey # Start the e2e mail stack (GreenMail IMAP/SMTP + the overfwd gateway) — used # by e2e-up.sh so `services/email.yaml` has a real mailbox to talk to. @@ -155,7 +160,9 @@ metabase-e2e: metabase-up @set -a && . docker/metabase/.env.metabase && set +a && \ cargo test -p overslash-api --features sql_policy --test api metabase -- --ignored --test-threads=1 -# Start the oversla.sh shortener dev stack (valkey + shortener on :8081) +# Start the oversla.sh shortener dev stack (valkey + shortener on :8081). +# The crate's integration tests do NOT need this — they talk to the Valkey +# `make local-db` starts (VALKEY_URL). This stack runs the service itself. shortener-dev: $(COMPOSE) -f docker/docker-compose.shortener.yml up --build @@ -243,14 +250,31 @@ worktree-clean: fi # Run all tests -test: - cargo test --workspace +# Needs the backing services up (`make local-db`): Postgres for the API crates, +# Valkey for oversla-sh. In a worktree, `make` re-exports .env.local, so +# DATABASE_URL / VALKEY_URL / REDIS_URL already point at this worktree's ports. +# nextest when it's installed — it is what CI runs, it gives each test its own +# process (a handful of env-var tests leak into each other under plain +# `cargo test`), and its default concurrency is tuned per machine. The fallback +# caps threads at 4, the number CLAUDE.md recommends for a plain cargo run. +test: require-services + @if command -v cargo-nextest >/dev/null 2>&1; then \ + cargo nextest run --workspace; \ + else \ + cargo test --workspace -- --test-threads=4; \ + fi + +# Fail with a usable hint instead of a wall of connection errors when the +# backing services aren't running. +require-services: + @bash bin/worktree-env.sh >/dev/null + @bash scripts/check-test-services.sh # CI check: line counts + decision numbering + fmt + clippy + test -check: line-count check-decisions +check: line-count check-decisions require-services cargo fmt --check cargo clippy --workspace -- -D warnings - cargo test --workspace + @$(MAKE) --no-print-directory test # Every .rs under crates/*/src must stay under 1000 lines (mirrors CI). line-count: diff --git a/bin/worktree-env.sh b/bin/worktree-env.sh index dee8279f..0ebfed88 100755 --- a/bin/worktree-env.sh +++ b/bin/worktree-env.sh @@ -48,6 +48,10 @@ WEB_BASE=$(( (HASH % 9000) + 18000 )) # REST API + SMTP submission port, and the overfwd gateway. Only the ports the # host reaches are published — overfwd talks to GreenMail's IMAP over the # compose network, so 3143 needs no host mapping. +# Valkey for the test suite (oversla-sh integration tests via VALKEY_URL, the +# API's resolver cache / rate limiter via REDIS_URL). Same store, two names — +# exactly how CI wires its `valkey` service. +VALKEY_BASE=$(( (HASH % 9000) + 46000 )) GREENMAIL_API_BASE=$(( (HASH % 9000) + 38000 )) GREENMAIL_SMTP_BASE=$(( (HASH % 9000) + 33000 )) OVERFWD_BASE=$(( (HASH % 9000) + 41000 )) @@ -71,6 +75,7 @@ PG_PORT=$(find_free_port "$PG_BASE" postgres) || exit 1 API_PORT=$(find_free_port "$API_BASE" api) || exit 1 DASH_PORT=$(find_free_port "$DASH_BASE" dashboard) || exit 1 WEB_PORT=$(find_free_port "$WEB_BASE" web) || exit 1 +VALKEY_PORT=$(find_free_port "$VALKEY_BASE" valkey) || exit 1 GREENMAIL_API_PORT=$(find_free_port "$GREENMAIL_API_BASE" greenmail-api) || exit 1 GREENMAIL_SMTP_PORT=$(find_free_port "$GREENMAIL_SMTP_BASE" greenmail-smtp) || exit 1 OVERFWD_PORT=$(find_free_port "$OVERFWD_BASE" overfwd) || exit 1 @@ -89,10 +94,13 @@ PG_HOST_PORT=${PG_PORT} API_HOST_PORT=${API_PORT} DASH_HOST_PORT=${DASH_PORT} OVERSLASH_WEB_PORT=${WEB_PORT} +VALKEY_HOST_PORT=${VALKEY_PORT} +VALKEY_URL=redis://localhost:${VALKEY_PORT} +REDIS_URL=redis://localhost:${VALKEY_PORT} GREENMAIL_API_PORT=${GREENMAIL_API_PORT} GREENMAIL_SMTP_PORT=${GREENMAIL_SMTP_PORT} OVERFWD_PORT=${OVERFWD_PORT} DATABASE_URL=postgres://overslash:overslash@localhost:${PG_PORT}/overslash EOF -echo "Worktree env: project=${PROJECT_NAME} pg=${PG_PORT} api=${API_PORT} dashboard=${DASH_PORT} web=${WEB_PORT} greenmail=${GREENMAIL_API_PORT} overfwd=${OVERFWD_PORT}" +echo "Worktree env: project=${PROJECT_NAME} pg=${PG_PORT} api=${API_PORT} dashboard=${DASH_PORT} web=${WEB_PORT} valkey=${VALKEY_PORT} greenmail=${GREENMAIL_API_PORT} overfwd=${OVERFWD_PORT}" diff --git a/crates/oversla-sh/tests/integration.rs b/crates/oversla-sh/tests/integration.rs index 2a21e955..be059173 100644 --- a/crates/oversla-sh/tests/integration.rs +++ b/crates/oversla-sh/tests/integration.rs @@ -1,7 +1,8 @@ //! Integration tests: full API flows against a real Valkey. //! //! Tests require a running Valkey reachable via `VALKEY_URL` (default: -//! `redis://localhost:6380` — what `make shortener-dev` exposes). If Valkey +//! `redis://localhost:6380` — what `make local-db` exposes; a worktree gets +//! its own port and writes `VALKEY_URL` into `.env.local`). If Valkey //! is not reachable, tests **panic** so broken CI surfaces loudly rather //! than passing green on an empty run. Match the `overslash-api` pattern of //! failing hard when a required test dependency is missing. @@ -26,7 +27,7 @@ async fn start() -> (SocketAddr, Client) { let storage = Storage::connect(&url).await.unwrap_or_else(|e| { panic!( "cannot connect to Valkey at {url}: {e}\n\ - hint: run `make shortener-dev` or set VALKEY_URL to a reachable instance" + hint: run `make local-db` or set VALKEY_URL to a reachable instance" ) }); storage diff --git a/docker/docker-compose.dev.yml b/docker/docker-compose.dev.yml index a3a5ad4a..5f7208c4 100644 --- a/docker/docker-compose.dev.yml +++ b/docker/docker-compose.dev.yml @@ -7,6 +7,12 @@ services: POSTGRES_USER: overslash POSTGRES_PASSWORD: overslash POSTGRES_DB: overslash + # The test suite runs many tests concurrently, and each one holds two pools + # (the test's own and the in-process API server's) against a database it + # clones from a template. The stock 100 connections run out well before the + # suite finishes, and the result is dozens of unrelated-looking failures. + # CI raises the same knob to 500 (the "Tune Postgres" step in ci.yml). + command: ["postgres", "-c", "max_connections=500"] ports: - "127.0.0.1:${PG_HOST_PORT:-55432}:5432" volumes: @@ -17,6 +23,21 @@ services: timeout: 3s retries: 5 + # Valkey — the backing store the test suite talks to, mirroring the `valkey` + # service in CI (.github/workflows/ci.yml): `oversla-sh`'s integration tests + # (VALKEY_URL) and the API's resolver cache / rate limiter (REDIS_URL) both + # point at it. No volume: test data is disposable, and a fresh store on every + # bring-up is the point. + valkey: + image: valkey/valkey:8-alpine + ports: + - "127.0.0.1:${VALKEY_HOST_PORT:-6380}:6379" + healthcheck: + test: ["CMD", "valkey-cli", "ping"] + interval: 5s + timeout: 3s + retries: 5 + api: build: context: .. diff --git a/docker/docker-compose.shortener.yml b/docker/docker-compose.shortener.yml index 9d792f1b..74c6e955 100644 --- a/docker/docker-compose.shortener.yml +++ b/docker/docker-compose.shortener.yml @@ -5,10 +5,14 @@ name: oversla-sh # can start it alongside `make dev` without collisions. services: + # Its own port variable and default: this stack runs the shortener *service*, + # while the test suite's Valkey comes from `make local-db` (VALKEY_HOST_PORT, + # 6380 by default). Sharing one variable meant the two stacks could not be up + # at the same time. valkey: image: valkey/valkey:8-alpine ports: - - "127.0.0.1:${VALKEY_HOST_PORT:-6380}:6379" + - "127.0.0.1:${SHORTENER_VALKEY_HOST_PORT:-6390}:6379" volumes: - valkey-data:/data healthcheck: diff --git a/scripts/check-test-services.sh b/scripts/check-test-services.sh new file mode 100755 index 00000000..44df544d --- /dev/null +++ b/scripts/check-test-services.sh @@ -0,0 +1,74 @@ +#!/usr/bin/env bash +# Preflight for `make test` / `make check`: are the backing services the test +# suite needs actually reachable? +# +# Without this, a missing Postgres or Valkey surfaces as hundreds of failed +# tests (oversla-sh's integration tests panic by design when VALKEY_URL is +# unreachable — see crates/oversla-sh/tests/integration.rs), which reads like a +# broken branch rather than a missing container. +# +# Reads DATABASE_URL / VALKEY_URL from the environment, falling back to +# .env.local (worktree ports) then .env, then to the compose defaults — the +# same precedence the test binaries see, since `make` exports .env.local and +# dotenvy never overrides an already-set variable. + +set -uo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" + +# Pull a KEY=value out of an env file without executing it. +from_file() { + local key="$1" file="$2" + [ -f "$file" ] || return 1 + local line + line=$(grep -E "^${key}=" "$file" | tail -1) || return 1 + [ -n "$line" ] || return 1 + printf '%s' "${line#*=}" +} + +resolve() { + local key="$1" default="$2" value + value="${!key:-}" + [ -n "$value" ] || value=$(from_file "$key" "$REPO_ROOT/.env.local") || true + [ -n "$value" ] || value=$(from_file "$key" "$REPO_ROOT/.env") || true + [ -n "$value" ] || value="$default" + printf '%s' "$value" +} + +# host:port out of a URL like scheme://[user[:pass]@]host[:port][/path]. +host_port() { + local url="$1" default_port="$2" hostport + hostport="${url#*://}" + hostport="${hostport%%/*}" + hostport="${hostport##*@}" + case "$hostport" in + *:*) printf '%s %s' "${hostport%%:*}" "${hostport##*:}" ;; + *) printf '%s %s' "$hostport" "$default_port" ;; + esac +} + +failed=0 + +probe() { + local label="$1" url="$2" default_port="$3" hint="$4" + local host port + read -r host port <<<"$(host_port "$url" "$default_port")" + if timeout 3 bash -c "exec 3<>/dev/tcp/${host}/${port}" 2>/dev/null; then + return 0 + fi + echo " ✗ ${label} unreachable at ${host}:${port} (${url})" >&2 + echo " ${hint}" >&2 + failed=1 +} + +DATABASE_URL_VALUE=$(resolve DATABASE_URL "postgres://overslash:overslash@localhost:55432/overslash") +VALKEY_URL_VALUE=$(resolve VALKEY_URL "redis://localhost:6380") + +probe "Postgres" "$DATABASE_URL_VALUE" 5432 "run \`make local-db\`, then \`make migrate\`" +probe "Valkey" "$VALKEY_URL_VALUE" 6379 "run \`make local-db\` (oversla-sh's tests panic without it)" + +if [ "$failed" -ne 0 ]; then + echo "" >&2 + echo "Backing services for the test suite are not up. See the hints above." >&2 + exit 1 +fi