fix(test): restore SENTRY_CONFIG_DIR in afterEach instead of deleting#240
Closed
fix(test): restore SENTRY_CONFIG_DIR in afterEach instead of deleting#240
Conversation
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.
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Telemetry
Other
🤖 This preview updates automatically when you update the PR. |
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)
…urrent 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)
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.
Member
|
Superceded by #242 |
BYK
added a commit
that referenced
this pull request
Feb 13, 2026
…ollution (#242) ## Summary Fixes 37 test failures caused by the Ubuntu 24.04 runner image upgrade (`20260201.15.1` → `20260209.23.1`, kernel 6.11 → 6.14) that changed `readdir` ordering, exposing a latent env var pollution bug. ## Root Cause Bun runs test files **sequentially in one thread** (load → run all tests → load next file). Several test files unconditionally `delete process.env.SENTRY_CONFIG_DIR` in their `afterEach` hooks. When the next file loads, its module-level code captures `undefined`, causing `join(undefined, ...)` → `TypeError`. The runner image upgrade changed the kernel which changed file discovery order, putting "deleting" files before "capturing" files. ## Changes - **`test/helpers.ts`**: Added `useTestConfigDir()` helper that creates a unique temp directory in `beforeEach`, points `SENTRY_CONFIG_DIR` at it, and **restores** (never deletes) the original value in `afterEach`. Also handles `closeDatabase()` and temp dir cleanup. - **11 test files migrated** to use `useTestConfigDir()`, eliminating: - 3 files that unconditionally deleted `SENTRY_CONFIG_DIR` (root cause) - 3 files with fragile module-level `process.env[CONFIG_DIR_ENV_VAR]!` captures - 2 files with `if (original) restore else delete` patterns (off-by-one on falsy check) - 2 files missing env var restore in `afterEach` - **`AGENTS.md`**: Added "Test Environment Isolation" section documenting the pattern and anti-patterns Net: **-87 lines** (157 added, 244 removed) — less boilerplate, more safety. ## Verification - `bun run typecheck` — passes - `bun run lint` — passes - `bun run test:unit` — **1545 tests pass, 0 failures** Supercedes #240
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three test files were deleting
process.env.SENTRY_CONFIG_DIRin theirafterEachhooks without restoring the value originally set bypreload.ts. Since Bun runs test files in parallel, this created a race condition: test files that capture the env var at module scope (likeregion.test.ts,config.test.ts,concurrent.test.ts) would getundefinedif their module loaded after one of these deletes ran.This caused 37 test failures across all recent PRs (#238, #237, #226, #221, #202) — likely triggered by a GHA runner image update (
20260201.15.1→20260209.23.1) that changed process scheduling enough to hit the race consistently.Changes
Save the original
SENTRY_CONFIG_DIRvalue inbeforeEachand restore it inafterEach, matching the pattern already used byschema.test.tsandfix.test.ts. Three files fixed:test/lib/version-check.test.ts(2 describe blocks)test/lib/db/project-cache.test.tstest/lib/db/install-info.test.tsTest Plan
Ran the full unit test suite locally — all 37 previously-failing tests now pass. The remaining local failures (
dsn/cache.test.tsSQLITE_IOERR_VNODE errors) are pre-existing onmain.