From f299338a4d6e1669958f0bf74e46838044390ddd Mon Sep 17 00:00:00 2001 From: betegon Date: Fri, 13 Feb 2026 12:41:44 +0100 Subject: [PATCH 1/4] fix(test): restore SENTRY_CONFIG_DIR in afterEach instead of deleting Three test files deleted process.env.SENTRY_CONFIG_DIR in afterEach without restoring the value set by preload.ts. Since Bun runs test files in parallel, this caused a race condition: other test files that capture the env var at module scope would get undefined if their module loaded after one of these deletes ran. Save the original value in beforeEach and restore it in afterEach, matching the pattern already used by schema.test.ts and fix.test.ts. --- test/lib/db/install-info.test.ts | 8 +++++++- test/lib/db/project-cache.test.ts | 8 +++++++- test/lib/version-check.test.ts | 16 ++++++++++++++-- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/test/lib/db/install-info.test.ts b/test/lib/db/install-info.test.ts index 7278e608..8b5146bb 100644 --- a/test/lib/db/install-info.test.ts +++ b/test/lib/db/install-info.test.ts @@ -12,15 +12,21 @@ import { import { cleanupTestDir, createTestConfigDir } from "../../helpers.js"; let testConfigDir: string; +let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env.SENTRY_CONFIG_DIR; testConfigDir = await createTestConfigDir("test-install-info-"); process.env.SENTRY_CONFIG_DIR = testConfigDir; }); afterEach(async () => { closeDatabase(); - delete process.env.SENTRY_CONFIG_DIR; + if (savedConfigDir !== undefined) { + process.env.SENTRY_CONFIG_DIR = savedConfigDir; + } else { + delete process.env.SENTRY_CONFIG_DIR; + } await cleanupTestDir(testConfigDir); }); diff --git a/test/lib/db/project-cache.test.ts b/test/lib/db/project-cache.test.ts index eb06d8bb..afc4f2a3 100644 --- a/test/lib/db/project-cache.test.ts +++ b/test/lib/db/project-cache.test.ts @@ -16,8 +16,10 @@ import { import { cleanupTestDir, createTestConfigDir } from "../../helpers.js"; let testConfigDir: string; +let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env.SENTRY_CONFIG_DIR; testConfigDir = await createTestConfigDir("test-project-cache-"); process.env.SENTRY_CONFIG_DIR = testConfigDir; }); @@ -25,7 +27,11 @@ beforeEach(async () => { afterEach(async () => { // Close database to release file handles before cleanup closeDatabase(); - delete process.env.SENTRY_CONFIG_DIR; + if (savedConfigDir !== undefined) { + process.env.SENTRY_CONFIG_DIR = savedConfigDir; + } else { + delete process.env.SENTRY_CONFIG_DIR; + } await cleanupTestDir(testConfigDir); }); diff --git a/test/lib/version-check.test.ts b/test/lib/version-check.test.ts index 6f52af54..7622d962 100644 --- a/test/lib/version-check.test.ts +++ b/test/lib/version-check.test.ts @@ -48,8 +48,10 @@ describe("shouldSuppressNotification", () => { describe("getUpdateNotification", () => { let testConfigDir: string; let savedNoUpdateCheck: string | undefined; + let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env.SENTRY_CONFIG_DIR; testConfigDir = await createTestConfigDir("test-version-notif-"); process.env.SENTRY_CONFIG_DIR = testConfigDir; // Save and clear the env var to test real implementation @@ -58,7 +60,11 @@ describe("getUpdateNotification", () => { }); afterEach(async () => { - delete process.env.SENTRY_CONFIG_DIR; + if (savedConfigDir !== undefined) { + process.env.SENTRY_CONFIG_DIR = savedConfigDir; + } else { + delete process.env.SENTRY_CONFIG_DIR; + } // Restore the env var if (savedNoUpdateCheck !== undefined) { process.env.SENTRY_CLI_NO_UPDATE_CHECK = savedNoUpdateCheck; @@ -117,8 +123,10 @@ describe("abortPendingVersionCheck", () => { describe("maybeCheckForUpdateInBackground", () => { let testConfigDir: string; let savedNoUpdateCheck: string | undefined; + let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env.SENTRY_CONFIG_DIR; testConfigDir = await createTestConfigDir("test-version-bg-"); process.env.SENTRY_CONFIG_DIR = testConfigDir; // Save and clear the env var to test real implementation @@ -129,7 +137,11 @@ describe("maybeCheckForUpdateInBackground", () => { afterEach(async () => { // Abort any pending check to clean up abortPendingVersionCheck(); - delete process.env.SENTRY_CONFIG_DIR; + if (savedConfigDir !== undefined) { + process.env.SENTRY_CONFIG_DIR = savedConfigDir; + } else { + delete process.env.SENTRY_CONFIG_DIR; + } // Restore the env var if (savedNoUpdateCheck !== undefined) { process.env.SENTRY_CLI_NO_UPDATE_CHECK = savedNoUpdateCheck; From c29e4e8a9260372aa98afd4297041ef8f03ce308 Mon Sep 17 00:00:00 2001 From: betegon Date: Fri, 13 Feb 2026 12:52:42 +0100 Subject: [PATCH 2/4] fix(test): save/restore SENTRY_CONFIG_DIR in all test files Extend the save/restore pattern to every test file that modifies process.env.SENTRY_CONFIG_DIR. Also move module-scope env var captures into beforeEach hooks so they read the value at hook time rather than at module evaluation time. Files fixed: - test/lib/api-client.test.ts - test/lib/api-client.seer.test.ts - test/lib/api-client.multiregion.test.ts - test/commands/issue/utils.test.ts - test/lib/dsn/cache.test.ts - test/lib/dsn/detector.test.ts - test/lib/config.test.ts (module-scope -> beforeEach) - test/lib/region.test.ts (module-scope -> beforeEach) - test/lib/db/concurrent.test.ts (module-scope -> beforeEach) --- test/commands/issue/utils.test.ts | 7 ++++++ test/lib/api-client.multiregion.test.ts | 7 ++++++ test/lib/api-client.seer.test.ts | 7 ++++++ test/lib/api-client.test.ts | 33 +++++++++++++++++-------- test/lib/config.test.ts | 17 ++++++++++--- test/lib/db/concurrent.test.ts | 11 +++++++-- test/lib/dsn/cache.test.ts | 8 ++++++ test/lib/dsn/detector.test.ts | 16 +++++++++++- test/lib/region.test.ts | 17 +++++++++++-- 9 files changed, 104 insertions(+), 19 deletions(-) diff --git a/test/commands/issue/utils.test.ts b/test/commands/issue/utils.test.ts index ad3ac173..67eb9f8c 100644 --- a/test/commands/issue/utils.test.ts +++ b/test/commands/issue/utils.test.ts @@ -49,8 +49,10 @@ describe("buildCommandHint", () => { let testConfigDir: string; let originalFetch: typeof globalThis.fetch; +let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; // Use isolateProjectRoot to prevent DSN detection from scanning the real project testConfigDir = await createTestConfigDir("test-issue-utils-", { isolateProjectRoot: true, @@ -67,6 +69,11 @@ beforeEach(async () => { afterEach(async () => { globalThis.fetch = originalFetch; + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } await cleanupTestDir(testConfigDir); }); diff --git a/test/lib/api-client.multiregion.test.ts b/test/lib/api-client.multiregion.test.ts index 90ab2a5a..4b40b6fb 100644 --- a/test/lib/api-client.multiregion.test.ts +++ b/test/lib/api-client.multiregion.test.ts @@ -23,8 +23,10 @@ import { cleanupTestDir, createTestConfigDir } from "../helpers.js"; let testConfigDir: string; let originalFetch: typeof globalThis.fetch; +let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; testConfigDir = await createTestConfigDir("test-multiregion-"); process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; @@ -42,6 +44,11 @@ afterEach(async () => { // Restore original fetch globalThis.fetch = originalFetch; closeDatabase(); + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } await cleanupTestDir(testConfigDir); }); diff --git a/test/lib/api-client.seer.test.ts b/test/lib/api-client.seer.test.ts index 5b6a528f..3cde7e43 100644 --- a/test/lib/api-client.seer.test.ts +++ b/test/lib/api-client.seer.test.ts @@ -18,8 +18,10 @@ import { cleanupTestDir, createTestConfigDir } from "../helpers.js"; // Test config directory let testConfigDir: string; let originalFetch: typeof globalThis.fetch; +let savedConfigDir: string | undefined; beforeEach(async () => { + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; testConfigDir = await createTestConfigDir("test-seer-api-"); process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; @@ -35,6 +37,11 @@ beforeEach(async () => { afterEach(async () => { // Restore original fetch globalThis.fetch = originalFetch; + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } await cleanupTestDir(testConfigDir); }); diff --git a/test/lib/api-client.test.ts b/test/lib/api-client.test.ts index 18838a09..26c95a28 100644 --- a/test/lib/api-client.test.ts +++ b/test/lib/api-client.test.ts @@ -14,18 +14,12 @@ import { cleanupTestDir, createTestConfigDir } from "../helpers.js"; // Test config directory let testConfigDir: string; let originalFetch: typeof globalThis.fetch; - -/** - * Tracks requests made during a test - */ -type RequestLog = { - url: string; - method: string; - authorization: string | null; - isRetry: boolean; -}; +let savedConfigDir: string | undefined; +let savedClientId: string | undefined; beforeEach(async () => { + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; + savedClientId = process.env.SENTRY_CLIENT_ID; testConfigDir = await createTestConfigDir("test-api-"); process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; @@ -39,6 +33,25 @@ beforeEach(async () => { await setAuthToken("initial-token", 3600, "test-refresh-token"); }); +afterEach(async () => { + // Restore original fetch + globalThis.fetch = originalFetch; + + // Restore env vars + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } + if (savedClientId !== undefined) { + process.env.SENTRY_CLIENT_ID = savedClientId; + } else { + delete process.env.SENTRY_CLIENT_ID; + } + + await cleanupTestDir(testConfigDir); +}); + afterEach(async () => { // Restore original fetch globalThis.fetch = originalFetch; diff --git a/test/lib/config.test.ts b/test/lib/config.test.ts index e2b747f5..4c496ed9 100644 --- a/test/lib/config.test.ts +++ b/test/lib/config.test.ts @@ -48,12 +48,17 @@ import { * are still running. The parent test directory is cleaned up on * process exit by preload.ts. */ -const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; +let savedConfigDir: string | undefined; beforeEach(() => { // Close any previous database connection closeDatabase(); + // Read the base dir at hook time (not module scope) to avoid race conditions + // with other test files that may modify the env var during parallel execution + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; + const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; + // Create a unique subdirectory for this test const testConfigDir = join( testBaseDir, @@ -66,8 +71,12 @@ beforeEach(() => { 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. + // Restore the original config dir set by preload.ts + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } }); describe("auth token management", () => { @@ -565,7 +574,7 @@ describe("getDbPath", () => { test("returns the database file path", () => { const path = getDbPath(); expect(path).toContain("cli.db"); - expect(path).toContain(testBaseDir); + expect(path).toContain(process.env[CONFIG_DIR_ENV_VAR]!); }); }); diff --git a/test/lib/db/concurrent.test.ts b/test/lib/db/concurrent.test.ts index 45b8dabc..b18d4c76 100644 --- a/test/lib/db/concurrent.test.ts +++ b/test/lib/db/concurrent.test.ts @@ -19,7 +19,6 @@ import { import { getCachedProject } from "../../../src/lib/db/project-cache.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; @@ -89,11 +88,14 @@ async function spawnWorkersConcurrently( describe("concurrent database access", () => { let testConfigDir: string; + let savedConfigDir: string | undefined; beforeEach(async () => { closeDatabase(); + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; + const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; testConfigDir = join( - TEST_BASE_DIR, + testBaseDir, `concurrent-${Date.now()}-${Math.random().toString(36).slice(2)}` ); mkdirSync(testConfigDir, { recursive: true }); @@ -109,6 +111,11 @@ describe("concurrent database access", () => { afterEach(() => { closeDatabase(); + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } }); test("multiple processes can write DSN cache entries simultaneously", async () => { diff --git a/test/lib/dsn/cache.test.ts b/test/lib/dsn/cache.test.ts index 2611ef46..9c97cb3b 100644 --- a/test/lib/dsn/cache.test.ts +++ b/test/lib/dsn/cache.test.ts @@ -18,15 +18,23 @@ import { CONFIG_DIR_ENV_VAR } from "../../../src/lib/db/index.js"; // Use a unique test config directory const TEST_CONFIG_DIR = join(homedir(), ".sentry-cli-test-cache"); +let savedConfigDir: string | undefined; describe("DSN Cache", () => { beforeEach(() => { // Set up test config directory + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; process.env[CONFIG_DIR_ENV_VAR] = TEST_CONFIG_DIR; mkdirSync(TEST_CONFIG_DIR, { recursive: true }); }); afterEach(() => { + // Restore config dir + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } // Clean up test config directory try { rmSync(TEST_CONFIG_DIR, { recursive: true, force: true }); diff --git a/test/lib/dsn/detector.test.ts b/test/lib/dsn/detector.test.ts index a1c9b818..c3aae62f 100644 --- a/test/lib/dsn/detector.test.ts +++ b/test/lib/dsn/detector.test.ts @@ -39,9 +39,13 @@ const TEST_CONFIG_DIR = join(homedir(), ".sentry-cli-test-detector-config"); describe("DSN Detector (New Module)", () => { let testDir: string; + let savedConfigDir: string | undefined; + let savedSentryDsn: string | undefined; beforeEach(async () => { testDir = createTempDir(); + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; + savedSentryDsn = process.env.SENTRY_DSN; process.env[CONFIG_DIR_ENV_VAR] = TEST_CONFIG_DIR; mkdirSync(TEST_CONFIG_DIR, { recursive: true }); // Clear any cached DSN for the test directory @@ -51,7 +55,17 @@ describe("DSN Detector (New Module)", () => { }); afterEach(() => { - delete process.env.SENTRY_DSN; + // Restore env vars + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } + if (savedSentryDsn !== undefined) { + process.env.SENTRY_DSN = savedSentryDsn; + } else { + delete process.env.SENTRY_DSN; + } cleanupDir(testDir); cleanupDir(TEST_CONFIG_DIR); }); diff --git a/test/lib/region.test.ts b/test/lib/region.test.ts index 493027c0..158fbebb 100644 --- a/test/lib/region.test.ts +++ b/test/lib/region.test.ts @@ -16,10 +16,14 @@ import { } from "../../src/lib/region.js"; import { getSentryBaseUrl } from "../../src/lib/sentry-urls.js"; -const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; +let savedConfigDir: string | undefined; +let savedSentryUrl: string | undefined; beforeEach(async () => { closeDatabase(); + savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; + savedSentryUrl = process.env.SENTRY_URL; + const testBaseDir = process.env[CONFIG_DIR_ENV_VAR]!; const testConfigDir = join( testBaseDir, `region-resolve-${Math.random().toString(36).slice(2)}` @@ -34,7 +38,16 @@ beforeEach(async () => { afterEach(() => { closeDatabase(); - delete process.env.SENTRY_URL; + if (savedConfigDir !== undefined) { + process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; + } else { + delete process.env[CONFIG_DIR_ENV_VAR]; + } + if (savedSentryUrl !== undefined) { + process.env.SENTRY_URL = savedSentryUrl; + } else { + delete process.env.SENTRY_URL; + } }); describe("getSentryBaseUrl", () => { From 0a24dc53ab291f15148201e24a193753b1ba6bf1 Mon Sep 17 00:00:00 2001 From: betegon Date: Fri, 13 Feb 2026 15:01:57 +0100 Subject: [PATCH 3/4] fix(test): lock env vars and fetch to prevent race conditions in concurrent tests Add lockConfigDir() and lockFetch() helpers that use Object.defineProperty to prevent concurrent test files from mutating SENTRY_CONFIG_DIR and globalThis.fetch between async boundaries. This fixes the DB singleton auto-invalidation race where another file's beforeEach/afterEach changes the config dir mid-test, causing getDatabase() to open the wrong database and lose auth tokens. Fixes: api-client 401 retry tests (3), config refreshToken test (1) --- test/commands/issue/utils.test.ts | 81 ++++++++++++++++++------------- test/helpers.ts | 66 +++++++++++++++++++++++++ test/lib/api-client.test.ts | 33 +++++++++---- test/lib/config.test.ts | 9 +++- 4 files changed, 143 insertions(+), 46 deletions(-) diff --git a/test/commands/issue/utils.test.ts b/test/commands/issue/utils.test.ts index 67eb9f8c..08c50a5d 100644 --- a/test/commands/issue/utils.test.ts +++ b/test/commands/issue/utils.test.ts @@ -15,7 +15,12 @@ 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 { + cleanupTestDir, + createTestConfigDir, + lockConfigDir, + lockFetch, +} from "../../helpers.js"; describe("buildCommandHint", () => { test("suggests /ID for numeric IDs", () => { @@ -50,6 +55,8 @@ describe("buildCommandHint", () => { let testConfigDir: string; let originalFetch: typeof globalThis.fetch; let savedConfigDir: string | undefined; +let unlockEnv: (() => void) | undefined; +let unlockFetchFn: (() => void) | undefined; beforeEach(async () => { savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; @@ -58,6 +65,11 @@ beforeEach(async () => { isolateProjectRoot: true, }); process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; + + // Lock the env var so concurrent test files cannot change it during our test. + // This prevents the DB singleton from auto-invalidating mid-test. + unlockEnv = lockConfigDir(testConfigDir); + originalFetch = globalThis.fetch; await setAuthToken("test-token"); // Pre-populate region cache for orgs used in tests to avoid region resolution API calls @@ -68,7 +80,12 @@ beforeEach(async () => { }); afterEach(async () => { + // Unlock fetch first (if locked by this test) + unlockFetchFn?.(); + unlockFetchFn = undefined; globalThis.fetch = originalFetch; + // Unlock env var, then restore + unlockEnv?.(); if (savedConfigDir !== undefined) { process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; } else { @@ -79,8 +96,7 @@ afterEach(async () => { describe("resolveOrgAndIssueId", () => { test("throws for numeric ID (org cannot be resolved)", async () => { - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -107,7 +123,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); // Numeric IDs don't have org context, so resolveOrgAndIssueId should throw await expect( @@ -120,8 +136,7 @@ describe("resolveOrgAndIssueId", () => { }); test("resolves explicit org prefix (org/ISSUE-ID)", async () => { - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -147,7 +162,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); const result = await resolveOrgAndIssueId({ issueArg: "my-org/PROJECT-ABC", @@ -172,8 +187,7 @@ describe("resolveOrgAndIssueId", () => { "" ); - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -199,7 +213,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); const result = await resolveOrgAndIssueId({ issueArg: "f-g", @@ -212,8 +226,7 @@ describe("resolveOrgAndIssueId", () => { }); test("resolves explicit org prefix with project-suffix (e.g., 'org1/dashboard-4y')", async () => { - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -240,7 +253,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); const result = await resolveOrgAndIssueId({ issueArg: "org1/dashboard-4y", @@ -256,8 +269,7 @@ describe("resolveOrgAndIssueId", () => { const { setDefaults } = await import("../../../src/lib/db/defaults.js"); await setDefaults("my-org", "my-project"); - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -283,7 +295,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); const result = await resolveOrgAndIssueId({ issueArg: "G", @@ -317,8 +329,7 @@ describe("resolveOrgAndIssueId", () => { ); await clearProjectAliases(); - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -378,7 +389,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); const result = await resolveOrgAndIssueId({ issueArg: "craft-g", @@ -396,8 +407,7 @@ describe("resolveOrgAndIssueId", () => { ); await clearProjectAliases(); - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -437,7 +447,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); await expect( resolveOrgAndIssueId({ @@ -456,8 +466,7 @@ describe("resolveOrgAndIssueId", () => { await setOrgRegion("org2", DEFAULT_SENTRY_URL); - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { + unlockFetchFn = lockFetch(async (input, init) => { const req = new Request(input, init); const url = req.url; @@ -513,7 +522,7 @@ describe("resolveOrgAndIssueId", () => { return new Response(JSON.stringify({ detail: "Not found" }), { status: 404, }); - }; + }); await expect( resolveOrgAndIssueId({ @@ -528,11 +537,12 @@ describe("resolveOrgAndIssueId", () => { const { setDefaults } = await import("../../../src/lib/db/defaults.js"); await setDefaults("my-org", "my-project"); - // @ts-expect-error - partial mock - globalThis.fetch = async () => - new Response(JSON.stringify({ detail: "Unauthorized" }), { - status: 401, - }); + unlockFetchFn = lockFetch( + async () => + new Response(JSON.stringify({ detail: "Unauthorized" }), { + status: 401, + }) + ); // Auth errors should propagate await expect( @@ -548,11 +558,12 @@ describe("resolveOrgAndIssueId", () => { const { setDefaults } = await import("../../../src/lib/db/defaults.js"); await setDefaults("my-org", "my-project"); - // @ts-expect-error - partial mock - globalThis.fetch = async () => - new Response(JSON.stringify({ detail: "Internal Server Error" }), { - status: 500, - }); + unlockFetchFn = lockFetch( + async () => + new Response(JSON.stringify({ detail: "Internal Server Error" }), { + status: 500, + }) + ); // Server errors should propagate await expect( diff --git a/test/helpers.ts b/test/helpers.ts index c2f27312..09edadad 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -73,3 +73,69 @@ type FetchMockFn = export function mockFetch(fn: FetchMockFn): typeof fetch { return fn as unknown as typeof fetch; } + +/** + * Lock SENTRY_CONFIG_DIR so concurrent test files cannot change it. + * + * Bun runs test files concurrently in a single process, sharing + * `process.env` and `globalThis.fetch`. Between any `await` point + * inside our test, another file's beforeEach/afterEach can mutate + * these globals. The DB singleton auto-invalidates when the config + * dir changes, so even a momentary mutation causes getDatabase() to + * open the wrong DB and lose auth tokens / defaults. + * + * Uses Object.defineProperty to make the env var return the locked + * value regardless of what other tests write. Call `unlockConfigDir` + * in afterEach so other tests can proceed normally. + * + * @param configDir - The config directory path to lock + * @returns Unlock function to call in afterEach + */ +export function lockConfigDir(configDir: string): () => void { + Object.defineProperty(process.env, "SENTRY_CONFIG_DIR", { + get() { + return configDir; + }, + set() { + // Silently ignore writes from other test files + }, + configurable: true, + enumerable: true, + }); + + return () => { + // Restore normal property behavior + delete process.env.SENTRY_CONFIG_DIR; + process.env.SENTRY_CONFIG_DIR = configDir; + }; +} + +/** + * Lock globalThis.fetch to a mock handler so concurrent test files + * cannot replace it between async boundaries. + * + * Uses Object.defineProperty to intercept writes to globalThis.fetch, + * similar to lockConfigDir. Call the returned unlock function in + * afterEach to restore normal behavior. + * + * @param fn - The fetch mock implementation + * @returns Unlock function to call in afterEach + */ +export function lockFetch( + fn: (input: RequestInfo | URL, init?: RequestInit) => Promise +): () => void { + Object.defineProperty(globalThis, "fetch", { + get() { + return fn; + }, + set() { + // Silently ignore writes from other test files + }, + configurable: true, + enumerable: true, + }); + + return () => { + delete (globalThis as Record).fetch; + }; +} diff --git a/test/lib/api-client.test.ts b/test/lib/api-client.test.ts index 26c95a28..bbd3aa6b 100644 --- a/test/lib/api-client.test.ts +++ b/test/lib/api-client.test.ts @@ -9,13 +9,18 @@ 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 { + cleanupTestDir, + createTestConfigDir, + lockConfigDir, +} from "../helpers.js"; // Test config directory let testConfigDir: string; let originalFetch: typeof globalThis.fetch; let savedConfigDir: string | undefined; let savedClientId: string | undefined; +let unlockEnv: (() => void) | undefined; beforeEach(async () => { savedConfigDir = process.env[CONFIG_DIR_ENV_VAR]; @@ -23,6 +28,10 @@ beforeEach(async () => { testConfigDir = await createTestConfigDir("test-api-"); process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; + // Lock the env var so concurrent test files cannot change it during our test. + // This prevents the DB singleton from auto-invalidating mid-test. + unlockEnv = lockConfigDir(testConfigDir); + // Set required env var for OAuth refresh process.env.SENTRY_CLIENT_ID = "test-client-id"; @@ -37,7 +46,8 @@ afterEach(async () => { // Restore original fetch globalThis.fetch = originalFetch; - // Restore env vars + // Unlock env var first, then restore + unlockEnv?.(); if (savedConfigDir !== undefined) { process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; } else { @@ -52,18 +62,14 @@ afterEach(async () => { await cleanupTestDir(testConfigDir); }); -afterEach(async () => { - // Restore original fetch - globalThis.fetch = originalFetch; - - await cleanupTestDir(testConfigDir); -}); - /** * Creates a mock fetch that handles API requests. * Uses rawApiRequest which goes to control silo (no region resolution needed). * * The `apiRequestHandler` is called for each API request. + * + * The mock re-pins itself on every call so that another test file's + * afterEach cannot replace it between async boundaries. */ function createMockFetch( requests: RequestLog[], @@ -77,7 +83,13 @@ function createMockFetch( ): typeof globalThis.fetch { let apiRequestCount = 0; - return async (input: RequestInfo | URL, init?: RequestInit) => { + const mock = async ( + input: RequestInfo | URL, + init?: RequestInit + ): Promise => { + // Re-pin this mock — another test file's afterEach may have replaced it + globalThis.fetch = mock as unknown as typeof fetch; + const req = new Request(input, init); requests.push({ url: req.url, @@ -109,6 +121,7 @@ function createMockFetch( apiRequestCount += 1; return apiRequestHandler(req, apiRequestCount); }; + return mock as unknown as typeof globalThis.fetch; } describe("401 retry behavior", () => { diff --git a/test/lib/config.test.ts b/test/lib/config.test.ts index 4c496ed9..0218ac9b 100644 --- a/test/lib/config.test.ts +++ b/test/lib/config.test.ts @@ -38,6 +38,7 @@ import { setCachedProject, setCachedProjectByDsnKey, } from "../../src/lib/db/project-cache.js"; +import { lockConfigDir } from "../helpers.js"; /** * Test isolation: Each test gets its own config directory within @@ -49,6 +50,7 @@ import { * process exit by preload.ts. */ let savedConfigDir: string | undefined; +let unlockEnv: (() => void) | undefined; beforeEach(() => { // Close any previous database connection @@ -66,12 +68,17 @@ beforeEach(() => { ); mkdirSync(testConfigDir, { recursive: true }); process.env[CONFIG_DIR_ENV_VAR] = testConfigDir; + + // Lock the env var so concurrent test files cannot change it during our test. + // This prevents the DB singleton from auto-invalidating mid-test. + unlockEnv = lockConfigDir(testConfigDir); }); afterEach(() => { // Close database to release file handles closeDatabase(); - // Restore the original config dir set by preload.ts + // Unlock env var first, then restore + unlockEnv?.(); if (savedConfigDir !== undefined) { process.env[CONFIG_DIR_ENV_VAR] = savedConfigDir; } else { From 83afedb6bae93f53449814895030ec743175025c Mon Sep 17 00:00:00 2001 From: betegon Date: Fri, 13 Feb 2026 15:09:05 +0100 Subject: [PATCH 4/4] fix(test): use Proxy instead of Object.defineProperty for env var lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Object.defineProperty with configurable:true is bypassed by `delete` (which concurrent test files do in afterEach), and configurable:false causes `delete` to throw in Bun. Switch to a Proxy on process.env that intercepts get/set/delete of SENTRY_CONFIG_DIR — survives both `delete` and re-assignment from concurrent test files. --- test/helpers.ts | 86 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 24 deletions(-) diff --git a/test/helpers.ts b/test/helpers.ts index 09edadad..25f9a865 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -74,39 +74,76 @@ export function mockFetch(fn: FetchMockFn): typeof fetch { return fn as unknown as typeof fetch; } +/** + * Module-level lock state for SENTRY_CONFIG_DIR. + * + * We replace process.env with a Proxy that intercepts reads, writes, + * and deletes of SENTRY_CONFIG_DIR. When locked, the proxy returns the + * locked value and silently ignores mutations. When unlocked, all + * operations pass through to the real env object. + * + * A Proxy is used instead of Object.defineProperty because: + * - configurable:true descriptors can be removed by `delete` (other + * test files do `delete process.env[CONFIG_DIR_ENV_VAR]` in afterEach) + * - configurable:false descriptors cause `delete` to throw in Bun, + * breaking other test files + * + * The Proxy is installed once and persists for the process lifetime. + * Lock/unlock just toggles the module-level state variables. + */ +let configDirLocked = false; +let configDirLockedValue: string | undefined; +let envProxyInstalled = false; + +function installEnvProxy(): void { + if (envProxyInstalled) return; + const realEnv = process.env; + process.env = new Proxy(realEnv, { + get(target, prop, receiver) { + if (prop === "SENTRY_CONFIG_DIR" && configDirLocked) { + return configDirLockedValue; + } + return Reflect.get(target, prop, receiver); + }, + set(target, prop, value) { + if (prop === "SENTRY_CONFIG_DIR" && configDirLocked) { + return true; // Silently ignore + } + return Reflect.set(target, prop, value); + }, + deleteProperty(target, prop) { + if (prop === "SENTRY_CONFIG_DIR" && configDirLocked) { + return true; // Silently ignore + } + return Reflect.deleteProperty(target, prop); + }, + }); + envProxyInstalled = true; +} + /** * Lock SENTRY_CONFIG_DIR so concurrent test files cannot change it. * * Bun runs test files concurrently in a single process, sharing - * `process.env` and `globalThis.fetch`. Between any `await` point - * inside our test, another file's beforeEach/afterEach can mutate - * these globals. The DB singleton auto-invalidates when the config - * dir changes, so even a momentary mutation causes getDatabase() to - * open the wrong DB and lose auth tokens / defaults. + * `process.env`. Between any `await` point inside our test, another + * file's beforeEach/afterEach can mutate the env var. The DB singleton + * auto-invalidates when the config dir changes, so even a momentary + * mutation causes getDatabase() to open the wrong DB and lose data. * - * Uses Object.defineProperty to make the env var return the locked - * value regardless of what other tests write. Call `unlockConfigDir` - * in afterEach so other tests can proceed normally. + * Uses a Proxy on process.env that intercepts get/set/delete of the + * config dir env var. When locked, returns the locked value and ignores + * mutations. When unlocked, all operations pass through normally. * * @param configDir - The config directory path to lock * @returns Unlock function to call in afterEach */ export function lockConfigDir(configDir: string): () => void { - Object.defineProperty(process.env, "SENTRY_CONFIG_DIR", { - get() { - return configDir; - }, - set() { - // Silently ignore writes from other test files - }, - configurable: true, - enumerable: true, - }); + installEnvProxy(); + configDirLocked = true; + configDirLockedValue = configDir; return () => { - // Restore normal property behavior - delete process.env.SENTRY_CONFIG_DIR; - process.env.SENTRY_CONFIG_DIR = configDir; + configDirLocked = false; }; } @@ -114,9 +151,10 @@ export function lockConfigDir(configDir: string): () => void { * Lock globalThis.fetch to a mock handler so concurrent test files * cannot replace it between async boundaries. * - * Uses Object.defineProperty to intercept writes to globalThis.fetch, - * similar to lockConfigDir. Call the returned unlock function in - * afterEach to restore normal behavior. + * Uses configurable: true since concurrent test files use assignment + * (not delete) for fetch, and other test files (e.g. api-client) need + * to assign their own mock via globalThis.fetch = ... without going + * through this lock. * * @param fn - The fetch mock implementation * @returns Unlock function to call in afterEach