diff --git a/.env.test.example b/.env.test.example index 9c8a776..c0871d5 100644 --- a/.env.test.example +++ b/.env.test.example @@ -1,11 +1,14 @@ -# Environment for the `api` Vitest project. Copy to `.env.test` (gitignored): -# cp .env.test.example .env.test +# Environment for the `api` Vitest project. `scripts/test.sh` copies this to +# `.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 diff --git a/package.json b/package.json index 8358495..df8c7d6 100644 --- a/package.json +++ b/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", diff --git a/playwright.config.ts b/playwright.config.ts index 061ce16..731c9a0 100644 --- a/playwright.config.ts +++ b/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 = { - DATABASE_URL: - process.env.DATABASE_URL ?? - 'postgresql://openframe:openframe@postgres-test:5432/openframe_test?schema=public', + DATABASE_URL, NEXTAUTH_URL: BASE_URL, NEXT_PUBLIC_APP_URL: BASE_URL, @@ -93,6 +122,25 @@ const APP_ENV: Record = { // 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', diff --git a/scripts/test.sh b/scripts/test.sh index fb3bd92..c846357 100755 --- a/scripts/test.sh +++ b/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 - die "$env_test not found." \ - 'Create it once with: cp .env.test.example .env.test' + if [ ! -f "$env_test_example" ]; then + die "Neither $env_test nor $env_test_example exists." \ + 'Both ship with Phase 2 of TESTING.md (section 5).' fi - die "Neither $env_test nor $env_test_example exists." \ - 'Both ship with Phase 2 of TESTING.md (section 5).' + cp "$env_test_example" "$env_test" + 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. diff --git a/tests/helpers/dev-env.ts b/tests/helpers/dev-env.ts new file mode 100644 index 0000000..771aeec --- /dev/null +++ b/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 { + const merged: Record = {}; + + 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]; +} diff --git a/tests/helpers/env.ts b/tests/helpers/env.ts index 798baf4..599c1bf 100644 --- a/tests/helpers/env.ts +++ b/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'; -/** - * 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()); +import { forgetAutoloadedDotenv, REPO_ROOT, TEST_ENV_PATH } from './dev-env'; +import { assertTestDatabase } from './test-database'; - 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'); +export { REPO_ROOT, TEST_ENV_PATH }; 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. diff --git a/tests/helpers/test-database.ts b/tests/helpers/test-database.ts new file mode 100644 index 0000000..13aed6b --- /dev/null +++ b/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' + ); +} diff --git a/tests/setup/component.ts b/tests/setup/component.ts index e6bccd2..d2982e1 100644 --- a/tests/setup/component.ts +++ b/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(); + + 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(), + }); +} diff --git a/scripts/test-db-bootstrap.ts b/tests/setup/db-bootstrap.ts similarity index 70% rename from scripts/test-db-bootstrap.ts rename to tests/setup/db-bootstrap.ts index 3c6b641..e07236b 100644 --- a/scripts/test-db-bootstrap.ts +++ b/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(() => { diff --git a/tests/unit/helpers/test-database.test.ts b/tests/unit/helpers/test-database.test.ts new file mode 100644 index 0000000..2b848bb --- /dev/null +++ b/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/); + }); +});