Merge pull request #44 from yusufipk/fix/test-suite-database-safety

fix(test): unbreak the deploy build and keep the suites off a real database

.env.test.example

@@ -1,11 +1,14 @@
-# Environment for the `api` Vitest project. Copy to `.env.test` (gitignored):
+# Environment for the `api` Vitest project. `scripts/test.sh` copies this to
-#   cp .env.test.example .env.test
+# `.env.test` (gitignored) on the first api or e2e run, so there is usually
+# nothing to do by hand.
 #
 # `tests/setup/db-global.ts` and `tests/setup/api.ts` both load this file (via
 # `tests/helpers/env.ts`) before anything imports `@/lib/db`, which reads
 # DATABASE_URL once at module load and memoizes the pool. An already-exported
 # variable always wins over the file, so CI can override DATABASE_URL without
-# editing anything.
+# editing anything. What bun autoloaded from a development `.env` does not count
+# as exported and is dropped first, or that file would quietly win here and
+# point the suites at a real deployment.
 
 # ---------------------------------------------------------------------------
 # DATABASE

package.json

@@ -23,7 +23,7 @@
     "verify": "bun run check && bun run test",
     "test:db:up": "podman compose -f docker-compose.test.yml up -d --wait postgres-test",
     "test:db:down": "podman compose -f docker-compose.test.yml down -v",
-    "test:db:bootstrap": "bun run scripts/test-db-bootstrap.ts",
+    "test:db:bootstrap": "bun run tests/setup/db-bootstrap.ts",
     "prepare": "husky",
     "postinstall": "prisma generate",
     "db:generate": "prisma generate",

playwright.config.ts

@@ -1,5 +1,22 @@
 import { defineConfig, devices } from '@playwright/test';
 
+import {
+  developmentEnvKeys,
+  forgetAutoloadedDotenv,
+  readTestEnvValue,
+} from './tests/helpers/dev-env';
+import { assertTestDatabase } from './tests/helpers/test-database';
+
+// Undo bun's automatic `.env` load before anything below reads process.env.
+// APP_ENV is built while this file is evaluated, so without this the suite
+// would build and start the app against whatever deployment `.env` describes,
+// with that deployment's R2 credentials, and then write test fixtures into it.
+//
+// Note this is not `import './tests/helpers/env'`: that would pull all of
+// `.env.test` in, DISABLE_RATE_LIMIT included, and the production `next build`
+// below refuses to run with that set.
+forgetAutoloadedDotenv();
+
 // ---------------------------------------------------------------------------
 // End-to-end suite. See TESTING.md section 6.
 //
@@ -27,7 +44,7 @@ const MANAGES_OWN_SERVER = !process.env.E2E_BASE_URL;
 /**
  * Environment for the app under test.
  *
- * `.env.test` is deliberately not reused here. Two reasons:
+ * `.env.test` is deliberately not loaded here. Two reasons:
  *
  *  1. `next build` runs with NODE_ENV=production and never loads `.env.test`,
  *     and NEXT_PUBLIC_APP_URL is inlined into the client bundle at build time,
@@ -36,11 +53,23 @@ const MANAGES_OWN_SERVER = !process.env.E2E_BASE_URL;
  *     `hasR2Config()` is derived from them, so putting them in `.env.test`
  *     would flip `isDirectFileUploadEnabled()` to true for 537 API tests that
  *     currently assert the unconfigured branch.
+ *
+ * DATABASE_URL is the exception, read out of `.env.test` on its own below: the
+ * app and the global setup that seeds it must never disagree about which
+ * database is under test.
  */
+const DATABASE_URL =
+  process.env.DATABASE_URL ??
+  readTestEnvValue('DATABASE_URL') ??
+  'postgresql://openframe:openframe@postgres-test:5432/openframe_test?schema=public';
+
+// The e2e suite registers users, uploads videos and deletes projects. Whatever
+// it is pointed at ends up holding test fixtures, so it has to be a test
+// database, and this is the last point before `next build` inherits the value.
+assertTestDatabase(DATABASE_URL);
+
 const APP_ENV: Record<string, string> = {
-  DATABASE_URL:
+  DATABASE_URL,
-    process.env.DATABASE_URL ??
-    'postgresql://openframe:openframe@postgres-test:5432/openframe_test?schema=public',
 
   NEXTAUTH_URL: BASE_URL,
   NEXT_PUBLIC_APP_URL: BASE_URL,
@@ -93,6 +122,25 @@ const APP_ENV: Record<string, string> = {
   // for the api suite, which mocks nodemailer; nothing mocks it here.
 };
 
+// Blank every variable a development env file defines and this config does not.
+//
+// forgetAutoloadedDotenv() above cleared them out of process.env, which is not
+// enough on its own: `next build` and `next start` run @next/env themselves and
+// read `.env` again, filling anything still undefined. So on a developer machine
+// the app under test would come up with that machine's configuration. SMTP_HOST
+// alone turns email verification on and fails the registration spec, and
+// DISABLE_RATE_LIMIT fails the production build outright, from inside
+// lib/rate-limit.ts, reported as an unrelated "Failed to collect page data".
+//
+// An empty value rather than a deletion, because deletion is what @next/env
+// undoes. The result is the environment CI already runs with, where these
+// variables simply do not exist.
+for (const key of developmentEnvKeys()) {
+  if (!(key in APP_ENV)) {
+    APP_ENV[key] = '';
+  }
+}
+
 export default defineConfig({
   testDir: './tests/e2e',
   outputDir: './test-results',

scripts/test.sh

@@ -142,14 +142,27 @@ require_compose_file() {
     'and e2e suites cannot run until that lands.'
 }
 
-require_env_test() {
+# Creates .env.test rather than telling the reader to copy it.
+#
+# The file is gitignored but holds nothing secret: it is the throwaway postgres
+# and minio credentials out of docker-compose.test.yml, written for exactly the
+# containers this script starts. There is no decision for anyone to make.
+#
+# It is also a safety measure. bun loads a plain `.env` into the environment on
+# its own, so with no .env.test the suites inherit whatever DATABASE_URL a
+# developer keeps in .env, which is usually a real deployment. The api setup
+# builds its schema with `prisma db push --accept-data-loss` and truncates every
+# table between tests. tests/helpers/test-database.ts refuses to run against a
+# database that is not named as a test one, and this keeps that refusal from
+# being something anybody has to see.
+ensure_env_test() {
   [ -f "$env_test" ] && return 0
-  if [ -f "$env_test_example" ]; then
+  if [ ! -f "$env_test_example" ]; then
-    die "$env_test not found." \
+    die "Neither $env_test nor $env_test_example exists." \
-      'Create it once with: cp .env.test.example .env.test'
+      'Both ship with Phase 2 of TESTING.md (section 5).'
   fi
-  die "Neither $env_test nor $env_test_example exists." \
+  cp "$env_test_example" "$env_test"
-    'Both ship with Phase 2 of TESTING.md (section 5).'
+  say 'created .env.test from .env.test.example'
 }
 
 require_playwright_config() {
@@ -257,7 +270,7 @@ run_mutation() {
 run_api() {
   say 'api suites'
   require_compose_file
-  require_env_test
+  ensure_env_test
   start_test_db
   run_in_bun_image "$network" "$install_step && bun run test:api"
 }
@@ -266,7 +279,7 @@ run_e2e() {
   say 'end-to-end specs'
   require_compose_file
   require_playwright_config
-  require_env_test
+  ensure_env_test
   start_test_db
   start_test_storage
   # The official Playwright image carries node and the browsers but not bun.

tests/helpers/dev-env.ts

@@ -0,0 +1,120 @@
+// Locating the checkout, and undoing bun's automatic `.env` load.
+//
+// Split out of helpers/env.ts so it can be imported without side effects.
+// Importing that module loads all of `.env.test` into process.env, which
+// playwright.config.ts must not do: the file carries DISABLE_RATE_LIMIT for the
+// api suites, and `next build` runs in production mode, where lib/rate-limit.ts
+// refuses to start with that set.
+
+import fs from 'node:fs';
+import path from 'node:path';
+import { parse as parseDotenv } from 'dotenv';
+
+/**
+ * Walks up from the working directory until it finds the checkout.
+ *
+ * This deliberately avoids `import.meta.url`, which would be the obvious way to
+ * resolve a path relative to this file: Playwright transpiles TypeScript to
+ * CommonJS unless package.json declares `"type": "module"`, and in CommonJS
+ * `import.meta` is a *syntax* error, so the e2e suite could not import this
+ * module at all. `__dirname` has the mirror-image problem under Vitest's ESM.
+ *
+ * The marker is prisma/schema.prisma as well as package.json, so a stray
+ * package.json inside node_modules cannot be mistaken for the checkout.
+ */
+function findRepoRoot(): string {
+  let current = path.resolve(process.cwd());
+
+  for (;;) {
+    if (
+      fs.existsSync(path.join(current, 'package.json')) &&
+      fs.existsSync(path.join(current, 'prisma', 'schema.prisma'))
+    ) {
+      return current;
+    }
+
+    const parent = path.dirname(current);
+    if (parent === current) {
+      throw new Error(
+        `Could not locate the OpenFrame checkout from ${process.cwd()}: no ancestor ` +
+          'directory holds both package.json and prisma/schema.prisma. Run the test ' +
+          'suites from the repository root.'
+      );
+    }
+    current = parent;
+  }
+}
+
+export const REPO_ROOT = findRepoRoot();
+
+export const TEST_ENV_PATH = path.join(REPO_ROOT, '.env.test');
+
+/**
+ * The env files a developer machine has and CI does not.
+ *
+ * bun autoloads these, and so does @next/env inside `next build` and
+ * `next start`, which is why both halves of the problem below need the same
+ * list. `.env.test` is deliberately absent: that one is the test configuration.
+ */
+const DEV_ENV_FILES = ['.env', '.env.local', '.env.production', '.env.production.local'];
+
+function readDevEnv(): Record<string, string> {
+  const merged: Record<string, string> = {};
+
+  for (const file of DEV_ENV_FILES) {
+    const filePath = path.join(REPO_ROOT, file);
+    if (!fs.existsSync(filePath)) continue;
+    Object.assign(merged, parseDotenv(fs.readFileSync(filePath)));
+  }
+
+  return merged;
+}
+
+/**
+ * Removes the values bun copied out of a plain `.env` on start-up.
+ *
+ * bun reads `.env` into process.env before the first line of a script runs, and
+ * nothing downstream can tell that apart from `DATABASE_URL=… bun run test:api`.
+ * Under the "an already-exported variable wins" contract the development `.env`
+ * therefore beat `.env.test` outright, which pointed the api suites at whatever
+ * deployment `.env` describes: `prisma db push --accept-data-loss` for the
+ * schema, then a truncate of every table between tests. The same values reached
+ * the app the e2e suite builds, R2 credentials included.
+ *
+ * Only entries whose current value is character-for-character what `.env` holds
+ * are dropped, so a real export still wins, which is what the per-suite
+ * databases of a parallel api run rely on. CI has no `.env`, so this is a no-op
+ * there.
+ */
+export function forgetAutoloadedDotenv(): void {
+  for (const [key, value] of Object.entries(readDevEnv())) {
+    if (process.env[key] === value) {
+      delete process.env[key];
+    }
+  }
+}
+
+/**
+ * Every variable a development env file defines.
+ *
+ * Deleting them from process.env only gets you half way for the e2e suite:
+ * `next build` and `next start` run @next/env themselves, which reads the same
+ * files again and fills in whatever is undefined. playwright.config.ts uses this
+ * list to blank the ones it does not set, which is the state CI is already in.
+ */
+export function developmentEnvKeys(): string[] {
+  return Object.keys(readDevEnv());
+}
+
+/**
+ * Reads a single variable out of `.env.test` without loading the rest of it.
+ *
+ * For playwright.config.ts, which needs DATABASE_URL to agree with the suite
+ * that seeds the database but must keep the rest of that file away from a
+ * production `next build`.
+ */
+export function readTestEnvValue(key: string): string | undefined {
+  if (!fs.existsSync(TEST_ENV_PATH)) return undefined;
+
+  return parseDotenv(fs.readFileSync(TEST_ENV_PATH))[key];
+}

tests/helpers/env.ts

@@ -13,49 +13,18 @@
 // Contract: an already-exported variable always wins. `.env.test` fills the
 // gaps. That is what lets CI export DATABASE_URL for a service container
 // without needing a `.env.test` file at all.
+//
+// With one correction, see forgetAutoloadedDotenv in helpers/dev-env.ts: bun
+// populates process.env from a plain `.env` before any of this runs, which the
+// contract above would otherwise read as a deliberate export.
 
 import fs from 'node:fs';
-import path from 'node:path';
 import { config as loadDotenv } from 'dotenv';
 
-/**
+import { forgetAutoloadedDotenv, REPO_ROOT, TEST_ENV_PATH } from './dev-env';
- * Walks up from the working directory until it finds the checkout.
+import { assertTestDatabase } from './test-database';
- *
- * This deliberately avoids `import.meta.url`, which would be the obvious way to
- * resolve a path relative to this file: Playwright transpiles TypeScript to
- * CommonJS unless package.json declares `"type": "module"`, and in CommonJS
- * `import.meta` is a *syntax* error, so the e2e suite could not import this
- * module at all. `__dirname` has the mirror-image problem under Vitest's ESM.
- *
- * The marker is prisma/schema.prisma as well as package.json, so a stray
- * package.json inside node_modules cannot be mistaken for the checkout.
- */
-function findRepoRoot(): string {
-  let current = path.resolve(process.cwd());
 
-  for (;;) {
+export { REPO_ROOT, TEST_ENV_PATH };
-    if (
-      fs.existsSync(path.join(current, 'package.json')) &&
-      fs.existsSync(path.join(current, 'prisma', 'schema.prisma'))
-    ) {
-      return current;
-    }
-
-    const parent = path.dirname(current);
-    if (parent === current) {
-      throw new Error(
-        `Could not locate the OpenFrame checkout from ${process.cwd()}: no ancestor ` +
-          'directory holds both package.json and prisma/schema.prisma. Run the test ' +
-          'suites from the repository root.'
-      );
-    }
-    current = parent;
-  }
-}
-
-export const REPO_ROOT = findRepoRoot();
-
-export const TEST_ENV_PATH = path.join(REPO_ROOT, '.env.test');
 
 let loaded = false;
 
@@ -63,6 +32,8 @@ export function loadTestEnv(): void {
   if (loaded) return;
   loaded = true;
 
+  forgetAutoloadedDotenv();
+
   if (fs.existsSync(TEST_ENV_PATH)) {
     loadDotenv({ path: TEST_ENV_PATH, quiet: true });
   }
@@ -75,6 +46,10 @@ export function loadTestEnv(): void {
     );
   }
 
+  // Deliberately after the file load and before anything opens a pool: this is
+  // the one place every path into the test setup goes through.
+  assertTestDatabase(process.env.DATABASE_URL);
+
   // Vitest sets this already, but db-global.ts also spawns the Prisma CLI and
   // lib/rate-limit.ts throws when DISABLE_RATE_LIMIT is set in production.
   // @types/node declares NODE_ENV as read-only, hence the cast.

tests/helpers/test-database.ts

@@ -0,0 +1,58 @@
+// Guard that keeps the test suites off a real database.
+//
+// This is deliberately a separate, side-effect-free module rather than part of
+// helpers/env.ts: importing that one loads .env.test and throws when
+// DATABASE_URL is missing, which a unit test cannot exercise.
+
+/**
+ * A database name that identifies a disposable test database.
+ *
+ * `test` has to be its own `_`/`-` delimited segment, so `openframe_test` and
+ * `openframe_test_api` (the per-suite databases the parallel api runs use) are
+ * accepted while `openframe` is not.
+ */
+const TEST_DATABASE_NAME = /(^|[_-])test([_-]|$)/i;
+
+/**
+ * Throws unless `url` names a test database.
+ *
+ * The reason this exists: bun loads a plain `.env` into `process.env` on its
+ * own, so anything that reaches the test setup outside Vitest, such as
+ * `bun run test:db:bootstrap`, inherits the DATABASE_URL of whatever deployment
+ * `.env` happens to describe when `.env.test` is absent. tests/setup/db-global.ts
+ * then builds the schema with `prisma db push --accept-data-loss`, and the api
+ * suites truncate every table between tests. Neither is something you want
+ * pointed at a database holding real rows, and the failure is silent: the
+ * bootstrap prints its usual success line either way.
+ *
+ * CI is unaffected because it exports DATABASE_URL for a service container
+ * named openframe_test.
+ */
+export function assertTestDatabase(url: string): void {
+  let parsed: URL;
+
+  try {
+    parsed = new URL(url);
+  } catch {
+    throw new Error(
+      'DATABASE_URL is not a valid connection string, so there is no way to ' +
+        'tell whether it points at a test database. Refusing to continue.'
+    );
+  }
+
+  const name = decodeURIComponent(parsed.pathname).replace(/^\//, '');
+
+  if (TEST_DATABASE_NAME.test(name)) return;
+
+  throw new Error(
+    `Refusing to run the test setup against database "${name}" on ` +
+      `${parsed.hostname}: the name does not mark it as a test database.\n\n` +
+      'The setup builds the schema with `prisma db push --accept-data-loss` ' +
+      'and the api suites truncate every table, so this would destroy real ' +
+      'data.\n\n' +
+      'A test database is one whose name carries a `test` segment, for ' +
+      'example openframe_test or openframe_test_api.\n\n' +
+      'The usual cause is a missing .env.test, which leaves DATABASE_URL to be ' +
+      'inherited from .env: cp .env.test.example .env.test'
+  );
+}

tests/setup/component.ts

@@ -93,3 +93,51 @@ Object.defineProperty(navigator, 'sendBeacon', {
   writable: true,
   value: () => true,
 });
+
+/**
+ * `localStorage` goes missing on Node 24 and newer, and only `localStorage`.
+ *
+ * Node ships its own experimental Web Storage global now, which evaluates to
+ * `undefined` unless the process was started with `--localstorage-file`. Vitest
+ * leaves an already-present global alone when it copies jsdom's window onto
+ * globalThis, so jsdom's implementation never lands and Node's empty one wins.
+ * `sessionStorage` has no counterpart in Node and comes through untouched,
+ * which is what makes the asymmetry visible.
+ *
+ * CI pins Node 22 and never sees this; a developer on a current Node does, as
+ * every test in guest-gate.test.tsx failing on `localStorage.clear()`. The
+ * guard means that when jsdom's own implementation is the one in scope, this
+ * leaves it alone.
+ */
+function createMemoryStorage(): Storage {
+  const entries = new Map<string, string>();
+
+  return {
+    get length() {
+      return entries.size;
+    },
+    key(index: number) {
+      return Array.from(entries.keys())[index] ?? null;
+    },
+    getItem(key: string) {
+      return entries.get(String(key)) ?? null;
+    },
+    setItem(key: string, value: string) {
+      entries.set(String(key), String(value));
+    },
+    removeItem(key: string) {
+      entries.delete(String(key));
+    },
+    clear() {
+      entries.clear();
+    },
+  };
+}
+
+if (typeof localStorage === 'undefined') {
+  Object.defineProperty(globalThis, 'localStorage', {
+    configurable: true,
+    writable: true,
+    value: createMemoryStorage(),
+  });
+}

tests/setup/db-bootstrap.ts

@@ -6,11 +6,15 @@
  * schema in place before the server starts. Both paths therefore call the same
  * setup function, so there is exactly one description of how a test database is
  * built (including why it uses `prisma db push` rather than `migrate deploy`,
- * which is documented at the top of tests/setup/db-global.ts).
+ * which is documented at the top of db-global.ts).
+ *
+ * This lives under tests/ rather than scripts/ because the production image
+ * ignores tests/ entirely, and a script that imports from it would break the
+ * typecheck that runs before every build.
  *
  * Usage: bun run test:db:bootstrap
  */
-import { setup } from '../tests/setup/db-global';
+import { setup } from './db-global';
 
 setup()
   .then(() => {

tests/unit/helpers/test-database.test.ts

@@ -0,0 +1,57 @@
+import { describe, expect, it } from 'vitest';
+
+import { assertTestDatabase } from '../../helpers/test-database';
+
+const CREDENTIALS = 'openframe:hunter2';
+
+function url(database: string, host = 'localhost:5432'): string {
+  return `postgresql://${CREDENTIALS}@${host}/${database}?schema=public`;
+}
+
+describe('assertTestDatabase', () => {
+  it.each([
+    ['the name the compose file and CI both use', 'openframe_test'],
+    ['a per-suite database from a parallel api run', 'openframe_test_api'],
+    ['a dash instead of an underscore', 'openframe-test'],
+    ['a leading test segment', 'test_openframe'],
+    ['nothing but the word itself', 'test'],
+    ['an upper-case spelling', 'openframe_TEST'],
+  ])('accepts %s', (_label, database) => {
+    expect(() => assertTestDatabase(url(database))).not.toThrow();
+  });
+
+  it.each([
+    ['the production database', 'openframe'],
+    ['a name that merely starts with the letters', 'testimonials'],
+    ['a name that merely ends with them', 'latest'],
+    ['an empty database name', ''],
+  ])('rejects %s', (_label, database) => {
+    expect(() => assertTestDatabase(url(database))).toThrow(/Refusing to run the test setup/);
+  });
+
+  it('rejects a remote host just the same when the name is not a test one', () => {
+    expect(() => assertTestDatabase(url('openframe', '157.90.147.190:3799'))).toThrow(
+      /database "openframe" on 157\.90\.147\.190/
+    );
+  });
+
+  it('points at the missing .env.test, which is what actually causes this', () => {
+    expect(() => assertTestDatabase(url('openframe'))).toThrow(
+      /cp \.env\.test\.example \.env\.test/
+    );
+  });
+
+  it('keeps the password out of the message, which ends up in logs', () => {
+    expect(() => assertTestDatabase(url('openframe'))).toThrow(
+      expect.objectContaining({ message: expect.not.stringContaining('hunter2') })
+    );
+  });
+
+  it('decodes a percent-encoded database name before judging it', () => {
+    expect(() => assertTestDatabase(url('openframe%5Ftest'))).not.toThrow();
+  });
+
+  it('refuses a connection string it cannot parse rather than assuming the best', () => {
+    expect(() => assertTestDatabase('not-a-url')).toThrow(/not a valid connection string/);
+  });
+});