diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index df76816..228005f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,9 +76,129 @@ 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 + + # 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 + # 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 + 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 + 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 "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: | + 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 + # 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}" + 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..367ccca 100644 --- a/README.md +++ b/README.md @@ -83,8 +83,24 @@ 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, 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 +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. ## Database seed data diff --git a/docs/VM-setup.md b/docs/VM-setup.md index d560b7b..9baf979 100644 --- a/docs/VM-setup.md +++ b/docs/VM-setup.md @@ -41,6 +41,14 @@ 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. 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: diff --git a/server/scripts/assert-postgres-tests.mjs b/server/scripts/assert-postgres-tests.mjs new file mode 100644 index 0000000..667914b --- /dev/null +++ b/server/scripts/assert-postgres-tests.mjs @@ -0,0 +1,125 @@ +// 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". +// +// 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. +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', +]; + +// 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); +} + +/** + * 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; + } + + // 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.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` : ''}`); + } + + if (errorLines.length > 0) { + errorLines.push( + 'The PostgreSQL tests did not run. Point TEST_DATABASE_URL at a migrated test database.', + ); + return { ok: false, passedLines, errorLines }; + } + + passedLines.push( + `All ${databaseBackedFiles.length} database-backed test files executed.`, + ); + return { ok: true, passedLines, errorLines }; +} + +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; +} + +// 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.`, + ); + }); +});