diff --git a/AGENTS.md b/AGENTS.md index 0a8a2013..e73fba2b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -383,6 +383,37 @@ Minimal comments, maximum clarity. Comments explain **intent and reasoning**, no | Unit tests | `*.test.ts` | `test/` (mirrors `src/`) | | E2E tests | `*.test.ts` | `test/e2e/` | +### Test Environment Isolation (CRITICAL) + +Tests that need a database or config directory **must** use `useTestConfigDir()` from `test/helpers.ts`. This helper: +- Creates a unique temp directory in `beforeEach` +- Sets `SENTRY_CONFIG_DIR` to point at it +- **Restores** (never deletes) the env var in `afterEach` +- Closes the database and cleans up temp files + +**NEVER** do any of these in test files: +- `delete process.env.SENTRY_CONFIG_DIR` — This pollutes other test files that load after yours +- `const baseDir = process.env[CONFIG_DIR_ENV_VAR]!` at module scope — This captures a value that may be stale +- Manual `beforeEach`/`afterEach` that sets/deletes `SENTRY_CONFIG_DIR` + +**Why**: Bun runs test files **sequentially in one thread** (load → run all tests → load next file). If your `afterEach` deletes the env var, the next file's module-level code reads `undefined`, causing `TypeError: The "paths[0]" property must be of type string`. + +```typescript +// CORRECT: Use the helper +import { useTestConfigDir } from "../helpers.js"; + +const getConfigDir = useTestConfigDir("my-test-prefix-"); + +// If you need the directory path in a test: +test("example", () => { + const dir = getConfigDir(); +}); + +// WRONG: Manual env var management +beforeEach(() => { process.env.SENTRY_CONFIG_DIR = tmpDir; }); +afterEach(() => { delete process.env.SENTRY_CONFIG_DIR; }); // BUG! +``` + ### Property-Based Testing Use property-based tests when verifying invariants that should hold for **any valid input**. diff --git a/package.json b/package.json index 4f2ddebd..51953b33 100644 --- a/package.json +++ b/package.json @@ -59,7 +59,7 @@ "engines": { "node": ">=22" }, - "packageManager": "bun@1.3.3", + "packageManager": "bun@1.3.9", "patchedDependencies": { "@stricli/core@1.2.5": "patches/@stricli%2Fcore@1.2.5.patch", "@sentry/core@10.38.0": "patches/@sentry%2Fcore@10.38.0.patch" diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index 5ac32ed3..1ecf0087 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -24,12 +24,20 @@ const SENTRY_URL = process.env.SENTRY_URL ?? "https://sentry.io"; * Build-time: Injected via Bun.build({ define: { SENTRY_CLIENT_ID: "..." } }) * Runtime: Can be overridden via SENTRY_CLIENT_ID env var (for self-hosted) * + * Read at call time (not module load time) so tests can set process.env.SENTRY_CLIENT_ID + * after module initialization. + * * @see script/build.ts */ declare const SENTRY_CLIENT_ID_BUILD: string | undefined; -const SENTRY_CLIENT_ID = - process.env.SENTRY_CLIENT_ID ?? - (typeof SENTRY_CLIENT_ID_BUILD !== "undefined" ? SENTRY_CLIENT_ID_BUILD : ""); +function getClientId(): string { + return ( + process.env.SENTRY_CLIENT_ID ?? + (typeof SENTRY_CLIENT_ID_BUILD !== "undefined" + ? SENTRY_CLIENT_ID_BUILD + : "") + ); +} // OAuth scopes requested for the CLI const SCOPES = [ @@ -85,7 +93,8 @@ async function fetchWithConnectionError( /** Request a device code from Sentry's device authorization endpoint */ function requestDeviceCode() { - if (!SENTRY_CLIENT_ID) { + const clientId = getClientId(); + if (!clientId) { throw new ConfigError( "SENTRY_CLIENT_ID is required for authentication", "Set SENTRY_CLIENT_ID environment variable or use a pre-built binary" @@ -99,7 +108,7 @@ function requestDeviceCode() { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: new URLSearchParams({ - client_id: SENTRY_CLIENT_ID, + client_id: clientId, scope: SCOPES, }), } @@ -142,7 +151,7 @@ function pollForToken(deviceCode: string): Promise { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: new URLSearchParams({ - client_id: SENTRY_CLIENT_ID, + client_id: getClientId(), device_code: deviceCode, grant_type: "urn:ietf:params:oauth:grant-type:device_code", }), @@ -313,7 +322,8 @@ export async function setApiToken(token: string): Promise { export function refreshAccessToken( refreshToken: string ): Promise { - if (!SENTRY_CLIENT_ID) { + const clientId = getClientId(); + if (!clientId) { throw new ConfigError( "SENTRY_CLIENT_ID is required for token refresh", "Set SENTRY_CLIENT_ID environment variable or use a pre-built binary" @@ -327,7 +337,7 @@ export function refreshAccessToken( method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: new URLSearchParams({ - client_id: SENTRY_CLIENT_ID, + client_id: clientId, grant_type: "refresh_token", refresh_token: refreshToken, }), diff --git a/test/commands/cli/fix.test.ts b/test/commands/cli/fix.test.ts index 82364b73..029949fd 100644 --- a/test/commands/cli/fix.test.ts +++ b/test/commands/cli/fix.test.ts @@ -3,19 +3,16 @@ */ import { Database } from "bun:sqlite"; -import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; -import { mkdirSync, rmSync } from "node:fs"; +import { describe, expect, mock, test } from "bun:test"; import { join } from "node:path"; import { fixCommand } from "../../../src/commands/cli/fix.js"; -import { - CONFIG_DIR_ENV_VAR, - closeDatabase, -} from "../../../src/lib/db/index.js"; +import { closeDatabase } from "../../../src/lib/db/index.js"; import { EXPECTED_TABLES, generatePreMigrationTableDDL, initSchema, } from "../../../src/lib/db/schema.js"; +import { useTestConfigDir } from "../../helpers.js"; /** * Generate DDL for creating a database with pre-migration tables. @@ -64,46 +61,12 @@ function createDatabaseWithMissingTables( ).run(); } -let testDir: string; -let originalConfigDir: string | undefined; - -beforeEach(() => { - // Save original config dir - originalConfigDir = process.env[CONFIG_DIR_ENV_VAR]; - - // Close any existing database connection - closeDatabase(); - - // Create unique test directory - const baseDir = originalConfigDir ?? "/tmp/sentry-cli-test"; - testDir = join( - baseDir, - `fix-test-${Date.now()}-${Math.random().toString(36).slice(2)}` - ); - mkdirSync(testDir, { recursive: true }); - process.env[CONFIG_DIR_ENV_VAR] = testDir; -}); - -afterEach(() => { - closeDatabase(); - // Restore original config dir - if (originalConfigDir) { - process.env[CONFIG_DIR_ENV_VAR] = originalConfigDir; - } else { - delete process.env[CONFIG_DIR_ENV_VAR]; - } - // Clean up test directory - try { - rmSync(testDir, { recursive: true, force: true }); - } catch { - // Ignore cleanup errors - } -}); +const getTestDir = useTestConfigDir("fix-test-"); describe("sentry cli fix", () => { test("reports no issues for healthy database", async () => { // Create healthy database - const db = new Database(join(testDir, "cli.db")); + const db = new Database(join(getTestDir(), "cli.db")); initSchema(db); db.close(); @@ -124,7 +87,7 @@ describe("sentry cli fix", () => { test("detects and reports missing columns in dry-run mode", async () => { // Create database with pre-migration tables (missing v4 columns) - const db = new Database(join(testDir, "cli.db")); + const db = new Database(join(getTestDir(), "cli.db")); createPreMigrationDatabase(db); db.close(); @@ -148,7 +111,7 @@ describe("sentry cli fix", () => { test("fixes missing columns when not in dry-run mode", async () => { // Create database with pre-migration tables (missing v4 columns) - const db = new Database(join(testDir, "cli.db")); + const db = new Database(join(getTestDir(), "cli.db")); createPreMigrationDatabase(db); db.close(); @@ -169,7 +132,7 @@ describe("sentry cli fix", () => { // Verify the column was actually added closeDatabase(); - const verifyDb = new Database(join(testDir, "cli.db")); + const verifyDb = new Database(join(getTestDir(), "cli.db")); const cols = verifyDb.query("PRAGMA table_info(dsn_cache)").all() as Array<{ name: string; }>; @@ -188,7 +151,7 @@ describe("sentry cli fix", () => { // that was previously missing tables (now fixed by auto-repair at startup). test("handles database that was auto-repaired at startup", async () => { // Create database missing dsn_cache - initSchema will create it when command runs - const db = new Database(join(testDir, "cli.db")); + const db = new Database(join(getTestDir(), "cli.db")); createDatabaseWithMissingTables(db, ["dsn_cache"]); db.close(); @@ -210,7 +173,7 @@ describe("sentry cli fix", () => { // Verify the table was created (by initSchema auto-repair) closeDatabase(); - const verifyDb = new Database(join(testDir, "cli.db")); + const verifyDb = new Database(join(getTestDir(), "cli.db")); const tables = verifyDb .query( "SELECT name FROM sqlite_master WHERE type='table' AND name='dsn_cache'" @@ -221,7 +184,7 @@ describe("sentry cli fix", () => { }); test("shows database path in output", async () => { - const db = new Database(join(testDir, "cli.db")); + const db = new Database(join(getTestDir(), "cli.db")); initSchema(db); db.close(); @@ -237,6 +200,6 @@ describe("sentry cli fix", () => { const output = stdoutWrite.mock.calls.map((c) => c[0]).join(""); expect(output).toContain("Database:"); - expect(output).toContain(testDir); + expect(output).toContain(getTestDir()); }); }); diff --git a/test/commands/issue/utils.test.ts b/test/commands/issue/utils.test.ts index ad3ac173..3d0c7ac3 100644 --- a/test/commands/issue/utils.test.ts +++ b/test/commands/issue/utils.test.ts @@ -13,9 +13,8 @@ import { } from "../../../src/commands/issue/utils.js"; import { DEFAULT_SENTRY_URL } from "../../../src/lib/constants.js"; import { setAuthToken } from "../../../src/lib/db/auth.js"; -import { CONFIG_DIR_ENV_VAR } from "../../../src/lib/db/index.js"; import { setOrgRegion } from "../../../src/lib/db/regions.js"; -import { cleanupTestDir, createTestConfigDir } from "../../helpers.js"; +import { useTestConfigDir } from "../../helpers.js"; describe("buildCommandHint", () => { test("suggests /ID for numeric IDs", () => { @@ -47,15 +46,13 @@ describe("buildCommandHint", () => { }); }); -let testConfigDir: string; +const getConfigDir = useTestConfigDir("test-issue-utils-", { + isolateProjectRoot: true, +}); + let originalFetch: typeof globalThis.fetch; beforeEach(async () => { - // Use isolateProjectRoot to prevent DSN detection from scanning the real project - testConfigDir = await createTestConfigDir("test-issue-utils-", { - isolateProjectRoot: true, - }); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; originalFetch = globalThis.fetch; await setAuthToken("test-token"); // Pre-populate region cache for orgs used in tests to avoid region resolution API calls @@ -65,9 +62,8 @@ beforeEach(async () => { await setOrgRegion("org1", DEFAULT_SENTRY_URL); }); -afterEach(async () => { +afterEach(() => { globalThis.fetch = originalFetch; - await cleanupTestDir(testConfigDir); }); describe("resolveOrgAndIssueId", () => { @@ -106,7 +102,7 @@ describe("resolveOrgAndIssueId", () => { await expect( resolveOrgAndIssueId({ issueArg: "123456789", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }) ).rejects.toThrow("Organization"); @@ -144,7 +140,7 @@ describe("resolveOrgAndIssueId", () => { const result = await resolveOrgAndIssueId({ issueArg: "my-org/PROJECT-ABC", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }); @@ -196,7 +192,7 @@ describe("resolveOrgAndIssueId", () => { const result = await resolveOrgAndIssueId({ issueArg: "f-g", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }); @@ -237,7 +233,7 @@ describe("resolveOrgAndIssueId", () => { const result = await resolveOrgAndIssueId({ issueArg: "org1/dashboard-4y", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }); @@ -280,7 +276,7 @@ describe("resolveOrgAndIssueId", () => { const result = await resolveOrgAndIssueId({ issueArg: "G", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }); @@ -298,7 +294,7 @@ describe("resolveOrgAndIssueId", () => { await expect( resolveOrgAndIssueId({ issueArg: "G", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }) ).rejects.toThrow("Cannot resolve issue suffix"); @@ -315,6 +311,14 @@ describe("resolveOrgAndIssueId", () => { const req = new Request(input, init); const url = req.url; + // getUserRegions - return empty regions to use fallback path + if (url.includes("/users/me/regions/")) { + return new Response(JSON.stringify({ regions: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + // listOrganizations call if ( url.includes("/organizations/") && @@ -375,7 +379,7 @@ describe("resolveOrgAndIssueId", () => { const result = await resolveOrgAndIssueId({ issueArg: "craft-g", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }); @@ -394,6 +398,14 @@ describe("resolveOrgAndIssueId", () => { const req = new Request(input, init); const url = req.url; + // getUserRegions - return empty regions to use fallback path + if (url.includes("/users/me/regions/")) { + return new Response(JSON.stringify({ regions: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + // listOrganizations call if ( url.includes("/organizations/") && @@ -435,7 +447,7 @@ describe("resolveOrgAndIssueId", () => { await expect( resolveOrgAndIssueId({ issueArg: "nonexistent-g", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }) ).rejects.toThrow("not found"); @@ -454,6 +466,14 @@ describe("resolveOrgAndIssueId", () => { const req = new Request(input, init); const url = req.url; + // getUserRegions - return empty regions to use fallback path + if (url.includes("/users/me/regions/")) { + return new Response(JSON.stringify({ regions: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + // listOrganizations call if ( url.includes("/organizations/") && @@ -511,7 +531,7 @@ describe("resolveOrgAndIssueId", () => { await expect( resolveOrgAndIssueId({ issueArg: "common-g", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }) ).rejects.toThrow("multiple organizations"); @@ -531,7 +551,7 @@ describe("resolveOrgAndIssueId", () => { await expect( resolveOrgAndIssueId({ issueArg: "G", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }) ).rejects.toThrow(); @@ -551,7 +571,7 @@ describe("resolveOrgAndIssueId", () => { await expect( resolveOrgAndIssueId({ issueArg: "G", - cwd: testConfigDir, + cwd: getConfigDir(), command: "explain", }) ).rejects.toThrow("500"); diff --git a/test/helpers.ts b/test/helpers.ts index c2f27312..98a1f37c 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -4,9 +4,11 @@ * Shared utilities for test setup and teardown. */ +import { afterEach, beforeEach } from "bun:test"; import { mkdirSync } from "node:fs"; import { mkdtemp, rm } from "node:fs/promises"; import { join, resolve } from "node:path"; +import { CONFIG_DIR_ENV_VAR, closeDatabase } from "../src/lib/db/index.js"; const TEST_TMP_DIR = resolve(import.meta.dir, "../.test-tmp"); mkdirSync(TEST_TMP_DIR, { recursive: true }); @@ -73,3 +75,47 @@ type FetchMockFn = export function mockFetch(fn: FetchMockFn): typeof fetch { return fn as unknown as typeof fetch; } + +/** + * Sets up an isolated test config directory with proper env var lifecycle. + * + * Registers beforeEach/afterEach hooks that create a unique config directory, + * point SENTRY_CONFIG_DIR at it, and restore the original value on teardown. + * This eliminates the fragile pattern of manually managing process.env in + * each test file, which caused cross-file pollution when afterEach hooks + * deleted the env var while other files were still loading. + * + * Must be called at module scope or inside a describe() block. + * + * @param prefix - Directory name prefix for the temp directory + * @param options - Configuration options (e.g., isolateProjectRoot) + * @returns Getter function for the current test's config directory path + */ +export function useTestConfigDir( + prefix = "sentry-test-", + options?: TestConfigDirOptions +): () => string { + let dir: string; + let savedConfigDir: string | undefined; + + beforeEach(async () => { + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; + closeDatabase(); + dir = await createTestConfigDir(prefix, options); + process.env[CONFIG_DIR_ENV_VAR] = dir; + }); + + afterEach(async () => { + closeDatabase(); + // Always restore the previous value — never delete. + // Deleting process.env.SENTRY_CONFIG_DIR causes failures in test files + // that load after this afterEach runs, because their module-level code + // (or beforeEach hooks) may read the env var and get undefined. + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } + await cleanupTestDir(dir); + }); + + return () => dir; +} diff --git a/test/lib/dsn/errors.test.ts b/test/isolated/dsn/errors.test.ts similarity index 100% rename from test/lib/dsn/errors.test.ts rename to test/isolated/dsn/errors.test.ts diff --git a/test/lib/dsn/resolver.test.ts b/test/isolated/dsn/resolver.test.ts similarity index 100% rename from test/lib/dsn/resolver.test.ts rename to test/isolated/dsn/resolver.test.ts diff --git a/test/lib/api-client.test.ts b/test/lib/api-client.test.ts index 18838a09..15ec7d57 100644 --- a/test/lib/api-client.test.ts +++ b/test/lib/api-client.test.ts @@ -8,11 +8,10 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { buildSearchParams, rawApiRequest } from "../../src/lib/api-client.js"; import { setAuthToken } from "../../src/lib/db/auth.js"; -import { CONFIG_DIR_ENV_VAR } from "../../src/lib/db/index.js"; -import { cleanupTestDir, createTestConfigDir } from "../helpers.js"; +import { useTestConfigDir } from "../helpers.js"; + +useTestConfigDir("test-api-"); -// Test config directory -let testConfigDir: string; let originalFetch: typeof globalThis.fetch; /** @@ -26,9 +25,6 @@ type RequestLog = { }; beforeEach(async () => { - testConfigDir = await createTestConfigDir("test-api-"); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; - // Set required env var for OAuth refresh process.env.SENTRY_CLIENT_ID = "test-client-id"; @@ -39,11 +35,9 @@ beforeEach(async () => { await setAuthToken("initial-token", 3600, "test-refresh-token"); }); -afterEach(async () => { +afterEach(() => { // Restore original fetch globalThis.fetch = originalFetch; - - await cleanupTestDir(testConfigDir); }); /** diff --git a/test/lib/config.test.ts b/test/lib/config.test.ts index e2b747f5..fed2c62e 100644 --- a/test/lib/config.test.ts +++ b/test/lib/config.test.ts @@ -4,8 +4,8 @@ * Integration tests for SQLite-based config storage. */ -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdirSync, writeFileSync } from "node:fs"; +import { describe, expect, test } from "bun:test"; +import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { clearAuth, @@ -38,37 +38,14 @@ import { setCachedProject, setCachedProjectByDsnKey, } from "../../src/lib/db/project-cache.js"; +import { useTestConfigDir } from "../helpers.js"; /** - * Test isolation: Each test gets its own config directory within - * the main test directory created by preload.ts. - * - * We don't delete test subdirectories in afterEach because tests - * run in parallel and the deletion can race with other tests that - * are still running. The parent test directory is cleaned up on - * process exit by preload.ts. + * Test isolation: Each test gets its own config directory via useTestConfigDir(). + * The helper creates a unique temp directory in beforeEach and restores + * the env var in afterEach (never deleting it). */ -const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; - -beforeEach(() => { - // Close any previous database connection - closeDatabase(); - - // Create a unique subdirectory for this test - const testConfigDir = join( - testBaseDir, - `test-${Math.random().toString(36).slice(2)}` - ); - mkdirSync(testConfigDir, { recursive: true }); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; -}); - -afterEach(() => { - // Close database to release file handles - closeDatabase(); - // Note: We don't delete the test directory here because tests run in parallel. - // The parent test directory is cleaned up on process exit by preload.ts. -}); +const getConfigDir = useTestConfigDir("test-config-"); describe("auth token management", () => { test("setAuthToken stores token", async () => { @@ -565,7 +542,7 @@ describe("getDbPath", () => { test("returns the database file path", () => { const path = getDbPath(); expect(path).toContain("cli.db"); - expect(path).toContain(testBaseDir); + expect(path).toContain(getConfigDir()); }); }); diff --git a/test/lib/db/concurrent.test.ts b/test/lib/db/concurrent.test.ts index 45b8dabc..a522fff6 100644 --- a/test/lib/db/concurrent.test.ts +++ b/test/lib/db/concurrent.test.ts @@ -8,8 +8,7 @@ * CLI usage (e.g., multiple terminals, CI jobs, editor integrations). */ -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdirSync } from "node:fs"; +import { beforeEach, describe, expect, test } from "bun:test"; import { join } from "node:path"; import { getCachedDsn } from "../../../src/lib/db/dsn-cache.js"; import { @@ -17,9 +16,9 @@ import { closeDatabase, } from "../../../src/lib/db/index.js"; import { getCachedProject } from "../../../src/lib/db/project-cache.js"; +import { useTestConfigDir } from "../../helpers.js"; const WORKER_SCRIPT = join(import.meta.dir, "concurrent-worker.ts"); -const TEST_BASE_DIR = process.env[CONFIG_DIR_ENV_VAR]!; type WorkerResult = { workerId: string; @@ -88,17 +87,9 @@ async function spawnWorkersConcurrently( } describe("concurrent database access", () => { - let testConfigDir: string; + const getConfigDir = useTestConfigDir("concurrent-"); beforeEach(async () => { - closeDatabase(); - testConfigDir = join( - TEST_BASE_DIR, - `concurrent-${Date.now()}-${Math.random().toString(36).slice(2)}` - ); - mkdirSync(testConfigDir, { recursive: true }); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; - // Pre-create the database before spawning workers. // This ensures schema initialization completes before concurrent access, // which avoids lock contention during initial table creation. @@ -107,14 +98,10 @@ describe("concurrent database access", () => { closeDatabase(); }); - afterEach(() => { - closeDatabase(); - }); - test("multiple processes can write DSN cache entries simultaneously", async () => { const workerCount = 5; const results = await spawnWorkersConcurrently( - testConfigDir, + getConfigDir(), workerCount, "write-dsn" ); @@ -124,7 +111,7 @@ describe("concurrent database access", () => { // Verify all entries are present in the database closeDatabase(); // Close to re-open with fresh connection - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; + process.env[CONFIG_DIR_ENV_VAR] = getConfigDir(); for (let i = 0; i < workerCount; i++) { const directory = `/test/project-${i}`; @@ -137,7 +124,7 @@ describe("concurrent database access", () => { test("multiple processes can write project cache entries simultaneously", async () => { const workerCount = 5; const results = await spawnWorkersConcurrently( - testConfigDir, + getConfigDir(), workerCount, "write-project" ); @@ -147,7 +134,7 @@ describe("concurrent database access", () => { // Verify all entries are present closeDatabase(); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; + process.env[CONFIG_DIR_ENV_VAR] = getConfigDir(); for (let i = 0; i < workerCount; i++) { const cached = await getCachedProject("org-456", `proj-${i}`); @@ -159,7 +146,7 @@ describe("concurrent database access", () => { test("mixed read/write operations from multiple processes succeed", async () => { const workerCount = 5; const results = await spawnWorkersConcurrently( - testConfigDir, + getConfigDir(), workerCount, "read-write" ); @@ -169,7 +156,7 @@ describe("concurrent database access", () => { // Each worker did 5 iterations, verify some entries closeDatabase(); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; + process.env[CONFIG_DIR_ENV_VAR] = getConfigDir(); for (let w = 0; w < workerCount; w++) { for (let i = 0; i < 5; i++) { @@ -185,7 +172,7 @@ describe("concurrent database access", () => { // Run a larger batch to increase contention likelihood const workerCount = 10; const results = await spawnWorkersConcurrently( - testConfigDir, + getConfigDir(), workerCount, "write-dsn" ); diff --git a/test/lib/db/install-info.test.ts b/test/lib/db/install-info.test.ts index 7278e608..138c6fb0 100644 --- a/test/lib/db/install-info.test.ts +++ b/test/lib/db/install-info.test.ts @@ -2,27 +2,15 @@ * Install Info Storage Tests */ -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { closeDatabase } from "../../../src/lib/db/index.js"; +import { describe, expect, test } from "bun:test"; import { clearInstallInfo, getInstallInfo, setInstallInfo, } from "../../../src/lib/db/install-info.js"; -import { cleanupTestDir, createTestConfigDir } from "../../helpers.js"; +import { useTestConfigDir } from "../../helpers.js"; -let testConfigDir: string; - -beforeEach(async () => { - testConfigDir = await createTestConfigDir("test-install-info-"); - process.env.SENTRY_CONFIG_DIR = testConfigDir; -}); - -afterEach(async () => { - closeDatabase(); - delete process.env.SENTRY_CONFIG_DIR; - await cleanupTestDir(testConfigDir); -}); +useTestConfigDir("test-install-info-"); describe("getInstallInfo", () => { test("returns null when no install info stored", () => { diff --git a/test/lib/db/project-cache.test.ts b/test/lib/db/project-cache.test.ts index eb06d8bb..be2e2044 100644 --- a/test/lib/db/project-cache.test.ts +++ b/test/lib/db/project-cache.test.ts @@ -4,8 +4,7 @@ * Tests for caching project information by orgId:projectId or DSN public key. */ -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { closeDatabase } from "../../../src/lib/db/index.js"; +import { describe, expect, test } from "bun:test"; import { clearProjectCache, getCachedProject, @@ -13,21 +12,9 @@ import { setCachedProject, setCachedProjectByDsnKey, } from "../../../src/lib/db/project-cache.js"; -import { cleanupTestDir, createTestConfigDir } from "../../helpers.js"; +import { useTestConfigDir } from "../../helpers.js"; -let testConfigDir: string; - -beforeEach(async () => { - testConfigDir = await createTestConfigDir("test-project-cache-"); - process.env.SENTRY_CONFIG_DIR = testConfigDir; -}); - -afterEach(async () => { - // Close database to release file handles before cleanup - closeDatabase(); - delete process.env.SENTRY_CONFIG_DIR; - await cleanupTestDir(testConfigDir); -}); +useTestConfigDir("test-project-cache-"); describe("getCachedProject", () => { test("returns undefined when no cache entry exists", async () => { diff --git a/test/lib/db/schema.test.ts b/test/lib/db/schema.test.ts index c83aeb3b..6b322edd 100644 --- a/test/lib/db/schema.test.ts +++ b/test/lib/db/schema.test.ts @@ -3,13 +3,8 @@ */ import { Database } from "bun:sqlite"; -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdirSync, rmSync } from "node:fs"; +import { describe, expect, test } from "bun:test"; import { join } from "node:path"; -import { - CONFIG_DIR_ENV_VAR, - closeDatabase, -} from "../../../src/lib/db/index.js"; import { CURRENT_SCHEMA_VERSION, EXPECTED_COLUMNS, @@ -21,6 +16,7 @@ import { repairSchema, tableExists, } from "../../../src/lib/db/schema.js"; +import { useTestConfigDir } from "../../helpers.js"; /** * Create a database with all tables but some missing (for testing repair). @@ -63,45 +59,11 @@ function createPreMigrationDatabase( ); } -let testDir: string; -let originalConfigDir: string | undefined; - -beforeEach(() => { - // Save original config dir - originalConfigDir = process.env[CONFIG_DIR_ENV_VAR]; - - // Close any existing database connection - closeDatabase(); - - // Create unique test directory - const baseDir = originalConfigDir ?? "/tmp/sentry-cli-test"; - testDir = join( - baseDir, - `schema-test-${Date.now()}-${Math.random().toString(36).slice(2)}` - ); - mkdirSync(testDir, { recursive: true }); - process.env[CONFIG_DIR_ENV_VAR] = testDir; -}); - -afterEach(() => { - closeDatabase(); - // Restore original config dir - if (originalConfigDir) { - process.env[CONFIG_DIR_ENV_VAR] = originalConfigDir; - } else { - delete process.env[CONFIG_DIR_ENV_VAR]; - } - // Clean up test directory - try { - rmSync(testDir, { recursive: true, force: true }); - } catch { - // Ignore cleanup errors - } -}); +const getTestDir = useTestConfigDir("schema-test-"); describe("tableExists", () => { test("returns true for existing table", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); db.exec("CREATE TABLE test_table (id INTEGER PRIMARY KEY)"); expect(tableExists(db, "test_table")).toBe(true); @@ -109,7 +71,7 @@ describe("tableExists", () => { }); test("returns false for non-existent table", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); expect(tableExists(db, "nonexistent")).toBe(false); db.close(); @@ -118,7 +80,7 @@ describe("tableExists", () => { describe("hasColumn", () => { test("returns true for existing column", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); db.exec("CREATE TABLE test_table (id INTEGER PRIMARY KEY, name TEXT)"); expect(hasColumn(db, "test_table", "id")).toBe(true); @@ -127,7 +89,7 @@ describe("hasColumn", () => { }); test("returns false for non-existent column", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); db.exec("CREATE TABLE test_table (id INTEGER PRIMARY KEY)"); expect(hasColumn(db, "test_table", "missing_column")).toBe(false); @@ -137,7 +99,7 @@ describe("hasColumn", () => { describe("getSchemaIssues", () => { test("returns empty array for healthy database", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); initSchema(db); const issues = getSchemaIssues(db); @@ -146,7 +108,7 @@ describe("getSchemaIssues", () => { }); test("detects missing table", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); // Create schema without dsn_cache using the helper createDatabaseWithMissingTables(db, ["dsn_cache"]); @@ -161,7 +123,7 @@ describe("getSchemaIssues", () => { }); test("detects missing column", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); // Create dsn_cache without v4 columns (pre-migration state) createPreMigrationDatabase(db, ["dsn_cache"]); @@ -185,7 +147,7 @@ describe("getSchemaIssues", () => { describe("repairSchema", () => { test("creates missing tables", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); db.exec("CREATE TABLE schema_version (version INTEGER PRIMARY KEY)"); db.query("INSERT INTO schema_version (version) VALUES (?)").run(1); @@ -204,7 +166,7 @@ describe("repairSchema", () => { }); test("adds missing columns", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); initSchema(db); // Remove migrated columns by recreating the table in pre-migration state @@ -228,7 +190,7 @@ describe("repairSchema", () => { }); test("returns empty result for healthy database", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); initSchema(db); const result = repairSchema(db); @@ -239,7 +201,7 @@ describe("repairSchema", () => { }); test("updates schema version after repair", () => { - const db = new Database(join(testDir, "test.db")); + const db = new Database(join(getTestDir(), "test.db")); db.exec("CREATE TABLE schema_version (version INTEGER PRIMARY KEY)"); db.query("INSERT INTO schema_version (version) VALUES (?)").run(1); diff --git a/test/lib/region.test.ts b/test/lib/region.test.ts index 493027c0..198400ed 100644 --- a/test/lib/region.test.ts +++ b/test/lib/region.test.ts @@ -5,27 +5,18 @@ */ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdirSync } from "node:fs"; -import { join } from "node:path"; import { setAuthToken } from "../../src/lib/db/auth.js"; -import { CONFIG_DIR_ENV_VAR, closeDatabase } from "../../src/lib/db/index.js"; import { setOrgRegion } from "../../src/lib/db/regions.js"; import { isMultiRegionEnabled, resolveOrgRegion, } from "../../src/lib/region.js"; import { getSentryBaseUrl } from "../../src/lib/sentry-urls.js"; +import { useTestConfigDir } from "../helpers.js"; -const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; +useTestConfigDir("region-resolve-"); beforeEach(async () => { - closeDatabase(); - const testConfigDir = join( - testBaseDir, - `region-resolve-${Math.random().toString(36).slice(2)}` - ); - mkdirSync(testConfigDir, { recursive: true }); - process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; // Clear any SENTRY_URL override for most tests delete process.env.SENTRY_URL; // Set up auth token for API tests @@ -33,7 +24,6 @@ beforeEach(async () => { }); afterEach(() => { - closeDatabase(); delete process.env.SENTRY_URL; }); diff --git a/test/lib/version-check.test.ts b/test/lib/version-check.test.ts index 6f52af54..44fc384e 100644 --- a/test/lib/version-check.test.ts +++ b/test/lib/version-check.test.ts @@ -13,7 +13,7 @@ import { maybeCheckForUpdateInBackground, shouldSuppressNotification, } from "../../src/lib/version-check.js"; -import { cleanupTestDir, createTestConfigDir } from "../helpers.js"; +import { useTestConfigDir } from "../helpers.js"; describe("shouldSuppressNotification", () => { test("suppresses for upgrade command", () => { @@ -46,24 +46,20 @@ describe("shouldSuppressNotification", () => { // Its probabilistic behavior is tested indirectly through maybeCheckForUpdateInBackground. describe("getUpdateNotification", () => { - let testConfigDir: string; + useTestConfigDir("test-version-notif-"); let savedNoUpdateCheck: string | undefined; - beforeEach(async () => { - testConfigDir = await createTestConfigDir("test-version-notif-"); - process.env.SENTRY_CONFIG_DIR = testConfigDir; + beforeEach(() => { // Save and clear the env var to test real implementation savedNoUpdateCheck = process.env.SENTRY_CLI_NO_UPDATE_CHECK; delete process.env.SENTRY_CLI_NO_UPDATE_CHECK; }); - afterEach(async () => { - delete process.env.SENTRY_CONFIG_DIR; + afterEach(() => { // Restore the env var if (savedNoUpdateCheck !== undefined) { process.env.SENTRY_CLI_NO_UPDATE_CHECK = savedNoUpdateCheck; } - await cleanupTestDir(testConfigDir); }); test("returns null when no version info is cached", () => { @@ -115,26 +111,22 @@ describe("abortPendingVersionCheck", () => { }); describe("maybeCheckForUpdateInBackground", () => { - let testConfigDir: string; + useTestConfigDir("test-version-bg-"); let savedNoUpdateCheck: string | undefined; - beforeEach(async () => { - testConfigDir = await createTestConfigDir("test-version-bg-"); - process.env.SENTRY_CONFIG_DIR = testConfigDir; + beforeEach(() => { // Save and clear the env var to test real implementation savedNoUpdateCheck = process.env.SENTRY_CLI_NO_UPDATE_CHECK; delete process.env.SENTRY_CLI_NO_UPDATE_CHECK; }); - afterEach(async () => { + afterEach(() => { // Abort any pending check to clean up abortPendingVersionCheck(); - delete process.env.SENTRY_CONFIG_DIR; // Restore the env var if (savedNoUpdateCheck !== undefined) { process.env.SENTRY_CLI_NO_UPDATE_CHECK = savedNoUpdateCheck; } - await cleanupTestDir(testConfigDir); }); test("does not throw when called", () => {