From 87e7aa5f91f2e37c7f05d04edf3544e493d7f0ec Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Wed, 4 Nov 2026 13:20:24 -0600 Subject: [PATCH 1/5] test: #21 run the PostgreSQL suites in CI The database-backed suites were skipped in CI because nothing set TEST_DATABASE_URL, so "27 skipped" was passing for the whole database layer. The new `integration` job creates a throwaway PostgreSQL cluster inside the runner's temp directory, socket-only, applies the migrations, runs the server suite and deletes the cluster. No shared test database, no secret to hand out, and the application's `crate` database is never in reach. `deploy` now requires the job, so a deploy cannot ignore the database tests. The cluster alone would still have left the failure mode from "skipped" to "silently empty", so `server/scripts/assert-postgres-tests.mjs` reads the JSON report and fails when any database-backed file has zero executed or any pending assertions. Verified against both reports: with the database it prints the four files and exits 0, without it fails on the three suites that skip. --- .github/workflows/ci.yml | 76 ++++++++++++++++++++- README.md | 17 ++++- docs/VM-setup.md | 6 ++ server/scripts/assert-postgres-tests.mjs | 85 ++++++++++++++++++++++++ 4 files changed, 181 insertions(+), 3 deletions(-) create mode 100644 server/scripts/assert-postgres-tests.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df76816..cf27047 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,9 +76,83 @@ jobs: exit 1 fi + # The database-backed suites are skipped when TEST_DATABASE_URL is unset, so + # until this job existed CI reported them as "27 skipped" and stayed green. + # The tests guard the things unit tests cannot see: SQL that only fails once + # PostgreSQL parses it, keyset paging across ties and NULLs, facet counts + # against the filters, and the MusicBrainz import preserving reviews. + # + # The cluster is created here and thrown away at the end. Deliberately not the + # application database and not a shared test database: no secret to hand out, + # no state to drift, and running the destructive seed is impossible by + # construction. It listens on a unix socket only, so it cannot accept a TCP + # connection even while it exists. + integration: + name: PostgreSQL integration tests + runs-on: [self-hosted, linux, x64] + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Install pnpm + uses: pnpm/action-setup@v4 + with: + version: 10.33.0 + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: 24 + cache: pnpm + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Start a throwaway PostgreSQL cluster + run: | + set -euo pipefail + export PGDATA="${RUNNER_TEMP}/pgdata" + rm -rf "${PGDATA}" + initdb -D "${PGDATA}" --auth=trust --no-sync + # -k puts the socket in the data directory and `listen_addresses=''` + # keeps it off the network entirely. + pg_ctl -D "${PGDATA}" -l "${PGDATA}/server.log" \ + -o "-c listen_addresses='' -k ${PGDATA}" start + createdb -h "${PGDATA}" crate_test + echo "PGDATA=${PGDATA}" >> "${GITHUB_ENV}" + echo "TEST_DATABASE_URL=postgres:///crate_test?host=${PGDATA}" >> "${GITHUB_ENV}" + psql -h "${PGDATA}" -d crate_test -tAc 'select version()' + + - name: Apply migrations + run: DATABASE_URL="$TEST_DATABASE_URL" pnpm --filter @t19/server db:migrate + + - name: Run the server test suite + # The JSON report is only there for the next step; the default reporter + # still prints the normal output a reader expects. + run: | + pnpm --filter @t19/server exec vitest run \ + --reporter=default \ + --reporter=json \ + --outputFile="${RUNNER_TEMP}/vitest.json" + + # A green run is not evidence on its own: without a database every + # PostgreSQL suite skips and vitest still exits 0. See the script header. + - name: Fail if the PostgreSQL tests were skipped + run: node server/scripts/assert-postgres-tests.mjs "${RUNNER_TEMP}/vitest.json" + + - name: Remove the cluster + if: always() + run: | + if [ -f "${PGDATA}/postmaster.pid" ]; then + pg_ctl -D "${PGDATA}" -m immediate stop || true + fi + rm -rf "${PGDATA}" + deploy: name: Deploy to the group VM - needs: quality + # Both jobs have to pass. Deploying with the database layer untested is the + # same gap as deploying a server that was never built (#44). + needs: [quality, integration] if: github.ref == 'refs/heads/main' && github.event_name == 'push' # Pinned to the `project2` label on purpose. This job writes to # /var/www/html/project2 and restarts crate-api, both of which only exist diff --git a/README.md b/README.md index 71f69a2..9c15d35 100644 --- a/README.md +++ b/README.md @@ -83,8 +83,21 @@ CI also builds the project and checks the client bundle size. | `pnpm db:fetch:musicbrainz` | Rebuild the local MusicBrainz snapshot (network required) | | `pnpm db:seed:large` | Reset to a large synthetic catalog | -Tests cover frontend components and backend logic. Additional PostgreSQL tests cover search, sorting, rating updates and filters. These run when `TEST_DATABASE_URL` is configured and require a separate test database with migrations applied. The -[VM setup guide](docs/VM-setup.md) includes API smoke tests. +Tests cover frontend components and backend logic. The database-backed suites +(search, sorting, filtering and facet counts, and the MusicBrainz import) need a +real PostgreSQL server and are skipped unless `TEST_DATABASE_URL` is set. + +CI starts a temporary PostgreSQL cluster for them, applies the migrations, runs +the suites and fails if any of them was skipped instead of executed +(`server/scripts/assert-postgres-tests.mjs`). To run them locally, point +`TEST_DATABASE_URL` at a database you do not mind losing - the tests insert and +delete fixture rows: + +```bash +TEST_DATABASE_URL=postgres://t19:password@127.0.0.1:5432/crate_test pnpm --filter @t19/server test +``` + +The [VM setup guide](docs/VM-setup.md) includes API smoke tests. ## Database seed data diff --git a/docs/VM-setup.md b/docs/VM-setup.md index d560b7b..541dc63 100644 --- a/docs/VM-setup.md +++ b/docs/VM-setup.md @@ -41,6 +41,12 @@ migrations, imports the committed MusicBrainz catalog snapshot and restarts `crate-api.service`. The import is repeatable and does not reset the database or delete user data. Deployment does not run either destructive seed command. +The `integration` job runs the database-backed test suites. It starts a +temporary PostgreSQL cluster in the runner's temp directory, listening on a unix +socket only, applies the migrations to it, runs the tests and deletes it +afterwards. It never connects to the `crate` database, so the names, fixtures +and cleanup in those tests cannot affect the application data. + Apache serves the frontend at `/project2/` and forwards `/graphql` requests to the API on port 3001: diff --git a/server/scripts/assert-postgres-tests.mjs b/server/scripts/assert-postgres-tests.mjs new file mode 100644 index 0000000..ca9a08a --- /dev/null +++ b/server/scripts/assert-postgres-tests.mjs @@ -0,0 +1,85 @@ +// Fails when the PostgreSQL test files were skipped instead of executed. +// +// Every PostgreSQL suite is wrapped in `describe.skipIf(!process.env.TEST_DATABASE_URL)`, +// so a CI job that forgets the variable still exits 0 with the whole database +// layer missing. `pnpm test` would report "61 passed | 27 skipped" and nothing +// would be wrong by its own account. This script reads the JSON report from +// that run and requires every database-backed file to have executed, which is +// the difference between "the tests pass" and "the tests ran". +// +// Usage: node server/scripts/assert-postgres-tests.mjs + +import { readFileSync } from 'node:fs'; + +// Files that cannot run without a database. Adding a new one here is deliberate: +// the list is the guard, so it should grow with the suite. +const databaseBackedFiles = [ + 'src/db/musicBrainzCatalog.test.ts', + 'src/search/facets.postgres.test.ts', + 'src/search/search.postgres.test.ts', + 'src/search/sort.postgres.test.ts', +]; + +const reportPath = process.argv[2]; +if (!reportPath) { + console.error('::error::Usage: node assert-postgres-tests.mjs '); + process.exit(1); +} + +let report; +try { + report = JSON.parse(readFileSync(reportPath, 'utf8')); +} catch { + console.error(`::error::Could not read the vitest report at ${reportPath}`); + process.exit(1); +} + +// The reporter names files by absolute path. Keep only the part after `server/`. +function relativeName(name) { + const normalized = name.replaceAll('\\', '/'); + const marker = '/server/'; + const index = normalized.lastIndexOf(marker); + return index === -1 ? normalized : normalized.slice(index + marker.length); +} + +const resultsByFile = new Map( + (report.testResults ?? []).map((file) => [relativeName(file.name), file]), +); + +let failed = false; +for (const file of databaseBackedFiles) { + const result = resultsByFile.get(file); + + if (!result) { + console.error(`::error::${file} did not run at all`); + failed = true; + continue; + } + + // A skipped suite still produces a "passed" file containing only pending + // assertions, so count what actually ran rather than trusting the file + // status. A file with some tests running and some skipped also fails, which + // catches `describe.skipIf` on one block of a mixed file. + const assertions = result.assertionResults ?? []; + const executed = assertions.filter((assertion) => assertion.status === 'passed').length; + const pending = assertions.filter((assertion) => assertion.status === 'pending').length; + + if (result.status !== 'passed' || executed === 0 || pending > 0) { + console.error( + `::error::${file} did not fully run (${result.status}, ${executed} passed, ${pending} skipped) - is TEST_DATABASE_URL set?`, + ); + failed = true; + continue; + } + + console.log(`${file} ${executed} passed`); +} + +if (failed) { + console.error( + '::error::The PostgreSQL tests did not run. Point TEST_DATABASE_URL at a migrated test database.', + ); + process.exit(1); +} + +console.log(`All ${databaseBackedFiles.length} database-backed test files executed.`); From 3fd506752c0973b8222f3bb75195b935be192c5e Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Wed, 4 Nov 2026 13:35:28 -0600 Subject: [PATCH 2/5] fix: #21 find the PostgreSQL server binaries on the runner The first CI run failed with 'initdb: command not found' in 12 seconds: Debian puts the server binaries in /usr/lib/postgresql//bin and only the client on PATH, so PostgreSQL 18 being installed is not enough. Try PATH first, then the newest installed major version, and fail with a message that names the missing package if neither works. --- .github/workflows/ci.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cf27047..e36f14a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -111,6 +111,24 @@ jobs: - name: Start a throwaway PostgreSQL cluster run: | set -euo pipefail + # Debian installs the server binaries under /usr/lib/postgresql//bin + # and only puts the client on PATH, so `initdb` is not found even with + # PostgreSQL 18 installed. Use PATH when it works, otherwise the newest + # installed major version. + if ! command -v initdb >/dev/null 2>&1; then + pg_bindir='' + for dir in /usr/lib/postgresql/*/bin; do + if [ -x "${dir}/initdb" ]; then + pg_bindir="${dir}" + fi + done + if [ -z "${pg_bindir}" ]; then + echo "::error::initdb was not found. Install a PostgreSQL server package on the runner." + exit 1 + fi + export PATH="${pg_bindir}:${PATH}" + fi + initdb --version export PGDATA="${RUNNER_TEMP}/pgdata" rm -rf "${PGDATA}" initdb -D "${PGDATA}" --auth=trust --no-sync From b9d12eb49050c706f3ac7b00c4369ce6a6949967 Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Thu, 5 Nov 2026 19:08:06 -0600 Subject: [PATCH 3/5] test: #21 catch partially skipped database files in the guard Review by rachelks on #72 found that the guard only looked at whole files and at assertions with status `pending`. Vitest reports a skipped assertion as `skipped`, not `pending`, so that counter was always zero: a file with eleven passing tests and one `it.skip` was reported as fully executed. Demonstrated on a real report: flipping one assertion in musicBrainzCatalog.test.ts to `skipped` made the old guard print "4 passed" and exit 0. It now exits 1 with "3 passed, 1 skipped". `skipped`, `pending` and `disabled` all block. `todo` is allowed and counted in the output, so a test written down on purpose does not fail CI while an unplanned skip cannot hide. The check is now `checkReport()`, called by the CLI, so it can be covered without spawning a process. Eight tests: the passing case, a missing file, a file where nothing ran, one skipped assertion in a passing file, a failed file, an allowed todo, and the CLI exit codes for both outcomes. The suite list is imported from the script, so adding a database suite does not silently weaken the test. --- server/scripts/assert-postgres-tests.mjs | 130 +++++++++++------ server/scripts/assert-postgres-tests.test.mjs | 136 ++++++++++++++++++ 2 files changed, 221 insertions(+), 45 deletions(-) create mode 100644 server/scripts/assert-postgres-tests.test.mjs diff --git a/server/scripts/assert-postgres-tests.mjs b/server/scripts/assert-postgres-tests.mjs index ca9a08a..667914b 100644 --- a/server/scripts/assert-postgres-tests.mjs +++ b/server/scripts/assert-postgres-tests.mjs @@ -7,33 +7,25 @@ // that run and requires every database-backed file to have executed, which is // the difference between "the tests pass" and "the tests ran". // +// It also counts assertions, not just files: an `it.skip` inside a file that +// otherwise runs still leaves the file `passed`. A skipped assertion is a +// failure here. `it.todo` is allowed, because that is how a planned test is +// written down on purpose, and it carries no expectation of running yet. +// // Usage: node server/scripts/assert-postgres-tests.mjs import { readFileSync } from 'node:fs'; +import { pathToFileURL } from 'node:url'; // Files that cannot run without a database. Adding a new one here is deliberate: // the list is the guard, so it should grow with the suite. -const databaseBackedFiles = [ +export const databaseBackedFiles = [ 'src/db/musicBrainzCatalog.test.ts', 'src/search/facets.postgres.test.ts', 'src/search/search.postgres.test.ts', 'src/search/sort.postgres.test.ts', ]; -const reportPath = process.argv[2]; -if (!reportPath) { - console.error('::error::Usage: node assert-postgres-tests.mjs '); - process.exit(1); -} - -let report; -try { - report = JSON.parse(readFileSync(reportPath, 'utf8')); -} catch { - console.error(`::error::Could not read the vitest report at ${reportPath}`); - process.exit(1); -} - // The reporter names files by absolute path. Keep only the part after `server/`. function relativeName(name) { const normalized = name.replaceAll('\\', '/'); @@ -42,44 +34,92 @@ function relativeName(name) { return index === -1 ? normalized : normalized.slice(index + marker.length); } -const resultsByFile = new Map( - (report.testResults ?? []).map((file) => [relativeName(file.name), file]), -); +/** + * Checks a parsed vitest JSON report. + * + * Returns the lines to print and whether the run should fail, so the same logic + * can be covered by tests without spawning a process. + */ +export function checkReport(report) { + const passedLines = []; + const errorLines = []; + + const resultsByFile = new Map( + (report.testResults ?? []).map((file) => [relativeName(file.name), file]), + ); + + for (const file of databaseBackedFiles) { + const result = resultsByFile.get(file); + + if (!result) { + errorLines.push(`${file} did not run at all`); + continue; + } -let failed = false; -for (const file of databaseBackedFiles) { - const result = resultsByFile.get(file); + // A skipped suite still produces a "passed" file containing only pending + // assertions, so count what actually ran rather than trusting the file + // status. A file with some tests running and some skipped also fails, which + // catches a stray `it.skip` as well as `describe.skipIf` on one block of a + // mixed file. + // + // Vitest reports a skipped assertion as `skipped`, not as jest's `pending`. + // Counting only `pending` is how a single `it.skip` in a database file went + // unnoticed: the file was `passed`, and every counter read zero. + const assertions = result.assertionResults ?? []; + const countOf = (status) => + assertions.filter((assertion) => assertion.status === status).length; + const executed = countOf('passed'); + const skipped = countOf('skipped') + countOf('pending') + countOf('disabled'); + const todo = countOf('todo'); - if (!result) { - console.error(`::error::${file} did not run at all`); - failed = true; - continue; + if (result.status !== 'passed' || executed === 0 || skipped > 0) { + errorLines.push( + `${file} did not fully run (${result.status}, ${executed} passed, ${skipped} skipped) - is TEST_DATABASE_URL set?`, + ); + continue; + } + + // `todo` is a test written down on purpose and not yet written, so it does + // not block. Reporting it keeps the count honest when one is added. + passedLines.push(`${file} ${executed} passed${todo > 0 ? `, ${todo} todo` : ''}`); } - // A skipped suite still produces a "passed" file containing only pending - // assertions, so count what actually ran rather than trusting the file - // status. A file with some tests running and some skipped also fails, which - // catches `describe.skipIf` on one block of a mixed file. - const assertions = result.assertionResults ?? []; - const executed = assertions.filter((assertion) => assertion.status === 'passed').length; - const pending = assertions.filter((assertion) => assertion.status === 'pending').length; - - if (result.status !== 'passed' || executed === 0 || pending > 0) { - console.error( - `::error::${file} did not fully run (${result.status}, ${executed} passed, ${pending} skipped) - is TEST_DATABASE_URL set?`, + if (errorLines.length > 0) { + errorLines.push( + 'The PostgreSQL tests did not run. Point TEST_DATABASE_URL at a migrated test database.', ); - failed = true; - continue; + return { ok: false, passedLines, errorLines }; } - console.log(`${file} ${executed} passed`); + passedLines.push( + `All ${databaseBackedFiles.length} database-backed test files executed.`, + ); + return { ok: true, passedLines, errorLines }; } -if (failed) { - console.error( - '::error::The PostgreSQL tests did not run. Point TEST_DATABASE_URL at a migrated test database.', - ); - process.exit(1); +function main() { + const reportPath = process.argv[2]; + if (!reportPath) { + console.error('::error::Usage: node assert-postgres-tests.mjs '); + return 1; + } + + let report; + try { + report = JSON.parse(readFileSync(reportPath, 'utf8')); + } catch { + console.error(`::error::Could not read the vitest report at ${reportPath}`); + return 1; + } + + const { ok, passedLines, errorLines } = checkReport(report); + for (const line of passedLines) console.log(line); + for (const line of errorLines) console.error(`::error::${line}`); + + return ok ? 0 : 1; } -console.log(`All ${databaseBackedFiles.length} database-backed test files executed.`); +// Only run when invoked as a script, so the tests can import checkReport. +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + process.exit(main()); +} diff --git a/server/scripts/assert-postgres-tests.test.mjs b/server/scripts/assert-postgres-tests.test.mjs new file mode 100644 index 0000000..638bda3 --- /dev/null +++ b/server/scripts/assert-postgres-tests.test.mjs @@ -0,0 +1,136 @@ +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { afterEach, describe, expect, it } from 'vitest'; + +import { checkReport, databaseBackedFiles } from './assert-postgres-tests.mjs'; + +const script = fileURLToPath(new URL('./assert-postgres-tests.mjs', import.meta.url)); + +const temporaryDirectories = []; + +afterEach(() => { + for (const directory of temporaryDirectories.splice(0)) { + rmSync(directory, { recursive: true, force: true }); + } +}); + +// Every file ran with passing assertions unless the test says otherwise. The +// file list comes from the script, so adding a suite there does not silently +// weaken this test. +function report({ omit = [], replace = {} } = {}) { + return { + testResults: databaseBackedFiles + .filter((file) => !omit.includes(file)) + .map((file) => ({ + name: `/home/runner/_work/T19-Project-2/T19-Project-2/server/${file}`, + status: 'passed', + assertionResults: [{ status: 'passed' }, { status: 'passed' }], + ...replace[file], + })), + }; +} + +function runCli(parsedReport) { + const directory = mkdtempSync(join(tmpdir(), 'assert-postgres-')); + temporaryDirectories.push(directory); + const reportPath = join(directory, 'vitest.json'); + writeFileSync(reportPath, JSON.stringify(parsedReport)); + + const { status, stdout, stderr } = spawnSync(process.execPath, [script, reportPath], { + encoding: 'utf8', + }); + + return { status, output: `${stdout}${stderr}` }; +} + +describe('assert-postgres-tests', () => { + it('passes when every database-backed file ran', () => { + const result = checkReport(report()); + + expect(result.ok).toBe(true); + expect(result.errorLines).toEqual([]); + expect(result.passedLines.at(-1)).toBe( + `All ${databaseBackedFiles.length} database-backed test files executed.`, + ); + }); + + it('fails when a database-backed file is missing from the report', () => { + const [missing] = databaseBackedFiles; + const result = checkReport(report({ omit: [missing] })); + + expect(result.ok).toBe(false); + expect(result.errorLines).toContain(`${missing} did not run at all`); + }); + + it('fails when a file reports passed but no assertion ran', () => { + const [file] = databaseBackedFiles; + const result = checkReport( + report({ replace: { [file]: { assertionResults: [{ status: 'skipped' }] } } }), + ); + + expect(result.ok).toBe(false); + expect(result.errorLines).toContain( + `${file} did not fully run (passed, 0 passed, 1 skipped) - is TEST_DATABASE_URL set?`, + ); + }); + + it('fails when one assertion in an otherwise passing file was skipped', () => { + const [file] = databaseBackedFiles; + const result = checkReport( + report({ + replace: { + [file]: { assertionResults: [{ status: 'passed' }, { status: 'skipped' }] }, + }, + }), + ); + + expect(result.ok).toBe(false); + expect(result.errorLines).toContain( + `${file} did not fully run (passed, 1 passed, 1 skipped) - is TEST_DATABASE_URL set?`, + ); + }); + + it('fails when a file did not pass, even with passing assertions', () => { + const [file] = databaseBackedFiles; + const result = checkReport(report({ replace: { [file]: { status: 'failed' } } })); + + expect(result.ok).toBe(false); + expect(result.errorLines).toContain( + `${file} did not fully run (failed, 2 passed, 0 skipped) - is TEST_DATABASE_URL set?`, + ); + }); + + it('allows a planned todo and reports the count', () => { + const [file] = databaseBackedFiles; + const result = checkReport( + report({ + replace: { + [file]: { assertionResults: [{ status: 'passed' }, { status: 'todo' }] }, + }, + }), + ); + + expect(result.ok).toBe(true); + expect(result.passedLines).toContain(`${file} 1 passed, 1 todo`); + }); + + it('exits non-zero from the CLI when the tests were skipped', () => { + const { status, output } = runCli(report({ omit: [databaseBackedFiles[0]] })); + + expect(status).toBe(1); + expect(output).toContain('::error::The PostgreSQL tests did not run.'); + }); + + it('exits zero from the CLI when every file ran', () => { + const { status, output } = runCli(report()); + + expect(status).toBe(0); + expect(output).toContain( + `${databaseBackedFiles.length} database-backed test files executed.`, + ); + }); +}); From b12ae463e9122e207421b53a55b6ced692e5c794 Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Thu, 5 Nov 2026 19:08:06 -0600 Subject: [PATCH 4/5] ci: #21 stop the cluster even when setup failed Review by rachelks on #72: the cleanup step could not find what it needed. The PostgreSQL binary directory was added to PATH by `export` inside the start step, which does not reach a later step, and PGDATA was written to GITHUB_ENV at the end of that step, so neither existed if initdb or createdb aborted the job. Cleanup then ran with an empty PGDATA and a missing pg_ctl. Both are now resolved in a "Locate PostgreSQL" step that runs before anything can fail: the bin directory goes to GITHUB_PATH, PGDATA to GITHUB_ENV. Cleanup therefore has both whether or not a cluster was ever created. Cleanup also refuses to delete the data directory while the server is still running. `pg_ctl stop || true` used to hide a failed stop and the following `rm -rf` then removed the directory under a live postmaster. It now checks that pg_ctl exists, waits for postmaster.pid to disappear and fails instead of deleting. Verified all three paths by running the step body: PGDATA unset exits 0, a directory with a postmaster.pid and no usable pg_ctl exits 1 and keeps the directory, and a real throwaway cluster stops and is removed. --- .github/workflows/ci.yml | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e36f14a..228005f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -108,7 +108,11 @@ jobs: - name: Install dependencies run: pnpm install --frozen-lockfile - - name: Start a throwaway PostgreSQL cluster + # Resolved once, up front, and exported through the job environment. The + # cleanup step runs with `if: always()` even when the cluster was never + # created, so it must not depend on variables set by the step that starts + # it: a failed initdb would otherwise leave PGDATA unset or empty. + - name: Locate PostgreSQL run: | set -euo pipefail # Debian installs the server binaries under /usr/lib/postgresql//bin @@ -126,10 +130,14 @@ jobs: echo "::error::initdb was not found. Install a PostgreSQL server package on the runner." exit 1 fi - export PATH="${pg_bindir}:${PATH}" + echo "${pg_bindir}" >> "${GITHUB_PATH}" fi + echo "PGDATA=${RUNNER_TEMP}/pgdata" >> "${GITHUB_ENV}" + + - name: Start a throwaway PostgreSQL cluster + run: | + set -euo pipefail initdb --version - export PGDATA="${RUNNER_TEMP}/pgdata" rm -rf "${PGDATA}" initdb -D "${PGDATA}" --auth=trust --no-sync # -k puts the socket in the data directory and `listen_addresses=''` @@ -137,7 +145,6 @@ jobs: pg_ctl -D "${PGDATA}" -l "${PGDATA}/server.log" \ -o "-c listen_addresses='' -k ${PGDATA}" start createdb -h "${PGDATA}" crate_test - echo "PGDATA=${PGDATA}" >> "${GITHUB_ENV}" echo "TEST_DATABASE_URL=postgres:///crate_test?host=${PGDATA}" >> "${GITHUB_ENV}" psql -h "${PGDATA}" -d crate_test -tAc 'select version()' @@ -161,8 +168,29 @@ jobs: - name: Remove the cluster if: always() run: | + set -uo pipefail + # PGDATA and PATH come from "Locate PostgreSQL", so they are set even + # when initdb or createdb aborted the job before a cluster existed. + if [ -z "${PGDATA:-}" ]; then + echo "PGDATA was never set, so there is no cluster to remove." + exit 0 + fi if [ -f "${PGDATA}/postmaster.pid" ]; then - pg_ctl -D "${PGDATA}" -m immediate stop || true + # Refuse to delete a data directory under a live server: that would + # leave the postmaster running against an unlinked directory. + if ! command -v pg_ctl >/dev/null 2>&1; then + echo "::error::pg_ctl is not on PATH; refusing to delete ${PGDATA} while PostgreSQL may be running." + exit 1 + fi + pg_ctl -D "${PGDATA}" -m immediate stop + for _ in $(seq 1 20); do + [ -f "${PGDATA}/postmaster.pid" ] || break + sleep 0.5 + done + if [ -f "${PGDATA}/postmaster.pid" ]; then + echo "::error::PostgreSQL did not stop; refusing to delete ${PGDATA}." + exit 1 + fi fi rm -rf "${PGDATA}" From 528cebb20901c3b716e7e86e4b77a89dd1ee940a Mon Sep 17 00:00:00 2001 From: Erik Hjelm Fjeldheim Date: Thu, 5 Nov 2026 19:08:06 -0600 Subject: [PATCH 5/5] docs: #21 say the test database needs migrations first Review by rachelks on #72: the local instructions pointed TEST_DATABASE_URL at a database but never said the schema has to exist. The suites query tables, they do not create them, so a fresh database fails with "relation does not exist". The snippet now applies the migrations to the same URL first. docs/VM-setup.md also notes that the cleanup step runs after a failure and will not delete a data directory under a running server. --- README.md | 11 +++++++---- docs/VM-setup.md | 6 ++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 9c15d35..367ccca 100644 --- a/README.md +++ b/README.md @@ -89,12 +89,15 @@ real PostgreSQL server and are skipped unless `TEST_DATABASE_URL` is set. CI starts a temporary PostgreSQL cluster for them, applies the migrations, runs the suites and fails if any of them was skipped instead of executed -(`server/scripts/assert-postgres-tests.mjs`). To run them locally, point -`TEST_DATABASE_URL` at a database you do not mind losing - the tests insert and -delete fixture rows: +(`server/scripts/assert-postgres-tests.mjs`). To run them locally, create a +database you do not mind losing - the tests insert and delete fixture rows - and +apply the migrations to it first. The suites expect the schema to exist and do +not create it themselves: ```bash -TEST_DATABASE_URL=postgres://t19:password@127.0.0.1:5432/crate_test pnpm --filter @t19/server test +export TEST_DATABASE_URL=postgres://t19:password@127.0.0.1:5432/crate_test +DATABASE_URL="$TEST_DATABASE_URL" pnpm --filter @t19/server db:migrate +pnpm --filter @t19/server test ``` The [VM setup guide](docs/VM-setup.md) includes API smoke tests. diff --git a/docs/VM-setup.md b/docs/VM-setup.md index 541dc63..9baf979 100644 --- a/docs/VM-setup.md +++ b/docs/VM-setup.md @@ -44,8 +44,10 @@ delete user data. Deployment does not run either destructive seed command. The `integration` job runs the database-backed test suites. It starts a temporary PostgreSQL cluster in the runner's temp directory, listening on a unix socket only, applies the migrations to it, runs the tests and deletes it -afterwards. It never connects to the `crate` database, so the names, fixtures -and cleanup in those tests cannot affect the application data. +afterwards. The cleanup step runs even when setup or a test failed, and refuses +to delete the data directory while the server is still running. It never +connects to the `crate` database, so the names, fixtures and cleanup in those +tests cannot affect the application data. Apache serves the frontend at `/project2/` and forwards `/graphql` requests to the API on port 3001: