test: #21 run the PostgreSQL integration tests in CI

.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/<major>/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

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
 

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:
 

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 <vitest-report.json>
+
+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 <vitest-report.json>');
+    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());
+}